Trampoline test refactor #4095

a-mpch · 2025-09-19T13:53:48Z

This PR is on top of @valentinewallace branch that simplifies how trampoline testing is done. These changes surfaced a bug on how we were building the errors when the receiver is also a Trampoline inside the route. Specifically the first on the route as we aren't routing yet.

The final commit fixes this bug adding trampoline shared secret so we double obfuscate errors in these cases.

ldk-reviews-bot · 2025-09-19T13:53:51Z

👋 I see @tankyleo was un-assigned.
If you'd like another reviewer assignment, please click here.

lightning/src/ln/channelmanager.rs

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.

graphite-app · 2025-09-19T14:21:19Z

lightning/src/ln/channelmanager.rs

 					&payment_hash,
 					onion_error
 				);
+				let secondary_shared_secret = trampoline_shared_secret.or(*phantom_shared_secret);


There appears to be a type mismatch in this line. trampoline_shared_secret is an Option<[u8; 32]> while phantom_shared_secret is a reference &Option<[u8; 32]>.

The .or() method expects both options to be of the same type. Consider using .or_else(|| *phantom_shared_secret) instead to properly handle the different option types.

Suggested change

let secondary_shared_secret = trampoline_shared_secret.or(*phantom_shared_secret);

let secondary_shared_secret = trampoline_shared_secret.or_else(|| *phantom_shared_secret);

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

codecov · 2025-09-19T14:32:44Z

Codecov Report

❌ Patch coverage is 99.11894% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.65%. Comparing base (0cb7f54) to head (fcc2651).
⚠️ Report is 166 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/blinded_payment_tests.rs	99.44%	1 Missing ⚠️
lightning/src/ln/channelmanager.rs	92.30%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4095      +/-   ##
==========================================
+ Coverage   88.61%   88.65%   +0.03%     
==========================================
  Files         176      180       +4     
  Lines      132120   135288    +3168     
  Branches   132120   135288    +3168     
==========================================
+ Hits       117083   119934    +2851     
- Misses      12367    12586     +219     
- Partials     2670     2768      +98

Flag	Coverage Δ
fuzzing	`21.76% <10.63%> (+0.21%)`	⬆️
tests	`88.49% <99.11%> (+0.03%)`	⬆️

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.

valentinewallace

Thanks @a-mpch!

lightning/src/ln/channelmanager.rs

lightning/src/ln/blinded_payment_tests.rs

lightning/src/ln/channelmanager.rs

valentinewallace · 2025-09-19T16:24:00Z

lightning/src/ln/onion_payment.rs

 			}, ..
 		} =>
 			(payment_data, keysend_preimage, custom_tlvs, sender_intended_htlc_amt_msat,
-				cltv_expiry_height, payment_metadata, None, false, keysend_preimage.is_none(), None),
+				cltv_expiry_height, payment_metadata, None, false, keysend_preimage.is_none(), None, None),


I think we should also be returning the trampoline ss here? Looks like if we adapted test_trampoline_unblinded_receive to cover both the success and the failure case, it would catch this

yes, just tested. Added a commit

a-mpch · 2025-09-19T20:05:56Z

Thanks @valentinewallace, all comments had been addressed. Re-ordered commits and added one for test_trampoline_unblinded_receive so it catches the one mentioned.

valentinewallace

LGTM if CI is happy

lightning/src/ln/onion_utils.rs

valentinewallace · 2025-09-19T21:26:55Z

lightning/src/ln/channelmanager.rs

 					&payment_hash,
 					onion_error
 				);
+				let secondary_shared_secret = trampoline_shared_secret.or(*phantom_shared_secret);


I think in the case of trampoline + phantom we may need a third layer of onion wrapping for the error packet... not convinced we need to worry about this though. Are you able to edit the trampoline issue to document this case for follow-up?

I can't edit the issue

If we don't support T+P, it may be better to error rather than prioritize T?

Error how? We need to fail backwards, I think? Added this to the tracking issue.

Hm, failing to fail isn't quite possible. There could be a log here perhaps if we unexpectedly get T+P?

A comment or log seems like a good idea to me. Log would be conditional which would add some logic so slightly prefer a comment

amended changes with a comment!

ldk-reviews-bot · 2025-09-22T00:00:39Z

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

a-mpch · 2025-09-22T13:41:10Z

@valentinewallace added the removal of the unused to the commit 80c6ad5

Also there are some flaky test that are failing also in other PRs. I think they are not related to this PR if CI is not happy.

tests::test_background_event_handling
 tests::test_payment_path_scoring

ldk-reviews-bot · 2025-09-24T00:01:37Z

🔔 2nd 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.

ldk-reviews-bot · 2025-09-27T00:00:39Z

🔔 3rd 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.

ldk-reviews-bot · 2025-09-29T00:01:20Z

🔔 4th 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.

ldk-reviews-bot · 2025-10-01T00:01:56Z

🔔 5th 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.

ldk-reviews-bot · 2025-10-06T00:00:07Z

🔔 1st Reminder

Hey @joostjager! 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.

lightning/src/ln/onion_utils.rs

lightning/src/ln/blinded_payment_tests.rs

joostjager · 2025-10-07T06:58:00Z

lightning/src/ln/channelmanager.rs

 					&payment_hash,
 					onion_error
 				);
+				let secondary_shared_secret = trampoline_shared_secret.or(*phantom_shared_secret);


If we don't support T+P, it may be better to error rather than prioritize T?

joostjager · 2025-10-07T07:11:09Z

lightning/src/ln/blinded_payment_tests.rs

 }

+#[test]
+fn test_trampoline_forward_payload_encoded_as_receive() {


Squashing this commit with the previous, would it make for a nicer diff?

Hmm, can you check out how it looks like squashed in the GH UI? It kind of looks like it mixes the tests in with each other. Not convinced it's more readable, definitely not gonna block on it though ofc. I agree it's a bit unfortunate how these test diffs aren't the easiest to review...

Good point about GH UI. And indeed, it doesn't look good at all.

lightning/src/ln/onion_utils.rs

lightning/src/ln/blinded_payment_tests.rs

joostjager · 2025-10-08T07:20:51Z

lightning/src/ln/channelmanager.rs

 					&payment_hash,
 					onion_error
 				);
+				let secondary_shared_secret = trampoline_shared_secret.or(*phantom_shared_secret);


Hm, failing to fail isn't quite possible. There could be a log here perhaps if we unexpectedly get T+P?

joostjager · 2025-10-08T07:21:39Z

lightning/src/ln/channelmanager.rs

 		(5, custom_tlvs, optional_vec),
 		(7, requires_blinded_error, (default_value, false)),
 		(9, payment_context, option),
+		(11, trampoline_shared_secret, option),


Instead of adding a new field, could it also work by repurposing phantom_shared_secret as secondary_shared_secret and have it contain either the T or P secret?

I like a separate field because then we leave the door open to support failing backwards properly (by wrapping the error onion three times) for P+T payments in the future

When handling HTLC failures in trampoline routing, error packets were not being properly encrypted with the trampoline shared secret. This caused error messages to be unreadable by the original sender when failures occurred within trampoline hops. The fix prioritizes trampoline_shared_secret over phantom_shared_secret when both are available, ensuring error packets can be properly decrypted by trampoline senders.

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.

This re-adds test coverage for a case that was removed in the previous commit.

Previously this commit, test only check for succesfully claimed case. Now tests that successfully fail the HTLC and sender can read the error.

a-mpch mentioned this pull request Sep 19, 2025

Trampoline Forwarding: Enforce trampoline constraints #3983

Open

graphite-app bot reviewed Sep 19, 2025

View reviewed changes

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

valentinewallace added 2 commits September 19, 2025 11:01

Simplify test_trampoline_unblinded_receive

c997f22

No need to construct unused blinded hop data or hardcode session privs/prng seeds.

ldk-reviews-bot requested a review from tankyleo September 19, 2025 14:04

a-mpch force-pushed the 2025-08-trampoline-test-refactor branch 2 times, most recently from b426301 to 1abdff1 Compare September 19, 2025 14:19

graphite-app bot reviewed Sep 19, 2025

View reviewed changes

valentinewallace self-requested a review September 19, 2025 14:28

valentinewallace reviewed Sep 19, 2025

View reviewed changes

a-mpch force-pushed the 2025-08-trampoline-test-refactor branch from 1abdff1 to a0f4c41 Compare September 19, 2025 20:04

a-mpch requested a review from valentinewallace September 19, 2025 20:06

valentinewallace previously approved these changes Sep 19, 2025

View reviewed changes

a-mpch dismissed valentinewallace’s stale review via cf9ba95 September 22, 2025 13:37

a-mpch force-pushed the 2025-08-trampoline-test-refactor branch from a0f4c41 to cf9ba95 Compare September 22, 2025 13:37

a-mpch requested a review from valentinewallace September 22, 2025 13:37

valentinewallace previously approved these changes Sep 22, 2025

View reviewed changes

tankyleo requested review from joostjager and removed request for tankyleo October 3, 2025 17:04

joostjager reviewed Oct 7, 2025

View reviewed changes

joostjager reviewed Oct 8, 2025

View reviewed changes

a-mpch and others added 4 commits October 8, 2025 12:53

Test trampoline fwd payload encoded as receive

94b5b1d

This re-adds test coverage for a case that was removed in the previous commit.

Add failure case on test unblided receive

fcc2651

Previously this commit, test only check for succesfully claimed case. Now tests that successfully fail the HTLC and sender can read the error.

a-mpch dismissed valentinewallace’s stale review via fcc2651 October 8, 2025 19:53

a-mpch force-pushed the 2025-08-trampoline-test-refactor branch from cf9ba95 to fcc2651 Compare October 8, 2025 19:53

valentinewallace approved these changes Oct 8, 2025

View reviewed changes

joostjager approved these changes Oct 9, 2025

View reviewed changes

	let secondary_shared_secret = trampoline_shared_secret.or(*phantom_shared_secret);
	let secondary_shared_secret = trampoline_shared_secret.or_else(\|\| *phantom_shared_secret);

Trampoline test refactor #4095

Are you sure you want to change the base?

Trampoline test refactor #4095

Conversation

a-mpch commented Sep 19, 2025

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

graphite-app bot Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

valentinewallace left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a-mpch commented Sep 19, 2025

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

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!

valentinewallace Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentinewallace Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Sep 22, 2025

Uh oh!

a-mpch commented Sep 22, 2025

Uh oh!

ldk-reviews-bot commented Sep 24, 2025

Uh oh!

ldk-reviews-bot commented Sep 27, 2025

Uh oh!

ldk-reviews-bot commented Sep 29, 2025

Uh oh!

ldk-reviews-bot commented Oct 1, 2025

Uh oh!

ldk-reviews-bot commented Oct 6, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentinewallace Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Sep 19, 2025 •

edited

Loading

codecov bot commented Sep 19, 2025 •

edited

Loading

valentinewallace left a comment •

edited

Loading

valentinewallace Oct 7, 2025 •

edited

Loading

valentinewallace Oct 8, 2025 •

edited

Loading

valentinewallace Oct 8, 2025 •

edited

Loading