Trampoline Payload Construction Method #3386

arik-so · 2024-10-28T02:35:51Z

Introduce a method for constructing Trampoline payloads.

This will allow the non-Trampoline method to be modified to generate a Trampoline onion and finish its own regular payload construction.

Builds on top of #3333.

lightning/src/routing/router.rs

TheBlueMatt · 2024-10-29T14:40:29Z

lightning/src/routing/router.rs

@@ -454,6 +454,9 @@ impl_writeable_tlv_based!(BlindedTail, {
 pub struct Path {
 	/// The list of unblinded hops in this [`Path`]. Must be at least length one.
 	pub hops: Vec<RouteHop>,
+	/// The list of unblinded Trampoline hops. If present, must be at least one.
+	/// The public key of the first Trampoline hop must match the public key of the last regular hop.


I don't see why this needs to be true. Can we just have each trampoline hop be next hops starting after the last unblinded node.

lightning/src/routing/router.rs

codecov · 2024-11-21T17:27:45Z

Codecov Report

Attention: Patch coverage is 93.42105% with 35 lines in your changes missing coverage. Please review.

Project coverage is 90.44%. Comparing base (1a8bf62) to head (9535d78).
Report is 19 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/onion_utils.rs	89.21%	16 Missing and 13 partials ⚠️
lightning/src/events/mod.rs	25.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3386      +/-   ##
==========================================
+ Coverage   89.74%   90.44%   +0.69%     
==========================================
  Files         130      130              
  Lines      107793   113400    +5607     
  Branches   107793   113400    +5607     
==========================================
+ Hits        96743   102569    +5826     
+ Misses       8651     8495     -156     
+ Partials     2399     2336      -63

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

TheBlueMatt

Ah I guess my previous comments still need to be addressed.

TheBlueMatt · 2024-11-25T15:39:39Z

lightning/src/routing/router.rs

@@ -404,13 +404,16 @@ pub struct BlindedTail {
 	pub excess_final_cltv_expiry_delta: u32,
 	/// The total amount paid on this [`Path`], excluding the fees.
 	pub final_value_msat: u64,
+	/// Used for determining the type of Trampoline path to construct
+	pub final_hop_supports_trampoline: bool


Should we instead consider storing a *Features of some form?

lightning/src/routing/router.rs

valentinewallace · 2024-12-10T21:31:39Z

lightning/src/ln/onion_utils.rs

+	Ok((res, value_msat, cltv))
+}
+
+fn build_trampoline_onion_payloads_callback<'a, H, B, F>(


Is there a way to DRY this with build_onion_payloads_callback, via implementing a trait on OutboundTrampolinePayload and OutboundOnionPayload that the common method is parameterized by? Kinda like what we do for decode_next_hop and how it works for both onion message and onion payment packets.

I'm not sure the payloads_callback is super DRYable, because all the response types that are being pushed vary strongly between RouteHop and TrampolineHop, but I think I was able to at least DRY up the onion_keys callback.

I ended up looking into this a bit more to see if we could avoid all the C&P, got this patch to compile: valentinewallace@974f212

It needs to be filled out a bit but take a look!

thanks! I incorporated it for the most part. The impl is a bit duplicative the way I did it, but the borrow checker and I are looking into simplifying it a bit

lightning/src/events/mod.rs

lightning/src/ln/onion_utils.rs

valentinewallace · 2024-12-12T20:26:44Z

lightning/src/ln/onion_utils.rs

+						cur_value_msat += final_value_msat;
+						callback(
+							PayloadCallbackAction::PushBack,
+							msgs::OutboundTrampolinePayload::BlindedReceive {


It looks like in https://github.com/lightning/bolts/blob/fa0594ac2af3531d734f1d707a146d6e13679451/bolt04/trampoline-payment-onion-test.json there can be final trampoline hops that are unblinded, but that doesn't look supported in the code atm. Are we explicitly only supporting trampoline to blinded paths?

yup, we deliberately are. We originally supported non-blinded destinations but decided not to.

lightning/src/ln/onion_route_tests.rs

lightning/src/events/mod.rs

valentinewallace · 2024-12-16T17:49:21Z

Haven't looked closely yet but it looks like the non-test changes in the latest commit belong in earlier commit(s). Feel free to squash IMO!

arik-so · 2024-12-16T19:38:46Z

I think the squashing should have taken care of that, though let me know if you'd like any other changes move around.

valentinewallace · 2024-12-17T19:46:40Z

lightning/src/ln/blinded_payment_tests.rs

@@ -1757,3 +1762,83 @@ fn route_blinding_spec_test_vector() {
 		_ => panic!("Unexpected error")
 	}
 }
+
+#[test]
+fn combined_trampoline_onion_creation_test() {


Could you add a comment or adjust the test name to document that these are from the spec test vectors?

lightning/src/ln/blinded_payment_tests.rs

lightning/src/ln/onion_utils.rs

lightning/src/routing/router.rs

valentinewallace · 2024-12-17T20:38:28Z

lightning/src/routing/router.rs

 }

 impl_writeable_tlv_based!(BlindedTail, {
 	(0, hops, required_vec),
 	(2, blinding_point, required),
 	(4, excess_final_cltv_expiry_delta, required),
 	(6, final_value_msat, required),
+	(8, final_hop_supports_trampoline, required),


Older versions won't be able to read these now since it's a required TLV, probably want to use an odd TLV and default_value

valentinewallace · 2024-12-17T20:46:53Z

lightning/src/routing/router.rs

+	/// The node_announcement features of the node at this hop. For the last hop, these may be
+	/// amended to match the features present in the invoice this node generated.
+	pub node_features: NodeFeatures,
+	/// The fee taken on this hop (for paying for the use of the *next* channel in the path).


Isn't it the fee taken for the next several hops until the next trampoline node, or the intro node? I think the docs for this field might need a glance to make sure they're accurate for trampoline, same for cltv_expiry_delta

valentinewallace · 2024-12-17T20:49:08Z

lightning/src/ln/onion_utils.rs

-		excess_final_cltv_expiry_delta: bt.excess_final_cltv_expiry_delta,
-	});
+
+	// don't include blinded tail when Trampoline hops are present


Probably should include why we're doing this

valentinewallace · 2024-12-18T01:29:53Z

lightning/src/ln/onion_utils.rs

+	Ok((res, value_msat, cltv))
+}
+
+fn build_trampoline_onion_payloads_callback<'a, H, B, F>(


I ended up looking into this a bit more to see if we could avoid all the C&P, got this patch to compile: valentinewallace@974f212

It needs to be filled out a bit but take a look!

tankyleo · 2024-12-18T21:55:23Z

lightning/src/events/mod.rs

@@ -1670,6 +1671,7 @@ impl Writeable for Event {
 					(2, payment_hash, option),
 					(4, path.hops, required_vec),
 					(6, path.blinded_tail, option),
+					(8, path.trampoline_hops, optional_vec),


I am trying to understand how types get assigned in LDK - it's an optional field here so shouldn't it have an odd type value ? thanks for clarifying.

those concepts are a bit orthogonal. Even means if it's present, you cannot ignore it, i.e. you have to understand how to parse it, whereas odd means it's fine to just skip that value if you don't know what to do with it.

Optional means that the value may not need to be set. So you can have optional even fields like here, where there may not be a value, but if there is one, it's important to understand it correctly.

The only scenario I cannot really think of a good use case for is required odd fields, which would suggest that the value must always be present, but is fine to skip when parsing.

thank you I understand now.

tankyleo · 2024-12-18T22:10:05Z

lightning/src/ln/channelmanager.rs

@@ -668,7 +668,7 @@ impl HTLCSource {
 	pub fn dummy() -> Self {
 		assert!(cfg!(not(feature = "grind_signatures")));
 		HTLCSource::OutboundRoute {
-			path: Path { hops: Vec::new(), blinded_tail: None },
+			path: Path { hops: Vec::new(), trampoline_hops: Vec::new(), blinded_tail: None },


nit: every other vec in this patch is initialized with vec![], perhaps make this consistent ? your call feel free to dismiss.

oh, good point!

ah, actually, the other element had already been initialized with Vec::new(), so I think that's more consistent in this instance

TheBlueMatt reviewed Oct 29, 2024

View reviewed changes

arik-so force-pushed the arik/trampoline/payload-construction branch 2 times, most recently from 7cad33e to 856ce97 Compare November 19, 2024 23:04

arik-so force-pushed the arik/trampoline/payload-construction branch from 856ce97 to 5ea3ada Compare November 21, 2024 17:15

TheBlueMatt reviewed Nov 25, 2024

View reviewed changes

arik-so force-pushed the arik/trampoline/payload-construction branch from 5ea3ada to af1818b Compare December 5, 2024 01:19

arik-so mentioned this pull request Dec 5, 2024

Trampoline #2299

Open

30 tasks

arik-so force-pushed the arik/trampoline/payload-construction branch 4 times, most recently from ee68783 to 4dd0fea Compare December 9, 2024 05:31

valentinewallace reviewed Dec 10, 2024

View reviewed changes

arik-so force-pushed the arik/trampoline/payload-construction branch 4 times, most recently from dc679c0 to 7477b1a Compare December 12, 2024 17:14

valentinewallace reviewed Dec 12, 2024

View reviewed changes

arik-so force-pushed the arik/trampoline/payload-construction branch from 7477b1a to ba71d6e Compare December 13, 2024 10:34

valentinewallace reviewed Dec 13, 2024

View reviewed changes

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

lightning/src/events/mod.rs Outdated Show resolved Hide resolved

lightning/src/events/mod.rs Outdated Show resolved Hide resolved

arik-so force-pushed the arik/trampoline/payload-construction branch 2 times, most recently from b934379 to 6569246 Compare December 13, 2024 22:54

Calculate Trampoline onion size dynamically

cec4473

arik-so force-pushed the arik/trampoline/payload-construction branch 2 times, most recently from 09c7f70 to adb0b88 Compare December 15, 2024 22:58

arik-so force-pushed the arik/trampoline/payload-construction branch from adb0b88 to 9535d78 Compare December 16, 2024 19:38

valentinewallace reviewed Dec 18, 2024

View reviewed changes

arik-so force-pushed the arik/trampoline/payload-construction branch from 9535d78 to ab320ba Compare December 18, 2024 03:15

arik-so force-pushed the arik/trampoline/payload-construction branch from ab320ba to e191a3d Compare December 18, 2024 07:18

tankyleo reviewed Dec 18, 2024

View reviewed changes

arik-so added 8 commits December 18, 2024 22:56

Create TrampolineHop struct

b2cad2f

Add trampoline_hops field to Path

f403137

f: trampoline_hop serialization

4228c8d

Trampoline payload construction method

106e1cc

Construct Trampoline component in create_payment_onion

1939205

f: rename combined unit test and use hex helpers

9ad4171

f: onion_payloads option

eb5dcfd

f: Trampoline/blinded path position rationale

f88778e

arik-so force-pushed the arik/trampoline/payload-construction branch from e191a3d to 249a1a0 Compare December 19, 2024 07:39

f: generic build_payloads function (thanks, Val!)

549fdd5

arik-so force-pushed the arik/trampoline/payload-construction branch from 249a1a0 to 549fdd5 Compare December 19, 2024 07:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trampoline Payload Construction Method #3386

Trampoline Payload Construction Method #3386

arik-so commented Oct 28, 2024 •

edited

Loading

TheBlueMatt Oct 29, 2024

codecov bot commented Nov 21, 2024 •

edited

Loading

TheBlueMatt left a comment

TheBlueMatt Nov 25, 2024

valentinewallace Dec 10, 2024 •

edited

Loading

arik-so Dec 11, 2024

valentinewallace Dec 18, 2024

arik-so Dec 18, 2024 •

edited

Loading

valentinewallace Dec 12, 2024

arik-so Dec 12, 2024

valentinewallace commented Dec 16, 2024

arik-so commented Dec 16, 2024

valentinewallace Dec 17, 2024

arik-so Dec 18, 2024

valentinewallace Dec 17, 2024

valentinewallace Dec 17, 2024

valentinewallace Dec 17, 2024

arik-so Dec 18, 2024

valentinewallace Dec 18, 2024

tankyleo Dec 18, 2024

arik-so Dec 18, 2024

tankyleo Dec 18, 2024

tankyleo Dec 18, 2024

arik-so Dec 18, 2024

arik-so Dec 19, 2024

Trampoline Payload Construction Method #3386

Are you sure you want to change the base?

Trampoline Payload Construction Method #3386

Conversation

arik-so commented Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Nov 21, 2024 • edited Loading

Codecov Report

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arik-so Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace commented Dec 16, 2024

arik-so commented Dec 16, 2024

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 commented Oct 28, 2024 •

edited

Loading

codecov bot commented Nov 21, 2024 •

edited

Loading

valentinewallace Dec 10, 2024 •

edited

Loading

arik-so Dec 18, 2024 •

edited

Loading