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

valentinewallace · 2026-02-09T19:34:29Z

Previously, the InboundUpdateAdd::Forwarded enum variant contained an
HTLCPreviousHopData, which had a lot of fields that were redundant with the
outer InboundHTLCOutput/Channel structs. Here we dedup those fields, which is
important because the pending InboundUpdateAdds are persisted whenever the
ChannelManager is persisted.

Add test Prevent HTLC double-forwards, prune forwarded onions #4303 (comment)
Correct inbound-vs-outbound edge data in generated event Prevent HTLC double-forwards, prune forwarded onions #4303 (comment)
- Test the above
Maybe update manager version bump to 2 pending outcome of Prevent HTLC double-forwards, prune forwarded onions #4303 (comment)

~~Based on #4303~~
Partially addresses #4286

ldk-reviews-bot · 2026-02-09T19:34:32Z

👋 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-02-09T21:17:01Z

Codecov Report

❌ Patch coverage is 96.03175% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.06%. Comparing base (caf0aac) to head (ab0ba65).
⚠️ Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	94.23%	3 Missing ⚠️
lightning/src/ln/channelmanager.rs	97.01%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4405      +/-   ##
==========================================
+ Coverage   86.05%   86.06%   +0.01%     
==========================================
  Files         156      156              
  Lines      103384   103431      +47     
  Branches   103384   103431      +47     
==========================================
+ Hits        88964    89021      +57     
+ Misses      11905    11899       -6     
+ Partials     2515     2511       -4

Flag	Coverage Δ
tests	`86.06% <96.03%> (+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.

Useful when using these macros in lightning-tests/upgrade_downgrade_tests

In the next commit, we want to dedup fields between the InboundUpdateAdd::Forwarded's HTLCPreviousHopData and the outer InboundHTLCOutput/Channel structs, since many fields are duplicated in both places at the moment. As part of doing this cleanly, we first refactor the method that retrieves these InboundUpdateAdds for reconstructing the set of pending HTLCs during ChannelManager deconstruction. Co-Authored-By: Claude Opus 4.5 <[email protected]>

Previously, the InboundUpdateAdd::Forwarded enum variant contained an HTLCPreviousHopData, which had a lot of fields that were redundant with the outer InboundHTLCOutput/Channel structs. Here we dedup those fields, which is important because the pending InboundUpdateAdds are persisted whenever the ChannelManager is persisted.

valentinewallace · 2026-02-11T22:54:59Z

Still need to finalize what manager version to write (#4303 (comment)), but otherwise this should be good to go.

lightning/src/ln/reload_tests.rs

TheBlueMatt

Didn't review the test changes but otherwise basically lgtm.

lightning/src/ln/channel.rs

joostjager

Clean PR.

The PR description motivates the dedup as important for reducing persistence size, how much is actually saved? Claude says 153 bytes per HTLC.

Given that the number of in-flight forwarded HTLCs is typically low, the net persistence savings seem marginal.

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

joostjager · 2026-02-12T11:38:16Z

lightning/src/ln/reload_tests.rs

+	// Each 1M sat outbound channel has 100M msat max in-flight, so 150M msat requires splitting.
+	let amt_msat = 150_000_000;
+	let (route, payment_hash, payment_preimage, payment_secret) =
+		get_route_and_payment_hash!(nodes[0], nodes[2], amt_msat);


In other tests, I've seen the routes being constructed in the test, for full control. Also a bit more self-documenting perhaps.

I prefer it this way, it's more concise and looks less janky IMO.

joostjager · 2026-02-12T12:07:18Z

One concern with reconstructing forwarding state from channel monitors: will trampoline make this significantly harder?

Today a forward is a 1-to-1 link between two channels, but with trampoline a single logical forward can involve multiple inbound and multiple outbound HTLCs across different channels (MPP on both sides). Reconstructing that M-to-N relationship from individual monitors, without any explicit record tying them together, seems pretty complicated?

TheBlueMatt · 2026-02-12T15:30:24Z

It should be relatively doable - we need to assign a "trampoline id" for a trampoline anyway so that the outbound edges know where to go to update state/claim (in the HTLCSource) so when we're reloading without state we should be able to just keep a map of "htlcs inbound to trampoline id" and track if there are any "htlcs outbound for trampoline id", equivalent to the current code that just tracks "htlcs inbound unresolved" and removes "htlcs outbound unresolved" from it.

joostjager · 2026-02-12T15:55:35Z

The id seems helpful indeed. But even with that, reconstruction adds complexity on top of what's already a non-trivial trampoline state machine. And also it is already hard to get the current reconstruction right.

I am still not fully sold that the idea of storing all state in the monitors one way or another, and the performance benefit that that might give us, is worth it at the current stage of the lightning game. I guess we'll find out.

Makes an upcoming commit cleaner: when we add a next_hop variable we want to distinguish it from the previous hop.

Adds support for passing user_channel_id into the pending_claims_to_replay vec, which is used by the ChannelManager on startup. For now user_channel_id is always set to None, but in upcoming commits we will set it to Some when the downstream channel is still open (this is currently a bug). Separated out here for reviewability.

We need these fields to generate a correct PaymentForwarded event if we need to claim this inbound HTLC backwards after restart and it's already been claimed and removed on the outbound edge.

Previously, we were spuriously using the upstream channel's info when we should've been using the downstream channel's.

Previously, if a forwarding node reloaded mid-HTLC-forward with a preimage in the outbound edge monitor and the outbound edge channel still open, and subsequently reclaimed the inbound HTLC backwards, the PaymentForwarded event would be missing the next_user_channel_id field.

Test that if we restart and had two inbound MPP-part HTLCs received over the same channel in the holding cell prior to shutdown, and we lost the holding cell prior to restart, those HTLCs will still be claimed backwards. Test largely written by Claude

We previously had 5 due to wanting some flexibility to bump versions in between, but eventually concluded that wasn't necessary.

valentinewallace · 2026-02-12T20:34:48Z

The PR description motivates the dedup as important for reducing persistence size, how much is actually saved? Claude says 153 bytes per HTLC.

It could potentially add up, especially for a node like c=, given how often we persist the manager. If 153 bytes is correct and all slots are filled then a single channel could have an additional ~73kib (Matt previously said his entire node is ~255kib, cc #4227 (comment)). Also nice to not have the potential for data to be inconsistent, of course, as you previously pointed out.

joostjager · 2026-02-13T09:10:01Z

Even after the dedup, the total per-HTLC storage for a forwarded-but-unresolved HTLC is still around 600 bytes (Clauded) across both channel sides.

Matt's 255 kb node size also depends heavily on how busy the node is. Not sure if that is a useful baseline number. Also 483 htlcs in flight is an extreme case.

Happy that the potential for inconsistencies has been fixed though!

joostjager

Much better with the struct extracted!

valentinewallace added this to the 0.3 milestone Feb 9, 2026

valentinewallace self-assigned this Feb 9, 2026

valentinewallace added the weekly goal Someone wants to land this this week label Feb 9, 2026

valentinewallace mentioned this pull request Feb 9, 2026

Prevent HTLC double-forwards, prune forwarded onions #4303

Merged

2 tasks

valentinewallace and others added 3 commits February 10, 2026 13:19

Trivial: use full path in test macros

eb31aeb

Useful when using these macros in lightning-tests/upgrade_downgrade_tests

valentinewallace force-pushed the 2026-02-dedup-htlc-fwd-data branch from 0d38512 to 94cb95d Compare February 10, 2026 22:42

valentinewallace mentioned this pull request Feb 11, 2026

Stale ChannelManager causes channel force-closures #4286

Open

17 tasks

valentinewallace marked this pull request as ready for review February 11, 2026 22:55

ldk-reviews-bot requested a review from tankyleo February 11, 2026 22:55

valentinewallace changed the title ~~Dedup InboundUpdateAdd::Forwarded data with outer structs~~ Dedup InboundUpdateAdd::Forwarded data; fix PaymentForwarded fields Feb 11, 2026

valentinewallace requested review from TheBlueMatt and joostjager and removed request for tankyleo February 11, 2026 23:00

valentinewallace commented Feb 12, 2026

View reviewed changes

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

TheBlueMatt reviewed Feb 12, 2026

View reviewed changes

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

joostjager reviewed Feb 12, 2026

View reviewed changes

valentinewallace added 6 commits February 12, 2026 15:15

Trivial: ChannelManager::read var rename prefactor

3246010

Makes an upcoming commit cleaner: when we add a next_hop variable we want to distinguish it from the previous hop.

Persist outbound channel info in inbound HTLCs

b3b59e6

We need these fields to generate a correct PaymentForwarded event if we need to claim this inbound HTLC backwards after restart and it's already been claimed and removed on the outbound edge.

Fix PaymentForwarded fields on restart claim

48010cb

Previously, we were spuriously using the upstream channel's info when we should've been using the downstream channel's.

Update RECONSTRUCT_HTLCS_FROM_CHANS_VERSION 5 -> 2

ab0ba65

We previously had 5 due to wanting some flexibility to bump versions in between, but eventually concluded that wasn't necessary.

valentinewallace force-pushed the 2026-02-dedup-htlc-fwd-data branch from 58edcb2 to ab0ba65 Compare February 12, 2026 20:30

valentinewallace requested review from TheBlueMatt and joostjager February 12, 2026 20:39

TheBlueMatt approved these changes Feb 12, 2026

View reviewed changes

joostjager approved these changes Feb 13, 2026

View reviewed changes

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

Are you sure you want to change the base?

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

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

valentinewallace commented Feb 11, 2026

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joostjager left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joostjager Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

valentinewallace Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

joostjager commented Feb 12, 2026

Uh oh!

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

Uh oh!

Uh oh!

joostjager commented Feb 12, 2026

Uh oh!

valentinewallace commented Feb 12, 2026

Uh oh!

joostjager commented Feb 13, 2026

Uh oh!

joostjager left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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

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

valentinewallace commented Feb 9, 2026 •

edited

Loading

ldk-reviews-bot commented Feb 9, 2026 •

edited

Loading

codecov bot commented Feb 9, 2026 •

edited

Loading

TheBlueMatt commented Feb 12, 2026 •

edited

Loading