[1/4] Route Blinding Receives: Groundwork #8752

ellemouton · 2024-05-14T13:45:17Z

To make review a bit more manageable, I've split out some of the groundwork needed for
the main route blinding receives PR. This PR thus has no functional changes.

Tracking Issue: #6690

PR Overview:

a couple of refactors & interface expansions so that we have easy access to the BlindedPayment associated with an edge in path finding later on.
Remove the need to communicate the "TLV Option" in a blinded path feature bit vector. This bit can be assumed for any nodes in a blinded path.
Add fields to BlindedRouteData that will be required for receives. Namely, PathID and Padding.
Bolt11 blinded payment path encoding

coderabbitai · 2024-05-14T13:45:23Z

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.
- @coderabbitai help me debug CodeRabbit configuration file.

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 an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to full the review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@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.

lightninglabs-deploy · 2024-05-23T15:30:59Z

@bitromortac: review reminder

bitromortac

Awesome that you managed to do end-to-end payments to blinded routes using Bolt 11, very cool work and a big achievement 🎉! I have looked a bit ahead how everything fits together in the next PRs, but will study them more in depth.

routing/additional_edge.go

routing/pathfind_test.go

bitromortac · 2024-05-27T09:19:31Z

routing/pathfind.go

+			// on `var_onion_option`, we can assume that all nodes
+			// in a blinded path support TLV onions without this
+			// being explicitly communicated to us.
+			if isBlindedEdge && isTLVOptionBit(feature) {


Up to the commit here isBlindedEdge could also be extracted from blindedPath comparing node pubkeys, which is why I think the previous addition of of BlindedPayment() to the AdditionalEdge interface isn't needed. But I see that you later change this to blindedPathSet. I'm not yet sure how that will work, maybe it would be better to shift the additions of BlindedPayment to a commit where it becomes important.

this whole PR is just set-up for the follow up PRs. It tries to get all the plumbing out the way in prep for those PRs so that there is less noise there.

yes at the moment there is only a single blinded payment that this edge could have come from but eventually (in upcoming PR), it will be a set of paths. Then we dont want to have to iterate through every path etc etc to find the path this edge is part of. Also, if this is an introduction node, then it could be that this node is the intro node to multiple different payments in which case we cant just compare pub keys to know which one it is part of.

bitromortac · 2024-05-27T10:16:45Z

record/blinded_data.go

+		)
+	}
+
+	if constraints != nil {


this represents data that is used by the receiver, right? are constraints then needed in the receiver case?

yes this is used by the receiver.

im not sure I follow the second part of the question?

record/blinded_data.go

bitromortac · 2024-05-27T10:28:17Z

record/blinded_data.go

+// is the same size and so each call will overwrite the Padding record.
+// Note that calling PadBy with an n value of 0 will still result in a zero
+// length TLV entry being added.
+func (b *BlindedRouteData) PadBy(n int) {


wouldn't it make sense to implement the exact version? We could assume the type to be constant size and the length to be variable to make it easier. We would just need to know the bigint transitions, right? It would make this call easier and safer to use

implement the exact version?

this cannot be done without going and changing the internals of the TLV package. Or do you mean doing the iterative stuff here instead of letting the caller do it?
if you mean that, then the reason why not that is cause only the caller knows the sizes of all the packages. it could be that all of them need to be buffered. Ie, when we call this the first time, we dont necessarily know the final size to pad by.

(more detail in commit message)

zpay32/invoice.go

bitromortac · 2024-05-27T12:38:23Z

zpay32/blinded_path.go

+
+// BlindedPaymentPath holds all the information a payer needs to know about a
+// blinded path to a receiver of a payment.
+type BlindedPaymentPath struct {


I think it makes sense to put this into the zpay32 package, just wanted to note that there is also routing.BlindedPayment, with similar data.

just checking if there is an actionable item here or just a note?

zpay32/invoice_test.go

This commit is purely a refactor. In it, we let the `BlindedEdge` struct carry a pointer to the `BlindedPayment` that it was derived from. This is done now because later on in the PR series, we will need more information about the `BlindedPayment` that an edge was derived from. Since we now pass in the whole BlindedPayment, we swap out the `cipherText` member for a `hopIndex` member so that we dont carry around two sources of truth in the same struct.

Expand the AdditionalEdges interface with a BlindedPayment method. In upcoming commits, we will want to know if an AdditionalEdge was derived from a blinded payment or not and we will also need some information from the blinded payment it was derived from. So we expand the interface here to avoid needing to do type casts later on. The new method may return nil if the edge was not derived from a blinded payment.

Add a constructor for unified edge. In upcoming commits, we will add a new member to unifiedEdge and a constructor forces us to not forget to populate a required member.

Later on in this series, we will need to know during path finding if an edge we are traversing was derived from a blinded payment path. In preparation for that, we add a BlindedPayment member to the `unifiedEdge` struct.

We can make some assumptions about the feature set of a blinded edge without having to be explicity told what those features are. Specifically, this applies to the `var_onion_option` feature bits which can be assumed for any node advertising `option_route_blinding`. See [this](https://github.com/lightning/bolts/blob/db278ab9b2baa0b30cfe79fb3de39280595938d3/proposals/route-blinding.md?plain=1#L219) suggestion in the route blinding proposal. This will allow us to condense the information that a recipient needs to send to the sender. This change is tested by updating the TestBlindedRouteConstruction test to no longer set the Features field in the BlindedPayment struct.

For the final hop in a blinded route, the SCID and RelayInfo fields will _not_ be set. So these fields need to be converted to optional records. The existing BlindedRouteData constructor is also renamed to `NewNonFinalBlindedRouteData` in preparation for a `NewFinalBlindedRouteData` constructor which will be used to construct the blinded data for the final hop which will contain a much smaller set of data. The SCID and RelayInfo parameters of the constructor are left as non-pointers in order to force the caller to set them in the case that the constructor is called for non-final nodes. The other option would be to create a single constructor where all parameters are optional but I think this makes it easier for the caller to make a mistake.

Add the PathID (tlv type 6) field to BlindedRouteData. This will be used for the final hop of a blinded route. A new constructor is also added for BlindedRouteData which can specifically be used for the final hop.

When we start creating blinded paths to ourselves, we will want to be able to pad the data for each hop so that the `encrypted_recipient_data` for each hop is the same. We add a `PadBy` method that allows a caller to add a certain number of bytes to the padding field. Note that adding n bytes won't always mean that the encoded payload will increase by size n since there will be overhead for the type and lenght fields for the new TLV field. This will also be the case when the number of bytes added results in a BigSize bucket jump for TLV length field. The responsibility of ensuring that the final payloads are the same size is left to the caller who may need to call PadBy iteratively to achieve the goal. I decided to leave this to the caller since doing this at the actual TLV level will be quite intrusive & I think it is uneccessary to touch that code for this unique use case.

Only include the final hop's cltv delta in the total timelock calculation if the route does not include a blinded path. This is because in a blinded path, the final hops final cltv delta will be included in the blinded path's accumlated cltv delta value. With this commit, we remove the responsibility of remembering not to set the `finalHop.cltvDelta` from the caller of `newRoute`. The relevant test is updated accordingly.

ellemouton

Thanks for the review @bitromortac 🙏

Updated & responded

ellemouton · 2024-05-27T18:00:57Z

routing/pathfind.go

+			// on `var_onion_option`, we can assume that all nodes
+			// in a blinded path support TLV onions without this
+			// being explicitly communicated to us.
+			if isBlindedEdge && isTLVOptionBit(feature) {


this whole PR is just set-up for the follow up PRs. It tries to get all the plumbing out the way in prep for those PRs so that there is less noise there.

record/blinded_data.go

ellemouton · 2024-05-27T18:04:25Z

record/blinded_data.go

+		)
+	}
+
+	if constraints != nil {


yes this is used by the receiver.

im not sure I follow the second part of the question?

ellemouton · 2024-05-27T18:06:38Z

record/blinded_data.go

+// is the same size and so each call will overwrite the Padding record.
+// Note that calling PadBy with an n value of 0 will still result in a zero
+// length TLV entry being added.
+func (b *BlindedRouteData) PadBy(n int) {


implement the exact version?

this cannot be done without going and changing the internals of the TLV package. Or do you mean doing the iterative stuff here instead of letting the caller do it?
if you mean that, then the reason why not that is cause only the caller knows the sizes of all the packages. it could be that all of them need to be buffered. Ie, when we call this the first time, we dont necessarily know the final size to pad by.

(more detail in commit message)

ellemouton · 2024-05-27T18:07:36Z

zpay32/blinded_path.go

+
+// BlindedPaymentPath holds all the information a payer needs to know about a
+// blinded path to a receiver of a payment.
+type BlindedPaymentPath struct {


just checking if there is an actionable item here or just a note?

ellemouton · 2024-05-28T14:14:58Z

routing/pathfind.go

+			// on `var_onion_option`, we can assume that all nodes
+			// in a blinded path support TLV onions without this
+			// being explicitly communicated to us.
+			if isBlindedEdge && isTLVOptionBit(feature) {


yes at the moment there is only a single blinded payment that this edge could have come from but eventually (in upcoming PR), it will be a set of paths. Then we dont want to have to iterate through every path etc etc to find the path this edge is part of. Also, if this is an introduction node, then it could be that this node is the intro node to multiple different payments in which case we cant just compare pub keys to know which one it is part of.

zpay32/invoice_test.go

In this commit, the ability is added to encode blinded payment paths and add them to a Bolt 11 invoice.

This commit adds a blinded_paths field to the PayReq proto message. A new helper called `CreateRPCBlindedPayments` is then added to convert the zpay32 type to the existing `lnrpc.BlindedPaymentPath` type and add this to the `PayReq` in the `DecodePayReq` rpc method.

ellemouton mentioned this pull request May 14, 2024

[2/4] Route Blinding Receives: Receive and send to a single blinded path in an invoice. #8735

Open

saubyk assigned ellemouton May 14, 2024

saubyk added this to the v0.18.1 milestone May 14, 2024

ellemouton force-pushed the rb-receives-setup branch 2 times, most recently from 485b765 to 451bda7 Compare May 15, 2024 09:02

ellemouton added the blinded paths label May 15, 2024

ellemouton force-pushed the rb-receives-setup branch 2 times, most recently from e92e850 to d537c35 Compare May 15, 2024 11:10

ellemouton changed the title ~~[1/3] Route Blinding Receives: Groundwork~~ [1/4] Route Blinding Receives: Groundwork May 16, 2024

saubyk requested a review from bitromortac May 16, 2024 15:26

bitromortac reviewed May 27, 2024

View reviewed changes

ellemouton added 9 commits May 27, 2024 13:48

routing+refactor: add a constructor for unifiedEdge

a563c47

Add a constructor for unified edge. In upcoming commits, we will add a new member to unifiedEdge and a constructor forces us to not forget to populate a required member.

routing: add BlindedPayment to unifiedEdge

6c40c1b

Later on in this series, we will need to know during path finding if an edge we are traversing was derived from a blinded payment path. In preparation for that, we add a BlindedPayment member to the `unifiedEdge` struct.

record: add PathID to BlindedRouteData

ec0352d

Add the PathID (tlv type 6) field to BlindedRouteData. This will be used for the final hop of a blinded route. A new constructor is also added for BlindedRouteData which can specifically be used for the final hop.

ellemouton force-pushed the rb-receives-setup branch from d537c35 to 04d97a4 Compare May 28, 2024 15:25

ellemouton commented May 28, 2024

View reviewed changes

ellemouton force-pushed the rb-receives-setup branch 2 times, most recently from 94f2078 to 85a1b2a Compare May 28, 2024 15:33

ellemouton added 3 commits May 28, 2024 11:54

zpay: encoding and decoding of a BlindedPaymentPath

3f07d5f

In this commit, the ability is added to encode blinded payment paths and add them to a Bolt 11 invoice.

docs: update release notes

aded983

ellemouton force-pushed the rb-receives-setup branch from 85a1b2a to aded983 Compare May 28, 2024 15:54

ellemouton requested a review from bitromortac May 28, 2024 18:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1/4] Route Blinding Receives: Groundwork #8752

[1/4] Route Blinding Receives: Groundwork #8752

ellemouton commented May 14, 2024

coderabbitai bot commented May 14, 2024 •

edited

Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

Documentation and Community

lightninglabs-deploy commented May 23, 2024

bitromortac left a comment

bitromortac May 27, 2024

ellemouton May 27, 2024

ellemouton May 28, 2024

bitromortac May 27, 2024

ellemouton May 27, 2024

bitromortac May 27, 2024

ellemouton May 27, 2024

bitromortac May 27, 2024

ellemouton May 27, 2024

ellemouton left a comment

ellemouton May 27, 2024

ellemouton May 27, 2024

ellemouton May 27, 2024

ellemouton May 27, 2024

ellemouton May 28, 2024

[1/4] Route Blinding Receives: Groundwork #8752

Are you sure you want to change the base?

[1/4] Route Blinding Receives: Groundwork #8752

Conversation

ellemouton commented May 14, 2024

PR Overview:

coderabbitai bot commented May 14, 2024 • edited

Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

Documentation and Community

lightninglabs-deploy commented May 23, 2024

bitromortac 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

ellemouton 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

coderabbitai bot commented May 14, 2024 •

edited

CodeRabbit Configration File (`.coderabbit.yaml`)