Do not track HTLC IDs as separate MPP parts which need claiming

[TheBlueMatt]

When we claim an MPP payment, we need to track which channels have
had the preimage durably added to their `ChannelMonitor` to ensure
we don't remove the preimage from any `ChannelMonitor`s until all
`ChannelMonitor`s have the preimage.
Previously, we tracked each MPP part, down to the HTLC ID, as a
part which we needed to get the preimage on disk for. However, this
is not necessary - once a `ChannelMonitor` has a preimage, it
applies it to all inbound HTLCs with the same payment hash.
Further, this can cause a channel to wait on itself in cases of
high-latency synchronous persistence -
* If we have receive an MPP payment for which multiple parts came
to us over the same channel,
* and claim the MPP payment, creating a `ChannelMonitorUpdate` for
the first part but enqueueing the remaining HTLC claim(s) in the
channel's holding cell,
* and we receive a `revoke_and_ack` for the same channel before
the `ChannelManager::claim_payment` method completes (as each
claim waits for the `ChannelMonitorUpdate` persistence),
* we will cause the `ChannelMonitorUpdate` for that
`revoke_and_ack` to go into the blocked set, waiting on the MPP
parts to be fully claimed,
* but when `claim_payment` goes to add the next
`ChannelMonitorUpdate` for the MPP claim, it will be placed in
the blocked set, since the blocked set is non-empty.
Thus, we'll end up with a `ChannelMonitorUpdate` in the blocked set
which is needed to unblock the channel since it is a part of the
MPP set which blocked the channel.

[ldk-reviews-bot]

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

[shaavan]

Looks great on first pass! Will take another look at the test to make sure I understand everything correctly.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @wpaulino! 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.

[wpaulino]

Great find!

Would an alternative way of fixing this be by only queueing a single monitor update for all the HTLC parts belonging to the same channel?

I'd also like to, if possible, prevent future cases like this where the channel suddenly becomes blocked resulting in a force close. Can/should we trigger a panic if a queued monitor update that we need to process to unblock a channel becomes blocked?

[G8XSU]

Diff looks good to me overall.

[TheBlueMatt]

Would an alternative way of fixing this be by only queueing a single monitor update for all the HTLC parts belonging to the same channel?

Yes, that would also fix this. I thought about trying to do that first, but its not as nice for a backport in that its a fairly nontrivial difference. The advantage that would have, of course, is that it'd be nice performance win on top (MPP claims in one commitment update and one ChannelMonitorUpdate rather than several). Of course the real way to do this is to finally fix the old //TODO: Delay the claimed_funds relaying just like we do outbound relay!, which would lean on the holding cell to queue up claims until a timer cycle which also gives us privacy, but, again, much larger patch for this fix and not something I want to backport.

I'd also like to, if possible, prevent future cases like this where the channel suddenly becomes blocked resulting in a force close. Can/should we trigger a panic if a queued monitor update that we need to process to unblock a channel becomes blocked?

Hmm, we definitely should, but its not entirely clear to me how. We don't track explicit dependencies in the form of which specific monitor updates we're waiting on to release a monitor update, so I don't really see a clean assertion path here. I guess we could assert for this specific bug that we don't want a channel with no pending monitor updates to be blocked on an MPP claim from itself (with a monitor update in the pending queue), but its pretty tailored to exactly this problem which doesn't seem all that useful.

[wpaulino]

CI is still failing

[TheBlueMatt]

Yea, it hard to repro locally so I'm kinda fighting CI directly :/

[TheBlueMatt]

Okay, it passed the last time modulo rustfmt, so it should again this time.

[TheBlueMatt]

Nope, still hanging :(

[codecov]

Codecov Report

Attention: Patch coverage is 91.39785% with 24 lines in your changes missing coverage. Please review.

Project coverage is 90.43%. Comparing base (1fc2726) to head (26e7e96).
Report is 45 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/chanmon_update_fail_tests.rs	94.18%	6 Missing and 4 partials ⚠️
lightning/src/sync/debug_sync.rs	77.50%	9 Missing ⚠️
lightning/src/util/test_utils.rs	82.35%	1 Missing and 2 partials ⚠️
lightning/src/ln/functional_test_utils.rs	86.66%	2 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3680 +/- ##
==========================================
+ Coverage 89.25% 90.43% +1.18% 
==========================================
Files 155 155 Lines 119708 129654 +9946 Branches 119708 129654 +9946 ==========================================
+ Hits 106845 117255 +10410 + Misses 10250 9864 -386 + Partials 2613 2535 -78 

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

[TheBlueMatt]

Okay, this is finally ready for review. The test is quite stable (tho doesn't work at all on windows).

[dunxen]

Okay, this is finally ready for review.

Nice. LGTM.

(tho doesn't work at all on windows)

Curious if it's some specific windows primitive that just won't play nice?

[TheBlueMatt]

Yea, windows is not a real operating system - its still, somehow, doing cooperative multitasking in 2025 (only within the same process, but still)

[TheBlueMatt]

Squashed.

[TheBlueMatt]

Hmm, think the patch fails on rebase,

$ git diff-tree -U1 c83280ab1 93b4479e4
diff --git a/lightning/src/ln/chanmon_update_fail_tests.rs b/lightning/src/ln/chanmon_update_fail_tests.rs
index b386f6fee..e1c95def4 100644
--- a/lightning/src/ln/chanmon_update_fail_tests.rs+++ b/lightning/src/ln/chanmon_update_fail_tests.rs@@ -3877,3 +3877,4 @@ fn test_single_channel_multiple_mpp() {
let node_cfgs = create_node_cfgs(9, &chanmon_cfgs);
- let node_chanmgrs = create_node_chanmgrs(9, &node_cfgs, &[None; 9]);+ let configs = [None, None, None, None, None, None, None, None, None];+ let node_chanmgrs = create_node_chanmgrs(9, &node_cfgs, &configs);
let mut nodes = create_network(9, &node_cfgs, &node_chanmgrs);

[G8XSU]

Lgtm!

[TheBlueMatt]

Backported in #3697