Skip to content

Skip ChannelManager persistence after monitor update completion #4424

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
joostjager wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Skip ChannelManager persistence after monitor update completion #4424

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4417881
Skip ChannelManager persistence after monitor update completion
joostjager Feb 18, 2026
c891c48
Set needs_persist_flag on every ChannelMonitorUpdate
joostjager Feb 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 35 additions & 8 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3317,8 +3317,11 @@ macro_rules! process_events_body {

// TODO: This behavior should be documented. It's unintuitive that we query
// ChannelMonitors when clearing other events.
if $self.process_pending_monitor_events() {
result = NotifyOption::DoPersist;
match $self.process_pending_monitor_events() {
NotifyOption::DoPersist => result = NotifyOption::DoPersist,
NotifyOption::SkipPersistHandleEvents if result == NotifyOption::SkipPersistNoEvents =>
result = NotifyOption::SkipPersistHandleEvents,
_ => {},
}
}

Expand Down Expand Up @@ -9930,6 +9933,9 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
if self.background_events_processed_since_startup.load(Ordering::Acquire) {
let update_res =
self.chain_monitor.update_channel(channel_id, &in_flight_updates[update_idx]);
// Ensure the ChannelManager is persisted so that `in_flight_monitor_updates`
// is in sync with the updates applied to the chain monitor.
self.needs_persist_flag.store(true, Ordering::Release);
let logger =
WithContext::from(&self.logger, Some(counterparty_node_id), Some(channel_id), None);
let update_completed = self.handle_monitor_update_res(update_res, logger);
Expand Down Expand Up @@ -12839,19 +12845,21 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
Ok(())
}

/// Process pending events from the [`chain::Watch`], returning whether any events were processed.
fn process_pending_monitor_events(&self) -> bool {
/// Process pending events from the [`chain::Watch`], returning a [`NotifyOption`] indicating
/// whether persistence is needed.
fn process_pending_monitor_events(&self) -> NotifyOption {
debug_assert!(self.total_consistency_lock.try_write().is_err()); // Caller holds read lock

let mut failed_channels: Vec<(Result<Infallible, _>, _)> = Vec::new();
let mut pending_monitor_events = self.chain_monitor.release_pending_monitor_events();
let has_pending_monitor_events = !pending_monitor_events.is_empty();
let mut result = NotifyOption::SkipPersistNoEvents;
for (funding_outpoint, channel_id, mut monitor_events, counterparty_node_id) in
pending_monitor_events.drain(..)
{
for monitor_event in monitor_events.drain(..) {
match monitor_event {
MonitorEvent::HTLCEvent(htlc_update) => {
result = NotifyOption::DoPersist;
let logger = WithContext::from(
&self.logger,
Some(counterparty_node_id),
Expand Down Expand Up @@ -12904,6 +12912,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
},
MonitorEvent::HolderForceClosed(_)
| MonitorEvent::HolderForceClosedWithInfo { .. } => {
result = NotifyOption::DoPersist;
let per_peer_state = self.per_peer_state.read().unwrap();
if let Some(peer_state_mutex) = per_peer_state.get(&counterparty_node_id) {
let mut peer_state_lock = peer_state_mutex.lock().unwrap();
Expand Down Expand Up @@ -12936,6 +12945,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
}
},
MonitorEvent::CommitmentTxConfirmed(_) => {
result = NotifyOption::DoPersist;
let per_peer_state = self.per_peer_state.read().unwrap();
if let Some(peer_state_mutex) = per_peer_state.get(&counterparty_node_id) {
let mut peer_state_lock = peer_state_mutex.lock().unwrap();
Expand All @@ -12957,6 +12967,17 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
}
},
MonitorEvent::Completed { channel_id, monitor_update_id, .. } => {
// Completed events don't require ChannelManager persistence.
// `channel_monitor_updated` clears transient flags and may
// trigger actions like forwarding HTLCs or finalizing claims,
// but all of these are re-derived on restart: the serialized
// `in_flight_monitor_updates` will still contain the completed
// entries, and deserialization will detect that the monitor is
// ahead, synthesizing a `MonitorUpdatesComplete` background
// event that re-drives `channel_monitor_updated`.
if result == NotifyOption::SkipPersistNoEvents {
result = NotifyOption::SkipPersistHandleEvents;
}
self.channel_monitor_updated(
Copy link
Collaborator

@TheBlueMatt TheBlueMatt Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need to carefully review everything this calls to make sure they set the store-required flag. Might be easier to just manually set the flag if there's anything to do in handle_post_monitor_update_chan_resume?

Copy link
Contributor Author

@joostjager joostjager Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By "anything to do in handle_post_monitor_update_chan_resume", do you mean specifically a new monitor update being created? Or is there more that isn't recoverable on restart?

Added a commit that sets needs_persist_flag in handle_new_monitor_update_locked_actions_handled_by_caller right after chain_monitor.update_channel to cover that case.

Have to say that certainty level instantly drops when working on this state machine 😬

&channel_id,
Some(monitor_update_id),
Expand All @@ -12971,7 +12992,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
let _ = self.handle_error(err, counterparty_node_id);
}

has_pending_monitor_events
result
}

fn handle_holding_cell_free_result(&self, result: FreeHoldingCellsResult) {
Expand Down Expand Up @@ -15005,8 +15026,14 @@ impl<

// TODO: This behavior should be documented. It's unintuitive that we query
// ChannelMonitors when clearing other events.
if self.process_pending_monitor_events() {
result = NotifyOption::DoPersist;
match self.process_pending_monitor_events() {
NotifyOption::DoPersist => result = NotifyOption::DoPersist,
NotifyOption::SkipPersistHandleEvents
if result == NotifyOption::SkipPersistNoEvents =>
{
result = NotifyOption::SkipPersistHandleEvents
},
_ => {},
}

if self.maybe_generate_initial_closing_signed() {
Expand Down
Loading