Move shared secret calculation into decode_next_payment_hop #3607
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Makes sense to me, care to clean up the commits? |
arik-so
force-pushed
the
arik/trampoline/inbound-prefactors-03
branch
from
8c9f53b to
49f9ba9
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3607 +/- ##
==========================================
+ Coverage 88.69% 89.48% +0.79%
==========================================
Files 149 149
Lines 117505 123481 +5976
Branches 117505 123481 +5976
==========================================
+ Hits 104219 110496 +6277
+ Misses 10794 10423 -371
- Partials 2492 2562 +70 ☔ View full report in Codecov by Sentry. |
TheBlueMatt
previously approved these changes
Feb 19, 2025
| }, | ||
| onion_utils::Hop::BlindedReceive(next_hop_data) => { | ||
| onion_utils::Hop::Receive { .. } | onion_utils::Hop::BlindedReceive { .. } => { | ||
| let inbound_onion_payload = match decoded_hop { |
There was a problem hiding this comment.
This kinda thing could probably go in a separate commit since its just a cleanup.
arik-so
force-pushed
the
arik/trampoline/inbound-prefactors-03
branch
from
49f9ba9 to
a0ce434
Compare
arik-so
force-pushed
the
arik/trampoline/inbound-prefactors-03
branch
from
a0ce434 to
912a5dd
Compare
Essentially a follow-up to 38284a0, deduplicating some additional code.
arik-so
force-pushed
the
arik/trampoline/inbound-prefactors-03
branch
from
912a5dd to
78ddff5
Compare
valentinewallace
previously approved these changes
Feb 19, 2025
lightning/src/ln/onion_payment.rs
Outdated
| Some(next_packet_pubkey), | ||
| )? | ||
| }, | ||
| onion_utils::Hop::Receive(received_data) => { | ||
| onion_utils::Hop::Receive{hop_data, shared_secret} => { |
There was a problem hiding this comment.
nit:
Suggested change
| onion_utils::Hop::Receive{hop_data, shared_secret} => { | |
| onion_utils::Hop::Receive { hop_data, shared_secret } => { |
lightning/src/ln/onion_payment.rs
Outdated
| None, allow_skimmed_fees, msg.skimmed_fee_msat, cur_height, | ||
| )? | ||
| }, | ||
| onion_utils::Hop::BlindedReceive(received_data) => { | ||
| onion_utils::Hop::BlindedReceive{hop_data, shared_secret} => { |
There was a problem hiding this comment.
nit:
Suggested change
| onion_utils::Hop::BlindedReceive{hop_data, shared_secret} => { | |
| onion_utils::Hop::BlindedReceive { hop_data, shared_secret } => { |
lightning/src/ln/onion_utils.rs
Outdated
| let shared_secret = node_signer | ||
| .ecdh(recipient, hop_pubkey, blinded_node_id_tweak.as_ref()) | ||
| .unwrap() | ||
| .secret_bytes(); |
There was a problem hiding this comment.
Can we avoid calling secret_bytes here and only call it below for decode_next_hop instead? Avoids doing SharedSecret::from_bytes in a bunch of places
lightning/src/ln/onion_utils.rs
Outdated
Comment on lines
1474
to
1477
| Hop::Forward { shared_secret, .. } => shared_secret.clone(), | ||
| Hop::BlindedForward { shared_secret, .. } => shared_secret.clone(), | ||
| Hop::Receive { shared_secret, .. } => shared_secret.clone(), | ||
| Hop::BlindedReceive { shared_secret, .. } => shared_secret.clone(), |
There was a problem hiding this comment.
nit: doesn't matter much but could return a reference (or dereference) instead of cloning
|
yup, will squash as soon as CI passes on my fork |
arik-so
force-pushed
the
arik/trampoline/inbound-prefactors-03
branch
from
2cce8d7 to
5afe97f
Compare
For Trampoline, we'll need to keep track of both the outer and inner onion's shared secrets. To this end, we're moving the secret calculation inside `decode_next_payment_hop` such that, when applicable, it can return both.
arik-so
force-pushed
the
arik/trampoline/inbound-prefactors-03
branch
from
5afe97f to
5291445
Compare
valentinewallace
approved these changes
Feb 19, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For Trampoline, we'll need to keep track of both the outer and inner onion's shared secrets. To this end, we're moving the secret calculation inside the hop decoding method such that, when applicable, it can return both.
This might end up being just one commit, but the primary consideration in this PR is whether to make the relay error return the shared secret, or do the
HTLCFailReasonwithin. I opted for the former due to the awkwardness the latter seemed to entail, but open to undoing it, or passing the channel/HTLC details into the decode method to be able to return anHTLCFailureMsg.