[wallet 3/3]: group key support for channel funding #1413

guggero · 2025-02-26T15:37:30Z

Adds the ability to fund multiple tranches of a grouped asset into a channel.

Corresponding itest: lightninglabs/lightning-terminal#987

coveralls · 2025-02-26T16:56:36Z

Pull Request Test Coverage Report for Build 14358733453

Details

63 of 457 (13.79%) changed or added relevant lines in 18 files are covered.
53 unchanged lines in 9 files lost coverage.
Overall coverage decreased (-0.007%) to 28.388%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
universe/base.go	0	2	0.0%
tapfreighter/wallet.go	0	5	0.0%
taprpc/tapchannelrpc/tapchannel.pb.go	0	5	0.0%
taprpc/taprootassets.pb.go	0	5	0.0%
tapdb/assets_store.go	34	41	82.93%
rpcserver.go	0	11	0.0%
tapfreighter/chain_porter.go	0	21	0.0%
tapchannel/aux_sweeper.go	0	22	0.0%
tapfreighter/interface.go	0	30	0.0%
tapchannel/commitment.go	0	44	0.0%

Files with Coverage Reduction	New Missed Lines	%
commitment/tap.go	1	72.05%
rpcserver.go	1	0.0%
asset/group_key.go	2	57.89%
tapchannel/aux_closer.go	3	0.0%
tapchannel/commitment.go	3	7.67%
tapchannel/aux_sweeper.go	4	0.0%
tapgarden/caretaker.go	4	68.53%
asset/mock.go	8	63.99%
asset/asset.go	27	48.1%

Totals
Change from base Build 14334641175:	-0.007%
Covered Lines:	25941
Relevant Lines:	91380

💛 - Coveralls

guggero · 2025-03-27T22:07:41Z

TODO for myself: add group key to channel funding blob, see lightninglabs/lightning-terminal@4768ba2#r1982366865.

Roasbeef · 2025-03-31T23:53:05Z

TODO for myself: add group key to channel funding blob, see

To be done in this PR?

Roasbeef · 2025-03-31T23:59:40Z

tapsend/send.go

+	// group key _or_ the asset ID but not both. That means, if the group
+	// key is set, we ignore the asset ID and allow multiple inputs of the
+	// same group to be selected.
+	DistinctSpecifier bool


Interesting....makes we wonder if we should actually have two asset "specifier" types: one for the XOR case, and one for the AND case. This bool seems to give the caller a hint that the specifier should be treated in an identeifier exclusive manner (one or the other).

Alternatively, for the use case we see in this PR, couldn't we have the caller create a specifier that only has a group key if funding is initiated using a group key?

The problem is, that even today we have both use cases where we specify the AND case and the XOR case. Both are valid use cases IMO and depending on what the user wants this boolean basically switches between them.
See #1413 (comment).

tapdb/assets_store.go

Roasbeef · 2025-04-01T00:03:12Z

rpcserver.go

@@ -7073,7 +7073,7 @@ func (r *rpcServer) FundChannel(ctx context.Context,
 	}

 	assetID, groupKey, err := parseAssetSpecifier(
-		req.GetAssetId(), "", nil, "",
+		req.GetAssetId(), "", req.GetGroupKey(), "",


Re the other comment, this is where we can implement the either or behavior:

We only allow asset ID or group key to be set

We make a specifier that only has one half populated

With that change, I don't think we'd need that new bool and behavior change in the db?

added something similar in 667080c

Hmm, yeah, maybe we can introduce a sub type ExclusiveSpecifier that embeds the original identifier (just so we don't need to change things everywhere)... Will think about it.

rfqmsg/custom_channel_data.go

Roasbeef · 2025-04-01T00:27:20Z

tapchannel/aux_closer.go

-	assets := commitState.LocalAssets.Val.Outputs
+	// Next, we'll collect all the asset IDs that were committed to the
+	// channel.
+	assetIDs := fn.Map(


IIUC, there can potentially be duplicates here: multiple UTXOs of a given asset ID were used to fund the channel? Perhaps it should be run through as a set first.

No, I don't think that should ever be the case. Even if you have multiple inputs of the same asset ID, we'd merge them into the same UTXO. Only for different asset IDs we need to keep distinct UTXOs around (because they'll be represented by distinct vPackets). Will add a comment to make that more clear.

Roasbeef · 2025-04-01T00:28:03Z

tapchannel/aux_closer.go

 		}

-		scriptKeys[channelAsset.AssetID.Val] = *newKey.PubKey
+		scriptKeys[assetID] = *newKey.PubKey


Based on the comment above, we may end up with script keys thgat was created/inserted but were never used as we overrode them here in the map.

Yeah, you're right... So perhaps for the very first one we should try to re-use the one we already derived?

Okay, my comment right above this was incorrect. Because we shouldn't have duplicate asset IDs (as explained in #1413 (comment)), we shouldn't overwrite keys here. So we don't derive keys that we then don't end up using.

tapchannel/aux_closer.go

Roasbeef · 2025-04-01T00:34:07Z

tapchannel/aux_sweeper.go

-		vIn := vPackets[idx].Inputs[0]
-		if vIn.Asset().HasSplitCommitmentWitness() {
-			//nolint:lll
-			rootAsset := vIn.Asset().PrevWitnesses[0].SplitCommitment.RootAsset


Hmm, I recall this was one of those bugs that took be a while to track down properly.

The root asset (funding output) in this case needs to have the updated witness w/ the control block in tact, as we don't apply the segwit encoding to root assets stored as part of a split commitment.

Hmm, yeah, I thought this was probably the case. But I removed these lines and ran a full integration test suite and nothing broke. Is it possible that this was fixed indirectly in a later version by adding the witness earlier in the flow? Because I checked and the commitment transaction should already have the witness (both in the root asset as well as in each split commitment's root asset copy). But I will double check again.

Okay, I found exactly why this isn't needed anymore.
This was fixed with c072d3f, which was in #1154.
Which was quite a while after de5bed6 (in #938) which introduced the temporary fix in the first place.
So it seems like it was indeed needed at some point but could've been removed in #1154 already.

tapscript/script.go

GeorgeTsagk

very clean changes 🧹

will use the itest to better understand certain details, overall looking good! 👍

GeorgeTsagk · 2025-04-01T10:11:24Z

tapchannel/aux_funding_controller.go

+		AssetSpecifier:    specifier,
+		Amount:            amt,
+		CoinSelectType:    tapsend.Bip86Only,
+		DistinctSpecifier: true,


after seeing that we default to true here, I'm not sure in which scenarios we'd want to use false?

Will we ever have the freedom of choosing whether we'll use the ID or the groupKey of a specifier that has both? Seems to me that the intuitive choice is always picking the groupKey, but LMK what you think

The thing is, we already have that case. Remember when we added the itest that creates channels from a grouped asset (but only a single tranche so it would work with the code we had before this change)?
In that case we only specify the asset ID in the funding request. But because it's a grouped asset, the new logic would then also query the DB and set the group key in the specifier. And in that case we actually want to query with both, so we only fetch exactly the asset ID we supplied (and not just any asset from the group).

GeorgeTsagk · 2025-04-01T14:08:32Z

tapscript/script.go

+	// Then we'll create the OP_RETURN leaf with the asset ID to make the
+	// resulting script key unique.
+	opReturnLeaf, err := asset.NewNonSpendableScriptLeaf(
+		asset.OpReturnVersion, id[:],


could there be a proof collision here if we somehow ended up having the same id[:]?

Is this collision taking place within the transfer's namespace or is it global?

This is only within a single on-chain outpoint (e.g. the funding transaction). The universe has uniqueness over the tuple (group_key_or_asset_id, script_key, outpoint). Which for multiple tranches of the same group key would all be the same for the individual UTXOs committed to the channel (because group_key would be set since it's a grouped asset, see how the universe ID works). So we just need to de-duplicate the script key by adding the asset ID to its tapscript path. And since we'd only at most have one UTXO per asset ID, there should be no additional collision possible (see comments above about merging multiple inputs of the same asset ID into a single one within the channel).

GeorgeTsagk · 2025-04-01T14:10:18Z

tapchannel/aux_closer.go

-	if err != nil {
-		return lfn.Errf[wire.TxWitness]("unable to serialize control "+
-			"block: %w", err)
+	useUniqueScriptKey := len(vPackets) > 1


we assume that two different vPacket's may not use the same asset ID?
if yes could throw a comment for clarity

Correct. The definition for a vPacket that we arrived at is that it reflects all the changes of a single assetID within one on-chain transaction. Meaning that all the inputs and all the outputs of one asset ID within a transfer would be merged into the same vPacket. Will add a comment to make more clear.

GeorgeTsagk · 2025-04-01T14:24:51Z

rpcserver.go

@@ -7073,7 +7073,7 @@ func (r *rpcServer) FundChannel(ctx context.Context,
 	}

 	assetID, groupKey, err := parseAssetSpecifier(
-		req.GetAssetId(), "", nil, "",
+		req.GetAssetId(), "", req.GetGroupKey(), "",


added something similar in 667080c

lightninglabs-deploy · 2025-04-08T16:25:18Z

@guggero, remember to re-request review from reviewers when ready

We've decided to only ever use a single asset ID within a virtual packet. Which means we'd use multiple virtual packets to represent transfers for different asset IDs. So we don't need those early TODOs anymore.

We defined a while ago that an entry in the transfer table corresponds to one on-chain transaction. That means, such a transfer can house multiple virtual packets from different asset IDs. The transfer inputs already reflect that correctly. But the outputs were lacking the asset ID that indicates what virtual packet the output belonged to. This commit fixes the problem by adding the asset ID to the transfer output struct.

The wrong error was assigned, which caused the proof delivery failures to not surface correctly.

Not sure why the asset ID was never used in the database proof query. But now that we potentially have multiple asset outputs with the same script key but different asset IDs in the same anchor transaction, it's crucial we also add this to the query.

We changed the semantics around transfers when we implemented committing multiple asset transfers to a single TX. We now allow multiple virtual packets to be combined in a single transfer. Which means we have one entry in the transfer table for each on-chain transaction. But each transfer can have inputs and outputs from different asset IDs (meaning different active vPackets). This implies that it's also possible to have multiple transfer outputs with the same script key but different asset IDs, which means we need to change the way we identify the proof files.

One allocation entry means one on-chain output. So we should not create multiple allocations for the same balance (e.g. if there are multiple asset pieces with different asset IDs in the local or remote balance). Instead we just create one allocation for the sum, then let the coin distribution algorithm decide what output goes where.

We're doing the same thing in three places, this commit unifies the code by using the same function everywhere.

We always add the witness to the commitment TX before we create any of the commitment outputs. So this witness should already be correct.

To avoid a proof collision in the universe, we need to make sure we derive unique funding output script keys for multiple asset IDs within the same channel funding outpoint. We do that by adding a second OP_RETURN leaf into the tapscript tree of the funding script key. That should ensure uniqueness of the top-level Taproot output key and only requires us to slightly alter the control block when creating the witness (it will now include an inclusion proof element for the OP_RETURN leaf).

guggero · 2025-04-09T15:10:30Z

Addressed all comments.

guggero mentioned this pull request Feb 26, 2025

tapd: add itest for grouped asset channel funding lightninglabs/lightning-terminal#987

Open

guggero self-assigned this Feb 26, 2025

guggero added this to the v0.6 milestone Feb 26, 2025

guggero added this to Taproot-Assets Project Board Feb 26, 2025

github-project-automation bot moved this to 🆕 New in Taproot-Assets Project Board Feb 26, 2025

guggero moved this from 🆕 New to 🔖 Ready in Taproot-Assets Project Board Feb 26, 2025

guggero moved this from 🔖 Ready to 🏗 In progress in Taproot-Assets Project Board Feb 26, 2025

This was referenced Feb 26, 2025

[wallet 2/3]: refactor funding logic to use allocation code #1407

Merged

[wallet 1/3]: refactor in preparation for group key channel funding #1402

Merged

guggero force-pushed the allocation-refactor branch from 8939b42 to ee66eb9 Compare February 26, 2025 15:43

guggero force-pushed the group-key-support branch from 129dd06 to 76938ad Compare February 26, 2025 15:43

guggero force-pushed the group-key-support branch from 76938ad to 2639fa6 Compare February 27, 2025 16:48

guggero force-pushed the allocation-refactor branch 2 times, most recently from 86112a5 to cdc7cb2 Compare March 4, 2025 13:40

guggero force-pushed the group-key-support branch from 2639fa6 to 9f24e16 Compare March 4, 2025 13:40

guggero force-pushed the allocation-refactor branch from cdc7cb2 to be499e2 Compare March 4, 2025 16:19

guggero force-pushed the group-key-support branch 2 times, most recently from db987a5 to 206da82 Compare March 4, 2025 19:22

guggero force-pushed the allocation-refactor branch from be499e2 to bb88417 Compare March 4, 2025 19:22

GeorgeTsagk self-requested a review March 6, 2025 10:28

guggero changed the base branch from allocation-refactor to main March 6, 2025 11:31

guggero force-pushed the group-key-support branch from 206da82 to ce40ed7 Compare March 6, 2025 11:31

This was referenced Mar 7, 2025

Support group keys for RFQ negotiation flows #1382

Merged

[feature]: docs: provide JSON examples for custom channel data #1438

Open

levmi moved this from 🏗 In progress to 👀 In review in Taproot-Assets Project Board Mar 24, 2025

levmi moved this from 👀 In review to 🏗 In progress in Taproot-Assets Project Board Mar 24, 2025

guggero force-pushed the group-key-support branch 3 times, most recently from 7356ae4 to cd1b95b Compare March 27, 2025 17:33

guggero mentioned this pull request Mar 27, 2025

[custom channels]: refactor channel JSON, add more custom data parsers #1441

Merged

guggero force-pushed the group-key-support branch from cd1b95b to 7dd72b9 Compare March 27, 2025 18:02

Roasbeef requested changes Apr 1, 2025

View reviewed changes

github-project-automation bot moved this from 🏗 In progress to 👀 In review in Taproot-Assets Project Board Apr 1, 2025

GeorgeTsagk reviewed Apr 1, 2025

View reviewed changes

guggero force-pushed the group-key-support branch from 5f63baa to 6e98981 Compare April 2, 2025 14:44

guggero mentioned this pull request Apr 3, 2025

[tapdb]: add script key type enum, run Golang based post migration checks #1198

Open

guggero added 16 commits April 9, 2025 14:40

universe: fix formatting

6457f8f

tapsend: remove TODOs to clarify

edf618b

We've decided to only ever use a single asset ID within a virtual packet. Which means we'd use multiple virtual packets to represent transfers for different asset IDs. So we don't need those early TODOs anymore.

tapchannelmsg: rename groupPubKey to scriptPubKey

04e4504

tapdb: allow coin selection by group key

c30822d

taprpc: add group key to funding request

35cc031

tapchannel: support group key based channel funding

db60642

tapfreighter: fix logging and error handling

c0139f4

The wrong error was assigned, which caused the proof delivery failures to not surface correctly.

tapchannel+tapsend: simplify interactive split root handling

3ed142f

tapchannel: refactor out commit packet witness creation

5730f62

We're doing the same thing in three places, this commit unifies the code by using the same function everywhere.

tapchannel: remove unnecessary witness update

600b3f8

We always add the witness to the commitment TX before we create any of the commitment outputs. So this witness should already be correct.

GitHub: update litd itest branch

5f672a7

guggero force-pushed the group-key-support branch from 6e98981 to 5f672a7 Compare April 9, 2025 13:42

guggero requested review from GeorgeTsagk and Roasbeef April 9, 2025 15:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wallet 3/3]: group key support for channel funding #1413

[wallet 3/3]: group key support for channel funding #1413

guggero commented Feb 26, 2025 •

edited

Loading

coveralls commented Feb 26, 2025 •

edited

Loading

guggero commented Mar 27, 2025

Roasbeef commented Mar 31, 2025

Roasbeef Mar 31, 2025

Roasbeef Apr 1, 2025

guggero Apr 1, 2025

Roasbeef Apr 1, 2025

GeorgeTsagk Apr 1, 2025

guggero Apr 1, 2025

Roasbeef Apr 1, 2025

guggero Apr 1, 2025

Roasbeef Apr 1, 2025

guggero Apr 1, 2025

guggero Apr 9, 2025

Roasbeef Apr 1, 2025

guggero Apr 1, 2025

guggero Apr 9, 2025

GeorgeTsagk left a comment

GeorgeTsagk Apr 1, 2025

guggero Apr 1, 2025

GeorgeTsagk Apr 1, 2025

guggero Apr 1, 2025

GeorgeTsagk Apr 1, 2025

guggero Apr 1, 2025

GeorgeTsagk Apr 1, 2025

lightninglabs-deploy commented Apr 8, 2025

guggero commented Apr 9, 2025

[wallet 3/3]: group key support for channel funding #1413

Are you sure you want to change the base?

[wallet 3/3]: group key support for channel funding #1413

Conversation

guggero commented Feb 26, 2025 • edited Loading

coveralls commented Feb 26, 2025 • edited Loading

Pull Request Test Coverage Report for Build 14358733453

Details

💛 - Coveralls

guggero commented Mar 27, 2025

Roasbeef commented Mar 31, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeorgeTsagk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lightninglabs-deploy commented Apr 8, 2025

guggero commented Apr 9, 2025

guggero commented Feb 26, 2025 •

edited

Loading

coveralls commented Feb 26, 2025 •

edited

Loading