[1/3]: Preparatory work for Forwarding Blinded Routes #8159

carlaKC · 2023-11-08T19:42:10Z

This PR is the first in a series to add the ability for LND to forward payments to blinded routes.

It contains a collection of prep work to get the codebase into shape for forwarding functionality:

Add encoding/decoding and validation for encrypted_data that is provided for blinded forwards.
Add blinding_point to update_add_htlc and persist across restarts.
Small refactors to prepare for decoding onion payloads with blinding points.

Note: the ability to generate/receive payments via blinded routes is out of scope for these PRs.

ellemouton

Very nice & clean! First pass done. Basically LGTM . Main comment is re potentially using the new tlv.RecordT and tlv.OptionalRecordT types for the various new tlv fields.

record/blinded_data.go

htlcswitch/hop/payload.go

lnwire/update_add_htlc.go

lnwallet/channel.go

coderabbitai · 2024-01-25T14:31:41Z

Important

Auto Review Skipped

Auto reviews are limited to the following labels: llm-review. Please add one of these labels to enable auto reviews.

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.

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-tests 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 tests 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 tests.
- @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.
The JSON schema for the configuration file is available here.
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/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

carlaKC · 2024-01-25T14:31:59Z

Just pushed rebase + small fixups, looking into moving TLVs over to generics!

lightninglabs-deploy · 2024-02-08T15:48:21Z

@bitromortac: review reminder
@carlaKC, remember to re-request review from reviewers when ready

ellemouton · 2024-02-14T13:10:32Z

Just following up on my previous comment:

I personally think it's fine for us to move ahead with the current structure of the new TLV fields. Better to stick to the current "standard" way and then leave the big re-hall of all TLV fields to another PR. Else we will need to reason about 2 different ways of doing the same thing.

Let me know if you agree with this @carlaKC - then i'll re-request myself here for final review which I will prio asap 🚀

carlaKC · 2024-02-14T14:06:28Z

I personally think it's fine for us to move ahead with the current structure of the new TLV fields. Better to stick to the current "standard" way and then leave the big re-hall of all TLV fields to another PR. Else we will need to reason about 2 different ways of doing the same thing.

Started working on switching over to generics here, and I agree that It may not be worth trying to make this jump now. From what I can tell generics work isn't totally there, so it's a bit patchwork adding it in this PR IMO.

Let me know if you agree with this @carlaKC - then i'll re-request myself here for final review which I will prio asap 🚀

I'll rebase then re-request you, thanks!!

Roasbeef · 2024-03-18T15:08:42Z

seems like we have so far only used the generics in extension TLVs (not native TLV structs like) so there aren't regular tlv.Stream utilities? Everything needs to be converted to ExtraOpaqueData and then packed. Not a lot of code, but is this how we want to handle things?

Nope, you can just serialize into a normal blob. Look at this commit where I encoded things into a normal blob: 784d236. It's just like before, you make a stream form the records, but now the attributes themselves are records.

while we have RecordT for primitive types, there don't seem to be the same for non-primitive TLVs? Hit this with feature vectors in this PR: we can't implement Record because features are encoded with different tlv types in different contexts, and there isn't a simple way to use generics without having implemented RecordProducer, so we still need boilerplate.

You can make it from anything that implements record producer:

lnd/tlv/record_type.go

Lines 34 to 44 in b0552da

    
           // NewRecordT creates a new RecordT type from a given RecordProducer type. This 
        
           // is useful to wrap a given record in this utility type, which also serves as 
        
           // an extra type annotation. The underlying type of the record is retained. 
        
           func NewRecordT[T TlvType, K any, V RecordProducerT[K]]( 
        
           	record K, 
        
           ) RecordT[T, K] { 
        
           	return RecordT[T, K]{ 
        
           		Val: record, 
        
           	} 
        
           }

. So for this, we'd have a single struct that wraps the feature vector to implement the Record() method, then that single struct can be re-used across any other attributes.

Adapting Option: handling optional records is quite clumsy? In rust, you can just if let Some(A) to access the inner value when it is present and remain in the scope of your current function. With the existing Unwrap utilities we're always using closures to do simple things like validateSome(A) if it's non-nil.

It's what we have right now, but doesn't at all appear to be a fundamental barrier. You can either revert back to using plain pointers, then be extra diligent about nil checks, use the existing utilities, or make your own along the way to mske things more egonomic. You can use WhenSome for when you want to run a chunk of logic only if the thing is there, the Map functions for when you want to have a return value, and the other hard checks if you want to force an error if something is non-nil UnwrapOrErr. The latter is pretty much the same amount of lines as a ptr != nil check.

In short, nothing you've mentioned appear to be a significant barrier. Particularly given that many of PRs and newly introduce sub-systems have started to use the new TLV records. I've pointed out areas where we can delete entire files, and also make nil checks more explicit, to avoid a dreaded HTLC of death panic error, or other DOS issues. I really don't see any reason for switching over in this PR. In the amount of back and forth, you likely could've just made the routine API switch. Compared to everything else in this PR, it's rather minor, and works to make TLV handling more uniform, and also less error prone (forgot to do a nil check for optional TLV, causing a panic).

carlaKC · 2024-03-18T20:58:39Z

In the amount of back and forth, you likely could've just made the routine API switch.

While I absolutely could have updated the PR to use generics without full confidence that it was the right decision as the author, I don't really think that's how we want to be writing code. I appreciate the time that you and @ellemouton have taken to point out where I've been wrong about my concerns, and have throughout this discussion made a good faith attempt to maintain a version that uses generics that I feel this comment conveniently handwaves away.

I still have my reservations about directly porting rust primitives over to golang implementations, but I recognize that I should have reviewed the PRs in question if I wanted a vote. I have switched the series over to use generics - thanks in advance to reviewers for the extra round / apologies for making it necessary.

ellemouton

Update to generics LGTM! Just left one suggestion that would reduce the number of lines changed a bit 👍

record/blinded_data.go

lnwire/features.go

lnwire/lnwire_test.go

lnwire/update_add_htlc.go

channeldb/channel.go

Roasbeef · 2024-03-21T18:41:16Z

lnwire/update_add_htlc.go

@@ -78,6 +78,19 @@ type UpdateAddHTLC struct {
 	ExtraData ExtraOpaqueData
 }

+// BlindingPointOrNil returns the blinding point associated with the update, or
+// nil.
+func (c *UpdateAddHTLC) BlindingPointOrNil() *btcec.PublicKey {


This can be simplified to c.BlindingPoint.UnwrapOr(nil).

cannot use nil as tlv.RecordT[*tlv.tlvType0, *secp256k1.PublicKey] value in argument to htlc.BlindingPoint.UnwrapOr?

iiuc we need two layers of unwrapping here to get the underlying type in the record rather than the type in the option?

Ah sure, had just glanced at that. Can also use WhenSomeV instead here to avoid needing to deref Val, but non-blocking ofc.

Philosophical point, but generally I like to review the option as sort of lazy evaluation, so you pipe it down all the way until you actually need to obtain the true value. Otherwise, by forcing evaluation sooner than needed, you just end up burdening the caller once again with required nil checks.

We'll need to pack feature vectors for route blinding, so we pull the encoding/decoding out into separate functions (currently contained in ChannelType). Though it's more lines of code, we keep most of the ChannelType assertions so that we strictly enforce use of the alias.

This commit adds encoding and decoding for blinded route data blobs. TLV fields such as path_id (which are only used for the final hop) are omitted to minimize the change size.

Co-authored-by: Calvin Zachman <calvin.zachman@protonmail.com>

Add blinding points to update_add_htlc. This TLV will be set for nodes that are relaying payments in blinded routes that are _not_ the introduction node.

This commit adds an optional blinding point to payment descriptors and persists them in our HTLC's extra data. A get/set pattern is used to populate the ExtraData on our disk representation of the HTLC so that callers do not need to worry about the underlying storage detail.

When we have payments inside of a blinded route, we need to know the incoming amount to be able to back-calculate the amount that we need to forward using the forwarding parameters provided in the blinded route encrypted data. This commit adds the payment amount to our DecodeHopIteratorRequest so that it can be threaded down to payment forwarding information creation in later commits.

This field is incorrectly suffixed as "msat", when it is actually interpreted as the proportional fee rate. This is the value that we should be using because the sender will calculate proportional fees accordingly. This is a breaking change to the RPC, but on an experimental and unreleased API.

Roasbeef

LGTM 🦋

Nice work!

Left some non-blocking comments re slightly different wayts to use some of the new APIs. The other thing that jumped out is that we can get away with just writing/reading ExtraData in a few more places to not have to worry about ensuring that all the new HTLC TLVs are properly written/read from disk. One example is when we add the endorsement bit directly or indirectly to lnd, the logic now also needs to make sure that's passed along vs just passing the TLV blob then leaving it to the caller at the last moment to unpack things. A similar comment applies to some of the option usage as well.

Roasbeef · 2024-03-28T21:37:31Z

htlcswitch/hop/payload.go

+	// Bolt 04 notes that we should enforce payment constraints _if_ they
+	// are present, so we do not fail if not provided.
+	var err error
+	blindedData.Constraints.WhenSome(


Directly returning something here, so can be written as:

err := fn.MapOptionZ(blindedData.Contraints, func(c ...) error { }) if err != nil { }

Non-blocking. Similar instances below.

Roasbeef · 2024-03-28T22:00:04Z

peer/brontide.go

-		return fmt.Sprintf("chan_id=%v, id=%v, amt=%v, expiry=%v, hash=%x",
-			msg.ChanID, msg.ID, msg.Amount, msg.Expiry, msg.PaymentHash[:])
+		var blindingPoint []byte
+		msg.BlindingPoint.WhenSome(


Can be written as:

blindingPoint := fn.MapOptionz(msg.BlindingPoint, func(b ..) []byte { return b.Val.SerializeCompressed() })

Non-blocking.

Roasbeef · 2024-03-28T22:02:21Z

channeldb/channel.go

@@ -2340,6 +2393,12 @@ func SerializeHtlcs(b io.Writer, htlcs ...HTLC) error {
 	}

 	for _, htlc := range htlcs {
+		// Populate TLV stream for any additional fields contained
+		// in the TLV.
+		if err := htlc.serializeExtraData(); err != nil {


Won't this information already be stored in ExtraData if you just take the opaque bytes from the update add HTLC message? Since we code, but then keep the blob as is.

Roasbeef · 2024-03-28T22:03:26Z

lnwallet/channel.go

@@ -736,6 +744,14 @@ func (c *commitment) toDiskCommit(ourCommit bool) *channeldb.ChannelCommitment {
 			Incoming:      false,
 		}
 		copy(h.OnionBlob[:], htlc.OnionBlob)
+		if htlc.BlindingPoint != nil {


Re above (and also related to future changes like storing the endorsement bit in an opaque manner), if we copied the extra records from the pay desc into this HTLC, then it's a more generic way to handle storing any future TLV data.

if we copied the extra records from the pay desc into this HTLC

def, but iirc from the original PR we wanted to be more intentional about what we store (ie, only things we care about) rather than including the full ExtraBytes and wasting space if people send us random junk along with the HTLC.

Roasbeef · 2024-03-28T22:03:47Z

lnwallet/channel.go

@@ -859,6 +882,12 @@ func (lc *LightningChannel) diskHtlcToPayDesc(feeRate chainfee.SatPerKWeight,
 		theirWitnessScript: theirWitnessScript,
 	}

+	htlc.BlindingPoint.WhenSome(func(b tlv.RecordT[


WhenSomeV is useful here again as it gives you just *btcec.PublicKey in this case.

Roasbeef · 2024-03-28T22:05:18Z

lnwallet/channel.go

@@ -3607,6 +3638,14 @@ func (lc *LightningChannel) createCommitDiff(
 				PaymentHash: pd.RHash,
 			}
 			copy(htlc.OnionBlob[:], pd.OnionBlob)
+			if pd.BlindingPoint != nil {


Similar point here re just copying over the raw bytes so you don't need to be concerned about the record mapping at this state.

Roasbeef · 2024-03-28T22:05:53Z

lnwallet/channel.go

@@ -3736,12 +3775,21 @@ func (lc *LightningChannel) getUnsignedAckedUpdates() []channeldb.LogUpdate {
 		// four messages that it corresponds to.
 		switch pd.EntryType {
 		case Add:
+			var b lnwire.BlindingPointRecord
+			if pd.BlindingPoint != nil {


If the pay desc just stores the record directly, then we also don't need to handle this shuffling.

Roasbeef · 2024-03-28T22:08:35Z

contractcourt/htlc_incoming_contest_resolver.go

@@ -520,9 +522,18 @@ func (h *htlcIncomingContestResolver) Supplement(htlc channeldb.HTLC) {
 func (h *htlcIncomingContestResolver) decodePayload() (*hop.Payload,
 	[]byte, error) {

+	var blindingPoint *btcec.PublicKey
+	h.htlc.BlindingPoint.WhenSome(


WhenSomeV

carlaKC added the blinded paths label Nov 8, 2023

This was referenced Nov 8, 2023

[2/3]: Support Forwarding of Blinded Payments #8160

Merged

multi: Forward Blinded Payments #7376

Closed

carlaKC self-assigned this Nov 8, 2023

saubyk added htlcswitch wire protocol Encoding of messages for the communication between nodes labels Nov 9, 2023

saubyk added this to the v0.18.0 milestone Nov 9, 2023

Roasbeef requested a review from ellemouton January 3, 2024 20:34

ellemouton reviewed Jan 15, 2024

View reviewed changes

record/blinded_data.go Outdated Show resolved Hide resolved

htlcswitch/hop/payload.go Outdated Show resolved Hide resolved

lnwire/update_add_htlc.go Outdated Show resolved Hide resolved

lnwallet/channel.go Outdated Show resolved Hide resolved

saubyk requested a review from bitromortac January 23, 2024 17:43

carlaKC force-pushed the 7298-1-forwardblindedroutes branch from df54ade to 3a492b2 Compare January 25, 2024 14:31

carlaKC force-pushed the 7298-1-forwardblindedroutes branch from 3a492b2 to 221c533 Compare February 14, 2024 14:24

carlaKC requested review from guggero, ProofOfKeags, Crypt-iQ, morehouse, Roasbeef and yyforyongyu as code owners February 14, 2024 14:24

carlaKC removed request for Roasbeef, guggero, ProofOfKeags, yyforyongyu, Crypt-iQ and morehouse February 14, 2024 14:25

carlaKC force-pushed the 7298-1-forwardblindedroutes branch from 221c533 to 8a37386 Compare February 14, 2024 15:32

carlaKC force-pushed the 7298-1-forwardblindedroutes branch from a7e2c2a to b79ad5b Compare March 18, 2024 20:32

ellemouton approved these changes Mar 19, 2024

View reviewed changes

record/blinded_data.go Show resolved Hide resolved

lnwire/features.go Show resolved Hide resolved

carlaKC requested review from bitromortac and calvinrzachman March 19, 2024 15:07

carlaKC force-pushed the 7298-1-forwardblindedroutes branch from b79ad5b to 116ce04 Compare March 19, 2024 15:30

bitromortac reviewed Mar 20, 2024

View reviewed changes

lnwire/lnwire_test.go Show resolved Hide resolved

lnwire/update_add_htlc.go Show resolved Hide resolved

channeldb/channel.go Show resolved Hide resolved

carlaKC force-pushed the 7298-1-forwardblindedroutes branch 2 times, most recently from e47f0ca to b79f5a1 Compare March 20, 2024 18:30

Roasbeef reviewed Mar 21, 2024

View reviewed changes

carlaKC force-pushed the 7298-1-forwardblindedroutes branch from b79f5a1 to 3275c13 Compare March 22, 2024 15:53

carlaKC and others added 14 commits March 27, 2024 09:36

htlcswitch: remove unused decode hop iterator

42069ef

record: add TLV encoding/decoding for blinded route data blobs

c48841a

This commit adds encoding and decoding for blinded route data blobs. TLV fields such as path_id (which are only used for the final hop) are omitted to minimize the change size.

multi: add validation of blinded route encrypted data

d8979d3

Co-authored-by: Calvin Zachman <calvin.zachman@protonmail.com>

lnwire: add blinding point to update_add_htlc TLVs

e4f90ec

Add blinding points to update_add_htlc. This TLV will be set for nodes that are relaying payments in blinded routes that are _not_ the introduction node.

channeldb: include extra data in htlc clone function

7596e76

htlcswitch: add blinding point to decode hop iterator request

7bf1daa

multi: pass blinding point through to reconstruction

4a93f4d

multi/refactor: clarify blinded payment fee rate field name

0aeb7c4

routing: fix comment on blinded path cltv delta

d67f652

docs: release notes for blinded routes prep work

2130022

carlaKC force-pushed the 7298-1-forwardblindedroutes branch from 3275c13 to 2130022 Compare March 27, 2024 13:48

Roasbeef approved these changes Mar 28, 2024

View reviewed changes

Roasbeef merged commit 1d61de2 into lightningnetwork:master Mar 28, 2024
25 of 27 checks passed

carlaKC mentioned this pull request Apr 2, 2024

Temp 7298 carlaKC/lnd#43

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1/3]: Preparatory work for Forwarding Blinded Routes #8159

[1/3]: Preparatory work for Forwarding Blinded Routes #8159

carlaKC commented Nov 8, 2023

ellemouton left a comment

coderabbitai bot commented Jan 25, 2024 •

edited

Auto Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

CodeRabbit Discord Community

carlaKC commented Jan 25, 2024

lightninglabs-deploy commented Feb 8, 2024

ellemouton commented Feb 14, 2024

carlaKC commented Feb 14, 2024

Roasbeef commented Mar 18, 2024

carlaKC commented Mar 18, 2024

ellemouton left a comment

Roasbeef Mar 21, 2024 •

edited

carlaKC Mar 22, 2024

Roasbeef Mar 28, 2024

Roasbeef left a comment

Roasbeef Mar 28, 2024

Roasbeef Mar 28, 2024

Roasbeef Mar 28, 2024

Roasbeef Mar 28, 2024

carlaKC Apr 2, 2024

Roasbeef Mar 28, 2024

Roasbeef Mar 28, 2024

Roasbeef Mar 28, 2024

Roasbeef Mar 28, 2024

[1/3]: Preparatory work for Forwarding Blinded Routes #8159

[1/3]: Preparatory work for Forwarding Blinded Routes #8159

Conversation

carlaKC commented Nov 8, 2023

ellemouton left a comment

Choose a reason for hiding this comment

coderabbitai bot commented Jan 25, 2024 • edited

Auto Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

CodeRabbit Discord Community

carlaKC commented Jan 25, 2024

lightninglabs-deploy commented Feb 8, 2024

ellemouton commented Feb 14, 2024

carlaKC commented Feb 14, 2024

Roasbeef commented Mar 18, 2024

carlaKC commented Mar 18, 2024

ellemouton left a comment

Choose a reason for hiding this comment

Roasbeef Mar 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

coderabbitai bot commented Jan 25, 2024 •

edited

CodeRabbit Configration File (`.coderabbit.yaml`)

Roasbeef Mar 21, 2024 •

edited