Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Complete route blinding support #2812

Merged

Conversation

valentinewallace
Copy link
Contributor

@valentinewallace valentinewallace commented Jan 2, 2024

We'll now support forwarding blinded payments as not-the-intro-node. Unblocks advertising the feature bit, though we don't advertise it yet in this PR. Since this completes blinded forwarding support, we also start advertising the feature bit.

Helps address #1970. There are still a few small follow-ups planned after this one.

@codecov-commenter
Copy link

codecov-commenter commented Jan 2, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (62d52c6) 88.50% compared to head (aae39b4) 88.91%.
Report is 10 commits behind head on main.

Files Patch % Lines
lightning/src/ln/channelmanager.rs 75.00% 1 Missing ⚠️
lightning/src/ln/peer_handler.rs 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2812      +/-   ##
==========================================
+ Coverage   88.50%   88.91%   +0.41%     
==========================================
  Files         114      114              
  Lines       92055    94794    +2739     
  Branches    92055    94794    +2739     
==========================================
+ Hits        81471    84290    +2819     
+ Misses       8079     8058      -21     
+ Partials     2505     2446      -59     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jkczyz jkczyz self-requested a review January 2, 2024 15:38
@valentinewallace valentinewallace added this to the 0.0.120 milestone Jan 5, 2024
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically LGTM, I think, though I need to dig in more deeply.

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
@valentinewallace valentinewallace force-pushed the 2023-12-blinded-forwarding branch 2 times, most recently from bd28d01 to eac0f70 Compare January 9, 2024 20:49
@jkczyz jkczyz mentioned this pull request Jan 10, 2024
// Another field will be added here when we support forwarding as a non-intro node.
/// If needed, this determines how this HTLC should be failed backwards, based on whether we are
/// the introduction node.
pub failure: BlindedFailure,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... maybe this should be named BlindedSource now that it is no longer just use in a failure context?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, it should only be used in a failure context. Could you point me to where you're seeing that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By "used" I meant more that it is used in this struct even though described "if needed". I suppose the struct name is fine given how it is actually used, but the variable name failure makes it seem like something has already failed when in fact in this context it might not ever. I'm fine if there's not a better name.

lightning/src/ln/functional_test_utils.rs Outdated Show resolved Hide resolved
Comment on lines 2391 to 2402
pub struct PassAlongPathArgs<'a, 'b, 'c, 'd> {
pub origin_node: &'a Node<'b, 'c, 'd>,
pub expected_path: &'a [&'a Node<'b, 'c, 'd>],
pub recv_value: u64,
pub payment_hash: PaymentHash,
pub payment_secret: Option<PaymentSecret>,
pub event: MessageSendEvent,
pub payment_claimable_expected: bool,
pub clear_recipient_events: bool,
pub expected_preimage: Option<PaymentPreimage>,
pub is_probe: bool,
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! 😎

Comment on lines 1056 to 1062
fn prop_fees_rng() {
do_prop_fees_rng(true);
do_prop_fees_rng(false);
}

#[cfg(feature = "std")]
fn do_prop_fees_rng(send_min: bool) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for using random values?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was written a long time ago to have actual testing for #2514. The random values helped catch some edge cases we weren't handling.

@valentinewallace
Copy link
Contributor Author

Because we may have a shorter than usual release cycle for 0.0.120, I moved the tests and the related test util refactors into #2823 in case it gives us a better chance of landing this in time.

Error handling will be completed in upcoming commit(s).
Previously, we were setting the final blinded hop's CLTV expiry height to
best_block_height + total_blinded_path_cltv_delta + shadow_cltv_offset. This is
incorrect, it should instead be set to best_block_height + shadow_cltv_offset
only -- it doesn't make sense to include the delta for the other blinded hops
in the final hop's expiry.

The reason this too-high final cltv value didn't cause test failures previously
is because of a 2nd bug that is fixed in an upcoming commit where the sender
adds the shadow offset twice to the total path CLTV expiry. This 2nd offset
meant that intermediate nodes had some buffer CLTV to subtract their delta from
while still (usually) have enough leftover to meet the expiry in the final hop's
onion.
Necessary to include it in the public PendingHTLCInfo struct in the next commit.
@valentinewallace
Copy link
Contributor Author

Rebased due to conflicts.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two questions, otherwise LGTM

@@ -9495,6 +9503,7 @@ impl_writeable_tlv_based!(PhantomRouteHints, {

impl_writeable_tlv_based!(BlindedForward, {
(0, inbound_blinding_point, required),
(1, failure, (default_value, BlindedFailure::FromIntroductionNode)),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't we previously only support forwarding as an intermediate node, so shouldn't the default here be FromBlindedNode?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, no we previously only supported intro node forwarding.

.map(|bp| BlindedForward {
inbound_blinding_point: bp,
failure: intro_node_blinding_point
.map_or(BlindedFailure::FromBlindedNode, |_| BlindedFailure::FromIntroductionNode),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused, I read the code here as "if intro_node_blinding_point is set (ie we're the introduction node), set the failure mode to FromBlindedNode, otherwise FromIntroductionNode). Isn't that the wrong way around.

Unrelatedly, FromIntroductionNode should probably read WereIntroductionNode, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

map_or syntax is a bit weird but BlindedFailure::FromBlindedNode is the default value that gets returned if intro_node_blinding_point is None.

Unrelatedly, FromIntroductionNode should probably read WereIntroductionNode, right?

Hmm... that reads to me as "were introduction node." Do you think the current name is unclear?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

map_or syntax is a bit weird but BlindedFailure::FromBlindedNode is the default value that gets returned if intro_node_blinding_point is None.

Ugh.

Hmm... that reads to me as "were introduction node." Do you think the current name is unclear?

Heh, fair, I read it as "this came from the introduction node" rather than "we're the introduction node"? I guess its a failure so it can be implied that we're the one failing? It doesnt matter too much, though, really.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

map_or syntax is a bit weird but BlindedFailure::FromBlindedNode is the default value that gets returned if intro_node_blinding_point is None.

Ugh.

Agreed regarding map_or parameters. Might be clearer as:

intro_node_blinding_point.map(|_| BlindedFailure::FromIntroductionNode).or(BlindedFailure::FromBlindedNode)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to use unwrap_or but did this.

Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Feel free to squash.

.map(|bp| BlindedForward {
inbound_blinding_point: bp,
failure: intro_node_blinding_point
.map_or(BlindedFailure::FromBlindedNode, |_| BlindedFailure::FromIntroductionNode),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

map_or syntax is a bit weird but BlindedFailure::FromBlindedNode is the default value that gets returned if intro_node_blinding_point is None.

Ugh.

Agreed regarding map_or parameters. Might be clearer as:

intro_node_blinding_point.map(|_| BlindedFailure::FromIntroductionNode).or(BlindedFailure::FromBlindedNode)

The excess delta is included in the final RouteHop::cltv_expiry_delta, so by
adding it explicitly to cur_cltv we were erroneously including it twice in the
total cltv expiry.

This could've add up to an extra MAX_SHADOW_CLTV_DELTA_OFFSET (432) blocks to
the total cltv expiry.
Now that we fully support forwarding blinded payments, we should advertise
support so nodes on the network can include us in their blinded paths.
Copy link

coderabbitai bot commented Jan 16, 2024

Walkthrough

The changes reflect enhancements to the blinded payment process within a Lightning Network implementation. A new test ensures three-hop blinded paths work correctly, while handling of failures in multi-hop scenarios has been improved. The BlindedForward structure, PendingHTLCRouting enum, and related functions now accommodate blinded failures. An optionality has been introduced to node blinding points, and the calculation of cltv_expiry_height has been adjusted. Additionally, route blinding can now be set as optional through a new method.

Changes

Files Change Summary
.../blinded_payment_tests.rs Added three_hop_blinded_path_success test; updated do_multi_hop_receiver_fail for various failure scenarios.
.../channelmanager.rs, .../onion_payment.rs Enhanced structures and enums (BlindedForward, PendingHTLCRouting, BlindedFailure) to handle blinded failures; updated related functions.
.../msgs.rs, .../onion_utils.rs Made intro_node_blinding_point optional; adjusted cltv_expiry_height in build_onion_payloads.
.../peer_handler.rs Introduced set_route_blinding_optional method.

Poem

In the code where payments flash,
A rabbit hopped, made a dash,
Through blinded paths, it twirled with glee,
"Hop success!" it cheered, "Privacy spree!" 🐰⚡️🎉

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.
  • 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 from git 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.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 3

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between ea5de93 and aae39b4.
Files selected for processing (6)
  • lightning/src/ln/blinded_payment_tests.rs (3 hunks)
  • lightning/src/ln/channelmanager.rs (6 hunks)
  • lightning/src/ln/msgs.rs (2 hunks)
  • lightning/src/ln/onion_payment.rs (4 hunks)
  • lightning/src/ln/onion_utils.rs (1 hunks)
  • lightning/src/ln/peer_handler.rs (1 hunks)
Additional comments: 14
lightning/src/ln/onion_payment.rs (3)
  • 15-15: The import of BlindedFailure and BlindedForward types, as well as other related items, is necessary for the new functionality related to route blinding.
  • 73-79: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [76-94]

The match arm for msgs::InboundOnionPayload::BlindedForward has been correctly updated to handle the new blinded payment logic, including the introduction of intro_node_blinding_point and the use of check_blinded_forward to validate the payment relay and constraints.

  • 108-114: The logic for creating a PendingHTLCInfo struct has been updated to include the new BlindedForward struct with the inbound_blinding_point and failure fields. This is consistent with the PR's objective to support route blinding.
lightning/src/ln/blinded_payment_tests.rs (1)
  • 493-514: The addition of the three_hop_blinded_path_success test function is consistent with the PR's objective to enhance route blinding capabilities. The test setup and assertions appear to be correct and follow the pattern of existing tests.
lightning/src/ln/peer_handler.rs (1)
  • 308-308: The addition of set_route_blinding_optional to the features struct aligns with the existing pattern of feature flag setters.
lightning/src/ln/msgs.rs (2)
  • 1717-1717: The change to make intro_node_blinding_point an Option<PublicKey> is consistent with the PR objectives and AI-generated summary, which indicates that the intro_node_blinding_point field should reflect the optional nature of this information in the context of route blinding.
  • 2397-2397: The use of intro_node_blinding_point within the BlindedForward variant is consistent with the change to its type. This ensures that the optional nature of the blinding point is correctly handled in the logic related to payment relay and constraints.
lightning/src/ln/channelmanager.rs (7)
  • 205-207: The addition of the failure field to the BlindedForward struct is consistent with the PR's objective to handle blinded failures.
  • 214-214: The blinded_failure method correctly matches against the BlindedForward struct to extract the failure field when present.
  • 310-317: The BlindedFailure enum is well-defined and appropriately differentiates between introduction node and blinded node failures.
  • 3033-3035: The logic to determine if the forwarding is from an introduction node using the presence of intro_node_blinding_point is sound.
  • 4380-4380: The assignment of blinded_failure when creating a PendingHTLCRouting instance is correctly implemented.
  • 9360-9360: The route blinding feature bit is correctly set as optional, aligning with the PR's note that the feature bit is not yet advertised.
  • 9506-9506: The serialization of the BlindedForward struct with the failure field using a TLV-based approach is correctly implemented.

res.push(msgs::OutboundOnionPayload::BlindedReceive {
sender_intended_htlc_amt_msat: *final_value_msat,
total_msat,
cltv_expiry_height: cltv,
cltv_expiry_height: cur_cltv + excess_final_cltv_expiry_delta,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cltv_expiry_height calculation has been modified to include excess_final_cltv_expiry_delta. This change could potentially introduce an off-by-one error if cur_cltv is not already inclusive of the current block height. It's important to ensure that cur_cltv represents the correct starting point before the addition of excess_final_cltv_expiry_delta.

To address this, verify that cur_cltv is calculated correctly elsewhere in the codebase, taking into account the current block height. If cur_cltv is already inclusive of the current block height, then this change is correct. Otherwise, adjust the calculation to ensure the correct cltv_expiry_height is set.

- cltv_expiry_height: cur_cltv,
+ cltv_expiry_height: cur_cltv + excess_final_cltv_expiry_delta,

Committable suggestion

IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
cltv_expiry_height: cur_cltv + excess_final_cltv_expiry_delta,
cltv_expiry_height: cur_cltv + excess_final_cltv_expiry_delta,

Comment on lines +563 to +567
let excess_final_cltv_delta_opt = if check == ReceiveCheckFail::ProcessPendingHTLCsCheck {
// Set the final CLTV expiry too low to trigger the failure in process_pending_htlc_forwards.
Some(TEST_FINAL_CLTV as u16 - 2)
} else { None };
let (_, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[2], Some(amt_msat), final_cltv_delta);
let (_, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[2], Some(amt_msat), excess_final_cltv_delta_opt);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable excess_final_cltv_delta_opt is used to set a CLTV delta that is too low, but the name does not clearly reflect its purpose. Consider renaming it to more accurately describe its function, such as insufficient_final_cltv_delta_opt.

- let excess_final_cltv_delta_opt = if check == ReceiveCheckFail::ProcessPendingHTLCsCheck {
+ let insufficient_final_cltv_delta_opt = if check == ReceiveCheckFail::ProcessPendingHTLCsCheck {

Committable suggestion

IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
let excess_final_cltv_delta_opt = if check == ReceiveCheckFail::ProcessPendingHTLCsCheck {
// Set the final CLTV expiry too low to trigger the failure in process_pending_htlc_forwards.
Some(TEST_FINAL_CLTV as u16 - 2)
} else { None };
let (_, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[2], Some(amt_msat), final_cltv_delta);
let (_, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[2], Some(amt_msat), excess_final_cltv_delta_opt);
let insufficient_final_cltv_delta_opt = if check == ReceiveCheckFail::ProcessPendingHTLCsCheck {
// Set the final CLTV expiry too low to trigger the failure in process_pending_htlc_forwards.
Some(TEST_FINAL_CLTV as u16 - 2)
} else { None };
let (_, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[2], Some(amt_msat), insufficient_final_cltv_delta_opt);

@@ -657,6 +681,7 @@ fn do_multi_hop_receiver_fail(check: ReceiveCheckFail) {
commitment_signed_dance!(nodes[2], nodes[1], (), false, true, false, false);
},
ReceiveCheckFail::ProcessPendingHTLCsCheck => {
assert_eq!(payment_event_1_2.msgs[0].cltv_expiry, nodes[0].best_block_info().1 + 1 + excess_final_cltv_delta_opt.unwrap() as u32);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assertion on line 684 checks the cltv_expiry against a value derived from the block height and an excess_final_cltv_delta_opt. However, the name excess_final_cltv_delta_opt suggests an excess value, while the context implies it is insufficient. This could lead to confusion. The variable should be renamed to reflect its purpose more clearly, as suggested in the previous comment.

- assert_eq!(payment_event_1_2.msgs[0].cltv_expiry, nodes[0].best_block_info().1 + 1 + excess_final_cltv_delta_opt.unwrap() as u32);
+ assert_eq!(payment_event_1_2.msgs[0].cltv_expiry, nodes[0].best_block_info().1 + 1 + insufficient_final_cltv_delta_opt.unwrap() as u32);

Committable suggestion

IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
assert_eq!(payment_event_1_2.msgs[0].cltv_expiry, nodes[0].best_block_info().1 + 1 + excess_final_cltv_delta_opt.unwrap() as u32);
assert_eq!(payment_event_1_2.msgs[0].cltv_expiry, nodes[0].best_block_info().1 + 1 + insufficient_final_cltv_delta_opt.unwrap() as u32);

@TheBlueMatt TheBlueMatt merged commit 53f2e2e into lightningdevkit:main Jan 17, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants