Return NotifyOption from process_pending_monitor_events
What changed, and why it matters
This commit is a small internal code cleanup in the Lightning Dev Kit (LDK) Rust library. It changes a helper function so it returns a richer status code instead of a simple true/false, letting callers decide more precisely whether to save state and whether to keep processing events. There is no direct evidence in the commit that this fixes a security vulnerability; it appears to be a correctness and maintainability improvement.
Treat as a normal code-quality refactor. Reviewers may want to verify that the new `NotifyOption` merging logic in `process_events_body` preserves the intended persistence behavior, especially the transition from `SkipPersistNoEvents` to `SkipPersistHandleEvents`. No immediate security response is indicated by the available materials.
Security signals we found
Refactor of event-processing/persistence notification logic
Changes when persistence notifier is triggered after monitor events
No explicit security bug, CVE, or vulnerability described in commit message or diff
Evidence from the diff
The patch refactors process_pending_monitor_events in lightning/src/ln/channelmanager.rs to return a NotifyOption enum (DoPersist, SkipPersistHandleEvents, SkipPersistNoEvents) rather than a bool. Call sites in process_events_body and get_and_clear_pending_msg_events are updated to consume the new return value. The change lets the event loop distinguish ‘no monitor events’ from ‘events handled but no persistence needed’ and avoids forcing a persistence notification when only monitor events were processed. The diff shows no new input validation, no bounds checks, no cryptographic changes, and no explicit bug or vulnerability disclosure.
Changed components
lightning/src/ln/channelmanager.rsprocess_pending_monitor_eventsprocess_events_body macroget_and_clear_pending_msg_eventsPersistenceNotifierGuardInspect captured patch +14 / −11
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 1174ccf..6d8dbe6 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -3512,8 +3512,12 @@ 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,
+ _ => {},
}
}
@@ -13732,13 +13736,16 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
Ok(post_update_data)
}
- /// 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 the appropriate
+ /// [`NotifyOption`] for persistence and event handling.
+ 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();
+ if pending_monitor_events.is_empty() {
+ return NotifyOption::SkipPersistNoEvents;
+ }
for (funding_outpoint, channel_id, mut monitor_events, counterparty_node_id) in
pending_monitor_events.drain(..)
{
@@ -13862,7 +13869,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
+ NotifyOption::DoPersist
}
fn handle_holding_cell_free_result(&self, result: FreeHoldingCellsResult) {
@@ -16001,8 +16008,6 @@ impl<
fn get_and_clear_pending_msg_events(&self) -> Vec<MessageSendEvent> {
let events = RefCell::new(Vec::new());
PersistenceNotifierGuard::optionally_notify(self, || {
- let mut result = NotifyOption::SkipPersistNoEvents;
-
// This method is quite performance-sensitive. Not only is it called very often, but it
// *is* the critical path between generating a message for a peer and giving it to the
// `PeerManager` to send. Thus, we should avoid adding any more logic here than we
@@ -16011,9 +16016,7 @@ 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;
- }
+ let mut result = self.process_pending_monitor_events();
if self.maybe_generate_initial_closing_signed() {
result = NotifyOption::DoPersist;
Why this scored 23/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.