Skip to content

Conversation

@guggero
Copy link
Contributor

@guggero guggero commented May 2, 2025

What this issue fixes is a bit hard to explain. So I'll just copy the comment that's above the fix:

	// [...] When we [coop] close a channel, the output of the
	// `lncli closedchannels` command will show the last commitment state of
	// the channel as the closing asset balance. Which is correct in terms
	// of balances. But if there are multiple different asset IDs (e.g., in
	// a grouped asset channel), then _how_ those pieces are distributed
	// within the commitment transaction depends on the order of the
	// allocations. And the order of the allocations is dependent on the
	// BTC amount and the pkScript of the BTC-level output. Both of which
	// are different in the coop close output (we set the asset-level output
	// BTC amount to the dummy amount, and the pkScript will be a newly
	// derived internal key with no sibling script path).
	// So, long story short: If we used the tapsend.DistributeCoins method
	// here, it could happen that the actual asset output distribution shown
	// in the `lncli closedchannels` command would be different from the
	// actual distribution in the co-op close transaction.
	// This could mostly be seen as an UX-only issue, but was actually
	// discovered while attempting to assert the final closing balance of
	// grouped asset channels in the litd integration test.

@guggero
Copy link
Contributor Author

guggero commented May 2, 2025

Corresponding itest case that found this issue is here: lightninglabs/lightning-terminal#1056

@coveralls
Copy link

coveralls commented May 2, 2025

Pull Request Test Coverage Report for Build 14883073599

Details

  • 18 of 121 (14.88%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall first build on coop-close-allocation-sort at 36.907%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tapchannel/aux_closer.go 0 13 0.0%
tapchannel/commitment.go 0 90 0.0%
Totals Coverage Status
Change from base Build 14866822237: 36.9%
Covered Lines: 26474
Relevant Lines: 71732

💛 - Coveralls

@levmi levmi moved this from 🆕 New to 👀 In review in Taproot-Assets Project Board May 5, 2025
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 📩

guggero added 3 commits May 7, 2025 14:14
The code that groups a list of proofs by their asset ID is useful in
other places, so we extract it into a re-usable function.
This commit adds a new helper function that creates a set of virtual
packets from an aux commitment state and a set of input proofs (as well
as some negotiated values during the coop channel close flow).

The purpose of this is to use the asset allocation that has already
happened to distribute the assets of different asset ID pieces within
the given channel commitment again for the virtual packets of a coop
close transaction.

The reason we want to use the already allocated outputs vs.
re-allocating them again is to make sure that the distribution matches
exactly that of the commitment.
See explanation in next commit for more details.
This commit fixes the actual issue. It makes sure that the way the asset
pieces of different asset IDs (for example in grouped asset channels)
are distributed matches exactly the last commitment transaction.
See comment above call to CommitmentToPackets in this commit for more
details.
@guggero guggero force-pushed the coop-close-allocation-sort branch from c2ef059 to 83823c5 Compare May 7, 2025 12:14
@guggero guggero requested a review from ffranr May 7, 2025 12:14
Copy link
Contributor

@ffranr ffranr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the added context in 1:1

@ffranr ffranr enabled auto-merge May 7, 2025 20:51
@ffranr ffranr added this pull request to the merge queue May 7, 2025
Merged via the queue into main with commit f1f595a May 7, 2025
18 checks passed
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Taproot-Assets Project Board May 7, 2025
@guggero guggero deleted the coop-close-allocation-sort branch May 8, 2025 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

5 participants