Support group keys on `SendPayment` & `AddInvoice` #1423

GeorgeTsagk · 2025-03-05T19:02:16Z

Description

This PR takes care of the rest of the work required to create and pay invoices by using group keys.

Using a group_key on SendPayment means that only channels that contain assets which belong to that group are going to be considered for sending this payment.
Using a group_key on AddInvoice means that only channels that contain assets which belong to that group may be considered for generating an RFQ quote and encoding it in the invoice.

Based on #1382

coveralls · 2025-03-05T19:20:13Z

Pull Request Test Coverage Report for Build 14033568327

Details

28 of 199 (14.07%) changed or added relevant lines in 7 files are covered.
21 unchanged lines in 6 files lost coverage.
Overall coverage decreased (-0.02%) to 27.902%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tapchannel/aux_invoice_manager.go	17	18	94.44%
rfqmsg/records.go	11	15	73.33%
taprpc/tapchannelrpc/tapchannel.pb.go	0	10	0.0%
tapchannel/aux_traffic_shaper.go	0	12	0.0%
rfq/manager.go	0	15	0.0%
rfq/order.go	0	24	0.0%
rpcserver.go	0	105	0.0%

Files with Coverage Reduction	New Missed Lines	%
asset/group_key.go	2	57.89%
tapchannel/aux_invoice_manager.go	2	86.18%
asset/asset.go	3	50.91%
asset/mock.go	3	64.72%
commitment/tap.go	4	71.82%
tapgarden/caretaker.go	7	68.53%

Totals
Change from base Build 13840646748:	-0.02%
Covered Lines:	25391
Relevant Lines:	91001

💛 - Coveralls

GeorgeTsagk · 2025-03-05T19:21:56Z

The current LiT itests are going to pass, as they will keep using a single asset ID for payments and invoices.

This LiT PR uses the new group key arguments for creating/paying invoices.

GeorgeTsagk · 2025-03-11T19:40:32Z

Rebased on main after merging base branch #1382

Roasbeef · 2025-03-21T00:16:33Z

rfqmsg/records.go

@@ -82,22 +84,43 @@ func (h *Htlc) Balances() []*AssetBalance {
 	return h.Amounts.Val.Balances
 }

+// SpecifierChecker is an interface that contains methods for checking certain
+// properties related to asset specifiers.
+type SpecifierChecker interface {


IIRC we have another instances of this elsewhere in the codebase?

Also is this something that can just be a function, or do we expect the actual implementation to vary depending on context?

yeah this is defined in both rfq and rfqmsg, keeping the one in rfq and importing it in rfqmsg makes an import cycle

keeping it only in rfqmsg makes sense import-wise but doesn't really make sense to keep this interface in rfqmsg..

Also about this being a function it makes sense. It's not going to be extended in the future with extra functions, and even if that happens we can convert it to an interface at that point in time. Right now it can just be a single func type 👍

rfq/order.go

tapchannel/aux_invoice_manager.go

Roasbeef · 2025-03-21T00:20:17Z

rfq/manager.go

@@ -949,6 +950,10 @@ func (m *Manager) getAssetGroupKey(ctx context.Context,
 	// Perform the DB query.
 	group, err := m.cfg.GroupLookup.QueryAssetGroup(ctx, id)
 	if err != nil {
+		if strings.Contains(err.Error(), "asset group is unknown") {


Can we do a more specific error check here so we don't need to rely on string matching?

Also when would this fail? Instances where we haven't synced the group key yet?

Can we do a more specific error check here so we don't need to rely on string matching?

We could do errors.Is but the required err to check against is in address pkg which leads to import cycle

Also when would this fail? Instances where we haven't synced the group key yet?

Yes, also in cases where the asset is not part of any group. This is the error that is returned after sql.ErrNoRows is encountered.

We should resolve that circular dependency. Can be done by applying fad16bf (in the upstream branch tinygo).

Roasbeef · 2025-03-21T00:21:37Z

rfq/manager.go

+		specifierGK := specifier.UnwrapGroupKeyToPtr()
+
+		// If the group lookup failed, let's see if the provided assetID
+		// is the hash of the group key, which is used by the sender to


This is a bit confusing, why aren't we just passing around the group key isntead of this special case where a hash of it can be provided?

If the look up fails, doesn't that mean we haven't verified this asset at all yet? That should have happened during funding, when we send the transition proofs of each of the assets.

This is a bit confusing, why aren't we just passing around the group key isntead of this special case where a hash of it can be provided?

Not sure, using the hash of it vs the actual group key was communicated offline with @guggero. Can't think of any reason why we would want to stick to the hash. So far it has been just an extra round of calculations before the check.

If the look up fails, doesn't that mean we haven't verified this asset at all yet? That should have happened during funding, when we send the transition proofs of each of the assets.

This function is also used by higher-level systems like the RFQ manager. On that level we only care about the HTLC custom records that we receive, so we want to check if the assetID/groupKey that is included matches that of the quote.

The group hash is only used as a placeholder since we don't know which actual asset IDs will be picked for this particular HTLC, this would be handled later in the flow.

Hmm, so see my other inline comment. So we do look up the value we put into the dummy ID again here.
Not sure what's better, doing an explicit hash or just use the x-coordinates of the group key directly.

But what we can do in any case is do the groupHash == id comparison before querying the database. Because if that matches, we know there isn't an asset ID in the DB. That would probably also allow us to get rid of the previous commit.

Roasbeef · 2025-03-21T00:23:19Z

tapchannel/aux_traffic_shaper.go

+		// balance in the HTLC is just a hint and the actual asset IDs
+		// will be picked later in the process.
+		groupKey := quote.Request.AssetSpecifier.UnwrapGroupKeyToPtr()
+		assetId = sha256.Sum256(groupKey.SerializeCompressed())


Do we actually need to start using a specifier for the HTLC balance TLVs?

Otherwise, we're introducing some overloading here, which is likely to be the source of confusion/bugs down the line.

As an example, if we're using a fake asset ID (which is actually the group key hash), won't all the logic to create the allocations for th actual TAP tree in the HTLC/commitment outputs fail?

Do we actually need to start using a specifier for the HTLC balance TLVs?

Could do that, but I wonder if just placing the group key in there is much simpler / smaller diff?

Otherwise, we're introducing some overloading here, which is likely to be the source of confusion/bugs down the line.

True, ideally would like the group key to not be in the same spot as the assetID. Explicit is always better

As an example, if we're using a fake asset ID (which is actually the group key hash), won't all the logic to create the allocations for th actual TAP tree in the HTLC/commitment outputs fail?

cc @guggero
I think this will only be used as a signal, the allocation logic would know of the specific asset IDs that compose this group channel and would create the correct allocations.

rpcserver.go

We add a new interface to the HTLC SumAssetBalance method, which helps check the identifier of the asset against a specifier. This allows for checking asset inclusion in a group, which is a bit involved and not the responsibility of the HTLC model.

We extend the interface of the rfq Policy in order to allow the specifier checker to be involved. This extends certain checks, and allows us to use asset specifiers that only have a group key.

We add the specifier checker interface to the AuxInvoiceManager too, as it is needed to validate incoming HTLCs which may use asset IDs that belong to a group, while the RFQ is based on a group key.

Adds some coverage to the invoice manager unit tests, which involve an RFQ quote over a group key, plus an HTLC with multiple asset balances, which may belong or not to the group.

We may be performing a group lookup on an asset that doesn't belong to a group. Instead of returning the error that originates from the ErrNoRows of sql we instead return a nil result, signalling that no group was found and no error occurred.

In some cases the sender of an asset HTLC may just encode the hash of the group key as the assetID of the balance in the custom records. This is done as a way to signal that any asset ID that belongs to this group may be picked to carry out the payment. This commit makes the specifier matcher aware of that case.

As mentioned in the previous commit, we occasionally want to just encode the hash of the group key as the asset ID of the balance that is encoded in the custom record. We make the ProduceHtlcExtraData hook aware of that case, which is triggered when the quote is made upon a group key and not a specific asset ID.

We have taken care of the groupkey RFQ negotiation in previous commits. All we need now to support sending a payment over a group of assets, is to propagate the user specifier groupkey to the corresponding fields.

In this commit we take the user defined group key and allow the asset specifier to be created over it. All the calls that accept it as an argument are already groupkey aware.

guggero

Did a first pass.

Concerning the discussion around group key in the asset sum entry.
I failed to consider the group key send case when designing the p2p messages.

Once an HTLC is locked in, the allocation code makes sure each HTLC has a specific, clearly defined set of asset pieces, all with IDs defined.
But as long as the HTLC is just transmitted (e.g. on the SendPayment RPC, in ProduceHtlcExtraData function of the aux traffic shaper, or even in the UpdateAddHtlc wire message), we don't actually know the ID of the piece(s) that are going to be used for the HTLC.
So in those scenarios we only really check two things in the policies:

The sum of the assets being transferred in the HTLC message
Whether the assets match the policy

For the second check, I think it makes sense to indirectly put the group key in there. Whether we hash it first (as we do for the universe key) or whether we just use the x-coordinate of the group key doesn't matter too much IMO. But I think we need to be very clear wherever we do that and definitely add a comment on why we're doing that (feel free to copy my explanation above).

guggero · 2025-03-24T19:58:42Z

rfqmsg/records.go

 	for idx := range h.Amounts.Val.Balances {
 		balance := h.Amounts.Val.Balances[idx]

-		if balance.AssetID.Val != targetAssetID {
+		ctxt, cancel := context.WithTimeout(
+			context.Background(), wait.DefaultTimeout,


I don't think we should use testing related code (from the lntest/wait package).
Maybe it makes sense to pass in the context into the SumAssetBalance call instead?
Or remove the context from the SpecifierChecker function type and use the surrounding context where it's used (creating a closure).

guggero · 2025-03-24T19:59:03Z

rfqmsg/records.go

+		)
+		defer cancel()
+
+		if specifierChecker == nil {


This check can be moved to the beginning of the method.

guggero · 2025-03-24T20:00:31Z

rfq/order.go

+// SpecifierChecker checks whether the passed specifier and asset ID match. If
+// the specifier contains a group key, it will check whether the asset belongs
+// to that group.
+type SpecifierChecker func(ctx context.Context, specifier asset.Specifier,


Can just use the one from rfqmsg IMO.

guggero · 2025-03-24T20:04:47Z

rfq/order.go

+	case c.AssetSpecifier.HasGroupPubKey():
+		groupKey := c.AssetSpecifier.UnwrapGroupKeyToPtr()
+
+		// We have performed checks for the asset IDs inside the HTLC
+		// against the specifier's group key in a previous step. Here
+		// we just need to provide a dummy value as the asset ID. The
+		// real asset IDs will be carefully picked in a later step in
+		// the process. What really matters now is the total amount.
+		assetID = sha256.Sum256(groupKey.SerializeCompressed())
+
+	case c.AssetSpecifier.HasId():
+		specifierID := *c.AssetSpecifier.UnwrapIdToPtr()
+		copy(assetID[:], specifierID[:])


This change seems unrelated to the rest of the commit. I think this should probably go into its own commit with its own commit message (explaining that the asset ID in the balance record doesn't really matter as the allocation code will actually take care of assigning specific asset IDs).

guggero · 2025-03-24T20:07:22Z

rfq/order.go

+		// We have performed checks for the asset IDs inside the HTLC
+		// against the specifier's group key in a previous step. Here
+		// we just need to provide a dummy value as the asset ID. The
+		// real asset IDs will be carefully picked in a later step in
+		// the process. What really matters now is the total amount.
+		assetID = sha256.Sum256(groupKey.SerializeCompressed())


I know I probably mentioned this at some point to just hash the group key. But I wonder if it makes sense to instead leave the asset ID empty? Or use a fixed, dummy value in both cases? Since I don't think we ever try to resolve this back...

guggero · 2025-03-24T20:19:51Z

rfq/manager.go

@@ -949,6 +950,10 @@ func (m *Manager) getAssetGroupKey(ctx context.Context,
 	// Perform the DB query.
 	group, err := m.cfg.GroupLookup.QueryAssetGroup(ctx, id)
 	if err != nil {
+		if strings.Contains(err.Error(), "asset group is unknown") {


We should resolve that circular dependency. Can be done by applying fad16bf (in the upstream branch tinygo).

guggero · 2025-03-24T20:23:48Z

rfq/manager.go

+		specifierGK := specifier.UnwrapGroupKeyToPtr()
+
+		// If the group lookup failed, let's see if the provided assetID
+		// is the hash of the group key, which is used by the sender to


Hmm, so see my other inline comment. So we do look up the value we put into the dummy ID again here.
Not sure what's better, doing an explicit hash or just use the x-coordinates of the group key directly.

But what we can do in any case is do the groupHash == id comparison before querying the database. Because if that matches, we know there isn't an asset ID in the DB. That would probably also allow us to get rid of the previous commit.

guggero · 2025-03-24T20:30:04Z

rpcserver.go

+	var specifier asset.Specifier
+
+	switch {
+	case len(req.AssetId) > 0 && len(req.GroupKey) > 0:


Could probably use parseAssetSpecifier here? Combined with asset.NewSpecifier(). Or maybe even specifierWithGroupKeyLookup if that makes sense here?

guggero · 2025-03-24T20:31:28Z

rpcserver.go

+		var rpcSpecifier rfqrpc.AssetSpecifier
+
+		switch {
+		case specifier.HasId():
+			assetID := specifier.UnwrapIdToPtr()
+			rpcSpecifier = rfqrpc.AssetSpecifier{
+				Id: &rfqrpc.AssetSpecifier_AssetId{
+					AssetId: assetID[:],
+				},
+			}
+
+		case specifier.HasGroupPubKey():
+			groupKey := specifier.UnwrapGroupKeyToPtr()
+			groupKeyBytes := groupKey.SerializeCompressed()
+			rpcSpecifier = rfqrpc.AssetSpecifier{
+				Id: &rfqrpc.AssetSpecifier_GroupKey{
+					GroupKey: groupKeyBytes,
+				},
+			}
+		}


Probably makes sense to extract this into a marshalAssetSpecifier function, as this seems to appear twice in the code now.

guggero · 2025-03-24T20:33:41Z

rpcserver.go

+	var specifier asset.Specifier
+
+	switch {
+	case len(req.AssetId) > 0 && len(req.GroupKey) > 0:


Same here and below re: using existing functions and/or extracting into function.

GeorgeTsagk self-assigned this Mar 5, 2025

GeorgeTsagk mentioned this pull request Mar 5, 2025

Support group keys on HTLC Interceptor & Invoice HTLC Modifier #1416

Closed

GeorgeTsagk force-pushed the rfq-negotiation-groupkey branch from 7c660bd to 6cf7190 Compare March 10, 2025 15:12

Base automatically changed from rfq-negotiation-groupkey to main March 11, 2025 17:31

GeorgeTsagk force-pushed the taprpc-groupkey-support branch from 9c9de09 to b53b3a2 Compare March 11, 2025 19:40

GeorgeTsagk force-pushed the taprpc-groupkey-support branch from b53b3a2 to f13f4a0 Compare March 19, 2025 11:22

GeorgeTsagk requested review from guggero and Roasbeef March 19, 2025 11:22

Roasbeef requested changes Mar 21, 2025

View reviewed changes

GeorgeTsagk added 11 commits March 24, 2025 11:58

rfq: policies use specifier checker

370ad51

We extend the interface of the rfq Policy in order to allow the specifier checker to be involved. This extends certain checks, and allows us to use asset specifiers that only have a group key.

tapchannel: aux invoice manager uses specifier checker

0f3a504

We add the specifier checker interface to the AuxInvoiceManager too, as it is needed to validate incoming HTLCs which may use asset IDs that belong to a group, while the RFQ is based on a group key.

tapchannel: add group key tests to AuxInvoiceManager

7ba0a4f

Adds some coverage to the invoice manager unit tests, which involve an RFQ quote over a group key, plus an HTLC with multiple asset balances, which may belong or not to the group.

taprpc: add groupkey to SendPaymentRequest

0143f37

rpcserver: SendPayment uses groupkey

911011e

We have taken care of the groupkey RFQ negotiation in previous commits. All we need now to support sending a payment over a group of assets, is to propagate the user specifier groupkey to the corresponding fields.

taprpc: add groupkey to AddInvoice

798597a

rpcserver: AddInvoice uses groupkey

27faee9

In this commit we take the user defined group key and allow the asset specifier to be created over it. All the calls that accept it as an argument are already groupkey aware.

GeorgeTsagk force-pushed the taprpc-groupkey-support branch from f13f4a0 to 27faee9 Compare March 24, 2025 11:00

guggero reviewed Mar 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support group keys on `SendPayment` & `AddInvoice` #1423

Support group keys on `SendPayment` & `AddInvoice` #1423

GeorgeTsagk commented Mar 5, 2025

coveralls commented Mar 5, 2025 •

edited

Loading

GeorgeTsagk commented Mar 5, 2025

GeorgeTsagk commented Mar 11, 2025

Roasbeef Mar 21, 2025

GeorgeTsagk Mar 24, 2025

GeorgeTsagk Mar 24, 2025

Roasbeef Mar 21, 2025

Roasbeef Mar 21, 2025

GeorgeTsagk Mar 24, 2025

guggero Mar 24, 2025

Roasbeef Mar 21, 2025

Roasbeef Mar 21, 2025

GeorgeTsagk Mar 24, 2025

guggero Mar 24, 2025

Roasbeef Mar 21, 2025

GeorgeTsagk Mar 24, 2025

guggero left a comment

guggero Mar 24, 2025

guggero Mar 24, 2025

guggero Mar 24, 2025

guggero Mar 24, 2025

guggero Mar 24, 2025

guggero Mar 24, 2025

guggero Mar 24, 2025

guggero Mar 24, 2025

guggero Mar 24, 2025

guggero Mar 24, 2025

Support group keys on SendPayment & AddInvoice #1423

Are you sure you want to change the base?

Support group keys on SendPayment & AddInvoice #1423

Conversation

GeorgeTsagk commented Mar 5, 2025

Description

coveralls commented Mar 5, 2025 • edited Loading

Pull Request Test Coverage Report for Build 14033568327

Details

💛 - Coveralls

GeorgeTsagk commented Mar 5, 2025

GeorgeTsagk commented Mar 11, 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

guggero 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Support group keys on `SendPayment` & `AddInvoice` #1423

Support group keys on `SendPayment` & `AddInvoice` #1423

coveralls commented Mar 5, 2025 •

edited

Loading