Defer ChainMonitor updates and persistence to flush (wrapper approach)

[joostjager]

Summary

Introduce DeferredChainMonitor, a wrapper around ChainMonitor that queues watch_channel and update_channel operations, returning InProgress until flush() is called. This enables batched persistence of monitor updates after ChannelManager persistence, ensuring correct ordering where the ChannelManager state is never ahead of the monitor state on restart.

The Problem

There's a race condition that can cause channel force closures: if the node crashes after writing channel monitors but before writing the channel manager, the monitors will be ahead of the manager on restart. This can lead to state desync and force closures.

The Solution

By deferring monitor writes until after the channel manager is persisted (via flush()), we ensure the manager is always at least as up-to-date as the monitors.

Key changes:

• DeferredChainMonitor queues monitor operations and returns InProgress
• Calling flush() applies pending operations and persists monitors
• All ChainMonitor traits (Listen, Confirm, EventsProvider, etc.) are passed through, allowing drop-in replacement
• Background processor updated to capture pending count before ChannelManager persistence, then flush after persistence completes

Performance Impact

Multi-channel, multi-node load testing (using ldk-server chaos branch) shows no measurable throughput difference between deferred and direct persistence modes.

This is likely because forwarding and payment processing are already effectively single-threaded: the background processor batches all forwards for the entire node in a single pass, so the deferral overhead doesn't add any meaningful bottleneck to an already serialized path.

For high-latency storage (e.g., remote databases), there is also currently no significant impact because channel manager persistence already blocks event handling in the background processor loop (test). If the loop were parallelized to process events concurrently with persistence, deferred writing would become comparatively slower since it moves the channel manager round trip into the critical path. However, deferred writing would also benefit from loop parallelization, and could be further optimized by batching the monitor and manager writes into a single round trip.

Alternative Designs Considered

Several approaches were explored to solve the monitor/manager persistence ordering problem:

1. Queue at KVStore level (#4310)

Introduces a QueuedKVStoreSync wrapper that queues all writes in memory, committing them in a single batch at chokepoints where data leaves the system (get_and_clear_pending_msg_events, get_and_clear_pending_events). This approach aims for true atomic multi-key writes but requires KVStore backends that support transactions (e.g., SQLite) - filesystem backends cannot achieve full atomicity.

Trade-offs: Most general solution but requires changes to persistence boundaries and cannot fully close the desync gap with filesystem storage.

2. Queue at Persister level (#4317)

Updates MonitorUpdatingPersister to queue persist operations in memory, with actual writes happening on flush(). Adds flush() to the Persist trait and ChainMonitor.

Trade-offs: Only fixes the issue for MonitorUpdatingPersister; custom Persist implementations remain vulnerable to the race condition.

3. Queue internally in ChainMonitor (#4351)

Modifies ChainMonitor directly to queue operations internally, returning InProgress until flush() is called.

Trade-offs: Requires an enormous amount of test changes since existing tests expect immediate persistence behavior.

[ldk-reviews-bot]

👋 Hi! I see this is a draft PR.
I'll wait to assign reviewers until you mark it as ready for review.
Just convert it out of draft status when you're ready for review!

[joostjager]

Added a DeferredChainMonitor wrapper instead of modifying ChainMonitor directly. The wrapper intercepts watch_channel and update_channel calls, queues them, and returns InProgress. When flush is called, it processes the queued operations and persists them in the correct order after ChannelManager persistence. This approach keeps ChainMonitor unchanged so that existing tests which expect synchronous behavior continue to work without modification. Only the background processor and production code paths use the deferred wrapper while the test suite can keep using ChainMonitor directly.

[joostjager]

Initially attempted to implement this as a thin adapter/wrapper that would sit between the ChannelManager and an existing ChainMonitor, forwarding calls while deferring the Watch operations. However, when integrating with ldk-node, this approach quickly ran into Rust ownership and lifetime issues since it required keeping both the original ChainMonitor and the wrapper around simultaneously. The current implementation takes a simpler approach where DeferredChainMonitor owns its own ChainMonitor internally and implements Deref to it, making it a complete drop-in replacement that can be instantiated with the same parameters as ChainMonitor while exposing all the same traits and methods.

[codecov]

Codecov Report

❌ Patch coverage is 86.73780% with 87 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.01%. Comparing base (4800a47) to head (bc5bfac).
⚠️ Report is 48 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/chain/deferred.rs	88.88%	60 Missing and 7 partials ⚠️
lightning-background-processor/src/lib.rs	68.75%	11 Missing and 4 partials ⚠️
lightning/src/chain/chainmonitor.rs	0.00%	5 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4345 +/- ##
========================================
Coverage 86.01% 86.01% ========================================
Files 156 157 +1 Lines 102781 103751 +970 Branches 102781 103751 +970 ========================================
+ Hits 88409 89245 +836 - Misses 11864 11976 +112 - Partials 2508 2530 +22 

Flag	Coverage Δ
tests	86.01% <86.73%> (+<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.

[joostjager]

Had to include a test that sets up from scratch, because the test infra is built around ChainMonitor. I am not sure if we should want something like this, or just let it be covered in ldk-node.

[joostjager]

This error was previously reported back to chan mgr, but now not anymore.

[joostjager]

Extended PR description with outcome of high-latency benchmark

[joostjager]

Rebased on top of the AChainMonitor trait change, so that deferred writing is optional in the background processor.

[joostjager]

Not happy about exposing this

[joostjager]

Not happy about exposing this either

[joostjager]

Not overly happy with the wrapper approach. It reintroduces type complexity (seven generic parameters, manual forwarding of traits), and inner()/get_cm() still leak the unwrapped ChainMonitor so perhaps fewer guarantees operations actually go through the deferred path. Also had to add a pub(crate) trait in production code purely for test mockability.

On testing, we should decide whether this gets full coverage within LDK or is integration-tested at the ldk-node level. Right now it's halfway both, with manual flush() calls sprinkled in places.

It does make me reconsider #4351 (queuing internally in ChainMonitor) with a runtime flag. This might also make testing easier, because it's the same ChainMonitor.

[TheBlueMatt]

Not overly happy with the wrapper approach. It reintroduces type complexity (seven generic parameters, manual forwarding of traits), and inner()/get_cm() still leak the unwrapped ChainMonitor so perhaps fewer guarantees operations actually go through the deferred path. Also had to add a pub(crate) trait in production code purely for test mockability.
..
It does make me reconsider #4351 (queuing internally in ChainMonitor) with a runtime flag. This might also make testing easier, because it's the same ChainMonitor.

Yea, I don't disagree I think? I mean its fine but the fact that the underlying leaks allowing misuse is definitely not ideal at all.

On testing, we should decide whether this gets full coverage within LDK or is integration-tested at the ldk-node level. Right now it's halfway both, with manual flush() calls sprinkled in places.

Given from the ChannelManager's PoV its "just" async persist, I don't think we need to worry about every functional test using the new logic or anything like that, but we should definitely have some test coverage where possible.

[TheBlueMatt]

IMO we should move this above the CM write. A common cause of FCs is actually buggy shutdown logic where the node is still running while the BP is being shut down, causing monitor writes after the CM is written in the exit write above.

[joostjager]

Done in #4351

[joostjager]

Moving to #4351 (queuing internally in ChainMonitor with a runtime flag). The main motivations:

1. The wrapper leaks the unwrapped ChainMonitor through inner()/get_cm(), so there's no compile-time guarantee that operations go through the deferred path. With internal queuing, there's only one ChainMonitor and no way to bypass it.

2. The wrapper requires seven generic parameters, manual trait forwarding for Listen/Confirm/EventsProvider/etc, and a pub(crate) trait added purely for test mockability. Internal queuing avoids all of that since it's the same ChainMonitor type regardless of mode.

3. Being inside ChainMonitor allows better coordination with internal locking and early validation. For example, watch_channel in deferred mode can check that the channel doesn't already exist in monitors or the pending queue, erroring immediately rather than deferring the failure to flush time.

4. Testing becomes simpler. Since it's the same ChainMonitor, existing test infrastructure works directly with a deferred flag. No need for a separate wrapper type in test utilities.

[joostjager]

Closing in favor of #4351