Uh oh!
There was an error while loading. Please reload this page.
Complete route blinding support - #2812
Conversation
Codecov ReportAttention:
❗ 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. |
TheBlueMatt
left a comment
There was a problem hiding this comment.
Basically LGTM, I think, though I need to dig in more deeply.
Uh oh!
There was an error while loading. Please reload this page.
bd28d01 to
eac0f70Compare| // 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, |
There was a problem hiding this comment.
Hmm... maybe this should be named BlindedSource now that it is no longer just use in a failure context?
There was a problem hiding this comment.
Hm, it should only be used in a failure context. Could you point me to where you're seeing that?
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading. Please reload this page.
| 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, | ||
| } |
| 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) { |
There was a problem hiding this comment.
What's the reason for using random values?
There was a problem hiding this comment.
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.
eac0f70 to
5663ebfComparevalentinewallace
commented
Jan 10, 2024
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.
5663ebf to
ae22997Comparevalentinewallace
commented
Jan 11, 2024
Rebased due to conflicts. |
TheBlueMatt
left a comment
There was a problem hiding this comment.
Two questions, otherwise LGTM
| impl_writeable_tlv_based!(BlindedForward, { | ||
| (0, inbound_blinding_point, required), | ||
| (1, failure, (default_value, BlindedFailure::FromIntroductionNode)), |
There was a problem hiding this comment.
Didn't we previously only support forwarding as an intermediate node, so shouldn't the default here be FromBlindedNode?
There was a problem hiding this comment.
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), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)There was a problem hiding this comment.
Had to use unwrap_or but did this.
| .map(|bp| BlindedForward { | ||
| inbound_blinding_point: bp, | ||
| failure: intro_node_blinding_point | ||
| .map_or(BlindedFailure::FromBlindedNode, |_| BlindedFailure::FromIntroductionNode), |
There was a problem hiding this comment.
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)See added docs.
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.
b9531da to
aae39b4CompareWalkthroughThe 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 Changes
Poem
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? TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Additionally, you can add CodeRabbit Configration File ( |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
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
BlindedFailureandBlindedForwardtypes, 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::BlindedForwardhas been correctly updated to handle the new blinded payment logic, including the introduction ofintro_node_blinding_pointand the use ofcheck_blinded_forwardto validate the payment relay and constraints.
- 108-114: The logic for creating a
PendingHTLCInfostruct has been updated to include the newBlindedForwardstruct with theinbound_blinding_pointandfailurefields. 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_successtest 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_optionalto thefeaturesstruct aligns with the existing pattern of feature flag setters.lightning/src/ln/msgs.rs (2)
- 1717-1717: The change to make
intro_node_blinding_pointanOption<PublicKey>is consistent with the PR objectives and AI-generated summary, which indicates that theintro_node_blinding_pointfield should reflect the optional nature of this information in the context of route blinding.- 2397-2397: The use of
intro_node_blinding_pointwithin theBlindedForwardvariant 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
failurefield to theBlindedForwardstruct is consistent with the PR's objective to handle blinded failures.- 214-214: The
blinded_failuremethod correctly matches against theBlindedForwardstruct to extract thefailurefield when present.- 310-317: The
BlindedFailureenum 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_pointis sound.- 4380-4380: The assignment of
blinded_failurewhen creating aPendingHTLCRoutinginstance 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
BlindedForwardstruct with thefailurefield using a TLV-based approach is correctly implemented.
| sender_intended_htlc_amt_msat: *final_value_msat, | ||
| total_msat, | ||
| cltv_expiry_height: cltv, | ||
| cltv_expiry_height: cur_cltv + excess_final_cltv_expiry_delta, |
There was a problem hiding this comment.
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.
| cltv_expiry_height: cur_cltv + excess_final_cltv_expiry_delta, | |
| cltv_expiry_height: cur_cltv + excess_final_cltv_expiry_delta, |
| 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); |
There was a problem hiding this comment.
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.
| 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_CLTVasu16 - 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_CLTVasu16 - 2) | |
| }else{None}; | |
| let(_, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[2],Some(amt_msat), insufficient_final_cltv_delta_opt); |
| 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); |
There was a problem hiding this comment.
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.
| assert_eq!(payment_event_1_2.msgs[0].cltv_expiry, nodes[0].best_block_info().1 + 1 + excess_final_cltv_delta_opt.unwrap()asu32); | |
| assert_eq!(payment_event_1_2.msgs[0].cltv_expiry, nodes[0].best_block_info().1 + 1 + insufficient_final_cltv_delta_opt.unwrap()asu32); |
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.