Check in-flight monitor updates in force_shutdown for LocalAnnounced HTLCs by joostjager · Pull Request #4434 · lightningdevkit/rust-lightning

joostjager · 2026-02-23T09:45:12Z

ldk-reviews-bot · 2026-02-23T09:45:15Z

👋 Thanks for assigning @TheBlueMatt 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-23T12:51:54Z

Codecov Report

❌ Patch coverage is 93.54839% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.97%. Comparing base (ec03159) to head (da66b8e).

Files with missing lines	Patch %	Lines
lightning/src/chain/channelmonitor.rs	83.33%	0 Missing and 1 partial ⚠️
lightning/src/ln/channel.rs	94.44%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4434      +/-   ##
==========================================
+ Coverage   85.93%   85.97%   +0.03%     
==========================================
  Files         159      159              
  Lines      104693   104712      +19     
  Branches   104693   104712      +19     
==========================================
+ Hits        89972    90022      +50     
+ Misses      12213    12187      -26     
+ Partials     2508     2503       -5

Flag	Coverage Δ
tests	`85.97% <93.54%> (+0.03%)`	⬆️

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.

wpaulino · 2026-02-23T20:02:37Z

lightning/src/ln/channelmanager.rs

-				chan.force_shutdown(reason)
+				let in_flight = in_flight_monitor_updates
+					.get(&chan_id)
+					.map(|(_, updates)| updates.iter().collect::<Vec<_>>())


Do we really need this allocation? Can we just pass the iterator to force_shutdown instead?

Good one, fixed

ldk-reviews-bot · 2026-02-23T20:02:42Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

TheBlueMatt

Hmmmmmmmmm, its really not quite so simple. If a monitor update is InProgress we generally expect the in-memory ChannelMonitor to have actually seen the new state update (though we expect that on restart there's a good chance it won't have that information and it'll be replayed). That means the monitor is theoretically "in charge" of resolving the HTLCs in question. While I think this patch is correct, I'm a bit hesitant to end up with two things in charge of resolving a specific HTLC. ISTM it also wouldn't be so complicated to immediately mark HTLCs as failed in the monitor upon receiving updates that try to add new HTLCs after a channel has been closed.

joostjager · 2026-02-24T07:32:55Z

ISTM it also wouldn't be so complicated to immediately mark HTLCs as failed in the monitor upon receiving updates that try to add new HTLCs after a channel has been closed.

Isn't the problem with this that the monitor can't see whether the commitment was already sent to the peer or blocked by async persistence?

TheBlueMatt · 2026-02-24T12:55:21Z

Right, the monitor has to assume it may have been sent to the peer. That's fine, though, it should be able to fail the HTLC(s) once the commitment transaction is locked.

joostjager · 2026-02-24T15:20:57Z

Hmm, it seems that the original issue isn't an issue then. I confirmed that mining the force close commitment tx indeed generates the payment failed event. Or is there still something missing?

TheBlueMatt · 2026-02-24T16:22:50Z

In the general case (ChannelMonitor in-memory in the same machine that runs ChannelManager) I believe there is not a runtime issue, no. I was thinking you'd identified an issue in the crash case, though (where we crash without persisting the monitor update and on restart lose it) but actually I'm not sure this is any different from existing payments logic - if you start a payment and nothing persists on restart its expected to be gone and downstream code has to handle that.

joostjager · 2026-02-24T16:38:20Z

The one that I identified wasn't a crash case. An assertion in the fuzzer identified that a payment was lost, but with current understanding I think it's just that the fuzzer didn't push the force-close forward enough (mine). Will close it for now and can look again if the payment remains lost even with confirmation of the commit tx. Unit test did confirm that it should just work.

joostjager · 2026-03-02T08:07:16Z

Reopening as this fix is now required for #4351.

joostjager · 2026-03-02T08:10:56Z

Hmmmmmmmmm, its really not quite so simple. If a monitor update is InProgress we generally expect the in-memory ChannelMonitor to have actually seen the new state update (though we expect that on restart there's a good chance it won't have that information and it'll be replayed). That means the monitor is theoretically "in charge" of resolving the HTLCs in question. While I think this patch is correct, I'm a bit hesitant to end up with two things in charge of resolving a specific HTLC. ISTM it also wouldn't be so complicated to immediately mark HTLCs as failed in the monitor upon receiving updates that try to add new HTLCs after a channel has been closed.

Now that we've decided that for deferred writes we no longer want to expect the in-memory ChannelMonitor to have seen the changes, I think the above doesn't apply anymore, and we might have to accept that two things are in charge for resolving a specific HTLC.

joostjager · 2026-03-02T08:13:29Z

lightning/src/ln/channel.rs

 			if let OutboundHTLCState::LocalAnnounced(_) = htlc.state {
-				for update in self.blocked_monitor_updates.iter() {
-					for update in update.update.updates.iter() {
+				let blocked_iter = self.blocked_monitor_updates.iter().map(|u| &u.update);


I do wonder if it is also possible for outbound htlcs to have already progressed to the Committed or later states? In that case, we might also want to catch that here.

joostjager · 2026-03-02T08:22:35Z

lightning/src/chain/channelmonitor.rs

+			events.pop().unwrap(),
+			Event::ChannelClosed { reason: ClosureReason::CommitmentTxConfirmed, .. }
+		));
+		expect_payment_failed_conditions_event(


This now no longer needs to wait for the force close to confirm.

…HTLCs When building dropped_outbound_htlcs during force_shutdown, LocalAnnounced HTLCs were only checked against blocked_monitor_updates. This relied on the implicit contract that in-progress monitor updates have always been applied to the in-memory ChannelMonitor already. If InProgress is returned without the monitor knowing about the update yet, the HTLC is silently dropped with no PaymentPathFailed or PaymentFailed event generated. Pass the in-flight monitor updates for the channel into force_shutdown so both blocked and in-flight updates are checked. This ensures LocalAnnounced HTLCs whose monitor updates have not yet been applied are properly included in dropped_outbound_htlcs and failed back to the sender. Closes lightningdevkit#4431 AI tools were used in preparing this commit.

TheBlueMatt

Now that we've decided that for deferred writes we no longer want to expect the in-memory ChannelMonitor to have seen the changes, I think the above doesn't apply anymore, and we might have to accept that two things are in charge for resolving a specific HTLC.

I don't think so. Instead, what we'll need to do is detect a new local counterparty state ChannelMontiorUpdate after the channel has close and track it to fail the HTLCs from it once the commitment transaction has 6 confs.

TheBlueMatt · 2026-03-02T16:15:38Z

Oh actually no I don't think we need to wait for 6 confs since we never sent the message, ie we can fail it immediately.

joostjager force-pushed the fix-force-close-in-progress-monitor-drops-htlc branch from 2d36c03 to cce8ccb Compare February 23, 2026 10:08

joostjager marked this pull request as ready for review February 23, 2026 13:49

joostjager requested a review from wpaulino February 23, 2026 17:34

wpaulino reviewed Feb 23, 2026

View reviewed changes

wpaulino requested a review from TheBlueMatt February 23, 2026 20:03

joostjager force-pushed the fix-force-close-in-progress-monitor-drops-htlc branch from cce8ccb to ad3036e Compare February 23, 2026 21:11

TheBlueMatt reviewed Feb 23, 2026

View reviewed changes

joostjager closed this Feb 24, 2026

This was referenced Feb 24, 2026

Payment silently stuck when channel force-closed during in-progress monitor update #4431

Closed

Defer ChainMonitor updates and persistence to flush() #4351

Open

joostjager reopened this Mar 2, 2026

joostjager commented Mar 2, 2026

View reviewed changes

joostjager requested a review from TheBlueMatt March 2, 2026 08:13

joostjager force-pushed the fix-force-close-in-progress-monitor-drops-htlc branch 2 times, most recently from 6421a28 to 8dac2fa Compare March 2, 2026 08:20

joostjager commented Mar 2, 2026

View reviewed changes

joostjager force-pushed the fix-force-close-in-progress-monitor-drops-htlc branch from 8dac2fa to da66b8e Compare March 2, 2026 08:23

TheBlueMatt reviewed Mar 2, 2026

View reviewed changes

Conversation

joostjager commented Feb 23, 2026

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

wpaulino Feb 23, 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!

joostjager Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Feb 23, 2026

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

joostjager commented Feb 24, 2026

Uh oh!

TheBlueMatt commented Feb 24, 2026

Uh oh!

joostjager commented Feb 24, 2026

Uh oh!

TheBlueMatt commented Feb 24, 2026

Uh oh!

joostjager commented Feb 24, 2026

Uh oh!

joostjager commented Mar 2, 2026

Uh oh!

joostjager commented Mar 2, 2026

Uh oh!

joostjager Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

joostjager Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Mar 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ldk-reviews-bot commented Feb 23, 2026 •

edited

Loading

codecov bot commented Feb 23, 2026 •

edited

Loading

wpaulino Feb 23, 2026 •

edited

Loading