Ack monitor events immediately
What the developer wrote
Authored by Valentine Wallace
Ack monitor events immediately
Currently, the resolution of HTLCs (and decisions on when HTLCs can be
forwarded) is the responsibility of Channel objects (a part of ChannelManager)
until the channel is closed, and then the ChannelMonitor thereafter. This leads
to some complexity around race conditions for HTLCs right around channel
closure. Additionally, there is lots of complexity reconstructing the state of
all HTLCs in the ChannelManager deserialization/loading logic.
Instead, we want to do all resolution in ChannelMonitors (in response to
ChannelMonitorUpdates) and pass them back to ChannelManager in the form of
MonitorEvents (similar to how HTLCs are resolved after channels are closed). In
order to have reliable resolution, we'll need to keep MonitorEvents around in
the ChannelMonitor until the ChannelManager has finished processing them. This
will simplify things - on restart instead of examining the set of HTLCs in
monitors we can simply replay all the pending MonitorEvents.
Here for the purposes of merging initial support for persistent monitor events,
we ack each immediately after it is received/handled by the ChannelManager,
which is equivalent to the behavior we had prior to monitor events becoming
persistent.
In upcoming work, we'll want to have much more special handling for HTLCUpdate
monitor events in particular -- e.g. for outbound payment claim events, we
should only ACK the monitor event when the PaymentSent event is processed,
until that point we want it to keep being provided back to us on startup.
All the other monitor events are trivial to ACK, since they don't need to be
re-processed on startup.
What changed, and why it matters
This commit changes how Lightning Dev Kit handles internal notifications called 'monitor events' that report things like completed payments or forced channel closures. Previously these events were not formally acknowledged after being processed. Now the code immediately acknowledges each event after handling it. This is a groundwork change to allow these events to be stored persistently in the future, so they can be safely replayed after a restart. It is described by the authors as behaviorally equivalent to the old code for now, not a security fix.
Treat as a structural/refactoring commit rather than an active vulnerability patch. Reviewers should verify that ack_monitor_event is only called after all state dependent on the event has been durably persisted, especially for HTLCEvent, because premature acknowledgment could affect payment reliability on crash recovery. Monitor the follow-up work mentioned in the commit message that will delay HTLCEvent acknowledgment.
Security signals we found
New acknowledgment API introduced for monitor events
Immediate ACK of HTLCEvent differs from stated future intent (delayed ACK until PaymentSent processed)
Potential reliability concern: if ChannelManager crashes after processing but before persisting, an ACKed HTLCEvent may be lost, though current behavior is described as equivalent to prior non-persistent handling
No explicit security bug fixed in commit message
Evidence from the diff
The patch adds calls to chain_monitor.ack_monitor_event(…) for each MonitorEvent variant after ChannelManager processes it. It introduces a MonitorEventSource struct bundling event_id and channel_id to identify the event being acknowledged. The commit message frames this as preparatory work for persistent monitor events: by acknowledging immediately, the current non-persistent behavior is preserved while the plumbing for durable events is merged. HTLCEvent is acknowledged immediately here, though the message notes future work will delay that acknowledgment until the PaymentSent event is processed. Other event types (HolderForceClosed, CommitmentTxConfirmed, Completed) are acknowledged immediately because they do not need replay on startup.
Changed components
lightning/src/ln/channelmanager.rsChannelManager monitor event processing loopChainMonitor event acknowledgment interfaceInspect captured patch +11 / −1
### lightning/src/ln/channelmanager.rs
@@ -42,6 +42,7 @@ use crate::chain::chaininterface::{
BroadcasterInterface, ConfirmationTarget, FeeEstimator, LowerBoundedFeeEstimator,
TransactionType,
};
+use crate::chain::chainmonitor::MonitorEventSource;
use crate::chain::channelmonitor::{
ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateStep, MonitorEvent,
WithChannelMonitor, ANTI_REORG_DELAY, CLTV_CLAIM_BUFFER, HTLC_FAIL_BACK_BUFFER,
@@ -14470,7 +14471,8 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
for (funding_outpoint, channel_id, mut monitor_events, counterparty_node_id) in
pending_monitor_events.drain(..)
{
- for (_event_id, monitor_event) in monitor_events.drain(..) {
+ for (event_id, monitor_event) in monitor_events.drain(..) {
+ let monitor_event_source = MonitorEventSource { event_id, channel_id };
match monitor_event {
MonitorEvent::HTLCEvent(htlc_update) => {
needs_persist = true;
@@ -14521,6 +14523,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
completion_update,
);
}
+ self.chain_monitor.ack_monitor_event(monitor_event_source);
},
MonitorEvent::HolderForceClosed(_)
| MonitorEvent::HolderForceClosedWithInfo { .. } => {
@@ -14555,6 +14558,9 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
failed_channels.push((Err(e), counterparty_node_id));
}
}
+ // Channel close monitor events do not need to be replayed on startup because we
+ // already check the monitors to see if the channel is closed.
+ self.chain_monitor.ack_monitor_event(monitor_event_source);
},
MonitorEvent::CommitmentTxConfirmed(_) => {
needs_persist = true;
@@ -14577,13 +14583,17 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
failed_channels.push((Err(e), counterparty_node_id));
}
}
+ // Channel close monitor events do not need to be replayed on startup because we
+ // already check the monitors to see if the channel is closed.
+ self.chain_monitor.ack_monitor_event(monitor_event_source);
},
MonitorEvent::Completed { channel_id, monitor_update_id, .. } => {
needs_persist |= self.channel_monitor_updated(
&channel_id,
Some(monitor_update_id),
&counterparty_node_id,
);
+ self.chain_monitor.ack_monitor_event(monitor_event_source);
},
}
}Why this scored 30/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.