Trampoline Forwarding: Enforce trampoline constraints #3983
Conversation
|
👋 Thanks for assigning @valentinewallace as a reviewer! |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3983 +/- ##
==========================================
- Coverage 88.78% 88.72% -0.06%
==========================================
Files 176 176
Lines 128139 129234 +1095
Branches 128139 129234 +1095
==========================================
+ Hits 113768 114663 +895
- Misses 11803 11975 +172
- Partials 2568 2596 +28
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:
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
|
🔔 1st Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
|
Also when I remove everything but the test code, the test does not fail - is that intentional ? I have not reviewed the test |
|
thanks @tankyleo !
Double checked the tests and yes, this happens because when receiving a trampoline it behaves exactly as receiving a non-trampoline payment. Having the same failure scenario. I added fixups for each of your comments and rebased in top of main |
a-mpch
force-pushed
the
2025-08-enforce-trampoline-constraint
branch
from
4deaa3e to
d37d6d8
Compare
|
Also go ahead and squash next time you push, and expand the commit message to describe in detail what you are doing, including in the tests :) Would help me out with reviewing the code too thank you ! |
a-mpch
force-pushed
the
2025-08-enforce-trampoline-constraint
branch
from
d37d6d8 to
3515844
Compare
|
@tankyleo Reading the initial PR needed a lot of explanation work 😅, thanks for the detailed review! All your comments should have been addressed and rebased and squashed. note that linting CI seems to be a problem on one of the new commits after rebase |
a-mpch
force-pushed
the
2025-08-enforce-trampoline-constraint
branch
from
3515844 to
5c6f023
Compare
a-mpch
force-pushed
the
2025-08-enforce-trampoline-constraint
branch
2 times, most recently
from
c41c202 to
ccd103f
Compare
a-mpch
force-pushed
the
2025-08-enforce-trampoline-constraint
branch
from
ccd103f to
91f5296
Compare
|
🔔 1st Reminder Hey @valentinewallace! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @valentinewallace! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @valentinewallace! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @valentinewallace! This PR has been waiting for your review. |
This simplifies the code and makes it more straightforward to test unblinded trampoline receives where we need to compute the trampoline session_priv when manually creating the inner onion. (The trampoline onion needs to be manually created because LDK does not natively support sending to unblinded trampolines, just receiving.)
No need to construct unused blinded hop data or hardcode session privs/prng seeds.
Previously, this test purported to test for a successful and a failing payment to a single-hop blinded path containing one trampoline node. However, to induce the failure the test was manually reconstructing the trampoline onion in a complicated way that encoded the final onion payload as a receive, when for its purposes it would be simplier for the recipient to just fail the payment backwards. In order to not regress in test coverage, the failure method the test was previously using is re-added in the next commit as a dedicated test. XXX this new test surfaced a bug that needs to be fixed
This re-adds test coverage for a case that was removed in the previous commit.
There was a problem hiding this comment.
I feel like a portion of the existing trampoline tests use a lot of internal APIs/hardcoded keys and are a bit messy as a result, when it's generally preferable to use the public API when testing (cc one of the tests flaked when working on the unrelated OffersMessageFlow PR #3639 (comment)). Mainly talking about the tests where we manually construct a replacement onion.
I think some of the tests in this PR double down on this approach, which is understandable but it seems like a good opportunity to instead fix the pre-existing tests and have cleaner tests moving forward if possible.
I looked into refactoring the tests and came up with this branch: https://github.com/valentinewallace/rust-lightning/tree/2025-08-trampoline-test-refactor Would you like to take it over and base this PR on it? One of the test refactors did surface a bug that will need to be fixed.
|
seems like a correct concern and approach. I'll take a look and base this PR on your branch! |
This commit adds three new local htlc failure error reasons: `TemporaryTrampolineFailure`, `TrampolineFeeOrExpiryInsufficient`, and `UnknownNextTrampoline` for trampoline payment forwarding failures.
We add a `check_trampoline_constraints` similar to `check_blinded_path_constraints` that compares the Trampoline onion's amount and CLTV values to the limitations imposed by the outer onion. Also, we add and modify the following tests: - Modified the unblinded receive to validate when receiving amount less than expected. - Modified test with wrong CLTV parameters that now fails with new enforcement of CLTV limits. - Add unblinded and blinded receive tests that forces trampoline onion's CLTV to be greater than the outer onion packet. Note that there are some TODOs to be fixed in following commits as we need the full trampoline forwarding feature to effectively test all cases. Co-authored-by: Arik Sosman <[email protected]>
a-mpch
force-pushed
the
2025-08-enforce-trampoline-constraint
branch
from
91f5296 to
cfbade2
Compare
|
@valentinewallace I rebased in top of your branch. |
| @@ -2535,6 +2535,6 @@ fn test_trampoline_forward_rejection() { | |||
| // Expect UnknownNextPeer error while we are unable to route forwarding Trampoline payments. | |||
| let payment_failed_conditions = PaymentFailedConditions::new() | |||
| .expected_htlc_error_data(LocalHTLCFailureReason::UnknownNextPeer, &[0; 0]); | |||
| expect_payment_failed_conditions(&nodes[0], payment_hash, false, payment_failed_conditions); | |||
There was a problem hiding this comment.
I don't think we should be switching this value to true here, because Carol is the one that failed and she's not the last hop in the path (at least from Alice's perspective)?
There was a problem hiding this comment.
I had the same thought when doing this.
Alice built a route expecting the trampoline to find a path to the end route or to the next trampoline. But she selected a node that does not support trampoline forwarding.
Spec says:
When processing a `trampoline_onion_packet`, a receiving node:
- If it doesn't support `trampoline_routing`:
- MUST report a route failure to the origin node
We are actually raising InvalidTrampolineForward that has the same error-code that UnknownNextPeer. I didn't find another error code that makes sense.
And UnkownNextTrampoline in spec is also a PERM error 🤔 meaning that if a trampoline raises an error, would make the permanent error on Alice's perspective (?)
There was a problem hiding this comment.
PERM means that that particular node that failed is permanently unable to handle the payment, but that shouldn't stop Alice from rerouting through another node, I believe.
In process_onion_failure we should be able to tell that the failure came from a trampoline node that was not the final node, I think?
Edit: I see this in the spec:
- if the _final node_ is returning the error:
- if the PERM bit is set:
- SHOULD fail the payment.
59dc83f to
30e81a9
Compare
| // If next hop is from a trampoline and there is only one trampoline hop | ||
| // in the trampoline route, means that the trampoline hop is also a | ||
| // final non-blinded node. | ||
| let is_next_hop_trampoline = | ||
| matches!(next_hop, Some((_, (Some(ErrorHop::TrampolineHop(..)), _)))); | ||
| if is_next_hop_trampoline { | ||
| is_from_final_non_blinded_node = | ||
| path.blinded_tail.as_ref().map_or(0, |bt| bt.trampoline_hops.len()) == 1 | ||
| } |
There was a problem hiding this comment.
I looked into why we needed this handling, since it wasn't super clear to me.
Please double check this work, but I believe the reason is that the trampoline recipient is not always encoding their failure packet correctly.
When I delete this code block, one of the test failures I get is test_trampoline_single_hop_receive. It looks like that's because in fail_htlc_backwards, we do not double-wrap the recipient's failure packet with the trampoline layer. In fail_htlc_backwards_internal, which gets called by fail_htlc_backwards, you can see that for phantom payments for example we will encode an additional layer to the onion error packet using the phantom shared secret, but we won't do the same for trampoline payments.
I think this means the trampoline_ss needs to be included in the HTLCPreviousHopData similar to what we do for phantom shared secrets in that method.
Let me know if this sounds right to you!
There was a problem hiding this comment.
I had the same idea but it would depend on the implementation of the recipient. I left my question on the spec related to it as I couldn't find this case on eclair.
I think that adding the double wrap would make sense, but I have doubts as the trampoline wouldn't be an "intermediate", so maybe just need to behave as a common blinded route?
I'll check out and implement as it is done in the phantom case anyway!
There was a problem hiding this comment.
Right... The recipient is a trampoline node in this case, so my thinking was that the suggested behavior would align with this part of the spec proposal:
https://github.com/lightning/bolts/pull/829/files#diff-c9739cca996bfa9ab52a39280655a99571ad6e17d51ffe2b2436d7fe2f68be80R225-R236
but it should be specified more in the BOLTs as well.
There was a problem hiding this comment.
We could also split this PR up, so the first stage is just the test refactors + the bugfix.
This PR is part of 1/N PRs in order to split up #3976
This commit checks amount and CLTV of the trampoline to the outer hop. If exceeds limitations we fail the forward using a
InboundHTLCErrwith specified local reason.