Decrypt Trampoline error onions by arik-so · Pull Request #3657 · lightningdevkit/rust-lightning

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.

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

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-10T14:22:11Z

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

joostjager

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

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.

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

- } => (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

-	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!

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

Comment threadlightning/src/ln/onion_utils.rs Outdated
Comment threadlightning/src/ln/onion_utils.rs Outdated
Comment threadlightning/src/ln/onion_utils.rs Outdated

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 af31dd1Compare March 10, 2025 06:47

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

joostjager reviewed Mar 10, 2025
View reviewed changes

Comment threadlightning/src/ln/onion_utils.rs Outdated

joostjager reviewed Mar 10, 2025
View reviewed changes

arik-so mentioned this pull request Mar 3, 2025
Trampoline #2299
Open
32 tasks

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

joostjager reviewed Mar 11, 2025
View reviewed changes

Comment threadlightning/src/ln/onion_utils.rs Outdated

joostjager reviewed Mar 11, 2025
View reviewed changes

Comment threadlightning/src/ln/onion_utils.rs Outdated

joostjager reviewed Mar 11, 2025
View reviewed changes

Comment threadlightning/src/routing/router.rs

joostjager reviewed Mar 11, 2025
View reviewed changes

Comment threadlightning/src/ln/onion_utils.rs Outdated

joostjager reviewed Mar 11, 2025
View reviewed changes

Comment threadlightning/src/ln/onion_utils.rs Outdated

arik-so force-pushed the arik/trampoline/error-decryption branch 3 times, most recently from 3e45cb2 to 36f8514Compare 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 83e58e9Compare 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

Comment threadlightning/src/ln/onion_utils.rs

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 98f2d38Compare March 13, 2025 07:47

joostjager reviewed Mar 13, 2025
View reviewed changes

Comment threadlightning/src/ln/onion_utils.rs Outdated

joostjager reviewed Mar 13, 2025
View reviewed changes

Comment threadlightning/src/ln/onion_utils.rs Outdated
Comment threadlightning/src/ln/onion_utils.rs Outdated
Comment threadlightning/src/ln/onion_utils.rs

arik-so force-pushed the arik/trampoline/error-decryption branch 5 times, most recently from 59993a8 to 0f5cbddCompare 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 f03cc62Compare 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

Comment threadlightning/src/ln/onion_utils.rs Outdated

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

arik-so force-pushed the arik/trampoline/error-decryption branch from f03cc62 to 6d63c24Compare 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:mainMar 14, 2025

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!

codecovBot 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!

joostjagerMar 13, 2025

Choose a reason for hiding this comment

Uh oh!

arik-soMar 13, 2025

Choose a reason for hiding this comment

Uh oh!

joostjagerMar 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!

valentinewallace commented Mar 13, 2025

Uh oh!

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

codecovBot commented Mar 10, 2025•
edited
Loading

joostjager commented Mar 10, 2025•
edited
Loading