prefactor: Allow multiple htlcs in/out in forwarding events for trampoline by carlaKC · Pull Request #4304 · lightningdevkit/rust-lightning

carlaKC · 2026-01-08T16:42:57Z

This PR is a prefactor to support trampoline in LDK. The last commit in #3976 contains the remainder of the trampoline logic, which may be useful to understanding the context for some of these refactors. These changes move us from a one HTLC in/ one HTLC out world to one where we allow multiple incoming and multiple outgoing HTLCs, to allow MPP trampoline:

A -->
         --> D
B --> us
         --> E
C -->

In this world, we only want to emit one PaymentForwarded/HTLCHandlingFailed event per trampoline forward. We will receive and process a update_fail/fulfill from each of our downstream peers (D+E), and need to understand whether we want to emit an event:

For claims: when we receive a preimage from D, we'll use it to claim all of our inbound HTLCs A+B+C. When E provides us with the preimage, we can rely on our existing duplicate detection to know that we don't need another event - it'll check the state of all our incoming htlcs and see that we've already settled them.
For failures: when D fails back its HTLC, we can't safely fail back our incoming HTLCs A+B+C until we've received a failure from E as well. This will be implemented in the next PR, by tracking trampoline payments in pending_outbound_payments. If we only fail back once the last outgoing HTLC is cleared, we'll only emit one event.

🧹 A few of these commits can definitely be squashed - I went with splitting move/rename commits up and using a few todo!s that are later filled in to help keep review more readable. I don't mind this, but happy to squash where people see fit!

Also could use some tests - will add once approach has an ACK.

ldk-reviews-bot · 2026-01-08T16:43:00Z

👋 Thanks for assigning @valentinewallace 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.

lightning/src/events/mod.rs

codecov · 2026-01-08T18:34:14Z

Codecov Report

❌ Patch coverage is 60.45340% with 157 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.51%. Comparing base (ef25534) to head (b9a8b09).
⚠️ Report is 29 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	59.82%	128 Missing and 9 partials ⚠️
lightning/src/events/mod.rs	47.05%	14 Missing and 4 partials ⚠️
lightning/src/chain/channelmonitor.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4304      +/-   ##
==========================================
- Coverage   86.60%   86.51%   -0.09%     
==========================================
  Files         158      158              
  Lines      102010   102381     +371     
  Branches   102010   102381     +371     
==========================================
+ Hits        88342    88578     +236     
- Misses      11258    11386     +128     
- Partials     2410     2417       +7

Flag	Coverage Δ
fuzzing	`36.87% <37.39%> (+0.06%)`	⬆️
tests	`85.80% <60.45%> (-0.09%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

ldk-reviews-bot · 2026-01-12T00:00:34Z

🔔 1st Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

CHANGELOG.md

TheBlueMatt · 2026-01-12T17:28:58Z

lightning/src/events/mod.rs

-					(11, next_user_channel_id, option),
-					(13, prev_node_id, option),
-					(15, next_node_id, option),
+					// Type 9 was prev_user_channel_id in 0.2 and earlier.


IMO it would be nice to write the old fields in cases where we have only one input. That way downgrade still includes the fields that users might have expected and even unwrapped safely.

IMO it would be nice to write the old fields in cases where we have only one input.

Would you be happy to just write the first HLTC to the legacy fields all the time, for the sake of simplicity?

Sure, seems fine too. We'll want to document the downgrade behavior in the docs on the event.

lightning/src/events/mod.rs

TheBlueMatt · 2026-01-12T18:12:09Z

lightning/src/events/mod.rs

 				25u8.write(writer)?;
 				write_tlv_fields!(writer, {
-					(0, prev_channel_id, required),
+					// Type 0 was prev_channel_id in 0.2 and earlier.


We definitely need to write this field still so that we can downgrade at all.

TheBlueMatt · 2026-01-12T18:13:49Z

lightning/src/ln/channelmanager.rs

 			1 => Ok(HTLCSource::PreviousHopData(Readable::read(reader)?)),
+			2 => {
+				let mut previous_hop_data = Vec::new();
+				let mut incoming_trampoline_shared_secret: crate::util::ser::RequiredWrapper<[u8; 32]> = crate::util::ser::RequiredWrapper(None);


init_and_read_tlv_fields may be helpful here.

lightning/src/ln/channelmanager.rs

ldk-reviews-bot · 2026-01-14T00:01:17Z

🔔 2nd Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-01-17T00:00:29Z

🔔 3rd Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-01-19T00:01:31Z

🔔 4th Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-01-21T00:02:23Z

🔔 5th Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

valentinewallace

I gave it an initial skim some time ago and Matt covered all the feedback I had at the time, so I think this should probably be good. Can we squash/rebase and I'll take a closer look? 🙏 yay trampoline progress!

In the commits that follow, we want to be able to free the other channel without emitting an event so that we can emit a single event for trampoline payments with multiple incoming HTLCs. We still want to go through the full claim flow for each incoming HTLC (and persist the EmitEventAndFreeOtherChannel event to be picked up on restart), but do not want multiple events for the same trampoline forward. Changing from upgradable_required to upgradable_option is forwards compatible - old versions of the software will always have written this field, newer versions don't require it to be there but will be able to read it as-is. This change is not backwards compatible, because older versions of the software will expect the field to be present but newer versions may not write it. An alternative would be to add a new event type, but that would need to have an even TLV (because the event must be understood and processed on restart to claim the incoming HTLC), so that option isn't backwards compatible either.

In preparation for trampoline failures, allow multiple previous channel ids. We'll only emit a single HTLCHandlingFailed for all of our failed back HTLCs, so we want to be able to express all of them in one event.

This commit adds a SendHTLCId for trampoline forwards, identified by their session_priv. As with an OutboundRoute, we can expect our HTLC to be uniquely identified by a randomly generated session_priv. TrampolineForward could also be identified by the set of all previous outbound scid/htlc id pairs that represent its incoming HTLC(s). We choose the 32 byte session_priv to fix the size of this identifier rather than 16 byte scid/id pairs that will grow with the number of incoming htlcs.

We only have payment details for HTLCSource::TrampolineForward available once we've dispatched the payment. If we get to the stage where we need a HTLCId for the outbound payment, we expect dispatch details to be present. Co-authored-by: Arik Sosman <git@arik.io> Co-authored-by: Maurice Poirrier <mpch@hey.com>

To create the right handling type based on source, add a helper. This is mainly useful for PreviousHopData/TrampolineForward. This helper maps an OutboundRoute to a HTLCHandlingFailureType::Forward. This value isn't actually used once we reach `forward_htlc_backwards_internal`, because we don't emit `HTLCHandlingFailed` events for our own payments. This issue is pre-existing, and could be addressed with an API change to the failure function, which is left out of scope of this work.

Will need to share this code when we add trampoline forwarding. This commit exactly moves the logic as-is, in preparation for the next commit that will update to suit trampoline. Co-authored-by: Arik Sosman <git@arik.io> Co-authored-by: Maurice Poirrier <mpch@hey.com>

When we introduce trampoline forwards, we're going to want to provide two external pieces of information to create events: - When to emit an event: we only want to emit one trampoline event, even when we have multiple incoming htlcs. We need to make multiple calls to claim_funds_from_htlc_forward_hop to claim each individual htlc, which are not aware of each other, so we rely on the caller's closure to decide when to emit Some or None. - Forwarding fees: we will not be able to calculate the total fee for a trampoline forward when an individual outgoing htlcs is fulfilled, because there may be other outgoing htlcs that are not accounted for (we only get the htlc_claim_value_msat for the single htlc that was just fulfilled). In future, we'll be able to provide the total fee from the channelmanager's top level view.

Implement payment claiming for `HTLCSource::TrampolineForward` by iterating through previous hop data and claiming funds for each HTLC. Co-authored-by: Arik Sosman <git@arik.io> Co-authored-by: Maurice Poirrier <mpch@hey.com>

We'll want this extracted when we need to handle trampoline and regular forwards. Co-authored-by: Arik Sosman <git@arik.io> Co-authored-by: Maurice Poirrier <mpch@hey.com>

Implement failure propagation for `HTLCSource::TrampolineForward` by iterating through previous hop data and failing each HTLC with `TemporaryTrampolineFailure`. Note that testing should be implemented when trampoline forward is completed. Co-authored-by: Arik Sosman <git@arik.io> Co-authored-by: Maurice Poirrier <mpch@hey.com>

Move recovery logic for `HTLCSource::PreviousHopData` into `channel_monitor_recovery_internal` to prepare for trampoline forward reuse. Co-authored-by: Arik Sosman <git@arik.io> Co-authored-by: Maurice Poirrier <mpch@hey.com>

Implement channel monitor recovery for trampoline forwards iterating over all hop data and updating pending forwards.

This commit uses the existing outbound payment claims replay logic to restore trampoline claims. If any single previous hop in a htlc source with multiple previous hops requires claim, we represent this with a single outbound claimed htlc because we assume that *all* of the incoming htlcs are represented in the source, and will be appropriately claimed (rather than submitting multiple claims, which will end up being duplicates of each other). This is the case for trampoline payments, where the htlc_source stores all previous hops.

carlaKC · 2026-02-12T17:31:32Z

Squashed old fixups and added a few changes that came up finishing up the full trampoline flow.
range-diff between previously reviewed version and new edits is here.

Rebase is non-trivial, so going to hold off on that until the new changes have had a look!

TheBlueMatt

I haven't thought too deeply about the replay but at a high level it makes sense. Will let @valentinewallace opine on it as well.

TheBlueMatt · 2026-02-17T21:16:38Z

lightning/src/ln/channelmanager.rs

+			} => Self::TrampolineForward {
+				session_priv: outbound_payment
+					.as_ref()
+					.map(|o| o.session_priv.secret_bytes())


IIUC there will be multiple HTLCSources for a single trampoline forward, due to differing TrampolineDispatch for each outbound MPP part. Thus, each outgoing HTLC will be/needs to have a different SentHTLCId. Might be worth commenting that here or on HTLCSource.

TheBlueMatt · 2026-02-17T21:24:21Z

lightning/src/ln/channelmanager.rs

+					onion_error.decode_onion_failure(&self.secp_ctx, &self.logger, &source);
+				let incoming_trampoline_shared_secret = Some(*incoming_trampoline_shared_secret);
+
+				// TODO: when we receive a failure from a single outgoing trampoline HTLC, we don't


This somewhat complicated landing things - if we add support for decoding new fields such that downgrades will not fail on load, we also need to actually handle things safely, which this isn't. Is your intention to hold off on this and just land all of trampoline in one go (probably for 0.4) or should we break some deserialization logic (maybe refuse to deserialize HTLCSources with trampolines for now) so we can make incremental progress?

I believe this will be fine to merge as-is because we'll store dispatched trampoline payments in pending_outbound_payments with an even TLV, so we wouldn't be able to downgrade while we have pending trampoline payments?

Failing to deserialize HTLCSource sounds like a good safety stop if we don't want to depend on that, would be nice to land incrementally.

ldk-reviews-bot · 2026-02-19T09:24:08Z

🔔 1st Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

valentinewallace

Didn't quite finish but made it more than halfway. Looks good! 🫡

valentinewallace · 2026-02-23T18:34:18Z

lightning/src/events/mod.rs

 	/// This event is generated when a payment has been successfully forwarded through us and a
 	/// forwarding fee earned.
 	///
+	/// Note that downgrading from 0.3 with pending trampoline forwards that use multipart payments


lightning/src/events/mod.rs

valentinewallace · 2026-02-23T18:41:04Z

lightning/src/events/mod.rs

+						// We can unwrap in the eagerly-evaluated default_value code because we
+						// always write legacy fields to be backwards compatible, and expect
+						// this field to be set because the legacy field was only None for versions
+						// before 0.0.107 and we do not allow upgrades with pending forwards to 0.1
+						// for any version before 0.0.123.


OOC, did you look into what version the user_channel_id/node_id fields started being included? Just curious if we can do something similar

tbh didn't look into it because they're both Option in HTLCPreviousHopData.

They were added to PaymentForwarded:

node_id = 0.1: I believe we still allow direct upgrades w/ pending htlcs so can't unwrap?

user_chanel_id = 0.0.123: so could do the same upgrade trick (comment should be <= 0.0.123, will fix), but would have to unwrap in claim_funds_internal

valentinewallace · 2026-02-23T18:43:03Z

lightning/src/events/mod.rs

+						(15, next_node_id_legacy, option),
+						// We can unwrap in the eagerly-evaluated default_value code because we
+						// always write legacy fields to be backwards compatible, and expect
+						// this field to be set because the legacy field was only None for versions


nit: maybe just make the channel_id_legacy fields required and mention in the commit message, and can omit that part of the comment?

Doesn't making them required make it harder for us to stop writing them in future? If they're required now, then downgrades to this version will still require that they be written even though the new field is present too.

Fine with either - not totally clear on the process for deprecating fields.

valentinewallace · 2026-02-23T19:36:00Z

lightning/src/ln/channelmanager.rs

 mod fuzzy_channelmanager {
 	use super::*;

+	/// Information about the outgoing payment dispatched to forward to the next trampoline.


nit: s/the outgoing payment/an MPP part of the outgoing payment?

valentinewallace · 2026-02-23T20:39:55Z

lightning/src/ln/channelmanager.rs

-			let reason = HTLCFailReason::from_failure_code(LocalHTLCFailureReason::ChannelClosed);
 			let (source, hash) = htlc_source;
-			self.fail_htlc_backwards_internal(&source, &hash, &reason, receiver, None);
+			let failure_type = source.failure_type(*counterparty_node_id, msg.channel_id);


I appreciate the rename from receiver to failure_type in this diff 😆

lightning/src/ln/channelmanager.rs

valentinewallace · 2026-02-23T22:17:39Z

lightning/src/ln/channelmanager.rs

@@ -1381,8 +1432,11 @@ impl_writeable_tlv_based_enum_upgradable!(MonitorUpdateCompletionAction,
 		(4, blocking_action, upgradable_required),
 		(5, downstream_channel_id, required),
 	},
-	(2, EmitEventAndFreeOtherChannel) => {
-		(0, event, upgradable_required),
+	(2, EmitEventOptionAndFreeOtherChannel) => {


I think we might be able to omit this change and repurpose the FreeOtherChannelImmediately variant, which seems equivalent to this variant when the event is None? At least this passes tests, though the manager read portion may need a closer look: https://pastebin.com/5BGVXHrB. It seems like a weird state if neither field is set, which would currently be allowed.

I took a look at that and got caught up on the expectation that FreeOtherChannelImmediately isn't persisted, because each trampoline claim is a NewClaim which pushes into monitor_update_blocked_actions which will get persisted (previously we'd only create this action for duplicates which are handled differently).

If that's okay (we can after all remove the assert like you've done) then agree this is the way to go 👍

Hmm that's a good point, I'm definitely not sure it makes sense to use FreeOtherChannelImmediately (btw, the patch I posted was Claude'd/sus). Mostly not a fan that the state EmitOptionalEventAndFreeOtherChannel { event: None, channel: None } is allowed, which seems like something we can lean on the compiler to prevent. Maybe it would make more sense to add a new variant FreeInboundChannelTrampoline?

Maybe it would make more sense to add a new variant FreeInboundChannelTrampoline?

Also looked at this but IIRC it led to quite a lot of duplication - let me claude up my own sus patch and see what it looks like.

Ooh yeah I would buy that it leads to a lot of extra code. Wdyt about something like this? 796704c i.e. add an extra bool to the existing FreeOtherChan variant. I'm not sure if we'll also want some way to explicitly detect that it's a partial trampoline case before creating the variant with the bool set, however.

carlaKC changed the title ~~pewd~~ prefactor Jan 8, 2026

carlaKC changed the title ~~prefactor~~ prefactor: Allow multiple htlcs in/out in forwarding events for trampoline Jan 8, 2026

carlaKC mentioned this pull request Jan 8, 2026

Trampoline forwarding [2]: HTLCSource variant refactors #4027

Closed

ldk-reviews-bot requested a review from joostjager January 8, 2026 16:53

carlaKC added this to Weekly Goals Jan 8, 2026

jkczyz assigned carlaKC Jan 8, 2026

carlaKC commented Jan 8, 2026

View reviewed changes

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

valentinewallace requested review from valentinewallace and removed request for joostjager January 9, 2026 16:44

TheBlueMatt reviewed Jan 12, 2026

View reviewed changes

carlaKC force-pushed the 2299-prefactor-htlcsource branch from b9a8b09 to 2c52690 Compare January 16, 2026 20:56

valentinewallace reviewed Jan 22, 2026

View reviewed changes

carlaKC and others added 11 commits February 9, 2026 14:15

ln/events: multiple htlcs in/out for trampoline PaymentForwarded

15c9067

ln/refactor: rename EmitEventAndFreeOtherChannel to note optional event

52c0eb9

ln+events: allow multiple prev_channel_id in HTLCHandlingFailed

724750b

In preparation for trampoline failures, allow multiple previous channel ids. We'll only emit a single HTLCHandlingFailed for all of our failed back HTLCs, so we want to be able to express all of them in one event.

events: add TrampolineForward variant to HTLCHandlingFailureType

79021ca

ln: add trampoline routing payment claiming

d4e4f15

Implement payment claiming for `HTLCSource::TrampolineForward` by iterating through previous hop data and claiming funds for each HTLC. Co-authored-by: Arik Sosman <git@arik.io> Co-authored-by: Maurice Poirrier <mpch@hey.com>

carlaKC and others added 6 commits February 12, 2026 19:15

ln/refactor: add blinded forwarding failure helper function

3349b08

We'll want this extracted when we need to handle trampoline and regular forwards. Co-authored-by: Arik Sosman <git@arik.io> Co-authored-by: Maurice Poirrier <mpch@hey.com>

ln: add channel monitor recovery for trampoline forwards

74a2c4d

Implement channel monitor recovery for trampoline forwards iterating over all hop data and updating pending forwards.

ln/refactor: move outgoing payment replay code into helper function

0e63698

carlaKC force-pushed the 2299-prefactor-htlcsource branch 2 times, most recently from 13c172a to 5cb4c2f Compare February 12, 2026 17:21

carlaKC mentioned this pull request Feb 12, 2026

Full Trampoline Support #4414

Draft

carlaKC requested review from TheBlueMatt and valentinewallace February 17, 2026 09:23

TheBlueMatt reviewed Feb 17, 2026

View reviewed changes

valentinewallace reviewed Feb 23, 2026

View reviewed changes

Comments

Conversation

carlaKC commented Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ldk-reviews-bot commented Jan 12, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ldk-reviews-bot commented Jan 14, 2026

Uh oh!

ldk-reviews-bot commented Jan 17, 2026

Uh oh!

ldk-reviews-bot commented Jan 19, 2026

Uh oh!

ldk-reviews-bot commented Jan 21, 2026

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

carlaKC commented Feb 12, 2026

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Feb 19, 2026

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentinewallace Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

carlaKC commented Jan 8, 2026 •

edited

Loading

ldk-reviews-bot commented Jan 8, 2026 •

edited

Loading

codecov bot commented Jan 8, 2026 •

edited

Loading

valentinewallace Feb 24, 2026 •

edited

Loading

valentinewallace Feb 24, 2026 •

edited

Loading