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

Route blinding MVP #2413

Merged

Conversation

valentinewallace
Copy link
Contributor

@valentinewallace valentinewallace commented Jul 13, 2023

Support sending to blinded payment paths and receiving to 1-hop paths. Partially addresses #1970. Error handling, forwarding and receiving to multi-hop blinded paths will be completed in follow-up.

  • finish tests

Based on #2411, #2412, #2128, #2459, #2514, #2503

@codecov-commenter
Copy link

codecov-commenter commented Aug 26, 2023

Codecov Report

Patch coverage: 93.42% and project coverage change: +0.15% 🎉

Comparison is base (448b191) 90.45% compared to head (ebdc4ae) 90.61%.
Report is 15 commits behind head on main.

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2413      +/-   ##
==========================================
+ Coverage   90.45%   90.61%   +0.15%     
==========================================
  Files         112      113       +1     
  Lines       58564    59002     +438     
  Branches    58564    59002     +438     
==========================================
+ Hits        52976    53466     +490     
+ Misses       5588     5536      -52     
Files Changed Coverage Δ
lightning/src/ln/mod.rs 96.15% <ø> (ø)
lightning/src/ln/onion_utils.rs 90.97% <76.00%> (-0.82%) ⬇️
lightning/src/ln/msgs.rs 86.20% <92.30%> (-0.01%) ⬇️
lightning/src/blinded_path/mod.rs 95.91% <100.00%> (-4.09%) ⬇️
lightning/src/blinded_path/payment.rs 84.21% <100.00%> (+7.23%) ⬆️
lightning/src/ln/blinded_payment_tests.rs 100.00% <100.00%> (ø)
lightning/src/ln/channelmanager.rs 87.93% <100.00%> (+1.22%) ⬆️
lightning/src/ln/outbound_payment.rs 90.79% <100.00%> (+0.09%) ⬆️

... and 10 files with indirect coverage changes

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

@valentinewallace valentinewallace marked this pull request as ready for review August 29, 2023 18:28
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.

Rather than splitting the pr into handle-it-all-then-add-error-handling, can we split it into "handle sending, with all error handling, handle receiving, with all error handling, and then handle forwarding, with all error handling"?

lightning/src/ln/outbound_payment.rs Show resolved Hide resolved
@valentinewallace
Copy link
Contributor Author

Rather than splitting the pr into handle-it-all-then-add-error-handling, can we split it into "handle sending, with all error handling, handle receiving, with all error handling, and then handle forwarding, with all error handling"?

I'm still looking into this but we can't materially test any error handling until we have forwarding, because 1-hop blinded paths are supposed to error the same as unblinded payments, i.e. not wipe the error or return malformed. Another option would be to split forwarding+error handling off into its own PR and land sending/1-hop receiving first.

@valentinewallace
Copy link
Contributor Author

Removed forwarding support for now, so all forwarding + error handling will be done in #2540 (should be updated tomorrow).

@valentinewallace valentinewallace force-pushed the 2023-07-route-blinding branch 3 times, most recently from c2715bf to 80405b4 Compare September 8, 2023 18:12
@valentinewallace valentinewallace changed the title Route blinding Route blinding sending and receiving Sep 8, 2023
@valentinewallace valentinewallace changed the title Route blinding sending and receiving Route blinding MVP Sep 11, 2023
@valentinewallace
Copy link
Contributor Author

valentinewallace commented Sep 11, 2023

Rebased and removed support for receiving to multi-hop blinded paths. This way we can ship an MVP BOLT 12 in 117 and complete route blinding support in 118, to avoid holding up the current release.

lightning/src/ln/onion_utils.rs Show resolved Hide resolved
lightning/src/ln/onion_utils.rs Show resolved Hide resolved
lightning/src/blinded_path/payment.rs Outdated Show resolved Hide resolved
lightning/src/ln/msgs.rs Show resolved Hide resolved
lightning/src/blinded_path/mod.rs Outdated Show resolved Hide resolved
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.

Looks good to squash!

lightning/src/ln/onion_utils.rs Show resolved Hide resolved
lightning/src/ln/onion_utils.rs Show resolved Hide resolved
This will be used in the next commit to deserialize encrypted TLVs for
receiving to 1-hop blinded paths.
Comment on lines +107 to +114
let ev = remove_first_msg_event_to_node(&nodes[1].node.get_our_node_id(), &mut events);
pass_along_path(&nodes[0], expected_route[0], amt_msat, payment_hash.clone(),
Some(payment_secret), ev.clone(), false, None);

let ev = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &mut events);
pass_along_path(&nodes[0], expected_route[1], amt_msat, payment_hash.clone(),
Some(payment_secret), ev.clone(), true, None);
claim_payment_along_route(&nodes[0], expected_route, false, payment_preimage);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: ev clones not needed

Comment on lines +1584 to +1585
amt_msat: u64,
total_msat: u64,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Heh, can we get away from the legacy names and use more descriptive names since we're adding new code here? Something about mpp_part_amt_msat and total_payment_msat?

@@ -1234,7 +1234,9 @@ impl OutboundPayments {
if route.paths.len() < 1 {
return Err(PaymentSendFailure::ParameterError(APIError::InvalidRoute{err: "There must be at least one path to send over".to_owned()}));
}
if recipient_onion.payment_secret.is_none() && route.paths.len() > 1 {
if recipient_onion.payment_secret.is_none() && route.paths.len() > 1
&& !route.paths.iter().any(|p| p.blinded_tail.is_some())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldnt this be all?

return Err(DecodeError::InvalidValue)
}
let enc_tlvs = encrypted_tlvs_opt.ok_or(DecodeError::InvalidValue)?.0;
let enc_tlvs_ss = node_signer.ecdh(Recipient::Node, &blinding_point, None)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like we should do blinded paths to a random key and not reuse the node id. That wouldn't avoid the new trait pass cause we need it for forwards, but for nodes with the node id on a separate hardware device/VLS we'd avoid the need to call out to the hardware device here for receives, which I think would be very nice.

@TheBlueMatt TheBlueMatt merged commit daf79f5 into lightningdevkit:main Sep 13, 2023
12 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants