Enhance `update_add_htlc` with remote peer's custom records #8660

GeorgeTsagk · 2024-04-17T19:03:16Z

Description

Based on #8633

This PR enhances the update_add_htlc message with a "custom records" field which may contain custom records that are meant to be transmitted to the direct peer.

This extra field is included in routerrpc.SendPaymentRequest and is also exposed in the forward interceptor

Closes #8618

coderabbitai · 2024-04-17T19:03:22Z

Important

Review Skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

Roasbeef · 2024-04-17T22:17:01Z

lnrpc/routerrpc/forward_interceptor.go

@@ -93,6 +93,7 @@ func (r *forwardInterceptor) onIntercept(
 		CustomRecords:           htlc.CustomRecords,
 		OnionBlob:               htlc.OnionBlob[:],
 		AutoFailHeight:          htlc.AutoFailHeight,
+		WireCustomRecords:       htlc.CustomPeerRecords,


I think this should just be htlc.ExtraData. This is everything else left over after we read all the current hard wired fields. Start on the latter commits, so this may have been something added elsewhere to manipulate the map version?

also CustomPeerRecords doesn't exist yet

I think this should just be htlc.ExtraData. This is everything else left over after we read all the current hard wired fields. Start on the latter commits, so this may have been something added elsewhere to manipulate the map version?

Don't really get the idea here. Do we want to never decode ExtraData and hand it over the RPC or do we want to decode it, populate fields that we understand, and then hand over the leftovers?

The latter doesn't really make sense to me, and if we want to stick with something closer to the former we're off-loading tlv related decoding responsibilities to the user, which doesn't really make sense? i.e they would have to know which key is for blinding point or custom records etc

also CustomPeerRecords doesn't exist yet

Probs lost in the rebase, will address

lnwallet/channel.go

htlcswitch/interceptable_switch.go

Crypt-iQ

first pass done, need to go over it again though

lnrpc/routerrpc/forward_interceptor.go

lnrpc/routerrpc/router_backend.go

routing/router.go

Crypt-iQ · 2024-04-23T19:51:48Z

lnrpc/routerrpc/forward_interceptor.go

@@ -93,6 +93,7 @@ func (r *forwardInterceptor) onIntercept(
 		CustomRecords:           htlc.CustomRecords,
 		OnionBlob:               htlc.OnionBlob[:],
 		AutoFailHeight:          htlc.AutoFailHeight,
+		WireCustomRecords:       htlc.CustomPeerRecords,


also CustomPeerRecords doesn't exist yet

routing/router.go

lnrpc/routerrpc/router.proto

htlcswitch/interceptable_switch.go

htlcswitch/link.go

lnwallet/channel.go

lightninglabs-deploy · 2024-05-15T04:04:27Z

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

dstadulis · 2024-05-15T14:11:27Z

Fixes lightninglabs/taproot-assets#897

guggero

Looks pretty good! Might need a rebase on top of @ffranr's latest state of #8633 (not sure if the unit test failures come from there or from this PR).

channeldb/channel.go

lnwallet/channel.go

lnwire/custom_records.go

carlaKC

First pass, a few high level comments:

Already noted by other reviewers, but restart flows need some more attention here (both including the custom fields and ensuring we always have this data persisted)
Meta/personal preference: move itest to the end of the PR rather than having several iterations of it throughout the code
One consideration that this PR introduces is that we're now going to store all the custom records that we're send with a HTLC, which could result in our storing a whole lot of junk that our peers send us - this was originally discussed when we added the update that allows us to store this data. I originally thought that a good way to approach this would be to allow users to specify the custom TLV values they want to store (or hardcode some notable ones that we want to keep by default), but I haven't fully thought that design through tbh.

itest/lnd_forward_interceptor_test.go

carlaKC · 2024-05-20T12:32:23Z

itest/lnd_forward_interceptor_test.go

+			65537: []byte("test"),
+		},
+	}
+	stream := alice.RPC.SendPayment(sendReq)


Would be good to test across restarts as well to get coverage for this comment

Added this to the tracking issue.

htlcswitch/interfaces.go

itest/list_on_test.go

carlaKC · 2024-05-20T13:24:03Z

lnwallet/channel.go

@@ -894,8 +931,12 @@ func (lc *LightningChannel) diskHtlcToPayDesc(feeRate chainfee.SatPerKWeight,

 	// Ensure that we'll restore any custom records which were stored as
 	// extra data on disk.
-	if len(htlc.ExtraData) != 0 {
-		pd.CustomRecords = fn.Some[tlv.Blob](htlc.ExtraData)
+	if len(htlc.CustomRecords) != 0 {


While we're setting the CustomRecords values above, we're not actually persisting them using the workaround added in #7710, which means that in certain restart cases we won't restore these records (diff to test db restore here).

So as-is we'll always have htlc.CustomRecords=0 because we're not persisting them with the channeldb.HTLC that needs to be turned into a PaymentDescriptor here.

The restart flow always takes me forever to reload into my brain, tried to write it all down here if that's at all helpful.

Right, persistence is missing atm.

So I'm not sure if the best way forward is persisting this as part of channeldb.HTLC or channeldb.PaymentCreationInfo as suggested here

So I'm not sure if the best way forward is persisting this as part of channeldb.HTLC or channeldb.PaymentCreationInfo as suggested #8660 (comment)

We need persistence in channeldb.HTLC so that the node that receives the incoming HTLC with custom TLVs is robust against restarts:

HTLC is irrevocably committed on the channel

Receiving node restarts before forwarding through the switch

HTLC is restored from disk (no custom records)

Interceptor receives HTLC without custom records

Added this to the tracking issue.

lnwallet/channel.go

carlaKC · 2024-05-20T19:01:45Z

htlcswitch/link.go

+						return err
+					}
+
+					addMsg.CustomRecords = r


Do we want to be overloading the CustomRecords field here?

pd.CustomRecords are the records we received on the incoming HTLC's update_add_htlc

addMsg.CustomRecords are the records that we're going to pack for the outgoing HTLC's update_add_htlc

This has the impact of always relaying all of the custom TLVs that we get on the incoming update_add_htlc to the outgoing update_add_htlc - confirmed this with a quick itest hack - and I don't see anything in the original issue saying that we want to do this?

nice catch
Agree, we don't want this side effect of naively relaying everything

I believe we actually want to block any type of relaying. These records should be propagated all the way until the interception point, where the user may decide if any outgoing htlc wire records will be defined

Assumed not, can prob easily fix by just adding a separate field to the intercepted packet.

I think this was actually introduced in a previous PR (#8633) that added the interceptor logic. I'll address this in a later PR, added it to the tracking issue for now: #8769
We really are blocked by this PR (which is just one piece in a long series of PRs), so I'll want to merge this into the staging branch asap and then address any remaining issues later.

The previous PR is correctly using this field to ResumeModified with CustomRecords that are provided by the interceptor.

Here, we're overloading it with the incoming update_add_htlc's CustomRecords (which introduces the issue). The fix is pretty simple - just add a field to htlcPacket and surface it separately on the interceptor?

Patch to fix is trivial, messy test to confirm that it solves the problem.

Ah, was looking at it the wrong way around... Thanks a lot for the patch, happily took it in 🙏

Thanks a lot for the patch

❤️ it needs gofmt :')

lnwire/custom_records.go

carlaKC · 2024-05-22T12:15:54Z

channeldb/channel.go

@@ -2568,6 +2574,11 @@ func (h *HTLC) serializeExtraData() error {
 		records = append(records, &b)
 	})

+	records, err := h.CustomRecords.ExtendRecordProducers(records)


Add test coverage for serialization change?

Added that to the tracking issue.

carlaKC · 2024-05-22T12:28:14Z

htlcswitch/link.go

+						return err
+					}
+
+					addMsg.CustomRecords = r


Patch to fix is trivial, messy test to confirm that it solves the problem.

itest/lnd_forward_interceptor_test.go

guggero

LGTM 🎉
Any remaining cleanups/unit tests are tracked here: #8769.

Merging to staging branch to unblock other work.

GeorgeTsagk self-assigned this Apr 17, 2024

Roasbeef reviewed Apr 17, 2024

View reviewed changes

htlcswitch/interceptable_switch.go Outdated Show resolved Hide resolved

GeorgeTsagk mentioned this pull request Apr 23, 2024

LightningPayment first hop custom records #8656

Closed

Crypt-iQ reviewed Apr 23, 2024

View reviewed changes

Roasbeef requested changes Apr 24, 2024

View reviewed changes

GeorgeTsagk force-pushed the interceptor-wire-records branch from 833c121 to ffb0718 Compare May 2, 2024 14:49

GeorgeTsagk changed the base branch from master to custom-channels-integration-rpc-channel May 2, 2024 14:50

GeorgeTsagk force-pushed the interceptor-wire-records branch 2 times, most recently from e9bd133 to 5203776 Compare May 2, 2024 18:02

GeorgeTsagk mentioned this pull request May 2, 2024

routing: add TlvTrafficShaper to bandwidth hints #8665

Merged

GeorgeTsagk force-pushed the interceptor-wire-records branch from 5203776 to a3033a9 Compare May 2, 2024 18:48

guggero changed the base branch from custom-channels-integration-rpc-channel to custom-channels-integration May 2, 2024 18:54

guggero force-pushed the custom-channels-integration branch from 15abb14 to 09b34f1 Compare May 14, 2024 07:49

dstadulis mentioned this pull request May 15, 2024

[tracking]: Add interceptor access to update_add_htlc remote peer's custom records lightninglabs/taproot-assets#897

Closed

GeorgeTsagk force-pushed the interceptor-wire-records branch 2 times, most recently from ac58e73 to 3163902 Compare May 15, 2024 14:53

GeorgeTsagk changed the base branch from custom-channels-integration to 0-19-staging May 15, 2024 14:54

GeorgeTsagk changed the title ~~Add interceptor access to update_add_htlc remote peer's custom records~~ Enhance update_add_htlc with remote peer's custom records May 15, 2024

GeorgeTsagk force-pushed the interceptor-wire-records branch from 3163902 to ff58154 Compare May 15, 2024 15:50

guggero reviewed May 16, 2024

View reviewed changes

channeldb/channel.go Show resolved Hide resolved

lnwallet/channel.go Show resolved Hide resolved

lnwallet/channel.go Outdated Show resolved Hide resolved

lnwallet/channel.go Show resolved Hide resolved

lnwire/custom_records.go Outdated Show resolved Hide resolved

dstadulis mentioned this pull request May 16, 2024

routing+rpc: enable senders to set a custom first-hop TLV blob for update_add_htlc #8618

Closed

3 tasks

saubyk linked an issue May 16, 2024 that may be closed by this pull request

routing+rpc: enable senders to set a custom first-hop TLV blob for update_add_htlc #8618

Closed

3 tasks

GeorgeTsagk force-pushed the interceptor-wire-records branch from ff58154 to ce2a005 Compare May 17, 2024 17:09

carlaKC reviewed May 20, 2024

View reviewed changes

lnwallet/channel.go Show resolved Hide resolved

carlaKC reviewed May 20, 2024

View reviewed changes

guggero mentioned this pull request May 21, 2024

[3/5]: multi: add new AuxFundingController for custom external funding flows #8622

Merged

GeorgeTsagk force-pushed the interceptor-wire-records branch from ce2a005 to b8cc123 Compare May 21, 2024 11:19

guggero mentioned this pull request May 22, 2024

tracking: remaining cleanup to address before merging 0-19-staging branch into master #8769

Open

GeorgeTsagk force-pushed the interceptor-wire-records branch from b8cc123 to a1677d7 Compare May 22, 2024 11:31

GeorgeTsagk requested a review from guggero May 22, 2024 11:33

GeorgeTsagk force-pushed the interceptor-wire-records branch from a1677d7 to 702f860 Compare May 22, 2024 12:10

carlaKC reviewed May 22, 2024

View reviewed changes

GeorgeTsagk added 3 commits May 22, 2024 14:44

multi: use wire records on payment and intercept flows

25b4066

lnrpc: add wire records fields to SendPayment & HtlcInterceptor

a5fcf33

itest: add interceptor wire records access test

1b1969b

guggero force-pushed the interceptor-wire-records branch from 702f860 to 1b1969b Compare May 22, 2024 12:58

carlaKC mentioned this pull request May 22, 2024

Add Experimental Endorsement Signalling #8390

Open

guggero approved these changes May 22, 2024

View reviewed changes

guggero merged commit 966f41f into lightningnetwork:0-19-staging May 22, 2024
22 of 27 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance `update_add_htlc` with remote peer's custom records #8660

Enhance `update_add_htlc` with remote peer's custom records #8660

GeorgeTsagk commented Apr 17, 2024 •

edited

coderabbitai bot commented Apr 17, 2024 •

edited

Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

Documentation and Community

Roasbeef Apr 17, 2024

Crypt-iQ Apr 23, 2024

GeorgeTsagk May 21, 2024

Crypt-iQ left a comment

Crypt-iQ Apr 23, 2024

lightninglabs-deploy commented May 15, 2024

dstadulis commented May 15, 2024

guggero left a comment

carlaKC left a comment

carlaKC May 20, 2024

guggero May 22, 2024

carlaKC May 20, 2024 •

edited

GeorgeTsagk May 21, 2024

carlaKC May 21, 2024

guggero May 22, 2024

carlaKC May 20, 2024

GeorgeTsagk May 21, 2024

carlaKC May 21, 2024

guggero May 22, 2024 •

edited

carlaKC May 22, 2024

carlaKC May 22, 2024

guggero May 22, 2024

carlaKC May 22, 2024

carlaKC May 22, 2024

guggero May 22, 2024

carlaKC May 22, 2024

guggero left a comment

Enhance update_add_htlc with remote peer's custom records #8660

Enhance update_add_htlc with remote peer's custom records #8660

Conversation

GeorgeTsagk commented Apr 17, 2024 • edited

Description

coderabbitai bot commented Apr 17, 2024 • edited

Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

Documentation and Community

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Crypt-iQ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lightninglabs-deploy commented May 15, 2024

dstadulis commented May 15, 2024

guggero left a comment

Choose a reason for hiding this comment

carlaKC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlaKC May 20, 2024 • edited

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 May 22, 2024 • edited

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

Enhance `update_add_htlc` with remote peer's custom records #8660

Enhance `update_add_htlc` with remote peer's custom records #8660

GeorgeTsagk commented Apr 17, 2024 •

edited

coderabbitai bot commented Apr 17, 2024 •

edited

CodeRabbit Configration File (`.coderabbit.yaml`)

carlaKC May 20, 2024 •

edited

guggero May 22, 2024 •

edited