Hold back HTLC monitor events while updates are in-flight
What the developer wrote
Authored by Valentine Wallace
Hold back HTLC monitor events while updates are in-flight
A ChannelMonitor applies a ChannelMonitorUpdate in-memory before the update
has been durably persisted, and may queue a MonitorEvent::HTLCEvent based on
the update's contents.
Acting on such an event before the update which generated it is persisted will
become unsafe when we begin generating monitor events for off-chain HTLC
failures: if we crash and lose the update that triggered the monitor event, we
lose the counterparty's revocation of the state containing the HTLC, yet we may
have already told the user that the payment failed.
Thus, have the ChainMonitor hold back a monitor's HTLC events while it has any
in-flight updates, and document the same requirement on
Watch::release_pending_monitor_events. Other monitor events, e.g. channel
closures, are still released immediately.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
What changed, and why it matters
This commit fixes a crash-recovery safety issue in the Lightning Dev Kit's channel monitor. Before the fix, the code could report a failed payment to the user based on a state change that had not yet been saved to disk. If the program crashed right after reporting the failure, it might restart without the counterparty's revocation that justified the failure, potentially leading to inconsistent payment state. The fix delays payment-failure notifications until the underlying state change has been durably saved.
Review the patch for completeness: ensure all callers of release_pending_monitor_events honor the new HTLC-event withholding requirement, and verify that pending HTLC failure events are released once the corresponding ChannelMonitorUpdate is acknowledged as persisted. Consider adding tests that simulate a crash between event surfacing and persistence to confirm the fix.
Security signals we found
Crash-recovery consistency bug in payment failure handling
In-memory state applied before durable persistence could lead to lost revocation
New filtering API to withhold HTLC failure events until persistence completes
Trait documentation updated to impose ordering requirement on implementers
Test helper force_channel_monitor_updated now cleans pending update list
Evidence from the diff
The patch changes ChainMonitor to suppress MonitorEvent::HTLCEvent events for HTLC failures while any ChannelMonitorUpdate for that monitor is still in-flight (not yet durably persisted). ChannelMonitor now exposes get_and_clear_pending_non_htlc_fail_events, which filters out HTLC failure events (payment_preimage.is_none()) while retaining them in pending_monitor_events. The Watch::release_pending_monitor_events trait documentation is updated to require this behavior. Other monitor events, such as channel closures, are still released immediately. The change is preparatory for future off-chain HTLC failure event generation.
Changed components
lightning/src/chain/chainmonitor.rslightning/src/chain/channelmonitor.rslightning/src/chain/mod.rsInspect captured patch +48 / −8
### lightning/src/chain/chainmonitor.rs
@@ -853,7 +853,10 @@ where
#[cfg(any(test, fuzzing))]
pub fn force_channel_monitor_updated(&self, channel_id: ChannelId, monitor_update_id: u64) {
let monitors = self.monitors.read().unwrap();
- let monitor = &monitors.get(&channel_id).unwrap().monitor;
+ let monitor_state = monitors.get(&channel_id).unwrap();
+ let monitor = &monitor_state.monitor;
+ let mut pending_monitor_updates = monitor_state.pending_monitor_updates.lock().unwrap();
+ pending_monitor_updates.retain(|update_id| *update_id > monitor_update_id);
self.push_update_completed_event(
monitor.get_funding_txo(),
channel_id,
@@ -1684,7 +1687,19 @@ where
let monitors = self.monitors.read().unwrap();
let mut pending_monitor_events = Vec::new();
for monitor_state in monitors.values() {
- let monitor_events = monitor_state.monitor.get_and_clear_pending_monitor_events();
+ // Hold back HTLC-failed monitor events for channels with in-flight updates. The monitor may
+ // have queued an event based on in-memory state from an as-yet-unpersisted update; surfacing
+ // it before persistence would let us fail upstream based on state that could be lost on a
+ // crash + reconnect. Other monitor events (e.g., channel close) aren't subject to those
+ // restrictions and can be released immediately.
+ let monitor_events = {
+ let pending_updates = monitor_state.pending_monitor_updates.lock().unwrap();
+ if monitor_state.has_pending_updates(&pending_updates) {
+ monitor_state.monitor.get_and_clear_pending_non_htlc_fail_events()
+ } else {
+ monitor_state.monitor.get_and_clear_pending_monitor_events()
+ }
+ };
if monitor_events.len() > 0 {
let monitor_funding_txo = monitor_state.monitor.get_funding_txo();
let monitor_channel_id = monitor_state.monitor.channel_id();
### lightning/src/chain/channelmonitor.rs
@@ -2232,7 +2232,15 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
/// Returned events are retained internally until [Self::ack_monitor_event] is called with their
/// ID.
pub fn get_and_clear_pending_monitor_events(&self) -> Vec<(u128, MonitorEvent)> {
- self.inner.lock().unwrap().get_and_clear_pending_monitor_events()
+ self.inner.lock().unwrap().get_and_clear_pending_monitor_events_filtered(|_| true)
+ }
+
+ /// Returns only non-HTLC-failure monitor events, retaining HTLC failures until monitor
+ /// updates have been durably persisted.
+ pub(super) fn get_and_clear_pending_non_htlc_fail_events(&self) -> Vec<(u128, MonitorEvent)> {
+ self.inner.lock().unwrap().get_and_clear_pending_monitor_events_filtered(
+ |ev| !matches!(ev, MonitorEvent::HTLCEvent(upd) if upd.payment_preimage.is_none()),
+ )
}
/// Removes a [`MonitorEvent`] by its event ID, acknowledging that it has been processed.
@@ -4754,11 +4762,25 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
self.pending_monitor_events.retain(|(id, _)| *id != event_id);
}
- fn get_and_clear_pending_monitor_events(&mut self) -> Vec<(u128, MonitorEvent)> {
- let mut ret = Vec::new();
- mem::swap(&mut ret, &mut self.pending_monitor_events);
- self.provided_monitor_events.extend(ret.iter().cloned());
- ret
+ /// Drains and returns the pending monitor events for which `predicate` returns true. Events that
+ /// don't match the predicate stay in `pending_monitor_events` so they're eligible for release on
+ /// a later call.
+ fn get_and_clear_pending_monitor_events_filtered<F: FnMut(&MonitorEvent) -> bool>(
+ &mut self, mut predicate: F,
+ ) -> Vec<(u128, MonitorEvent)> {
+ let mut released = Vec::new();
+ let mut retained = Vec::new();
+ // Note: we can use Vec::extract_if here once MSRV reaches 1.87
+ for entry in self.pending_monitor_events.drain(..) {
+ if predicate(&entry.1) {
+ released.push(entry);
+ } else {
+ retained.push(entry);
+ }
+ }
+ self.pending_monitor_events = retained;
+ self.provided_monitor_events.extend(released.iter().cloned());
+ released
}
/// Gets the set of events that are repeated regularly (e.g. those which RBF bump
### lightning/src/chain/mod.rs
@@ -430,6 +430,9 @@ pub trait Watch<ChannelSigner: EcdsaChannelSigner> {
/// further events may be returned here until the [`ChannelMonitor`] has been fully persisted
/// to disk.
///
+ /// No [`MonitorEvent::HTLCEvent`]s for failing HTLCs may be returned for a [`ChannelMonitor`]
+ /// while any [`ChannelMonitorUpdate`] applied to it has not yet been fully persisted to disk.
+ ///
/// For details on asynchronous [`ChannelMonitor`] updating and returning
/// [`MonitorEvent::Completed`] here, see [`ChannelMonitorUpdateStatus::InProgress`].
fn release_pending_monitor_events(Why this scored 57/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.