Parse inbound Trampoline onions #3620

arik-so · 2025-02-25T09:34:14Z

Initially seeking concept ACK due to the refactor of passing Hop instead of InboundOnionPayload to create_receive_ and create_fwd_pending_htlc_info, which we need in order to have access to both the outer and inner onions for Trampoline handling.

valentinewallace · 2025-02-25T19:28:20Z

Don't see any issues right now with that. I'm wondering where the pathfinding to the next trampoline hop part is going to go?

arik-so · 2025-02-27T17:01:14Z

Rebased this onto #3626 so CI should no longer be failing.

codecov · 2025-02-28T19:44:47Z

Codecov Report

Attention: Patch coverage is 76.55502% with 49 lines in your changes missing coverage. Please review.

Project coverage is 90.13%. Comparing base (d136406) to head (913970f).
Report is 31 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/msgs.rs	10.63%	40 Missing and 2 partials ⚠️
lightning/src/ln/onion_payment.rs	96.34%	1 Missing and 2 partials ⚠️
lightning/src/ln/channelmanager.rs	97.26%	2 Missing ⚠️
lightning/src/ln/onion_utils.rs	71.42%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3620      +/-   ##
==========================================
+ Coverage   88.59%   90.13%   +1.54%     
==========================================
  Files         151      152       +1     
  Lines      118454   126851    +8397     
  Branches   118454   126851    +8397     
==========================================
+ Hits       104941   114335    +9394     
+ Misses      10994    10060     -934     
+ Partials     2519     2456      -63

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

TheBlueMatt

Mostly looks good, a few nits and one or two real comments.

TheBlueMatt · 2025-02-28T19:45:08Z

lightning/src/ln/msgs.rs

 			(16, payment_metadata, option),
 			(18, total_msat, (option, encoding: (u64, HighZeroBytesDroppedBigSize))),
+			(20, trampoline_onion_packet, option),


Here, and the 14 above, should we use placeholder values (ie high numbers) for now until the BOLT PR gets merged?

we can, and I think we briefly used to in 3386, though Eclair's already been using these type IDs.

Ah, okay, probably fine, then?

TheBlueMatt · 2025-02-28T19:50:04Z

lightning/src/ln/msgs.rs

@@ -3000,6 +3092,15 @@ impl<NS: Deref> ReadableArgs<(Option<PublicKey>, NS)> for InboundOnionPayload wh
 				amt_to_forward: amt.ok_or(DecodeError::InvalidValue)?,
 				outgoing_cltv_value: cltv_value.ok_or(DecodeError::InvalidValue)?,
 			}))
+		} else if let Some(outgoing_node_id) = outgoing_node_id {
+			if payment_data.is_some() || payment_metadata.is_some() || encrypted_tlvs_opt.is_some() ||


We should update the other branches to check that outgoing_node_id is none (man there's gotta be an easier way to write this pattern....

great point!

TheBlueMatt · 2025-02-28T19:51:37Z

lightning/src/ln/msgs.rs

@@ -3036,16 +3048,6 @@ impl<NS: Deref> ReadableArgs<(Option<PublicKey>, NS)> for InboundOnionPayload wh
 				amt_to_forward: amt.ok_or(DecodeError::InvalidValue)?,
 				outgoing_cltv_value: cltv_value.ok_or(DecodeError::InvalidValue)?,
 			}))
-		} else if let Some(trampoline_onion_packet) = trampoline_onion_packet {


lets not move code we just introduced in the previous commit? Maybe just squash these two...

yeah I moved the second commit here for easy either squashing or dismissal

TheBlueMatt · 2025-02-28T21:11:09Z

lightning/src/ln/msgs.rs

@@ -1811,6 +1811,23 @@ mod fuzzy_internal_msgs {
 		pub outgoing_cltv_value: u32,
 	}

+	#[allow(unused)]
+	pub struct InboundTrampolineForwardPayload {
+		pub outgoing_node_id: NodeId,


"outgoing node id" implies to me that this is the next hop from us, rather than "next hop node id" which I think is what was meant here?

would next_node_id be better?

Yea, or maybe next_trampoline?

TheBlueMatt · 2025-02-28T21:11:37Z

lightning/src/ln/msgs.rs

+
+	#[allow(unused)]
+	pub struct InboundTrampolineBlindedForwardPayload {
+		pub outgoing_node_id: NodeId,


lightning/src/ln/msgs.rs

TheBlueMatt · 2025-02-28T21:17:13Z

lightning/src/ln/msgs.rs

@@ -1842,9 +1870,17 @@ mod fuzzy_internal_msgs {

 	pub enum InboundOnionPayload {
 		Forward(InboundOnionForwardPayload),
+		#[allow(unused)]


for this kind of thing I'd much rather #[cfg(trampoline)] than #[allow(unused)]

that would imply cfg-based behavioral changes inside InboundOnionPayload's read implementation, but happy to change if that's ok

I think thats fine?

TheBlueMatt · 2025-02-28T21:26:45Z

lightning/src/blinded_path/payment.rs

@@ -344,6 +363,9 @@ impl UnauthenticatedReceiveTlvs {
 pub(crate) enum BlindedPaymentTlvs {
 	/// This blinded payment data is for a forwarding node.
 	Forward(ForwardTlvs),
+	/// This blinded payment data is for a forwarding Trampoline node.
+	#[allow(unused)]


This is used tho?

used, but never constructed

TheBlueMatt · 2025-02-28T21:46:00Z

lightning/src/ln/msgs.rs

 		Receive(InboundOnionReceivePayload),
 		BlindedForward(InboundOnionBlindedForwardPayload),
 		BlindedReceive(InboundOnionBlindedReceivePayload),
+
+		// These payloads should be seen inside an inner Trampoline onion


Should these go through a different enum/decode method since they shouldn't appear in the same context?

certainly an option. I think we already discussed this, it would just be a lot of code duplication

Oh? There's not that much code for InboundOnionPayload to begin with? What would get duplicated aside from the one decode_tlv_stream call?

I just pushed a version with the duplicated read implementation. The receive variants, being identical to the ones we'd expect in the outer onion, naturally also increase the duplication

valentinewallace

Generally looks good, just have some nits here!

valentinewallace · 2025-02-27T20:30:16Z

lightning/src/ln/msgs.rs

+	pub struct InboundTrampolineEntrypointPayload {
+		pub amt_to_forward: u64,
+		pub outgoing_cltv_value: u32,
+		pub multipath_trampoline_data: Option<FinalOnionHopData>,


Could you add a comment for why this is optional?

ah, actually, you're right. Per https://github.com/lightning/bolts/blob/079f761bf68caa48544bd6bf0a29591d43425b0b/04-onion-routing.md#requirements-2 it's mandatory

lightning/src/ln/onion_utils.rs

lightning/src/ln/msgs.rs

lightning/src/blinded_path/payment.rs

valentinewallace · 2025-03-04T00:33:38Z

lightning/src/ln/onion_payment.rs

@@ -167,6 +273,8 @@ pub(super) fn create_recv_pending_htlc_info(
 			 sender_intended_htlc_amt_msat, cltv_expiry_height, None, Some(payment_context),
 			 intro_node_blinding_point.is_none(), true, invoice_request)
 		}
+		#[cfg(trampoline)]
+		onion_utils::Hop::TrampolineReceive { .. } | onion_utils::Hop::TrampolineBlindedReceive { .. } => todo!(),


Is the todo left intentionally?

yup, will handle receiving and add error decoding tests in next PR

valentinewallace · 2025-03-04T00:54:58Z

lightning/src/ln/onion_utils.rs

+				);
+				match decoded_trampoline_hop {
+					Ok((
+						next_trampoline_hop_data,


nit: the indentation is a bit intense below, do you think we could avoid the nested matches?

I'm looking into this

Can you match on

Ok((InboundTrampolinePayload::Forward, Some(..))) => { .. }, Ok((InboundTrampolinePayload::BlindedForward, Some(..))) => { .. }, Ok((InboundTrampolinePayload::Receive, None)) => { .. }, ... etc

and just have a catch-all for forward-encoded-as-receive error and vice versa?

ah, sure. I thought you wanted some more depth removed, but this works nicely, thanks

TheBlueMatt

This LGTM, feel free to squash the fixup and @valentinewallace can let us know if we can land it.

TheBlueMatt · 2025-03-04T21:20:50Z

lightning/src/ln/msgs.rs

@@ -1860,6 +1905,18 @@ mod fuzzy_internal_msgs {
 			multipath_trampoline_data: Option<FinalOnionHopData>,
 			trampoline_packet: TrampolineOnionPacket,
 		},
+		/// This is used for Trampoline hops that are not the blinded path intro hop.
+		/// We would only ever construct this variant when we are a Trampoline node forwarding a


Wait, I'm confused here. So we construct this when we're forwarding a trampoline and the next trampoline hop we want to use is not the blinded path entry point?

exactly right, we do this when we're already inside the blinded path, either when we're the intro node, or the intro node was already behind us. We do so because we cannot necessarily provide the blinding point in the update_add message given that the next Trampoline hop is not necessarily our next peer.

TheBlueMatt · 2025-03-04T21:39:30Z

lightning/src/ln/onion_payment.rs

+	},
+	#[cfg(trampoline)]
+	Trampoline {
+		next_trampoline: NodeId,


nit: might be nice to use a PublicKey here rather than a NodeId since we're happy to fail-back if the pk isn't valid.

would you like the msgs.rs parsing to also read a PublicKey, but store it as a NodeId in the inbound payload structs, or shall I just change all of them to PublicKey instead?

I assume PublicKey? The only place we don't use a PublicKey is in update_add_htlc parsing cause then we'd disconnect our immediate peer just because the sender provided an invalid pk. Anywhere else should be a PublicKey cause we generally actually need to use it for crypto things, and we need a PublicKey for that anyway (except network graph, where full keys are too slow).

TheBlueMatt · 2025-03-05T17:10:04Z

Oops needs rebase :/

valentinewallace

LGTM!

lightning/src/ln/msgs.rs

TheBlueMatt · 2025-03-06T00:39:43Z

Grr looks like this needs rebased again.

Given that blinded Trampoline hops cannot use blinded intermediate hops between each other, the `current_path_key` used for decrypting the blinded Trampoline onion cannot be included in the update_add_htlc message due to the potential presence of intermediate hops. That means that when we're not the payment initiator, but a Trampoline hop forwarding a payment along a blinded path, we must include the blinding point inside the outer onion, which this commit will allow us to do.

For pending HTLC info processing, we will later need to access both the outer and the Trampoline payloads. Thanks to the refactor eliminating invalid Hop states, this is now possible by accessing the Hop struct, which will carry both outer and Trampoline payload data when applicable.

Check inbound onion payloads for the presence of an inner onion, which we will later also need to decrypt to handle Trampoline payments.

We will be using the same logic for decoding onion payloads for outer and for Trampoline onions. To accommodate the Trampoline payloads, we parse a next node ID rather than an SCID.

When encrypting errors, we currently have the ability to use two shared secrets for Phantom Node payments. Trampoline also requires the re-encryption of encrypted errors, first using the Trampoline shared secret, and then using the outer one. Given these two cryptographically equivalent use cases, we're renaming the phantom_shared_secret parameter to secondary_shared_secret, and explaining the now two contexts in which it will be applicable.

When parsing inbound HTLCs, we need to provide supplemental information about potential outgoing hops, which hitherto always required the next SCID. However, Trampoline uproots the approach because the sender did not specify the SCID; rather, they rely on us to do the intermediate routing. Thus we now need to distinguish between SCID-based and node- ID-based outgoing routing.

In this commit, we start decrypting the inner onion we receive, and lay some groundwork towards handling the subsequent routing or payment receipt.

TheBlueMatt marked this pull request as draft February 25, 2025 18:37

arik-so force-pushed the arik/trampoline/inbound-decoding branch 3 times, most recently from 4d145a3 to d22e0fe Compare February 27, 2025 16:59

arik-so added the weekly goal Someone wants to land this this week label Feb 27, 2025

arik-so force-pushed the arik/trampoline/inbound-decoding branch 5 times, most recently from f64afa4 to 3013c22 Compare February 28, 2025 17:56

arik-so marked this pull request as ready for review February 28, 2025 17:58

arik-so force-pushed the arik/trampoline/inbound-decoding branch 3 times, most recently from 392bc6d to 913970f Compare February 28, 2025 19:35

TheBlueMatt reviewed Feb 28, 2025

View reviewed changes

arik-so force-pushed the arik/trampoline/inbound-decoding branch 2 times, most recently from cd67a23 to 7631640 Compare March 2, 2025 18:28

valentinewallace self-requested a review March 3, 2025 15:40

valentinewallace reviewed Mar 4, 2025

View reviewed changes

arik-so force-pushed the arik/trampoline/inbound-decoding branch from 7631640 to 226a6e3 Compare March 4, 2025 02:01

TheBlueMatt reviewed Mar 4, 2025

View reviewed changes

arik-so force-pushed the arik/trampoline/inbound-decoding branch from 226a6e3 to 6852765 Compare March 5, 2025 02:55

arik-so force-pushed the arik/trampoline/inbound-decoding branch 2 times, most recently from ba373d2 to 5850d83 Compare March 5, 2025 18:33

valentinewallace previously approved these changes Mar 5, 2025

View reviewed changes

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

arik-so force-pushed the arik/trampoline/inbound-decoding branch from 5850d83 to 05b142b Compare March 5, 2025 19:35

arik-so added 8 commits March 5, 2025 16:55

Remove some trailing spaces

6f03a9f

Parse inbound TrampolineEntrypoint payload

22f2d4e

Check inbound onion payloads for the presence of an inner onion, which we will later also need to decrypt to handle Trampoline payments.

Parse Trampoline onion payloads

142abf5

We will be using the same logic for decoding onion payloads for outer and for Trampoline onions. To accommodate the Trampoline payloads, we parse a next node ID rather than an SCID.

Decrypt Trampoline onions

0fe9da1

In this commit, we start decrypting the inner onion we receive, and lay some groundwork towards handling the subsequent routing or payment receipt.

arik-so dismissed valentinewallace’s stale review via 0fe9da1 March 6, 2025 00:59

arik-so force-pushed the arik/trampoline/inbound-decoding branch from 05b142b to 0fe9da1 Compare March 6, 2025 00:59

Parse inbound Trampoline onions #3620

Are you sure you want to change the base?

Parse inbound Trampoline onions #3620

Conversation

arik-so commented Feb 25, 2025

valentinewallace commented Feb 25, 2025

arik-so commented Feb 27, 2025

codecov bot commented Feb 28, 2025

Codecov Report

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arik-so Feb 28, 2025 • edited Loading

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

Choose a reason for hiding this comment

valentinewallace 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

TheBlueMatt 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

TheBlueMatt commented Mar 5, 2025

valentinewallace left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Mar 6, 2025

arik-so Feb 28, 2025 •

edited

Loading