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

Auto 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

Roasbeef · 2024-04-17T22:18:34Z

lnwallet/channel.go

+
+ // WireRecordsBlob contains the TLV records blob that was included in
+ // the original wire message that added this HTLC.
+ WireRecordsBlob lnwire.CustomRecordsBlob


So I have something of this logic in: 99db910

Later commits then ensure that this is threaded all the way down to disk. This takes an approach of just accepting the blob, instead of the wire record map.

Should attempt to rebase over that. That also handles all the accounting for restart/retransmission logic not in this PR.

htlcswitch/interceptable_switch.go

Crypt-iQ

first pass done, need to go over it again though

Crypt-iQ · 2024-04-22T17:19:52Z

lnrpc/routerrpc/forward_interceptor.go

@@ -119,6 +121,24 @@ func (r *forwardInterceptor) resolveFromClient(
 Action: htlcswitch.FwdActionResume,
 })

+ case ResolveHoldForwardAction_RESUME_MODIFIED:
+ // Modify HTLC and resume forward.
+ outgoingAmtMsat := lnwire.MilliSatoshi(in.OutgoingAmountMsat)


I think this value always needs to be set o/w it will set the outgoing amt to 0?

Crypt-iQ · 2024-04-23T19:42:25Z

lnrpc/routerrpc/router_backend.go

+ if err := firstHopRecords.Validate(); err != nil {
+ return nil, err
+ }
+ payIntent.FirstHopCustomRecords = firstHopRecords


FirstHopCustomRecords introduced in a later commit so this commit won't compile

Crypt-iQ · 2024-04-23T19:47:13Z

routing/router.go

@@ -713,7 +713,7 @@ func (r *ChannelRouter) Start() error {
 // be tried.
 _, _, err := r.sendPayment(
 0, payment.Info.PaymentIdentifier, 0,
- paySession, shardTracker,
+ paySession, shardTracker, nil,


If we resume the payment, shouldn't we include the TLVs?

Yeah, we want to include this information on resume.

I think we want to add this to channeldb.PaymentCreationInfo as a new optional TLV field. We store the bytes in its own key, so no issue w.r.t migrations or anything.

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

Roasbeef · 2024-04-24T02:42:08Z

routing/router.go

@@ -713,7 +713,7 @@ func (r *ChannelRouter) Start() error {
 // be tried.
 _, _, err := r.sendPayment(
 0, payment.Info.PaymentIdentifier, 0,
- paySession, shardTracker,
+ paySession, shardTracker, nil,


Yeah, we want to include this information on resume.

I think we want to add this to channeldb.PaymentCreationInfo as a new optional TLV field. We store the bytes in its own key, so no issue w.r.t migrations or anything.

Roasbeef · 2024-04-24T02:51:38Z

lnrpc/routerrpc/router.proto

@@ -963,6 +963,9 @@ message ForwardHtlcInterceptRequest {
 // The block height at which this htlc will be auto-failed to prevent the
 // channel from force-closing.
 int32 auto_fail_height = 10;
+
+ // The custom records of the peer's wire message.
+ map<uint64, bytes> wire_custom_records = 11;


To make this explicit, I think we should name the field: incoming_htlc_custom_wire_records.

Roasbeef · 2024-04-24T02:52:17Z

htlcswitch/interceptable_switch.go

@@ -486,6 +486,21 @@ func (s *InterceptableSwitch) interceptForward(packet *htlcPacket,
 return false, nil
 }

+ var customRecordsBlob []byte
+
+ htlc.CustomRecordsBlob.WhenSome(


Other comment re ExtraData also applies here. Will only note it here to avoid duplicate comments.

Roasbeef · 2024-04-24T02:53:20Z

htlcswitch/interceptable_switch.go

+ )
+
+ customPeerRecords := new(record.CustomSet)
+ customPeerRecords.Decode(bytes.NewReader(customRecordsBlob))


Should propagate the decode error. IIUC, that can fail if the incoming wire records aren't canonical for w/e reason.

Roasbeef · 2024-04-24T02:53:32Z

htlcswitch/interceptable_switch.go

+ customPeerRecords := new(record.CustomSet)
+ customPeerRecords.Decode(bytes.NewReader(customRecordsBlob))
+
+ packet.wireRecords = *customPeerRecords


incomingWireRecords?

Roasbeef · 2024-04-24T02:53:59Z

htlcswitch/interceptable_switch.go

+ CustomRecords: f.packet.customRecords,
+ OnionBlob: f.htlc.OnionBlob,
+ AutoFailHeight: f.autoFailHeight,
+ CustomPeerRecords: f.packet.wireRecords,


Should add the Incoming prefix.

Roasbeef · 2024-04-24T02:55:06Z

htlcswitch/link.go

+ Amount: fwdInfo.AmountToForward,
+ PaymentHash: pd.RHash,
+ BlindingPoint: fwdInfo.NextBlinding,
+ CustomRecordsBlob: pd.WireRecordsBlob,


Hmm, I don't think we want to store this here. Instead, we should add it only to the htlcPacket field we generate. This'll transport the wire TLV records of the incoming HTLC. But here we've already modified the outgoing HTLC to have these values (the interceptor might wnat them to be different, or they shouldn't propagate to this hop as well).

Roasbeef · 2024-04-24T02:55:29Z

htlcswitch/link.go

+ Amount: fwdInfo.AmountToForward,
+ PaymentHash: pd.RHash,
+ BlindingPoint: fwdInfo.NextBlinding,
+ CustomRecordsBlob: pd.WireRecordsBlob,


Same here re only adding it to the htlcPacket.

Roasbeef · 2024-04-24T02:56:12Z

lnwallet/channel.go

+
+ // WireRecordsBlob contains the TLV records blob that was included in
+ // the original wire message that added this HTLC.
+ WireRecordsBlob lnwire.CustomRecordsBlob


Should attempt to rebase over that. That also handles all the accounting for restart/retransmission logic not in this PR.

Roasbeef · 2024-04-24T02:56:32Z

lnwallet/channel.go

+ HtlcIndex: lc.remoteUpdateLog.htlcCounter,
+ OnionBlob: htlc.OnionBlob[:],
+ BlindingPoint: htlc.BlindingPoint,
+ WireRecordsBlob: htlc.CustomRecordsBlob,


ExtraData see: a0f19a5#r1569611486

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).

guggero · 2024-05-16T11:18:05Z

channeldb/channel.go

@@ -2550,6 +2550,8 @@ type HTLC struct {
 // HTLC. It is stored in the ExtraData field, which is used to store
 // a TLV stream of additional information associated with the HTLC.
 BlindingPoint lnwire.BlindingPointRecord
+
+ CustomRecords lnwire.CustomRecords


nit: missing Godoc comment. And maybe we should mention here that this will be encoded into the ExtraData alongside the BlindingPoint when encoded (for sending on the wire or storing to the DB).

guggero · 2024-05-16T11:24:22Z

lnwallet/channel.go

@@ -3924,7 +4014,8 @@ func (lc *LightningChannel) createCommitDiff(newCommit *commitment,

 // getUnsignedAckedUpdates returns all remote log updates that we haven't
 // signed for yet ourselves.
-func (lc *LightningChannel) getUnsignedAckedUpdates() []channeldb.LogUpdate {
+func (lc *LightningChannel) getUnsignedAckedUpdates() ([]channeldb.LogUpdate,
+ error) {


nit: add newline after multi-line method definition.

guggero · 2024-05-16T11:24:48Z

lnwallet/channel.go

 auxLeaves, err := AuxLeavesFromCommit(
- chanState, *remoteCommitView.toDiskCommit(false), leafStore,
+ chanState, *diskCommit, leafStore,


nit: can all fit on a single line now (same further below).

guggero · 2024-05-16T11:26:28Z

lnwallet/channel.go


 lc.Lock()
 defer lc.Unlock()

- pd := lc.htlcAddDescriptor(htlc, openKey)
- if err := lc.validateAddHtlc(pd, buffer); err != nil {
+ pd, err := lc.htlcAddDescriptor(htlc, openKey)


Looks like we dropped the call to lc.validateAddHtlc() here. Perhaps by accident? Sounds like that's still needed.

guggero · 2024-05-16T11:28:10Z

lnwire/custom_records.go

@@ -14,6 +14,26 @@ const (
 MinCustomRecordsTlvType = 65536
 )

+// ParseCustomRecords decodes a set of custom records from a byte slice.
+func ParseCustomRecords(b tlv.Blob) (CustomRecords, error) {


Looks like this ended up a duplicate of NewCustomRecordsFromTlvBlob below. So can just use that.
Or maybe rename NewCustomRecordsFromTlvBlob to ParseCustomRecords which is way shorter.

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.

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

itest/lnd_forward_interceptor_test.go

+ dbgInfo, err := bob.RPC.LN.GetDebugInfo(
+ ctx, &lnrpc.GetDebugInfoRequest{},
+ )
+ require.NoError(ht, err, "failed to get Barol node debug info")


:s/Barol/Carol

carlaKC · 2024-05-20T12:31:33Z

itest/lnd_forward_interceptor_test.go

+// testForwardInterceptorFirstHopRecords
+func testForwardInterceptorFirstHopRecords(ht *lntest.HarnessTest) {


nit: needs godoc

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

carlaKC · 2024-05-20T12:47:05Z

htlcswitch/interfaces.go

@@ -357,6 +357,10 @@ type InterceptedPacket struct {
 // OnionBlob is the onion packet for the next hop
 OnionBlob [lnwire.OnionPacketSize]byte

+ // WireCustomRecords are user-defined records that were defined by the
+ // peer that forwarded this htlc to us.
+ WireCustomRecords record.CustomSet


This name is a little ambiguous to me (since everything arrives on the wire), WDYT of IncomingAddCustomRecords - a little longer but it's clearer about where these records come from, when I read the name without seeing the godoc for the field it's unclear to me whether these are from update_add_htlc or the htlc's payload.

carlaKC · 2024-05-20T12:51:39Z

itest/list_on_test.go

@@ -426,6 +426,10 @@ var allTestCases = []*lntest.TestCase{
 Name: "forward interceptor modified htlc",
 TestFunc: testForwardInterceptorModifiedHtlc,
 },
+ {
+ Name: "forward interceptor first hop records",


structural comment: seems to me that it would make sense to move this itest commit to the end of the PR so that we don't have to have two iterations of it (one here, one adding coverage using the interceptor later)

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.

carlaKC · 2024-05-20T14:17:32Z

lnwallet/channel.go

@@ -6416,6 +6570,16 @@ func (lc *LightningChannel) ReceiveHTLC(htlc *lnwire.UpdateAddHTLC) (uint64, err
 BlindingPoint: htlc.BlindingPoint,
 }

+ if len(htlc.CustomRecords) > 0 {


If ExtraData contains a protocol-range TLV that isn't blinding point, all of our htlc.CustomRecords will be overwritten because we'll hit ExtraData != nil?

While we're not expecting this (there aren't any other defined protocol TLVs for update_add_htlc right now), I think we should account for the possibility that other nodes in the network may relay these fields in the future because as-is it'll break our code.

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?

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

Open

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

Open

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

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

Open

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

Open

3 tasks

GeorgeTsagk added 4 commits May 17, 2024 13:27

lnrpc: add first hop records field to SendPayment

f5e6d34

routing: encode first hop records in update_add_htlc message

4badde6

itest: add first hop records interceptor test

771b624

lnrpc: add wire records to intercept request

3ee61e9

GeorgeTsagk added 2 commits May 17, 2024 13:27

htlcswitch+lnwallet: persist peer custom records

9f15cb3

itest: add interceptor wire records access test

ce2a005

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

		// testForwardInterceptorFirstHopRecords
		func testForwardInterceptorFirstHopRecords(ht *lntest.HarnessTest) {

Enhance update_add_htlc with remote peer's custom records #8660

Are you sure you want to change the base?

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

Auto 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

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

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

lightninglabs-deploy commented May 15, 2024

dstadulis commented May 15, 2024

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

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

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

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

CodeRabbit Configration File (`.coderabbit.yaml`)

carlaKC May 20, 2024 •

edited