Decrypt Trampoline error onions #3657

arik-so · 2025-03-10T06:36:17Z

No description provided.

ldk-reviews-bot · 2025-03-10T06:36:19Z

👋 Thanks for assigning @joostjager 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.

arik-so

Draft for now due to the clones, but am open to suggestions.

lightning/src/ln/onion_utils.rs

ldk-reviews-bot · 2025-03-10T06:37:33Z

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

codecov · 2025-03-10T06:56:13Z

Codecov Report

Attention: Patch coverage is 96.69211% with 13 lines in your changes missing coverage. Please review.

Project coverage is 89.25%. Comparing base (4c43a5b) to head (e7d5c53).

Files with missing lines	Patch %	Lines
lightning/src/ln/onion_utils.rs	96.66%	7 Missing and 6 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3657      +/-   ##
==========================================
+ Coverage   89.24%   89.25%   +0.01%     
==========================================
  Files         155      155              
  Lines      119280   119584     +304     
  Branches   119280   119584     +304     
==========================================
+ Hits       106446   106735     +289     
- Misses      10240    10247       +7     
- Partials     2594     2602       +8

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

joostjager · 2025-03-10T10:14:13Z

lightning/src/ln/onion_utils.rs

 	// Handle packed channel/node updates for passing back for the route handler
-	let callback = |shared_secret, _, _, route_hop_opt: Option<&RouteHop>, route_hop_idx| {
+	for (route_hop_idx, (route_hop_option, shared_secret)) in onion_keys.into_iter().enumerate() {


I like this refactoring. And I am also wondering if you can take it another step further by extracting this main for loop into a function that returns the FailureLearnings. To avoid this modification of variables outside of the loop.

definitely doable, but I first wanna make sure the approach is sound

joostjager

Overall clean approach where indeed failure message processing isn't touched all that much. Perhaps add some more comments explaining the code, especially for devs who do not fully understand trampoline, and isolate a bit more refactor into separate commits.

One question that I haven't fully answered is how this intersects with attributable failures. That pertains mainly to the spec level of course.

joostjager · 2025-03-10T10:14:37Z

lightning/src/ln/onion_utils.rs

 		if res.is_some() {
-			return;
+			break;


res.is_some is never going to happen now with the breaks?

turns out it actually was, but I made it a bit more obvious

I do think all of these breaks would be avoidable with a simple return statement though once the loop is in its own function

yes, that would also help with accidentally missing a break in some future change

joostjager · 2025-03-10T10:17:04Z

lightning/src/ln/onion_utils.rs

+						cltv_expiry_delta: 36,
+					}
+				],
+				hops: vec![


Interesting. So here a combination is made of three trampoline nodes, where the last one is also the start of the blinded path towards the final destination? Maybe useful to add a high level comment explaining this test.

I added comments explaining that that particular unit test is just for testing cryptograph, and not for the test vectors. I added a separate unit test for the test vectors, but we still need intermediate Trampoline hop forwarding support to properly handle those components of the test vector.

can both tests be merged, possibly by extending the test vectors?

lightning/src/ln/onion_utils.rs

joostjager · 2025-03-10T10:49:34Z

lightning/src/ln/onion_utils.rs

-		path.blinded_tail.as_ref(),
-		session_priv,
+		blinded_tail,
+		outer_session_priv.as_ref().unwrap_or(session_priv),


I had to look twice at why outer_session_priv is set to None for non-trampoline, and is again unwrap_or'ed here. Perhaps it is easier to follow if setting this specific key also near let (blinded_tail, outer_session_priv) = ...?

yeah, optimally all of these Trampoline-specific overrides would be done in one location, I'm looking how to best combine them

Maybe something can be gained just with naming. By always have an outer_session_priv and optionally also an inner_session_priv?

joostjager · 2025-03-10T10:51:44Z

lightning/src/ln/onion_utils.rs

 		},
 	)
 	.expect("Route we used spontaneously grew invalid keys in the middle of it?");

+	path.blinded_tail.as_ref().map(|bt| {


Some comments explaining what's happening could be helpful. The goal is I think to create a list of onion keys that is the concatenation of the path to the first trampoline, the trampoline nodes, and then the blinded path?

lightning/src/ln/onion_utils.rs

joostjager · 2025-03-10T14:22:11Z

It might be nice to link to a tracking issue that links all trampoline work together?

joostjager · 2025-03-11T10:58:34Z

lightning/src/ln/onion_utils.rs

@@ -943,6 +943,7 @@ fn decrypt_onion_error_packet(packet: &mut Vec<u8>, shared_secret: SharedSecret)
 #[inline]
 pub(super) fn process_onion_failure<T: secp256k1::Signing, L: Deref>(
 	secp_ctx: &Secp256k1<T>, logger: &L, htlc_source: &HTLCSource, mut encrypted_packet: Vec<u8>,
+	secondary_session_priv: Option<SecretKey>,


If it is just for testing, it shouldn't be on the public interface?

perhaps an internal façade would do the trick?

ah, actually, the interface isn't public, it's just visible to the crate

lightning/src/ln/onion_utils.rs

lightning/src/routing/router.rs

lightning/src/ln/onion_utils.rs

In an upcoming commit, we will need to decrypt error onions constructed from multiple session_privs. In order to simplify the code legibility, we move from a single-iteration model to one where we first aggregate the shared secrets, and then use them for the error decryption.

We currently check whether our hop is the last in the path by accessing the hops vector by the next index. However, once we start handling Trampoline hops that will become inadequate. Instead, we switch it to check whether there is a subsequent element in the iterator.

When we start handling Trampoline, the hops in our error decryption path could be either `RouteHop`s or `TrampolineHop`s. To avoid excessive code duplication, we introduce an enum with some methods for common accessors.

We don't need to recalculate the blinded hop count on each iteration, and clarify the meaning of `is_from_final_hop` to mean that it refers to all non-blinded hops, including Trampoline.

Rather than solely iterating over `RouteHop`s, we now also append the shared secrets from the inner onion containing `TrampolineHop`s.

Create unit tests covering the hybrid outer and inner onion shared secrets, as well as the Trampoline error test vectors. Additionally, we allow the `outer_session_priv` to be overridden to accommodate the test vector requirements.

joostjager

No major points. Still really happy with the commit structure.

joostjager · 2025-03-12T10:34:45Z

lightning/src/ln/onion_utils.rs

-		if res.is_some() {
-			return;
-		}
+	let mut onion_keys = Vec::with_capacity(path.hops.len());


Shouldn't the length of the blinded tail be added here too?

when I move out the blinded_hop_count to optimize the initial vector capacity allocation, the pre-Trampoline optimization commit becomes just the is_final_hop rename, which I think should just be squashed into the subsequent commit that sees the introduction of Trampoline nodes

joostjager · 2025-03-12T10:36:34Z

lightning/src/ln/onion_utils.rs

+			onion_keys.push((route_hop_option.cloned(), shared_secret))
+		},
+	)
+	.expect("Route we used spontaneously grew invalid keys in the middle of it?");


I suppose it doesn't matter what text is in here, but isn't this an implementation detail that the caller can't really know?

it is. Really it is for us, I think, because this should never be getting hit

joostjager · 2025-03-12T10:40:21Z

lightning/src/ln/onion_utils.rs

@@ -1008,13 +1009,13 @@ where
 		// from the current hop (i.e., the next hop's inbound channel).
 		let num_blinded_hops = path.blinded_tail.as_ref().map_or(0, |bt| bt.hops.len());
 		// For 1-hop blinded paths, the final `path.hops` entry is the recipient.
-		is_from_final_node = route_hop_idx + 1 == path.hops.len() && num_blinded_hops <= 1;
+		is_from_final_node = iterator.peek().is_none() && num_blinded_hops <= 1;


iterator.peek().is_none() - is this really the same as route_hop_idx + 1 == path.hops.len(), because the latter doesn't include the blinded tail?

it is only the same when num_blinded_hops <= 1, but because that's also a condition, the end result is equivalent

joostjager · 2025-03-12T10:41:22Z

lightning/src/ln/onion_utils.rs

-			match path.hops.get(route_hop_idx + 1) {
-				Some(hop) => hop,
-				None => {
+			match iterator.peek() {


Store peek in a variable instead of peeking twice (here and above)?

If you're touching this code anyway, it might benefit from a comment explaining why we are now getting the next hop.

joostjager · 2025-03-12T10:46:36Z

lightning/src/ln/onion_utils.rs

-		path.blinded_tail.as_ref(),
-		session_priv,
+		blinded_tail,
+		outer_session_priv.as_ref().unwrap_or(session_priv),


Maybe something can be gained just with naming. By always have an outer_session_priv and optionally also an inner_session_priv?

joostjager · 2025-03-12T10:48:57Z

lightning/src/ln/onion_utils.rs

 		|shared_secret, _, _, route_hop_option: Option<&RouteHop>, _| {
 			onion_keys.push((route_hop_option.map(|rh| ErrorHop::RouteHop(rh)), shared_secret))
 		},
 	)
 	.expect("Route we used spontaneously grew invalid keys in the middle of it?");

+	if path.has_trampoline_hops() {


Maybe this method can become get_trampoline_hops returning an option, and then only calling it once at the top and reusing the result? That would avoid &path.blinded_tail.as_ref().unwrap().trampoline_hops below.

if we have a blinded tail, the number of Trampoline hops within it could still be 0. Should this method then return None, or Some with an empty array?

Ah okay, there are two cases. Yes, this is just a thought. If you think it doesn't really solve anything in terms or readability, just ignore.

joostjager · 2025-03-12T10:50:45Z

lightning/src/ln/onion_utils.rs

-		let session_priv_hash = Sha256::hash(&session_priv.secret_bytes()).to_byte_array();
-		SecretKey::from_slice(&session_priv_hash[..]).expect("You broke SHA-256!")
+		secondary_session_priv.unwrap_or_else(|| {
+			let session_priv_hash = Sha256::hash(&session_priv.secret_bytes()).to_byte_array();


Perhaps an alternative is to now move the hashing to the caller, so that this function just always gets a secondary key (not just for testing)?

hm, I think it's better for the hashing to happen inside the method so the caller needs not be aware of the process, and I can see if I can hide the secondary_session_priv behind a test cfg.

joostjager · 2025-03-12T10:57:09Z

lightning/src/ln/onion_utils.rs

+			payment_id: PaymentId([1; 32]),
+		};
+
+		{


A comment per test case would be nice here. Reorg worked out well.

joostjager · 2025-03-12T10:59:09Z

lightning/src/ln/onion_utils.rs

 		if res.is_some() {
-			return;
+			break;


yes, that would also help with accidentally missing a break in some future change

joostjager · 2025-03-12T10:59:40Z

lightning/src/ln/onion_utils.rs

+						cltv_expiry_delta: 36,
+					}
+				],
+				hops: vec![


can both tests be merged, possibly by extending the test vectors?

joostjager · 2025-03-12T12:41:04Z

lightning/src/ln/onion_utils.rs

@@ -987,7 +987,8 @@ where
 	.expect("Route we used spontaneously grew invalid keys in the middle of it?");

 	// Handle packed channel/node updates for passing back for the route handler
-	for (route_hop_idx, (route_hop_option, shared_secret)) in onion_keys.into_iter().enumerate() {
+	let mut iterator = onion_keys.into_iter().peekable();


I am not sure if this commit is really the best idea. I've been (incompletely) rebasing attributable failures on top, and there I need that route_hop_idx again to index into the hmacs and payloads.

main...joostjager:rust-lightning:attr-errs-on-trampoline

here's an example commit that adds an index to the peekable enumeration: 2ef4f27

Would that at all be helpful for attribution? Given our offline discussion, you'd still need to check whether the hop is regular or Trampoline, I think, but it should make it simpler.

valentinewallace

Looks good! Don't have any feedback on the first pass. Commit history is indeed very reviewable. Will take another look and more closely at the tests tomorrow.

arik-so commented Mar 10, 2025

View reviewed changes

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

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

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

arik-so requested a review from joostjager March 10, 2025 06:38

arik-so force-pushed the arik/trampoline/error-decryption branch from dd8098b to af31dd1 Compare March 10, 2025 06:47

arik-so force-pushed the arik/trampoline/error-decryption branch 2 times, most recently from 9caa2b6 to a2dc11f Compare March 10, 2025 07:25

joostjager reviewed Mar 10, 2025

View reviewed changes

arik-so mentioned this pull request Mar 3, 2025

Trampoline #2299

Open

30 tasks

arik-so force-pushed the arik/trampoline/error-decryption branch from a2dc11f to e7d5c53 Compare March 11, 2025 05:39

joostjager reviewed Mar 11, 2025

View reviewed changes

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

joostjager reviewed Mar 11, 2025

View reviewed changes

lightning/src/routing/router.rs Show resolved Hide resolved

joostjager reviewed Mar 11, 2025

View reviewed changes

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

joostjager reviewed Mar 11, 2025

View reviewed changes

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

arik-so force-pushed the arik/trampoline/error-decryption branch 2 times, most recently from d3cef50 to 3e45cb2 Compare March 11, 2025 20:54

arik-so added 2 commits March 11, 2025 14:22

arik-so force-pushed the arik/trampoline/error-decryption branch from 3e45cb2 to 36f8514 Compare March 11, 2025 21:34

arik-so marked this pull request as ready for review March 11, 2025 21:36

ldk-reviews-bot requested a review from valentinewallace March 11, 2025 21:36

arik-so added 4 commits March 11, 2025 18:26

Introduce ErrorHop enum

7641788

When we start handling Trampoline, the hops in our error decryption path could be either `RouteHop`s or `TrampolineHop`s. To avoid excessive code duplication, we introduce an enum with some methods for common accessors.

Pre-Trampoline optimizations

6863ebd

We don't need to recalculate the blinded hop count on each iteration, and clarify the meaning of `is_from_final_hop` to mean that it refers to all non-blinded hops, including Trampoline.

Handle Trampoline hops in error decryption

e60adce

Rather than solely iterating over `RouteHop`s, we now also append the shared secrets from the inner onion containing `TrampolineHop`s.

Trampoline error decryption and vector tests

83e58e9

Create unit tests covering the hybrid outer and inner onion shared secrets, as well as the Trampoline error test vectors. Additionally, we allow the `outer_session_priv` to be overridden to accommodate the test vector requirements.

arik-so force-pushed the arik/trampoline/error-decryption branch from 36f8514 to 83e58e9 Compare March 12, 2025 01:27

arik-so requested a review from joostjager March 12, 2025 06:36

joostjager reviewed Mar 12, 2025

View reviewed changes

valentinewallace reviewed Mar 12, 2025

View reviewed changes

Decrypt Trampoline error onions #3657

Are you sure you want to change the base?

Decrypt Trampoline error onions #3657

Conversation

arik-so commented Mar 10, 2025

ldk-reviews-bot commented Mar 10, 2025 • edited Loading

arik-so left a comment

Choose a reason for hiding this comment

ldk-reviews-bot commented Mar 10, 2025

codecov bot commented Mar 10, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joostjager left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joostjager commented Mar 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joostjager left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arik-so Mar 12, 2025 • edited Loading

Choose a reason for hiding this comment

valentinewallace left a comment

Choose a reason for hiding this comment

ldk-reviews-bot commented Mar 10, 2025 •

edited

Loading

codecov bot commented Mar 10, 2025 •

edited

Loading

joostjager commented Mar 10, 2025 •

edited

Loading

arik-so Mar 12, 2025 •

edited

Loading