Prevent HTLC double-forwards, prune forwarded onions #4303

valentinewallace · 2026-01-06T22:02:32Z

Fixes two bugs where we would otherwise double-forward an HTLC
Prunes inbound HTLC onions once they are irrevocably forwarded (#4227 Followups #4280)
Prepares for LDK 0.5 where we can stop persisting legacy ChannelManager forward maps and fully reconstruct them from Channel data. See the follow-up tagged 0.5: (0.5) Deprecate legacy pending HTLC ChannelManager persistence #4359

Closes #4280, partially addresses #4286

~~Based on #4289~~

Reconstruct pending HTLC set from Channels in prod based on the serialized manager version Fix double-forward, prefer legacy forward maps #4289 (comment)
Fix double-forward if outbound HTLC is in holding cell (#4227 Followups #4280 (comment))

ldk-reviews-bot · 2026-01-06T22:02:35Z

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

codecov · 2026-01-06T23:43:32Z

Codecov Report

❌ Patch coverage is 87.71930% with 49 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.05%. Comparing base (1d90fce) to head (3b75eee).
⚠️ Report is 44 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	87.84%	27 Missing and 8 partials ⚠️
lightning/src/ln/channel.rs	88.50%	9 Missing and 1 partial ⚠️
lightning/src/util/ser.rs	40.00%	0 Missing and 3 partials ⚠️
lightning/src/ln/functional_test_utils.rs	94.73%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4303      +/-   ##
==========================================
+ Coverage   86.03%   86.05%   +0.01%     
==========================================
  Files         156      156              
  Lines      103036   103334     +298     
  Branches   103036   103334     +298     
==========================================
+ Hits        88652    88919     +267     
- Misses      11876    11902      +26     
- Partials     2508     2513       +5

Flag	Coverage Δ
tests	`86.05% <87.71%> (+0.01%)`	⬆️

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.

lightning/src/ln/channel.rs

valentinewallace · 2026-01-14T17:19:52Z

Pushed a rebase on main due to #4289 landing

joostjager · 2026-01-29T18:22:22Z

Haven't reviewed yet, but the following question came up: in the previous pr and in this pr, double-forward bugs are fixed. Couldn't the fuzzer detect this?

joostjager

The logic in this PR is quite complex, with multiple interacting state machines (inbound HTLC state, outbound HTLC state, holding cells, monitor updates) that need to stay consistent across restarts. The fact that multiple double-forward bugs were discovered during development suggests the state space is large enough that targeted unit tests may not provide sufficient coverage.

I keep wondering if the existing fuzzer can exercise these restart scenarios with in-flight HTLCs at various stages. The current strategy of adding a specific regression test might be too reactive?

lightning/src/ln/channel.rs

joostjager · 2026-01-30T10:28:02Z

lightning/src/ln/channelmanager.rs

+//
+// If 0.3 or 0.4 reads this manager version, it knows that the legacy maps were not written and
+// acts accordingly.
+const RECONSTRUCT_HTLCS_FROM_CHANS_VERSION: u8 = 2;


Didn't you want to make this version 10, to be able to insert other upgrades in between?

lightning/src/ln/channel.rs

joostjager · 2026-01-30T10:36:55Z

lightning/src/ln/channel.rs

+	Forwarded {
+		/// Useful if we need to fail or claim this HTLC backwards after restart, if it's missing in the
+		/// outbound edge.
+		hop_data: HTLCPreviousHopData,


I think there is some data in here that is redundant with InboundHTLCOutput. Not sure if we care

I'm not sure either. @TheBlueMatt any opinion? This is the simplest way so I might save the dedup for followup.

Hmm, yea, would still be quite nice to reduce size here but its alright to do it in a followup (just has to happen in 0.3!)

@TheBlueMatt @joostjager I sketched this out: 9c8a92e

It adds a fair amount of code and only saves the htlc_id and cltv_expiry fields. Does that seem worth it? I'm not super convinced but I guess it could add up

Hm yes, I don't know if it is worth it. Note that in WithOnion there are also duplications. I think it could have been

WithOnion { onion_routing_packet: OnionPacket, skimmed_fee_msat: Option<u64>, blinding_point: Option<PublicKey>, },

Generally I like deduplicated data. No risk of an invalid state. But no strong opinion here.

Wait can't we also drop the channel_id, outpoint, counterparty_node_id, user_channel_id, and prev_outbound_scid_alias?

channel_id and prev_outbound_scid_alias are used in fail_htlc_backwards_internal. counteparty_node_id, user_channel_id, and outpoint are used in claim_fund_internal / claim_funds_from_hop.

Duh, we do have those fields already because we have the whole inbound Channel. Will proceed with de-duplicating in follow-up.

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

TheBlueMatt · 2026-01-30T21:55:57Z

lightning/src/ln/channel.rs

+	Forwarded {
+		/// Useful if we need to fail or claim this HTLC backwards after restart, if it's missing in the
+		/// outbound edge.
+		hop_data: HTLCPreviousHopData,


Hmm, yea, would still be quite nice to reduce size here but its alright to do it in a followup (just has to happen in 0.3!)

valentinewallace · 2026-02-02T19:59:06Z

Rebased since #4332 landed, haven't pushed updates addressing feedback yet

TheBlueMatt

LGTM except for the last hunk in the second-to-last commit. Really have no idea what's going on there.

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2026-02-04T15:45:20Z

lightning/src/ln/channelmanager.rs

+				for (hash, hop_data, outbound_amt_msat) in
+					mem::take(&mut already_forwarded_htlcs).drain(..)
+				{
+					if hash != payment_hash {


I'm really confused by this hunk. This loop is really just being used for inbound payments, not forwarded ones, so this seems like it should belong further up where we're creating pending_claims_to_replay. I'm not really sure why this is in the loop at all - it looks at the channel we're iterating over when scanning pending_claims_to_replay but not when looking at the already_forwarded_htlcs entry?

We're also keying by payment hash - skipping any failures that happen to have the same payment hash as a forwarded payment, possibly resulting in unrelated HTLCs (eg an MPP that got forwarded through us twice) getting forgotten (not that its critical to forget them, just that its weird).

Discussed quite a bit offline. I added a fixup converting already_forwarded_htlcs to a hashmap.

The conclusion of the discussion was, instead of the current approach, to: "when iterating through stored preimages, rather than using already_forwarded_htlcs, ask the channel for its list of [all] inbound htlcs and add the unclaimed ones in general to pending_claims_to_replay."

However, I think the only HTLCs we can easily add to pending_claims_to_replay are the ones that are already in already_forwarded_htlcs. All other types of inbound unclaimed HTLCs don't have an HTLCSource available. So the discussed approach seems like a decent amount of extra code to manually construct the HTLCSources, for what seem like unreachable codepaths (since if the monitor and the manager are that out-of-sync about HTLC state, then the manager is stale and we probably FC'd the channel anyway).

I think a big part of the complaint about this code hunk is that it wasn't accessing data that was specific to a channel, so that part should at least be fixed now with the hashmap update.

(Btw, I will push the ChannelMonitor docs updates we discussed on the next push.)

Discussed more offline yesterday, pushed the approach we seemed to land on.

It's not ideal that we claim an inbound HTLC that doesn't come from a specific outbound HTLC's source, but it doesn't seem worth it to worry about too much here. Basically, we don't always want to claim an HTLC just because we have the preimage for it, because there can be an attack where a malicious intermediate node receives an HTLC to forward, then forwards a 1-msat to another node with the same payment_hash to see if they'll claim it, then claims the inbound HTLC without forwarding. It's unlikely to happen in this case because it relies on a node restart.

joostjager · 2026-02-05T08:13:39Z

lightning/src/ln/channelmanager.rs

+		>,
+		                            prev_hop: &HTLCPreviousHopData| {
+			if let hash_map::Entry::Occupied(mut entry) =
+				already_forwarded_htlcs.entry(prev_hop.channel_id)


It seems one other change here is that prev_outbound_scid_alias is no longer used?

Good point, will fix on the next push after resolving the discussion with Matt above.

Actually, I'm not sure it makes sense to check the scid since we're already checking the channel_id.

lightning/src/ln/channel.rs

valentinewallace · 2026-02-06T17:54:30Z

Rebased due to conflicts, will address feedback on the next push

We recently added support for reconstructing ChannelManager::decode_update_add_htlcs on startup, using data present in the Channels. However, we failed to prune HTLCs from this rebuilt map if a given inbound HTLC was already forwarded to the outbound edge and in the outbound holding cell (this bug could've caused us to double-forward HTLCs, fortunately it never shipped). As part of fixing this bug, we clean up the overall pruning approach by: 1. If the Channel is open, then it is the source of truth for what HTLCs are outbound+pending (including pending in the holding cell) 2. If the Channel is closed, then the corresponding ChannelMonitor is the source of truth for what HTLCs are outbound+pending Previously, we would only consider the monitor's pending HTLCs, which ignored holding cell HTLCs. Co-Authored-By: Claude Opus 4.5 <[email protected]>

This cleanup falls out of the changes made in the previous commit. Separated out here for reviewability.

We recently added support for reconstructing ChannelManager::decode_update_add_htlcs on startup, using data present in the Channels. However, we failed to prune HTLCs from this rebuilt map if a given HTLC was already forwarded+removed from the outbound edge and resolved in the inbound edge's holding cell. Here we fix this bug that would have caused us to double-forward inbound HTLC forwards, which fortunately was not shipped. Co-Authored-By: Claude Opus 4.5 <[email protected]>

In 0.3+, we are taking steps to remove the requirement of regularly persisting the ChannelManager and instead rebuild the set of HTLC forwards (and the manager generally) from Channel{Monitor} data. We previously merged support for reconstructing the ChannelManager::decode_update_add_htlcs map from channel data, using a new HTLC onion field that will be present for inbound HTLCs received on 0.3+ only. However, we now want to add support for pruning this field once it's no longer needed so it doesn't get persisted every time the manager gets persisted. At the same time, in a future LDK version we need to detect whether the field was ever present to begin with to prevent upgrading with legacy HTLCs present. We accomplish both by converting the plain update_add option that was previously serialized to an enum that can indicate whether the HTLC is from 0.2- versus 0.3+-with-onion-pruned (a variant for the latter is added in the next commit). Actual pruning of the new update_add field is added in the next commit.

We store inbound committed HTLCs' onions in Channels for use in reconstructing the pending HTLC set on ChannelManager read. If an HTLC has been forwarded to the outbound edge, we no longer need to persist the inbound edge's onion and can prune it here.

We recently merged (test-only, for now) support for the ChannelManager reconstructing its set of pending HTLCs from Channel{Monitor} data, rather than using its own persisted maps. But because we want test coverage of both the new reconstruction codepaths as well as the old persisted map codepaths, in tests we would decide between those two sets of codepaths randomly. We now want to add some tests that require using the new codepaths, so here we add an option to explicitly set whether to reconstruct or not rather than choosing randomly.

Cleans it up a bit in preparation for adding a new variant in the next commit.

In a recent commit, we added support for pruning an inbound HTLC's persisted onion once the HTLC has been irrevocably forwarded to the outbound edge. Here, we add a check on startup that those inbound HTLCs were actually handled. Specifically, we check that the inbound HTLC is either (a) currently present in the outbound edge or (b) was removed via claim. If neither of those are true, we infer that the HTLC was removed from the outbound edge via fail and fail the inbound HTLC backwards.

In 0.3+, we are taking steps to remove the requirement of regularly persisting the ChannelManager and instead rebuild the set of HTLC forwards (and the manager generally) from Channel{Monitor} data. We previously merged support for reconstructing the ChannelManager::decode_update_add_htlcs map from channel data, using a new HTLC onion field that will be present for inbound HTLCs received on 0.3+ only. The plan is that in upcoming LDK versions, the manager will reconstruct this map and the other forward/claimable/pending HTLC maps will automatically repopulate themselves on the next call to process_pending_htlc_forwards. As such, once we're in a future version that reconstructs the pending HTLC set, we can stop persisting the legacy ChannelManager maps such as forward_htlcs, pending_intercepted_htlcs since they will never be used. For 0.3 to be compatible with this future version, in this commit we detect that the manager was last written on a version of LDK that doesn't persist the legacy maps. In that case, we don't try to read the old forwards map and run the new reconstruction logic only.

valentinewallace · 2026-02-06T19:14:40Z

Addressed feedback (sorry, there's a spurious change from accidentally rebasing on main a 2nd time): diff

valentinewallace · 2026-02-06T19:15:37Z

lightning/src/ln/channelmanager.rs

+		// that it is handled.
+		let mut already_forwarded_htlcs: HashMap<
+			(ChannelId, PaymentHash),
+			Vec<(HTLCPreviousHopData, u64)>,


@TheBlueMatt I think we can't avoid having a Vec here, because there can be multiple HTLCs corresponding to the same (ChannelId, PaymentHash) tuple due to MPP?

Yea, that makes sense. Can claude add a test for having two HTLCs forwarded over the same path with MPP?

Yeah I'll go for that test. We'll want two claims for the same payment where both HTLC claims went into the inbound holding cell prior to shutdown (and the holding cell was lost after restart).

But now I'm wondering, since we need to loop anyway, is it really worth having a hashmap given that each value is a Vec? Seems like extra allocations...

Update: going to add the test in follow-up PR #4405

A lot less to loop over :). At some point we really need to do a prevec like Bitcoin Core's...

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2026-02-09T21:28:39Z

lightning/src/ln/channelmanager.rs

+		// that it is handled.
+		let mut already_forwarded_htlcs: HashMap<
+			(ChannelId, PaymentHash),
+			Vec<(HTLCPreviousHopData, u64)>,


A lot less to loop over :). At some point we really need to do a prevec like Bitcoin Core's...

TheBlueMatt · 2026-02-09T22:07:05Z

lightning/src/ln/channelmanager.rs

+//
+// If 0.3 or 0.4 reads this manager version, it knows that the legacy maps were not written and
+// acts accordingly.
+const RECONSTRUCT_HTLCS_FROM_CHANS_VERSION: u8 = 5;


Why 5 and not 2?

Requested by @joostjager in case we need to bump again in the meantime, cc #4303 (comment)

Given we haven't bumped it since we implemented persistence years ago I'm pretty skeptical lol. Also, if we do define a new version we'll need a similar migration gap so we can just call it 2 and any new version upgrades will presumably need to start at 3 and go to 0.6 or whatever.

Yeah I mentioned that first bit, I think he was thinking we might the flexibility of having a bump that's in-between? @joostjager I don't mind either way, let us know your thoughts

the version field is only really useful for stuff like this - backwards-incompat changes where we're removing fields. Anything else can go through TLVs, so I don't really think we can have some change in between.

Why wouldn't it be possible that we want to remove other non-tlv fields on a different timeline, in between?

joostjager

Ack on hashmap key change. My concerns about the complexity of this PR and the absence of fuzzing coverage remain.

joostjager · 2026-02-10T11:17:17Z

lightning/src/ln/channelmanager.rs

+								payment_preimage,
+								outbound_amt_msat,
+								is_channel_closed,
+								counterparty_node_id,


pending_claims_to_replay gets destructured further down into downstream_* fields (the outgoing side, and ending up in PaymentForwarded as "next_*"?), but it is populated here with data from the inbound monitor? Maybe this was covered in previous discussion, but not 100% sure.

You are right :( I think early on in the PR I thought this was the downstream channel and forgot to go back and correct it. I have an unaddressed todo that probably would've caught this... Unfortunately, fixing this means persisting more data in the inbound edge HTLC so we can remember more stuff about the outbound edge.

But this means that the event data is incorrect. Maybe the event fields can then better be made optional?

I fixed the incorrect data in #4405.

TheBlueMatt · 2026-02-10T15:32:20Z

Landing, changes can happen in a followup.

valentinewallace added this to the 0.3 milestone Jan 6, 2026

valentinewallace mentioned this pull request Jan 6, 2026

Reconstruct ChannelManager forwarded HTLCs maps from Channels #4227

Merged

3 tasks

valentinewallace force-pushed the 2025-12-reconstruct-fwds-followup-2 branch from 01aa4c0 to 4012864 Compare January 7, 2026 21:50

valentinewallace self-assigned this Jan 8, 2026

valentinewallace added this to Weekly Goals Jan 8, 2026

valentinewallace added the weekly goal Someone wants to land this this week label Jan 8, 2026

valentinewallace force-pushed the 2025-12-reconstruct-fwds-followup-2 branch from 4012864 to e2ea1ed Compare January 8, 2026 21:16

valentinewallace marked this pull request as ready for review January 8, 2026 21:16

ldk-reviews-bot requested a review from wpaulino January 8, 2026 21:16

valentinewallace removed the request for review from wpaulino January 8, 2026 21:16

TheBlueMatt reviewed Jan 11, 2026

View reviewed changes

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

valentinewallace force-pushed the 2025-12-reconstruct-fwds-followup-2 branch from e2ea1ed to c40741b Compare January 14, 2026 17:19

valentinewallace mentioned this pull request Jan 15, 2026

Stale ChannelManager causes channel force-closures #4286

Open

17 tasks

valentinewallace force-pushed the 2025-12-reconstruct-fwds-followup-2 branch from c40741b to 3f0f811 Compare January 29, 2026 16:09

valentinewallace changed the title ~~Follow-ups to #4227 (Part 2)~~ Prevent HTLC double-forwards, prune forwarded onions Jan 29, 2026

valentinewallace force-pushed the 2025-12-reconstruct-fwds-followup-2 branch from 3f0f811 to fce5071 Compare January 29, 2026 16:27

valentinewallace requested review from TheBlueMatt and joostjager January 29, 2026 16:28

This was referenced Jan 29, 2026

Split ChannelManager::read into two stages #4332

Merged

(0.5) Deprecate legacy pending HTLC ChannelManager persistence #4359

Draft

joostjager reviewed Jan 30, 2026

View reviewed changes

TheBlueMatt reviewed Jan 30, 2026

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

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

TheBlueMatt reviewed Jan 30, 2026

View reviewed changes

valentinewallace force-pushed the 2025-12-reconstruct-fwds-followup-2 branch from fce5071 to 84093ca Compare February 2, 2026 19:58

valentinewallace force-pushed the 2025-12-reconstruct-fwds-followup-2 branch from 84093ca to 94815bd Compare February 2, 2026 23:21

TheBlueMatt reviewed Feb 4, 2026

View reviewed changes

valentinewallace dismissed joostjager’s stale review via 18268f2 February 4, 2026 22:38

valentinewallace force-pushed the 2025-12-reconstruct-fwds-followup-2 branch from 4e69514 to 18268f2 Compare February 4, 2026 22:38

joostjager reviewed Feb 5, 2026

View reviewed changes

valentinewallace mentioned this pull request Feb 5, 2026

ChannelManager read refactor follow ups #4374

Merged

valentinewallace commented Feb 5, 2026

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

valentinewallace force-pushed the 2025-12-reconstruct-fwds-followup-2 branch from 18268f2 to 1b1408c Compare February 6, 2026 17:54

valentinewallace and others added 11 commits February 6, 2026 14:02

Trivial: document some fields on MonitorRestoreUpdates

a36c85a

Simplify channel_closed check on manager read

7304cc9

This cleanup falls out of the changes made in the previous commit. Separated out here for reviewability.

Trivially refactor reload_node macro

30dbf40

Cleans it up a bit in preparation for adding a new variant in the next commit.

Fix docs on ChannelMonitor::payment_preimages

3b75eee

valentinewallace force-pushed the 2025-12-reconstruct-fwds-followup-2 branch from 1b1408c to 3b75eee Compare February 6, 2026 19:12

valentinewallace commented Feb 6, 2026

View reviewed changes

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

valentinewallace mentioned this pull request Feb 9, 2026

Dedup InboundUpdateAdd::Forwarded data; fix PaymentForwarded fields #4405

Open

4 tasks

TheBlueMatt approved these changes Feb 9, 2026

View reviewed changes

valentinewallace requested a review from joostjager February 9, 2026 22:14

joostjager approved these changes Feb 10, 2026

View reviewed changes

TheBlueMatt merged commit 4b1cbf7 into lightningdevkit:main Feb 10, 2026
19 of 21 checks passed

github-project-automation bot moved this to Done in Weekly Goals Feb 10, 2026

Prevent HTLC double-forwards, prune forwarded onions #4303

Prevent HTLC double-forwards, prune forwarded onions #4303

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

joostjager commented Jan 29, 2026

Uh oh!

joostjager left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentinewallace commented Feb 2, 2026

Uh oh!

TheBlueMatt left a comment

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!

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!

valentinewallace commented Feb 6, 2026

Uh oh!

valentinewallace commented Feb 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

valentinewallace commented Jan 6, 2026 •

edited

Loading

ldk-reviews-bot commented Jan 6, 2026 •

edited

Loading

codecov bot commented Jan 6, 2026 •

edited

Loading

valentinewallace commented Jan 14, 2026 •

edited

Loading

valentinewallace Feb 4, 2026 •

edited

Loading

TheBlueMatt Feb 9, 2026 •

edited

Loading