Trampoline Forwarding: Enforce trampoline constraints #3983

a-mpch · 2025-08-02T04:01:04Z

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 InboundHTLCErr with specified local reason.

ldk-reviews-bot · 2025-08-02T04:01:07Z

I've assigned @tankyleo as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

codecov · 2025-08-02T04:06:29Z

Codecov Report

❌ Patch coverage is 83.94649% with 48 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.76%. Comparing base (381416a) to head (3515844).

Files with missing lines	Patch %	Lines
lightning/src/ln/onion_payment.rs	48.38%	31 Missing and 1 partial ⚠️
lightning/src/ln/blinded_payment_tests.rs	95.67%	7 Missing and 3 partials ⚠️
lightning/src/ln/onion_utils.rs	0.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3983      +/-   ##
==========================================
- Coverage   88.94%   88.76%   -0.18%     
==========================================
  Files         174      173       -1     
  Lines      124575   124753     +178     
  Branches   124575   124753     +178     
==========================================
- Hits       110797   110732      -65     
- Misses      11278    11602     +324     
+ Partials     2500     2419      -81

Flag	Coverage Δ
fuzzing	`?`
tests	`88.76% <83.94%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

lightning/src/ln/onion_utils.rs

ldk-reviews-bot · 2025-08-04T04:12:11Z

🔔 1st Reminder

Hey @tankyleo! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

tankyleo

This is a more mechanical review as I am not familiar with this part of the codebase :)

lightning/src/ln/onion_utils.rs

lightning/src/ln/onion_payment.rs

ldk-reviews-bot · 2025-08-06T02:30:38Z

👋 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.

lightning/src/ln/channelmanager.rs

tankyleo · 2025-08-06T03:01:02Z

Also when I remove everything but the test code, the test does not fail - is that intentional ? I have not reviewed the test

a-mpch · 2025-08-06T20:33:02Z

thanks @tankyleo !

Also when I remove everything but the test code, the test does not fail - is that intentional ? I have not reviewed the test

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 figured out that for this PR it's ok to have one test that checks for the CLTV constraint and later when having the capacity of forwarding we can test the constraint when forwarding related to the amt.

I added fixups for each of your comments and rebased in top of main

tankyleo · 2025-08-06T22:59:44Z

lightning/src/ln/blinded_payment_tests.rs

+		.with_payment_preimage(payment_preimage)
+		.without_claimable_event()
+		.expect_failure(HTLCHandlingFailureType::Receive { payment_hash });
+
 	do_pass_along_path(args);


Confirmed the test fails, then passes with the added code in the PR thank you! Can you help me understand why we fail here when we remove the code from the PR ?

Without knowing much I would have expected us to fail at expect_payment_failed_conditions further below :)

My intention was to separate the big PR into smaller ones. This one seemed as a possibility to separate work but many of the code must be tested when the final feature of forwarding arrives.
That being said, in order to test the enforcement of the failed conditions, I modified the CLTV of the outer onion to something lower but acceptable (payment would be received) and maintaining the cltv_expiry of the receiving (that probably isn't clear, I'll modify it).
So in the case of no enforcement, payment is received. In case with enforcement it should fail because of the CLTV that the trampoline has is greater than the outer blinded path.

tankyleo · 2025-08-06T23:02:03Z

lightning/src/ln/blinded_payment_tests.rs

+					.expected_htlc_error_data(LocalHTLCFailureReason::FinalIncorrectHTLCAmount, &[0, 0, 0, 0, 0, 0, 3, 232]);
+			expect_payment_failed_conditions(&nodes[0], payment_hash, false, payment_failed_conditions);
+		},
+		TrampolineConstraintFailureScenarios::TrampolineCLTVGreaterThanOnion => {
+				let payment_failed_conditions = PaymentFailedConditions::new()
+					.expected_htlc_error_data(LocalHTLCFailureReason::FinalIncorrectCLTVExpiry, &[0, 0, 0, 84]);


Would be cool to document how we got these numbers, why .., 0, 3, 232 and why .., 0, 0, 84 ? Someone who comes here later might have a hard time understanding these constants :)

tankyleo · 2025-08-06T23:03:40Z

lightning/src/ln/blinded_payment_tests.rs

@@ -2257,6 +2485,8 @@ fn test_trampoline_unblinded_receive() {
 		&secp_ctx, path.into_iter(), &carol_alice_trampoline_session_priv,
 	).unwrap();

+	let trampoline_cltv_value = if failure_scenario == TrampolineConstraintFailureScenarios::TrampolineCLTVGreaterThanOnion { 52 } else { 72 };


Similar here, why did you pick 52 and 72 for the thresholds ? Thank you!

tankyleo · 2025-08-06T23:05:40Z

lightning/src/ln/blinded_payment_tests.rs

+		}
+		{
+		    let payment_failed_conditions = PaymentFailedConditions::new()
+				    .expected_htlc_error_data(LocalHTLCFailureReason::FinalIncorrectHTLCAmount, &[0, 0, 0, 0, 0, 0, 3, 232]);


Similar question here about where the 3, 232 comes from :)

tankyleo · 2025-08-06T23:14:24Z

lightning/src/ln/onion_payment.rs

+fn check_trampoline_onion_constraints(
+	outer_hop_data: &msgs::InboundTrampolineEntrypointPayload, trampoline_cltv_value: u32,
+	trampoline_amount: u64,
+) -> Result<(), (LocalHTLCFailureReason, Vec<u8>)> {


Here how about we set the Err variant to InboundHTLCErr, and we keep the map_err to a LocalHTLCFailureReason::InvalidOnionBlinding reason when inside a blinded path ?

tankyleo · 2025-08-06T23:15:17Z

lightning/src/ln/blinded_payment_tests.rs

+	TrampolineAmountGreaterThanOnion,
+}
+
+fn do_test_trampoline_unblinded_receive_contraint_failure(failure_scenario: TrampolineConstraintFailureScenarios) {


nit: fix typo :)

tankyleo · 2025-08-06T23:16:31Z

lightning/src/ln/blinded_payment_tests.rs

@@ -2086,7 +2086,7 @@ fn do_test_trampoline_single_hop_receive(success: bool) {
 						pubkey: carol_node_id,
 						node_features: Features::empty(),
 						fee_msat: amt_msat,
-						cltv_expiry_delta: 24,


Why is this change needed ? Thanks for the context.

To calculate the outer onion of this receiving hop we do cltv_expiry_delta of the hop plus the current height
In this case (24 + 32 = 56)
Then when building the trampoline we do excess_final_ctlv_expiry_delta + current_height (39+32 = 72)

With the new constraints trampoline > outer onion. So it fails.

tankyleo · 2025-08-07T00:07:11Z

lightning/src/ln/blinded_payment_tests.rs

-						fee_msat: amt_msat,
-						cltv_expiry_delta: 24,
+						fee_msat: 0,
+						cltv_expiry_delta: 72,


Similar here about why this is needed thank you (take a look at this comment on the original commit, 4132e78 it might be clearer)

tankyleo · 2025-08-07T00:17:22Z

lightning/src/ln/blinded_payment_tests.rs

+}
+
+#[test]
+fn test_trampoline_enforced_constrait_cltv() {


One more typo to squash here :)

tankyleo · 2025-08-07T00:20:56Z

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 !

We add a `check_trampoline_contraints` similar to `check_blinded_path_constraints` that compares the Trampoline onion's amount and CLTV values to the limitations impose by outer onion. We also add the expected errors by the spec: `TemporaryTrampolineFailure`, `TrampolineFeeOrExpiryInsufficient` and `UnknownNextTrampoline` to be used in case of errors when doing forwarding or validating the constraints. Finally, we add and modified the following tests: - Modified the unblinded receive to validate when receiving amt less than the expected. - Modified test with wrong CLTV parameters that has fails with new enforcement of CTLV limits. - Add unblinded receive test that forces trampoline onion's CLTV to be greater than the the outer onion packet. Note that there are some TODO's to be fixed in following commits as we need the full trampoline forwarding feature to effectivelly test all caseos. Co-authored-by: Arik Sosman <git@arik.io>

a-mpch · 2025-08-07T22:02:57Z

@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

graphite-app bot reviewed Aug 2, 2025

View reviewed changes

lightning/src/ln/onion_utils.rs Outdated Show resolved Hide resolved

ldk-reviews-bot requested a review from tankyleo August 2, 2025 04:11

tankyleo reviewed Aug 6, 2025

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

a-mpch force-pushed the 2025-08-enforce-trampoline-constraint branch from 4deaa3e to d37d6d8 Compare August 6, 2025 20:35

tankyleo reviewed Aug 7, 2025

View reviewed changes

a-mpch force-pushed the 2025-08-enforce-trampoline-constraint branch from d37d6d8 to 3515844 Compare August 7, 2025 22:01

Trampoline Forwarding: Enforce trampoline constraints #3983

Are you sure you want to change the base?

Trampoline Forwarding: Enforce trampoline constraints #3983

Uh oh!

Conversation

a-mpch commented Aug 2, 2025

Uh oh!

ldk-reviews-bot commented Aug 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

ldk-reviews-bot commented Aug 4, 2025

Uh oh!

tankyleo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ldk-reviews-bot commented Aug 6, 2025

Uh oh!

Uh oh!

tankyleo commented Aug 6, 2025

Uh oh!

a-mpch commented Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tankyleo commented Aug 7, 2025

Uh oh!

a-mpch commented Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ldk-reviews-bot commented Aug 2, 2025 •

edited

Loading

codecov bot commented Aug 2, 2025 •

edited

Loading

a-mpch commented Aug 6, 2025 •

edited

Loading

a-mpch commented Aug 7, 2025 •

edited

Loading