Custom channels itest: Use group keys on some payments & invoices #995

GeorgeTsagk · 2025-03-05T19:15:22Z

Description

We sprinkle around some group keys across the grouped asset custom channels test. Some of the invoices and payments in that test now use a group key instead of a specific asset ID.

Related tapd PR: https://github.com/lightninglabs/taproot-assets/pull/1423/commits

For the liquidity edge cases itest, we now support running it in assetID & groupKey mode, meaning we can go through the full suite of edge cases by using either a single assetID or a groupKey.

Roasbeef

I think this is a good start as a sanity check for some of the new logic. I think we'll want to more or less run each test w/ both asset ID and group key, but that might eventually be a bit unwieldy when it comes to execution time.

Ultimately, we do expect most of the primary use cases to use group keys, so pehrhaps it's fine to lean towards coverage there?

Roasbeef · 2025-03-06T00:15:57Z

itest/assets_test.go

+
+	case len(groupID) > 0:
+		groupHash := sha256.Sum256(groupID)
+		targetID = hex.EncodeToString(groupHash[:])


Hmm, do we need to modify what we return as JsonHtlc to start to include a distinct group key field? IIRC, it's all TLV within the db/lnd, so shouldn't be a big lift there, and should be backwards compat.

Hmm, good idea. We can definitely add that to make certain things easier. Even if it's just informative to the user and useful in itests (but not explicitly expected in the RFQ logic).

GeorgeTsagk · 2025-03-07T11:43:42Z

I think this is a good start as a sanity check for some of the new logic. I think we'll want to more or less run each test w/ both asset ID and group key, but that might eventually be a bit unwieldy when it comes to execution time.

Ultimately, we do expect most of the primary use cases to use group keys, so pehrhaps it's fine to lean towards coverage there?

I agree with you here, ideally we'd want to even focus more on group key usage compared to single asset IDs. I'll think of an approach that lets us run everything in assetID / groupkey mode, and see if it's worth the trouble.

Otherwise, we could just add groupkey explicit itests, and support the hybrid mode on the more extensive edge-case itests

guggero

Concept ACK

guggero · 2025-03-07T13:33:03Z

itest/assets_test.go

+
+	case len(groupID) > 0:
+		groupHash := sha256.Sum256(groupID)
+		targetID = hex.EncodeToString(groupHash[:])


Hmm, good idea. We can definitely add that to make certain things easier. Even if it's just informative to the user and useful in itests (but not explicitly expected in the RFQ logic).

GeorgeTsagk · 2025-03-19T14:06:44Z

Added an extra commit which lets us run the liquidity edge cases in assetID & groupKey mode

This way the edge case coverage is repeated for group keys without re-writing the whole test

We could take a similar approach for all other tests, so please take a look and LMK if you agree with that approach. It would ofcs lead to running everything twice, so double total itest time.

GeorgeTsagk · 2025-03-26T17:28:39Z

Changed base to #998 in order to be able to test with the related tapd PR

GeorgeTsagk · 2025-03-27T10:20:52Z

rebased on master

guggero

Looks pretty good! Have some ideas to reduce the size of the diff though.

itest/assets_test.go

guggero · 2025-03-27T22:08:46Z

itest/assets_test.go

+		targetID = hex.EncodeToString(assetID)
+
+	case len(groupID) > 0:
+		groupHash := sha256.Sum256(groupID)


Same here, re x-coordinate. I wonder why this didn't cause things to fail? Or did this still point to the old commit in tapd?

I think you started the review a bit before I rebased & used latest ref to tapd, which also changed these lines in the LiT side

ok so it was the wrong commit (other comment)

itest/litd_custom_channels_test.go

itest/assets_test.go

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

itest/litd_custom_channels_test.go

@@ -1896,10 +1918,13 @@ func testCustomChannelsLiquidityEdgeCases(ctx context.Context,

 	logBalance(t.t, nodes, assetID, "10k sats")

+	opts = []payOpt{}


nit: var opts []payOpt to match empty initialization style above?

can't redeclare it, i'm just setting it again

itest/assets_test.go

guggero · 2025-03-27T22:17:48Z

itest/litd_custom_channels_test.go

+
+	// Run liquidity edge cases and only use group keys for invoices and
+	// payments.
+	testCustomChannelsLiquidtyEdgeCasesCore(ctx, net, t, true)


Can't we run this directly in testCustomChannelsLiquidityEdgeCases? Meaning to call the same sub test function there once with true once with false.
Each new test has quite a bit of overhead, so the more we can combine, the better.

Yes thought of that. We can't really do that as it will crash due to re-creating all the nodes again in the same network.

Ideally, we would want to have a helper function that resets the state to a "clean slate" as far as liquidity goes, so that we can re-run the same suite and have the same liquidity footprint across multiple runs.

Maybe can make an issue/todo for this and introduce it in a later PR.

Yeah, I've actually come full circle now and started taking apart the HTLC force close test in #1017 and making two out of them. That helps with starting with a clean slate.
And the performance optimizations I have in #987 should make it so that there is no additional overhead for running things in the same test vs. in separate ones.
So this is okay the way it is now.

GeorgeTsagk · 2025-03-31T20:15:39Z

unified the helpers / itests commit to make things simpler

guggero

LGTM pending nits and update to the actual merged commit of tapd.

itest/litd_custom_channels_test.go

guggero · 2025-04-03T15:48:56Z

itest/litd_custom_channels_test.go

+
+	// Run liquidity edge cases and only use group keys for invoices and
+	// payments.
+	testCustomChannelsLiquidtyEdgeCasesCore(ctx, net, t, true)


Yeah, I've actually come full circle now and started taking apart the HTLC force close test in #1017 and making two out of them. That helps with starting with a clean slate.
And the performance optimizations I have in #987 should make it so that there is no additional overhead for running things in the same test vs. in separate ones.
So this is okay the way it is now.

We add support for group keys on payments and invoices across the itests. This includes all the required helpers and optional arguments to make adding invoices and sending payments group-key enabled. In order to run the liquidity edge cases twice, once using only asset IDs and once using only group keys, we need to extract the main logic into a re-usable function that has the group key option on the top level. If the group key flag is set, then we append the respective group key opt to the payment & invoice related calls.

lightninglabs-deploy · 2025-04-10T16:09:25Z

@Roasbeef: review reminder

GeorgeTsagk self-assigned this Mar 5, 2025

GeorgeTsagk mentioned this pull request Mar 5, 2025

Support group keys on SendPayment & AddInvoice lightninglabs/taproot-assets#1423

Merged

Roasbeef reviewed Mar 6, 2025

View reviewed changes

guggero self-requested a review March 6, 2025 11:08

levmi added this to Taproot-Assets Project Board Mar 6, 2025

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

levmi moved this from 🆕 New to 🏗 In progress in Taproot-Assets Project Board Mar 6, 2025

guggero reviewed Mar 7, 2025

View reviewed changes

GeorgeTsagk force-pushed the tapd-groupkeys branch from 2d7defe to 072cc37 Compare March 19, 2025 14:04

GeorgeTsagk requested review from Roasbeef and guggero March 19, 2025 14:07

GeorgeTsagk force-pushed the tapd-groupkeys branch from 072cc37 to e75169c Compare March 26, 2025 17:28

GeorgeTsagk force-pushed the tapd-groupkeys branch from e75169c to 673009d Compare March 27, 2025 10:20

guggero reviewed Mar 27, 2025

View reviewed changes

GeorgeTsagk force-pushed the tapd-groupkeys branch from 673009d to da3cca1 Compare March 31, 2025 20:13

GeorgeTsagk requested a review from guggero April 1, 2025 14:34

guggero approved these changes Apr 3, 2025

View reviewed changes

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

GeorgeTsagk added 2 commits April 4, 2025 15:19

(temp) build: bump tapd

b9d0cd9

GeorgeTsagk force-pushed the tapd-groupkeys branch from da3cca1 to 9f885cc Compare April 4, 2025 13:19

levmi requested review from ffranr and removed request for Roasbeef April 15, 2025 18:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom channels itest: Use group keys on some payments & invoices #995

Custom channels itest: Use group keys on some payments & invoices #995

GeorgeTsagk commented Mar 5, 2025 •

edited

Loading

Roasbeef left a comment

Roasbeef Mar 6, 2025

guggero Mar 7, 2025

GeorgeTsagk commented Mar 7, 2025

guggero left a comment

guggero Mar 7, 2025

GeorgeTsagk commented Mar 19, 2025

GeorgeTsagk commented Mar 26, 2025

GeorgeTsagk commented Mar 27, 2025

guggero left a comment

guggero Mar 27, 2025

GeorgeTsagk Mar 31, 2025

GeorgeTsagk Mar 31, 2025

guggero Mar 27, 2025

GeorgeTsagk Mar 31, 2025

guggero Mar 27, 2025

GeorgeTsagk Mar 31, 2025

guggero Apr 3, 2025

GeorgeTsagk commented Mar 31, 2025

guggero left a comment

guggero Apr 3, 2025

lightninglabs-deploy commented Apr 10, 2025

		@@ -1896,10 +1918,13 @@ func testCustomChannelsLiquidityEdgeCases(ctx context.Context,

		logBalance(t.t, nodes, assetID, "10k sats")

		opts = []payOpt{}

Custom channels itest: Use group keys on some payments & invoices #995

Are you sure you want to change the base?

Custom channels itest: Use group keys on some payments & invoices #995

Conversation

GeorgeTsagk commented Mar 5, 2025 • edited Loading

Description

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeorgeTsagk commented Mar 7, 2025

guggero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeorgeTsagk commented Mar 19, 2025

GeorgeTsagk commented Mar 26, 2025

GeorgeTsagk commented Mar 27, 2025

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

GeorgeTsagk commented Mar 31, 2025

guggero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lightninglabs-deploy commented Apr 10, 2025

GeorgeTsagk commented Mar 5, 2025 •

edited

Loading