Enforce Trampoline Constraints (replacement) #4226
Conversation
|
👋 Thanks for assigning @valentinewallace as a reviewer! |
lightning/src/ln/onion_payment.rs
Outdated
| }), | ||
| onion_utils::Hop::TrampolineForward { next_trampoline_hop_data, next_trampoline_hop_hmac, new_trampoline_packet_bytes, trampoline_shared_secret, .. } => { | ||
| onion_utils::Hop::TrampolineForward { ref outer_hop_data, next_trampoline_hop_data, next_trampoline_hop_hmac, new_trampoline_packet_bytes, trampoline_shared_secret, .. } => { | ||
| // TODO: return reason as forward issue, not as receiving issue when forwarding is ready. |
There was a problem hiding this comment.
I'm not quite clear on what this todo means, still catching up on some context - @a-mpch could you help me out?
carlaKC
changed the title
|
Assigning reviewers who looked at the original PR - please free yourself if not appropriate! |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4226 +/- ##
========================================
Coverage 89.32% 89.33%
========================================
Files 180 180
Lines 138641 138877 +236
Branches 138641 138877 +236
========================================
+ Hits 123844 124068 +224
- Misses 12174 12185 +11
- Partials 2623 2624 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
One thing to note about the tests here: we've currently only got coverage for blinded receives checking the constraints (this is what the original PR had). We could also add coverage for blinded forwards in the failure case (can't do for success because we just fail the forwards rn), which would make codecov a bit happier. I think this is worth doing, but it would mean adding a bit more code to the mega test helper - interested on hearing thoughts on how readable others find it before adding another layer to an already quite dense test! Can also easily be a small follow. up. |
|
Needs rebase :( I did find the new testing not ideal from a readability PoV at first glance, going to take a closer look in a bit but let me know if you see any obvious ways to improve things. It may be that the nature of what we're testing makes it hard to improve things though. |
| } | ||
|
|
||
| // Creates a replacement onion that is used to produce scenarios that we don't support, specifically | ||
| // unblinded receives and invalid payloads. |
There was a problem hiding this comment.
Here and a few other places, I thought we did support unblinded trampoline receives but didn't support unblinded trampoline sends. At least I can't see where we reject them + it seems like we understand how to decode the payload 👀 may be missing something though
There was a problem hiding this comment.
Badly worded comment - we don't support creating payloads (sends) with unblinded receives, I'll fix!
lightning/src/ln/onion_payment.rs
Outdated
| onion_utils::Hop::TrampolineForward { next_trampoline_hop_data, next_trampoline_hop_hmac, new_trampoline_packet_bytes, trampoline_shared_secret, .. } => { | ||
| onion_utils::Hop::TrampolineForward { ref outer_hop_data, next_trampoline_hop_data, next_trampoline_hop_hmac, new_trampoline_packet_bytes, trampoline_shared_secret, .. } => { | ||
| // TODO: return reason as forward issue, not as receiving issue when forwarding is ready. | ||
| check_trampoline_onion_constraints(outer_hop_data, next_trampoline_hop_data.outgoing_cltv_value, next_trampoline_hop_data.amt_to_forward)?; |
There was a problem hiding this comment.
I'm a bit confused why we're calling this method here. It only returns IncorrectFinal* errors, which seems like it should only be used for receives and not forwards. I might be missing something from the trampoline spec here? Or do we need a new similar method for forwards that will return some of the new errors, like TrampolineFeeOrExpiryInsufficient, if the cltv/forward amount are mismatching between the inner and outer onion?
There was a problem hiding this comment.
This is the todo I'm not so sure about. It seems to me like the approach in the original PR is to add this validation here (even though it's not in the spec) and then remove it when we add more forwarding logic. cc @a-mpch?
I think that we can remove check_trampoline_onion_constraints on the forwarding branches and get to them in subsequent PRs, but just want to make sure I'm not missing context from the original change.
There was a problem hiding this comment.
We need to check the trampoline constraints in forwarding branches when implementing the forwarding. We can safely remove it and re-added in follow up PRs.
As at this point we don't have implemented this error: https://github.com/lightning/bolts/pull/836/files#diff-ad65f0beaac5cef88f5fd7a8b9ca36cbc5a790f4815b4b50b9ea794a55aaf012R1441
That would be returned here.
There was a problem hiding this comment.
Going to go ahead with removing the forwarding check and will re-add when we get to more forwarding logic.
|
🔔 1st Reminder Hey @tankyleo! This PR has been waiting for your review. |
To allow formatting on new code, move to per-function skips.
Remove skip without fixing up any of the ugly formatting, so that the diff is a bit more readable in review.
This commit adds three new local htlc failure error reasons: `TemporaryTrampolineFailure`, `TrampolineFeeOrExpiryInsufficient`, and `UnknownNextTrampoline` for trampoline payment forwarding failures.
carlaKC
force-pushed
the
3983-trampoline-constraints
branch
from
b43670e to
531cca6
Compare
valentinewallace
left a comment
valentinewallace
left a comment
There was a problem hiding this comment.
LGTM, feel free to squash!
Tests are added to cover validation of blinded and unblinded trampoline payloads against their outer onion. These are consolidated with our existing coverage for successful receives. Co-authored-by: Arik Sosman <[email protected]> Co-authored-by: Maurice Poirrier <[email protected]>
carlaKC
force-pushed
the
3983-trampoline-constraints
branch
from
136641d to
6b5cd30
Compare
4 participants
This PR replaces #3983, adding validation of trampoline onions (as compared to the outer onion). It makes some quite significant changes to the tests in the original PR to consolidate blinded and unblinded tests for success/failure scenarios into a single helper (apologies to reviewers who've already looked at the tests, but I think this DRYs it up quite nicely).
While we're here, it also moves
rustfmt::skipto a per-function level onblinded_payment_tests.rsand formats the existing test helper that we're modifying in a pre-factor so that the new code can be formatted.