route blinding: follow ups #8976

ellemouton · 2024-08-05T12:44:42Z

Make blinded path config options configurable on a per-RPC level (rather than having them as part of LND main config)
Add a NodeOmissionList so that we can force certain nodes to not be included in our blinded paths
continue cycling through path candidates if we run into an error with one particular path - just log error instead
log chosen paths
error out of Decode invoice if unknown feature bit

Fixes #8965

coderabbitai · 2024-08-05T12:44:46Z

Important

Review skipped

Auto reviews are limited to specific labels.

Labels to auto review (1)

llm-review

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 do a full 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 Configuration 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

Great follow ups!

Re giving CLI/RPC clients a better error messages, two options come to mind:

Modify zpay32.Decode to accept an optional lnwire.FeatureVector of the features that we understand. After decode, it checks that new optional field to know if it should error out or not. For older clients, this can at least print that they don't understand feature bit YYY.
Do it deeper in the router.

The first option seems preferable, as then it'll error out at the earliest layer possible in the RPC server.

cmd/lncli/cmd_invoice.go

itest/lnd_route_blinding_test.go

lncfg/routing.go

lnrpc/lightning.proto

routing/blindedpath/blinded_path_test.go

routing/pathfind.go

Roasbeef · 2024-08-05T23:04:41Z

routing/pathfind.go

@@ -1253,6 +1257,12 @@ func processNodeForBlindedPath(g Graph, node route.Vertex,
 		return nil, false, nil
 	}

+	// If we have explicitly been told to ignore this node for blinded paths
+	// then we skip it too.
+	if restrictions.nodeOmissionSet[node] {


Do we also need to mark the node as visited to avoid repeatedly relaxing into it?

ah actually - i think let's leave as is cause alreadyVisited is used to get the idea of how long this path currently is. So adding this node in there will result in incorrect path length check

ellemouton · 2024-08-06T07:16:36Z

Modify zpay32.Decode to accept an optional lnwire.FeatureVe

ah yeah that is a great idea! incoming....

The only thing is that this nicer error message will only be useful for a future feature bit & not for route blinding cause nodes updating to this version will understand route blinding.

ellemouton

cool - updated!

also added logging of the chosen paths . Example:

[DBG] BLPT: Route selected for blinded path: [02cfe333599d4ecbe15673981b66121369dde97c5a72b6ba261f03cc2f96dff2ce (intro node)]--<485984139673600>-->[0360771d81f5a3d4bdc5cac0c46b946a572684ae66b8d30ab1774b18d9219d6f36]--->[dummy hop]

lncfg/routing.go

ellemouton · 2024-08-06T07:29:20Z

routing/pathfind.go

@@ -1253,6 +1257,12 @@ func processNodeForBlindedPath(g Graph, node route.Vertex,
 		return nil, false, nil
 	}

+	// If we have explicitly been told to ignore this node for blinded paths
+	// then we skip it too.
+	if restrictions.nodeOmissionSet[node] {


ah actually - i think let's leave as is cause alreadyVisited is used to get the idea of how long this path currently is. So adding this node in there will result in incorrect path length check

bitromortac

Looks great 🔥, nice additions, needs a clarification concerning a test case and maybe some safety checks concerning the cli flags.

cmd/lncli/cmd_invoice.go

routing/router_test.go

Fix the blinded path probability sorting function. Also fix the test assertion function.

since `AddInvoiceData` is config _per invoice_ where as `AddInvoiceConfig` is config for the invoice server itself and so pretty much should stay the same for the lifetime of LND. This change sets us up for moving some of the blinded path config options to be changeable per AddInvoice call rather that having fixed config values in the config file.

Extend the configurability of blinded paths in invoices by adding the ability to change the global config options on a per-RPC basis.

In this commit, we adjust BuildBlindedPaymentPaths to only error out completely if none of the paths it received from FindRoutes resulted in a usable blinded path.

This commit adds two functional options to the zpay32.Decode function. `WithKnownFeatureBits` allows the caller to overwrite the default set of known feature bits used by the function. `WithErrorOnUnknownFeatureBit` allows the caller to instruct the function to error out if the invoice that is decoded contaijns unknown feature bits. We then use this new error-out option from the `rpcServer`'s `extractPaymentIntent` method.

In this commit, we log selected blinded paths.

bitromortac

LGTM 🎉

so that they dont clash with the htlc-endorsement feature bits.

Roasbeef · 2024-08-08T00:23:32Z

The only thing is that this nicer error message will only be useful for a future feature bit & not for route blinding cause nodes updating to this version will understand route blinding.

Heh, yeah good point! So then this change is more for w/e future instance where we added more required feature bits. In theory we could also back port the new feature bit logic to older versions, but we haven't yet solidified our policy for that.

Roasbeef

LGTM 🐊

Roasbeef · 2024-08-08T00:37:10Z

routing/router.go

@@ -746,7 +746,7 @@ func (r *ChannelRouter) FindBlindedPaths(destination route.Vertex,

 	// Sort the routes based on probability.
 	sort.Slice(routes, func(i, j int) bool {
-		return routes[i].probability < routes[j].probability
+		return routes[i].probability > routes[j].probability


Ah sure, increasing order of prob!

ellemouton force-pushed the rb-follow-ups branch from 757913b to b5960f3 Compare August 5, 2024 12:45

ellemouton mentioned this pull request Aug 5, 2024

lnrpc: update falafel version #8974

Merged

ellemouton force-pushed the rb-follow-ups branch 2 times, most recently from 127b1d9 to 77a9983 Compare August 5, 2024 17:42

Roasbeef requested changes Aug 5, 2024

View reviewed changes

ellemouton force-pushed the rb-follow-ups branch from 77a9983 to 766113a Compare August 6, 2024 10:37

ellemouton commented Aug 6, 2024

View reviewed changes

ellemouton force-pushed the rb-follow-ups branch 3 times, most recently from f248efd to 7fd98ce Compare August 6, 2024 13:13

ellemouton requested a review from Roasbeef August 6, 2024 13:14

ellemouton force-pushed the rb-follow-ups branch from 7fd98ce to f2f51c4 Compare August 6, 2024 14:04

saubyk added this to the v0.19.0 milestone Aug 6, 2024

ellemouton force-pushed the rb-follow-ups branch from f2f51c4 to 3a4165a Compare August 6, 2024 16:24

saubyk assigned ellemouton Aug 6, 2024

saubyk requested a review from bitromortac August 6, 2024 16:41

saubyk modified the milestones: v0.19.0, v0.18.3 Aug 6, 2024

bitromortac reviewed Aug 7, 2024

View reviewed changes

cmd/lncli/cmd_invoice.go Outdated Show resolved Hide resolved

cmd/lncli/cmd_invoice.go Show resolved Hide resolved

cmd/lncli/cmd_invoice.go Outdated Show resolved Hide resolved

routing/router_test.go Outdated Show resolved Hide resolved

routing/router_test.go Outdated Show resolved Hide resolved

ellemouton force-pushed the rb-follow-ups branch from 3a4165a to 1012d14 Compare August 7, 2024 14:52

ellemouton added 8 commits August 7, 2024 17:01

routing: fix MC probability order for blinded paths

e47160d

Fix the blinded path probability sorting function. Also fix the test assertion function.

multi: let blinded path invoice options be set per addinvoice call

3de6c54

Extend the configurability of blinded paths in invoices by adding the ability to change the global config options on a per-RPC basis.

routing/blindedpath: dont error out unless all paths fail

b490dee

In this commit, we adjust BuildBlindedPaymentPaths to only error out completely if none of the paths it received from FindRoutes resulted in a usable blinded path.

multi: add node omission list for blinded paths

e4619af

log: register blindedpath pkg logger

9547739

blindedpath: log chosen blinded paths

697f514

In this commit, we log selected blinded paths.

docs: add release note

d47ff3a

ellemouton force-pushed the rb-follow-ups branch from 1012d14 to d47ff3a Compare August 7, 2024 15:03

ellemouton requested a review from bitromortac August 7, 2024 15:52

bitromortac approved these changes Aug 7, 2024

View reviewed changes

lnwire: update Bolt11 blinded path feature bits

f1a58dc

so that they dont clash with the htlc-endorsement feature bits.

Roasbeef approved these changes Aug 8, 2024

View reviewed changes

Roasbeef merged commit 459ee9b into lightningnetwork:master Aug 8, 2024
28 of 32 checks passed

ellemouton deleted the rb-follow-ups branch August 8, 2024 05:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

route blinding: follow ups #8976

route blinding: follow ups #8976

ellemouton commented Aug 5, 2024 •

edited

Loading

coderabbitai bot commented Aug 5, 2024

Review skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

Roasbeef left a comment

Roasbeef Aug 5, 2024

ellemouton Aug 6, 2024

ellemouton Aug 6, 2024

ellemouton commented Aug 6, 2024 •

edited

Loading

ellemouton left a comment •

edited

Loading

ellemouton Aug 6, 2024

bitromortac left a comment

bitromortac left a comment

Roasbeef commented Aug 8, 2024

Roasbeef left a comment

Roasbeef Aug 8, 2024

route blinding: follow ups #8976

route blinding: follow ups #8976

Conversation

ellemouton commented Aug 5, 2024 • edited Loading

coderabbitai bot commented Aug 5, 2024

Review skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

Roasbeef left a comment

Choose a reason for hiding this comment

Roasbeef Aug 5, 2024

Choose a reason for hiding this comment

ellemouton Aug 6, 2024

Choose a reason for hiding this comment

ellemouton Aug 6, 2024

Choose a reason for hiding this comment

ellemouton commented Aug 6, 2024 • edited Loading

ellemouton left a comment • edited Loading

Choose a reason for hiding this comment

ellemouton Aug 6, 2024

Choose a reason for hiding this comment

bitromortac left a comment

Choose a reason for hiding this comment

bitromortac left a comment

Choose a reason for hiding this comment

Roasbeef commented Aug 8, 2024

Roasbeef left a comment

Choose a reason for hiding this comment

Roasbeef Aug 8, 2024

Choose a reason for hiding this comment

ellemouton commented Aug 5, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

ellemouton commented Aug 6, 2024 •

edited

Loading

ellemouton left a comment •

edited

Loading