WIP: Trampoline forwarding #3711

arik-so · 2025-04-07T08:34:35Z

Forward Trampoline onions. Currently a work in progress, still missing these cleanups:

New HTLCSource variant for storing the session_priv instead of throwing them away
Tests for forwarding failures
Additional tests for failures that can occur both when forwarding and receiving Trampoline onions
Determine skimmable amount, charging a routing fee

ldk-reviews-bot · 2025-04-07T08:34:37Z

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

joostjager

Skimmed the PR. Change set definitely much shorter than expected.

What I didn't really see is the interpretation of the result of the trampoline forward and potential retry when it fails?

joostjager · 2025-04-07T09:27:03Z

lightning/src/ln/channelmanager.rs

+										previously_failed_blinded_path_idxs: vec![],
+									},
+									final_value_msat: outgoing_amount,
+									max_total_routing_fee_msat: Some(incoming_amount - outgoing_amount),


Shouldn't the trampoline node itself earn something too?

yes, figuring out how to determine a good amount to skim

I think it should just include the fee on the local outgoing channel in the total fee during pathfinding? Normally that is not necessary, but this is a payment that is really a forward.

should be addressed now

joostjager · 2025-04-07T09:28:40Z

lightning/src/ln/blinded_payment_tests.rs

@@ -2460,3 +2460,165 @@ fn test_trampoline_forward_rejection() {
 		expect_payment_failed_conditions(&nodes[0], payment_hash, false, payment_failed_conditions);
 	}
 }
+
+#[test]
+fn test_unblinded_trampoline_forward() {


Interesting that you do an unblinded forward. I know it has to be supported, but it isn't what an LDK sender would do right?

In principle there's nothing preventing us from placing an unblinded Trampoline hop before a blinded one. But the reason I'm testing these first is less cryptography overhead.

We do now test both.

joostjager · 2025-04-07T09:30:15Z

lightning/src/ln/channelmanager.rs

+								// applying non-strict forwarding.
+								// The channel with the least amount of outbound liquidity will be used to maximize the
+								// probability of being able to successfully forward a subsequent HTLC.
+								let maybe_optimal_channel = peer_state.channel_by_id.values_mut()


I'd not expect this code to be repeated, because it also applies to non-trampoline forwards? I do realize this is draft.

I ended up needing to move this into the else clause, resulting in potentially even more repetition, but also some minor differences. I'm now looking into deduplication.

joostjager · 2025-04-07T09:34:18Z

lightning/src/ln/channelmanager.rs

+							let outgoing_amount = next_packet_details.outgoing_amt_msat;
+							let outgoing_cltv = next_packet_details.outgoing_cltv_value;
+
+							let routes = self.router.find_route(


Is there any risk in this potentially long running operation blocking important processes?

this should be running in the background, so I don't believe so

It is running in the background processor I think, but it could still be blocking the processing of other events? Or is execution really suspended here while other event handling proceeds?

To break this down a bit more precisely, are you suggesting creating an entirely separate loop for that altogether, independent of the PendingHTLCForward processing loop, and push an event for e. g. TrampolineForwardPathFound?

joostjager · 2025-04-07T09:35:09Z

lightning/src/ln/channelmanager.rs

+								InFlightHtlcs::new()
+							).unwrap();
+
+							inter_trampoline_path = routes.paths.first().map(|path| path.hops.clone());


No path found case to be handled I think?

yeah, ironing that out :)

You should hopefully find the test coverage to be satisfactory now :)

lightning/src/ln/channelmanager.rs

valentinewallace · 2025-04-08T17:05:55Z

lightning/src/ln/channelmanager.rs

@@ -4353,8 +4355,6 @@ where
 				// we don't have the channel here.
 				return Err(("Refusing to forward over real channel SCID as our counterparty requested.", 0x4000 | 10));


Do we perform this check for trampoline forwards, once we know the outgoing channel?

currently we do not given we take the channel immediately from the pathfinding logic

lightning/src/ln/channelmanager.rs

valentinewallace · 2025-04-08T17:16:20Z

lightning/src/ln/channelmanager.rs

+				match trampoline_outgoing_scid {
+					Some(scid) => scid,
+					None => {
+						return Err(("Cannot forward by Node ID without SCID.", 0x4000 | 10));


nit: add debug_assert(false) if this should be unreachable

currently inapplicable with the logic reshuffle, but if we should reimplement a can_forward call post-pathfinding, will become applicable again

valentinewallace · 2025-04-08T17:20:54Z

lightning/src/ln/channelmanager.rs

+		/// Path to send the packet to the next hop
+		hops: Vec<RouteHop>


Does this lock us into using only 1 path for trampoline forwards? Seems like that could impact payment reliability

Determined offline to be fine pending implementation of retry logic.

I thought in our offline discussion we said we want to get the persistence correct now, though? Also MPP seems applicable for the first try as well, not just retries. Let me know what I'm missing there.

Updated the persistence for MPP scenarios.

valentinewallace · 2025-04-10T14:45:38Z

lightning/src/events/mod.rs

+	/// We couldn't forward to the next Trampoline node. That may happen if we cannot find a route,
+	/// or if the route we found didn't work out
+	FailedTrampolineForward {
+		/// The node ID of the next Trampoline hop we tried forwarding to
+		requested_next_node_id: PublicKey,
+		/// The channel we tried forwarding over, if we have settled on one
+		forward_scid: Option<u64>,
+	},


This is all about to change a lot with #3700 so may want to use an existing variant instead

Should we wait until #3700 lands? Or I can coördinate with Carla? It seems to be the existing variants really don't map nicely, but I won't die on that hill.

Yeah it might be a good idea to coordinate with her, I think 3700 should be landing soon.

valentinewallace · 2025-04-10T14:57:56Z

lightning/src/ln/channelmanager.rs

+		/// Path to send the packet to the next hop
+		hops: Vec<RouteHop>


I thought in our offline discussion we said we want to get the persistence correct now, though? Also MPP seems applicable for the first try as well, not just retries. Let me know what I'm missing there.

lightning/src/ln/channelmanager.rs

joostjager

No review was requested yet, but had a quick look and dropped a few comments.

joostjager · 2025-04-11T11:46:11Z

lightning/src/ln/blinded_payment_tests.rs

+
+	let replacement_onion = {
+		// create a substitute onion where the last Trampoline hop is an unblinded receive, which we
+		// (deliberately) do not support out of the box, therefore necessitating this workaround


Couldn't this be a more casual failure, just no liquidity or something like that?

I am also wondering whether all the trampoline failure cases are covered currently. So there is blind and non-blinded, but also failures directly from a first trampoline node, from a second trampoline, and from a normal node in between the first and second.

Sure, we can create a more casual failure. Regarding the forwarding failure cases, hopefully they should be much better covered now, though the sky's the limit.

Do you have a preference for what the more casual failure should be exactly? A channel with insufficient balance? I'm trying to think of scenarios where the pathfinding wouldn't fail, but the actual sending would, which are also easy to simulate.

joostjager · 2025-04-11T11:48:14Z

lightning/src/ln/channelmanager.rs

+											max_path_count: 1,
+											max_path_length: MAX_PATH_LENGTH_ESTIMATE / 2,
+											max_channel_saturation_power_of_half: 2,
+											previously_failed_channels: vec![],


Not needed for retries?

@joostjager could you clarify that? It seems like we should track channels that failed us in the past for retries specifically

Yes, that's what I mean. Then we need info from previous routing attempts here. But I suppose that'll be added in a follow up that does retries?

I am wondering though if postponing the retries to a later PR might require redoing some of the logic that is added in this one.

Ah sorry, yeah when we get a failure we should insert the channel ID in here for retries.

I am wondering though if postponing the retries to a later PR might require redoing some of the logic that is added in this one.

I'm also wondering about this, going to try to take a closer look at that on the next pass.

joostjager · 2025-04-11T11:51:55Z

lightning/src/ln/blinded_payment_tests.rs

+	do_pass_along_path(args);
+
+	if success {
+		claim_payment(&nodes[0], &[&nodes[1], &nodes[2], &nodes[4], &nodes[3]], payment_preimage);


It is probably a good idea to cover the trampoline routing retry somewhere too?

joostjager · 2025-04-11T11:53:25Z

lightning/src/ln/channelmanager.rs

+									}
+								}
+								()
+							},


This is a lot of code added. Is it possible to do some decomposition in methods?

I would also suggest to add the maximum amount of comments in all of this code to explain as much as possible.

valentinewallace

Re-request review when you want another look :)

lightning/src/ln/onion_payment.rs

valentinewallace · 2025-04-11T17:07:09Z

lightning/src/ln/channelmanager.rs

@@ -7067,6 +7091,61 @@ where
 					failed_next_destination: destination,
 				}, None));
 			},
+			HTLCSource::TrampolineForward { previous_hop_data, incoming_trampoline_shared_secret, .. } => {


Could we expand the commit message to talk about the behavior being added here?

yup, updated the commit message. Perhaps a comment might be worthwhile, too

valentinewallace · 2025-04-11T17:08:46Z

lightning/src/ln/channelmanager.rs

+			previous_hop_data: HTLCPreviousHopData,
+			incoming_trampoline_shared_secret: [u8; 32],
+			hops: Vec<RouteHop>,
+			session_priv: SecretKey,


Could you add a comment explaining this field?

valentinewallace · 2025-04-11T17:36:15Z

lightning/src/ln/channelmanager.rs

+											push_trampoline_forwarding_failure(format!("Could not to route to next Trampoline hop {next_node_id} via forwarding channel {outgoing_scid}"), htlc_source, Some(outgoing_scid), 0x2000 | 25, Vec::new());
+											continue;
+										}
+									}


Is it possible to call self.can_forward_htlc here, now that we have the outgoing scid?

should we still do this immediately after pathfinding?

valentinewallace · 2025-04-11T17:54:54Z

lightning/src/ln/channelmanager.rs

+											max_path_count: 1,
+											max_path_length: MAX_PATH_LENGTH_ESTIMATE / 2,
+											max_channel_saturation_power_of_half: 2,
+											previously_failed_channels: vec![],


@joostjager could you clarify that? It seems like we should track channels that failed us in the past for retries specifically

codecov · 2025-04-12T02:59:27Z

Codecov Report

Attention: Patch coverage is 88.63965% with 157 lines in your changes missing coverage. Please review.

Project coverage is 89.17%. Comparing base (481e5c7) to head (688ad3f).

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	70.92%	111 Missing and 5 partials ⚠️
lightning/src/ln/onion_payment.rs	57.53%	25 Missing and 6 partials ⚠️
lightning/src/ln/blinded_payment_tests.rs	99.20%	7 Missing ⚠️
lightning/src/chain/channelmonitor.rs	33.33%	2 Missing ⚠️
lightning/src/ln/onion_utils.rs	95.45%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3711      +/-   ##
==========================================
+ Coverage   89.13%   89.17%   +0.04%     
==========================================
  Files         156      156              
  Lines      123477   124824    +1347     
  Branches   123477   124824    +1347     
==========================================
+ Hits       110056   111308    +1252     
- Misses      10752    10828      +76     
- Partials     2669     2688      +19

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

arik-so · 2025-04-16T08:44:01Z

Hey guys, rephrased the commits, addressed what I think was most of your comments, and would like to squash a large portion of the fixup commits that ended up in here.

joostjager

Only reviewed the first commit and posting what I have. Need to gather a lot more context to properly review the rest of the PR.

joostjager · 2025-04-16T10:21:33Z

lightning/src/ln/blinded_payment_tests.rs

-						fee_msat: amt_msat,
-						cltv_expiry_delta: 24,
+						fee_msat: 0,
+						cltv_expiry_delta: 72,


Shouldn't the delta be 48 too then if it is the final hop?

joostjager · 2025-04-16T10:21:52Z

lightning/src/ln/blinded_payment_tests.rs

@@ -2270,6 +2269,8 @@ fn test_trampoline_unblinded_receive() {
 		route_params: None,
 	};

+	// outer 56


joostjager · 2025-04-16T10:24:36Z

lightning/src/ln/blinded_payment_tests.rs

+	let args = if underpay {
+		args.with_payment_preimage(payment_preimage)
+			.without_claimable_event()
+			.expect_failure(HTLCDestination::FailedPayment { payment_hash })


Isn't it now waiting on more mpp shards to get to 2*amt?

joostjager · 2025-04-16T10:28:15Z

lightning/src/ln/blinded_payment_tests.rs

+		}
+		{
+			let payment_failed_conditions = PaymentFailedConditions::new()
+				.expected_htlc_error_data(0x2000 | 26, &[0; 0]);


Isn't the spec saying failure data is u32,u32,u16 ?

joostjager · 2025-04-16T10:34:41Z

lightning/src/ln/onion_payment.rs

+				InboundHTLCErr {
+					msg: "Underflow calculating outbound amount or CLTV value for Trampoline forward",
+					err_code: 0x2000 | 26,
+					err_data: Vec::new(),


No err data like specced in bolt?

joostjager · 2025-04-16T10:36:33Z

lightning/src/ln/onion_payment.rs

@@ -148,6 +166,15 @@ pub(super) fn create_fwd_pending_htlc_info(
 					err_data: vec![0; 32],
 				}
 			})?;
+			check_trampoline_sanity(outer_hop_data, outgoing_cltv_value, amt_to_forward).map_err(|()| {


naming consistency: check_blinded_forward vs check_blinded_forward_constraints vs check_trampoline_sanity

joostjager · 2025-04-16T10:37:42Z

lightning/src/ln/onion_payment.rs

 			trampoline_hop_data: msgs::InboundOnionReceivePayload {
 				payment_data, keysend_preimage, custom_tlvs, sender_intended_htlc_amt_msat,
 				cltv_expiry_height, payment_metadata, ..
 			}, ..
-		} =>
+		} => {
+			check_trampoline_sanity(outer_hop_data, cltv_expiry_height, sender_intended_htlc_amt_msat).map_err(|()| {


For the final hop, shouldn't it be equal? I know it is not in the spec like that.

joostjager · 2025-04-16T10:46:03Z

lightning/src/ln/onion_payment.rs

@@ -577,7 +624,34 @@ where
 				outgoing_cltv_value
 			})
 		}
-		onion_utils::Hop::TrampolineForward { next_trampoline_hop_data: msgs::InboundTrampolineForwardPayload { amt_to_forward, outgoing_cltv_value, next_trampoline }, trampoline_shared_secret, incoming_trampoline_public_key, .. } => {
+		onion_utils::Hop::TrampolineForward { ref outer_hop_data, next_trampoline_hop_data: msgs::InboundTrampolineForwardPayload { amt_to_forward, outgoing_cltv_value, next_trampoline }, outer_shared_secret, trampoline_shared_secret, incoming_trampoline_public_key, .. } => {
+			if let Err(()) = check_trampoline_sanity(outer_hop_data, outgoing_cltv_value, amt_to_forward) {


Why does sanity need to be checked both here and in create_fwd_pending_htlc_info ?

joostjager · 2025-04-16T10:48:53Z

lightning/src/ln/onion_utils.rs

@@ -1902,7 +1904,7 @@ where
 					&hop_data.trampoline_packet.hop_data,
 					hop_data.trampoline_packet.hmac,
 					Some(payment_hash),
-					(blinding_point, node_signer),
+					(hop_data.current_path_key, node_signer),


Is this an unrelated fix?

joostjager · 2025-04-16T10:49:24Z

lightning/src/ln/onion_utils.rs

@@ -1557,6 +1557,8 @@ impl HTLCFailReason {
 		else if failure_code == 21 { debug_assert!(data.is_empty()) }
 		else if failure_code == 22 | PERM { debug_assert!(data.len() <= 11) }
 		else if failure_code == 23 { debug_assert!(data.is_empty()) }
+		else if failure_code == 25 | NODE { debug_assert!(data.is_empty()) }
+		else if failure_code == 26 | NODE { debug_assert!(data.is_empty()) }


Only 25 is empty according to spec?

valentinewallace

Getting some comments out since I think a rebase is coming in a bit

valentinewallace · 2025-04-16T15:35:08Z

lightning/src/ln/onion_payment.rs

+fn check_trampoline_sanity(outer_hop_data: &msgs::InboundTrampolineEntrypointPayload, trampoline_cltv_value: u32, trampoline_amount: u64) -> Result<(), ()> {
+	if outer_hop_data.outgoing_cltv_value < trampoline_cltv_value {
+		return Err(());
+	}
+	if outer_hop_data.multipath_trampoline_data.as_ref().map_or(outer_hop_data.amt_to_forward, |mtd| mtd.total_msat) < trampoline_amount {


nit: wrap at 100chars

valentinewallace · 2025-04-16T15:35:58Z

lightning/src/ln/onion_payment.rs

+		onion_utils::Hop::TrampolineForward { ref outer_hop_data, next_trampoline_hop_data, next_trampoline_hop_hmac, new_trampoline_packet_bytes, trampoline_shared_secret, .. } => {
+			check_trampoline_sanity(outer_hop_data, next_trampoline_hop_data.outgoing_cltv_value, next_trampoline_hop_data.amt_to_forward).map_err(|()| {


nit: wrap at 100chars and below

valentinewallace · 2025-04-16T19:15:10Z

lightning/src/ln/channelmanager.rs

 /// Uniquely describes an HTLC by its source. Just the guaranteed-unique subset of [`HTLCSource`].
 pub(crate) enum SentHTLCId {
 	PreviousHopData { short_channel_id: u64, htlc_id: u64 },
 	OutboundRoute { session_priv: [u8; SECRET_KEY_SIZE] },
+	TrampolineForward { session_priv: [u8; SECRET_KEY_SIZE], previous_hop_data: Vec<PreviousHopIdData> }


s/previous_hop_data/previous_hop_ids since there are multiple now?

valentinewallace · 2025-04-16T19:29:15Z

lightning/src/ln/channelmanager.rs

@@ -624,10 +624,17 @@ impl Readable for InterceptId {
 }

 #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
+pub(crate) struct PreviousHopIdData {


nit: does the Data suffix add anything? May be nicer to remove

valentinewallace · 2025-04-16T19:32:50Z

lightning/src/ln/channelmanager.rs

+				// todo: what do we want to do with this given we do not wish to propagate it directly?
+				let _decoded_onion_failure = onion_error.decode_onion_failure(&self.secp_ctx, &self.logger, &source);
+
+				for current_hop_data in previous_hop_data {


Could we use upstream_hop_data or just hop_data? Kinda confusing to say "for current hop in previous hops { .. }".

valentinewallace · 2025-04-16T20:05:27Z

lightning/src/ln/channelmanager.rs

+					let failure = match blinded_failure {
+						Some(BlindedFailure::FromIntroductionNode) => {
+							let blinded_onion_error = HTLCFailReason::reason(INVALID_ONION_BLINDING, vec![0; 32]);
+							let err_packet = blinded_onion_error.get_encrypted_failure_packet(
+								&incoming_packet_shared_secret, &Some(incoming_trampoline_shared_secret.clone())
+							);
+							HTLCForwardInfo::FailHTLC { htlc_id, err_packet }
+						},
+						Some(BlindedFailure::FromBlindedNode) => {
+							HTLCForwardInfo::FailMalformedHTLC {
+								htlc_id,
+								failure_code: INVALID_ONION_BLINDING,
+								sha256_of_onion: [0; 32]
+							}
+						},
+						None => {
+							let err_code = 0x2000 | 25;
+							let err_packet = HTLCFailReason::reason(err_code, Vec::new())
+								.get_encrypted_failure_packet(&incoming_packet_shared_secret, &Some(incoming_trampoline_shared_secret.clone()));
+							HTLCForwardInfo::FailHTLC { htlc_id, err_packet }
+						}
+					};
+
+					push_forward_event = self.decode_update_add_htlcs.lock().unwrap().is_empty();
+					let mut forward_htlcs = self.forward_htlcs.lock().unwrap();
+					push_forward_event &= forward_htlcs.is_empty();
+
+					match forward_htlcs.entry(short_channel_id) {
+						hash_map::Entry::Occupied(mut entry) => {
+							entry.get_mut().push(failure);
+						},
+						hash_map::Entry::Vacant(entry) => {
+							entry.insert(vec!(failure));
+						}
+					}
+
+					mem::drop(forward_htlcs);
+
+					let mut pending_events = self.pending_events.lock().unwrap();
+					pending_events.push_back((events::Event::HTLCHandlingFailed {
+						prev_channel_id: channel_id,
+						failed_next_destination: destination.clone(),
+					}, None));


Is it possible to DRY this with the HTLCSource::PreviousHopData handling above?

Ensure that the Trampoline onion's amount and CLTV values do not exceed the limitations imposed by the outer onion.

To process errors returned from downstream nodes when forwarding between Trampoline nodes, we need to store information beyond what's available in `HTLCPreviousHopData`, such as the used hops and the newly generated outer onion's session_priv. To that end, we add a new variant to `HTLCSource` in this commit, which also future-proofs mapping an outbound forward to an incoming MPP.

The previously existing `HTLCDestination` do not map nicely to the failure event of a Trampoline forward, so we introduce a new variant to fill the gap.

In this commit, we expand our forwarding logic to do ad-hoc pathfinding to subsequent Trampoline nodes, covering both blinded and unblinded scenarios. We allow the forwards to use MPP, but we do not yet implement inbound Trampoline MPP handling, nor retry logic. We further modify our error propagation logic to handle Trampoline forward HTLC failures eagerly, i.e. when we receive a failure from the downstream node, we immediately fail or inbound MPP components.

joostjager · 2025-04-17T19:41:55Z

lightning/src/ln/channelmanager.rs

+								let mut recipient_features = Bolt11InvoiceFeatures::empty();
+								recipient_features.set_basic_mpp_optional();
+
+								let route = match self.router.find_route(


One question that came to my mind is to what extent pathfinding here is a DoS vector. It's probably quite easy to keep trampoline nodes busy searching?

valentinewallace · 2025-04-18T23:00:39Z

Thinking about the current approach in the last commit where we find a route --

I think things may be cleaner if we move a lot of the handling of trampoline forwards to the outbound_payments module. In this approach, we would have a similar method to the existing send_payment_* methods there that creates route parameters, registers a PendingOutboundPayment::Retryable that's a trampoline, and forwards via the internal method find_route_and_send_payment.

The benefit of this approach is that retries will be handled automatically and it keeps us from adding a bunch of code to process_pending_htlc_forwards/adds more encapsulation in general.

I think some changes needed for this would be:

when we're processing a trampoline forward in process_pending_htlc_forwards, we'll call into the outbound payments module to handle the pathfinding and the forward, and if it fails then fail the trampoline HTLC backwards
modify ChannelManager's send_payment_along_path to construct the correct HTLC source and payment onion, since this method is called from outbound_payments
if an outbound trampoline forward fails, when we notice this in fail_htlc_backwards_internal we'll call another new method PendingOutboundPayments::trampoline_htlc_failed(..) similar to the fail_htlc method in that module, which will either retry for us or tell us to fail the HTLC(s) backwards
will need to make sure we generate the right events/don't generate PaymentFailed for failed trampolines

Thoughts on this alternate approach?

arik-so force-pushed the arik/trampoline/forwarding branch from 3ad2b6b to 4070974 Compare April 7, 2025 08:42

joostjager reviewed Apr 7, 2025

View reviewed changes

arik-so force-pushed the arik/trampoline/forwarding branch 2 times, most recently from 0931218 to 92fa368 Compare April 7, 2025 20:57

arik-so mentioned this pull request Apr 7, 2025

Trampoline #2299

Open

31 tasks

valentinewallace reviewed Apr 8, 2025

View reviewed changes

arik-so force-pushed the arik/trampoline/forwarding branch 4 times, most recently from c3f5089 to e0a2a9f Compare April 10, 2025 06:19

valentinewallace reviewed Apr 10, 2025

View reviewed changes

arik-so force-pushed the arik/trampoline/forwarding branch from e0a2a9f to 4c6f1ef Compare April 10, 2025 18:21

joostjager reviewed Apr 11, 2025

View reviewed changes

valentinewallace reviewed Apr 11, 2025

View reviewed changes

arik-so force-pushed the arik/trampoline/forwarding branch from 4c6f1ef to 2abfc25 Compare April 12, 2025 02:50

arik-so force-pushed the arik/trampoline/forwarding branch 6 times, most recently from dfd0be7 to 688ad3f Compare April 16, 2025 08:41

arik-so marked this pull request as ready for review April 16, 2025 08:42

arik-so requested review from joostjager and valentinewallace April 16, 2025 08:42

joostjager reviewed Apr 16, 2025

View reviewed changes

joostjager self-requested a review April 16, 2025 11:33

valentinewallace reviewed Apr 16, 2025

View reviewed changes

arik-so force-pushed the arik/trampoline/forwarding branch 2 times, most recently from bbc9414 to 8817662 Compare April 17, 2025 07:53

arik-so added 9 commits April 17, 2025 11:05

TODO: rephrase: blinded hop error handling

50f826f

Enforce Trampoline constraints

df22b62

Ensure that the Trampoline onion's amount and CLTV values do not exceed the limitations imposed by the outer onion.

f: remove duplicated trampoline onion constraint checks

9888bbf

f: formatting

7a875d2

f: last tramp error

a258d01

f: rename PreviousHopIdData

2e49bc3

Expand HTLCDestination variants for Trampoline forwards

037403e

The previously existing `HTLCDestination` do not map nicely to the failure event of a Trampoline forward, so we introduce a new variant to fill the gap.

arik-so force-pushed the arik/trampoline/forwarding branch from 8817662 to 0b86a98 Compare April 17, 2025 19:26

joostjager reviewed Apr 17, 2025

View reviewed changes

		@@ -4353,8 +4355,6 @@ where
		// we don't have the channel here.
		return Err(("Refusing to forward over real channel SCID as our counterparty requested.", 0x4000 \| 10));

		/// Path to send the packet to the next hop
		hops: Vec<RouteHop>

		onion_utils::Hop::TrampolineForward { ref outer_hop_data, next_trampoline_hop_data, next_trampoline_hop_hmac, new_trampoline_packet_bytes, trampoline_shared_secret, .. } => {
		check_trampoline_sanity(outer_hop_data, next_trampoline_hop_data.outgoing_cltv_value, next_trampoline_hop_data.amt_to_forward).map_err(\|()\| {

WIP: Trampoline forwarding #3711

Are you sure you want to change the base?

WIP: Trampoline forwarding #3711

Conversation

arik-so commented Apr 7, 2025 • edited Loading

ldk-reviews-bot commented Apr 7, 2025 • edited Loading

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

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arik-so Apr 16, 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 Apr 11, 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

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

codecov bot commented Apr 12, 2025 • edited Loading

Codecov Report

arik-so commented Apr 16, 2025

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

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

arik-so commented Apr 7, 2025 •

edited

Loading

ldk-reviews-bot commented Apr 7, 2025 •

edited

Loading

arik-so Apr 10, 2025 •

edited

Loading

arik-so Apr 16, 2025 •

edited

Loading

joostjager Apr 11, 2025 •

edited

Loading

codecov bot commented Apr 12, 2025 •

edited

Loading