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.

lightning/src/ln/onion_utils.rs

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.

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?

lightning/src/ln/onion_utils.rs

lightning/src/routing/router.rs

lightning/src/ln/onion_utils.rs

joostjager

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

lightning/src/ln/onion_utils.rs

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.

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.

lightning/src/ln/onion_utils.rs

joostjager · 2025-03-13T17:48:10Z

lightning/src/ln/onion_utils.rs

-		} => (path, session_priv, first_hop_htlc_msat),
-		_ => {
-			unreachable!()
+	let (path, first_hop_htlc_msat) = match htlc_source {


Would you want to lift this out too, or is that a step too far?

I am not sure if we want to do that quite yet given that I expect to have another HTLCSource variant we'll need to create and support here for the forwarding scenario

joostjager · 2025-03-13T17:48:32Z

lightning/src/ln/onion_utils.rs

-	mut encrypted_packet: OnionErrorPacket,
+pub(super) fn process_onion_failure_inner<T: secp256k1::Signing, L: Deref>(
+	secp_ctx: &Secp256k1<T>, logger: &L, htlc_source: &HTLCSource, outer_session_priv: &SecretKey,
+	inner_session_priv: Option<&SecretKey>, mut encrypted_packet: OnionErrorPacket,


yes, love the explicitness of inner and outer

joostjager

LGTM

valentinewallace

Looks good, can always fix this nit in follow-up!

lightning/src/ln/onion_utils.rs

Rather than solely iterating over `RouteHop`s, we now also append the shared secrets from the inner onion containing `TrampolineHop`s. Additionally, we allow the `outer_session_priv` to be overridden to accommodate the test vector requirements. We do so by separating out a façade method without the override.

valentinewallace · 2025-03-13T20:15:41Z

Small diff since @joostjager's ACK so this can land when CI is happy!

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

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

joostjager reviewed Mar 10, 2025

View reviewed changes

arik-so mentioned this pull request Mar 3, 2025

Trampoline #2299

Open

31 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 Outdated Show resolved Hide resolved

joostjager reviewed Mar 11, 2025

View reviewed changes

lightning/src/ln/onion_utils.rs Outdated 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 Outdated 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 3 times, most recently 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 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

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

valentinewallace reviewed Mar 12, 2025

View reviewed changes

arik-so added 2 commits March 13, 2025 00:33

Introduce ErrorHop enum

7ded4c3

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.

arik-so force-pushed the arik/trampoline/error-decryption branch from 83e58e9 to 98f2d38 Compare March 13, 2025 07:47

joostjager reviewed Mar 13, 2025

View reviewed changes

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

joostjager reviewed Mar 13, 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 Show resolved Hide resolved

arik-so force-pushed the arik/trampoline/error-decryption branch 5 times, most recently from 59993a8 to 0f5cbdd Compare March 13, 2025 17:40

joostjager reviewed Mar 13, 2025

View reviewed changes

arik-so force-pushed the arik/trampoline/error-decryption branch from 0f5cbdd to f03cc62 Compare March 13, 2025 18:06

joostjager previously approved these changes Mar 13, 2025

View reviewed changes

valentinewallace previously approved these changes Mar 13, 2025

View reviewed changes

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

arik-so dismissed stale reviews from valentinewallace and joostjager via 6d63c24 March 13, 2025 20:14

arik-so force-pushed the arik/trampoline/error-decryption branch from f03cc62 to 6d63c24 Compare March 13, 2025 20:14

valentinewallace approved these changes Mar 13, 2025

View reviewed changes

joostjager approved these changes Mar 13, 2025

View reviewed changes

arik-so merged commit 87c5cae into lightningdevkit:main Mar 14, 2025
25 of 27 checks passed

Decrypt Trampoline error onions #3657

Decrypt Trampoline error onions #3657

Uh oh!

Conversation

arik-so commented Mar 10, 2025

Uh oh!

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

Uh oh!

Uh oh!

arik-so left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ldk-reviews-bot commented Mar 10, 2025

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

joostjager left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joostjager commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joostjager left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joostjager Mar 13, 2025

Choose a reason for hiding this comment

Uh oh!

arik-so Mar 13, 2025

Choose a reason for hiding this comment

Uh oh!

joostjager Mar 13, 2025

Choose a reason for hiding this comment

Uh oh!

joostjager left a comment

Choose a reason for hiding this comment

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ldk-reviews-bot commented Mar 10, 2025 •

edited

Loading

codecov bot commented Mar 10, 2025 •

edited

Loading

joostjager commented Mar 10, 2025 •

edited

Loading