Defer monitor update completions after funding spend
What changed, and why it matters
This commit fixes an internal bug in the Lightning Dev Kit's chain monitoring code. When a channel is being force-closed (because the funding transaction was spent), the code could previously get stuck: it would report a monitor update as 'in progress' forever, blocking legitimate payment completion events like 'PaymentClaimed' or 'PaymentForwarded'. In some cases, this could also break the ordering rules for monitor updates. The fix defers the completion signal until after the force-close events are processed, allowing payments to complete normally and preventing the ordering violation. There is no direct evidence this was exploitable by an attacker; it appears to be a correctness/robustness fix.
Review and merge if not already merged. Run the updated regression tests and monitor for any downstream effects on event ordering in ChannelManager. Consider whether any other completion-action paths could be similarly deferred.
Security signals we found
Fixes invariant violation in monitor update completion ordering
Prevents indefinite blocking of payment completion actions (PaymentClaimed, PaymentForwarded)
Changes a #[should_panic] test to a passing test, indicating previously reachable panic/assertion is now avoided
Touches force-close and funding-spend handling paths in ChainMonitor
Adds debug_assert enforcing that update_monitor errors only occur post-close
Evidence from the diff
In ChainMonitor::update_channel, when a ChannelMonitor is post-close (no_further_updates_allowed() is true) and the persister returns Completed, the code now returns InProgress to ChannelManager and queues a MonitorEvent::Completed in pending_monitor_events. In release_pending_monitor_events, per-monitor force-close events are emitted before draining the deferred completions. This resolves two issues: (1) phantom InProgress updates that never completed when update_monitor returned Err after funding_spend_seen, blocking MonitorUpdateCompletionActions indefinitely; and (2) a subsequent post-close update returning Completed violating the invariant that prior InProgress updates must complete first. The test test_monitor_update_fail_after_funding_spend is renamed and changed from #[should_panic] to a passing test verifying PaymentClaimed eventually fires.
Changed components
lightning/src/chain/chainmonitor.rslightning/src/ln/chanmon_update_fail_tests.rsInspect captured patch +144 / −45
diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs
index 396ee27..07d835d 100644
--- a/lightning/src/chain/chainmonitor.rs
+++ b/lightning/src/chain/chainmonitor.rs
@@ -1250,7 +1250,41 @@ where
}
}
- if update_res.is_err() {
+ debug_assert!(
+ update_res.is_ok() || monitor.no_further_updates_allowed(),
+ "update_monitor returned Err but channel is not post-close",
+ );
+
+ // We also check update_res.is_err() as a defensive measure: an
+ // error should only occur on a post-close monitor (validated by
+ // the debug_assert above), but we defer here regardless to avoid
+ // returning Completed for a failed update.
+ if (update_res.is_err() || monitor.no_further_updates_allowed())
+ && persist_res == ChannelMonitorUpdateStatus::Completed
+ {
+ // The channel is post-close (funding spend seen, lockdown, or
+ // holder tx signed). Return InProgress so ChannelManager freezes
+ // the channel until the force-close MonitorEvents are processed.
+ // Push a Completed event into pending_monitor_events so it gets
+ // picked up after the per-monitor events in the next
+ // release_pending_monitor_events call.
+ let funding_txo = monitor.get_funding_txo();
+ let channel_id = monitor.channel_id();
+ self.pending_monitor_events.lock().unwrap().push((
+ funding_txo,
+ channel_id,
+ vec![MonitorEvent::Completed {
+ funding_txo,
+ channel_id,
+ monitor_update_id: monitor.get_latest_update_id(),
+ }],
+ monitor.get_counterparty_node_id(),
+ ));
+ log_debug!(
+ logger,
+ "Deferring completion of ChannelMonitorUpdate id {:?} (channel is post-close)",
+ update_id,
+ );
ChannelMonitorUpdateStatus::InProgress
} else {
persist_res
@@ -1614,8 +1648,9 @@ where
for (channel_id, update_id) in self.persister.get_and_clear_completed_updates() {
let _ = self.channel_monitor_updated(channel_id, update_id);
}
- let mut pending_monitor_events = self.pending_monitor_events.lock().unwrap().split_off(0);
- for monitor_state in self.monitors.read().unwrap().values() {
+ 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();
if monitor_events.len() > 0 {
let monitor_funding_txo = monitor_state.monitor.get_funding_txo();
@@ -1629,6 +1664,10 @@ where
));
}
}
+ // Drain pending_monitor_events (which includes deferred post-close
+ // completions) after per-monitor events so that force-close
+ // MonitorEvents are processed by ChannelManager first.
+ pending_monitor_events.extend(self.pending_monitor_events.lock().unwrap().split_off(0));
pending_monitor_events
}
}
diff --git a/lightning/src/ln/chanmon_update_fail_tests.rs b/lightning/src/ln/chanmon_update_fail_tests.rs
index 87856c9..0d8a4a0 100644
--- a/lightning/src/ln/chanmon_update_fail_tests.rs
+++ b/lightning/src/ln/chanmon_update_fail_tests.rs
@@ -3905,11 +3905,28 @@ fn do_test_durable_preimages_on_closed_channel(
}
if !close_chans_before_reload {
check_closed_broadcast(&nodes[1], 1, false);
- let reason = ClosureReason::CommitmentTxConfirmed;
- check_closed_event(&nodes[1], 1, reason, &[node_a_id], 100000);
+ // When hold=false, get_and_clear_pending_events also triggers
+ // process_background_events (replaying the preimage and force-close updates)
+ // and resolves the deferred completions, firing PaymentForwarded alongside
+ // ChannelClosed. When hold=true, only ChannelClosed fires.
+ let evs = nodes[1].node.get_and_clear_pending_events();
+ let expected = if hold_post_reload_mon_update { 1 } else { 2 };
+ assert_eq!(evs.len(), expected, "{:?}", evs);
+ assert!(evs.iter().any(|e| matches!(
+ e,
+ Event::ChannelClosed { reason: ClosureReason::CommitmentTxConfirmed, .. }
+ )));
+ if !hold_post_reload_mon_update {
+ assert!(evs.iter().any(|e| matches!(e, Event::PaymentForwarded { .. })));
+ check_added_monitors(&nodes[1], mons_added);
+ }
}
nodes[1].node.timer_tick_occurred();
- check_added_monitors(&nodes[1], mons_added);
+ // For !close_chans_before_reload && !hold, background events were already replayed
+ // during get_and_clear_pending_events above, so timer_tick adds no monitors.
+ let expected_mons =
+ if !close_chans_before_reload && !hold_post_reload_mon_update { 0 } else { mons_added };
+ check_added_monitors(&nodes[1], expected_mons);
// Finally, check that B created a payment preimage transaction and close out the payment.
let bs_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
@@ -3924,44 +3941,61 @@ fn do_test_durable_preimages_on_closed_channel(
check_closed_broadcast(&nodes[0], 1, false);
expect_payment_sent(&nodes[0], payment_preimage, None, true, true);
+ if close_chans_before_reload && !hold_post_reload_mon_update {
+ // For close_chans_before_reload with hold=false, the deferred completions
+ // haven't been processed yet. Trigger process_pending_monitor_events now.
+ let _ = nodes[1].node.get_and_clear_pending_msg_events();
+ check_added_monitors(&nodes[1], 0);
+ }
+
if !close_chans_before_reload || close_only_a {
// Make sure the B<->C channel is still alive and well by sending a payment over it.
let mut reconnect_args = ReconnectArgs::new(&nodes[1], &nodes[2]);
reconnect_args.pending_responding_commitment_signed.1 = true;
- // The B<->C `ChannelMonitorUpdate` shouldn't be allowed to complete, which is the
- // equivalent to the responding `commitment_signed` being a duplicate for node B, thus we
- // need to set the `pending_responding_commitment_signed_dup` flag.
- reconnect_args.pending_responding_commitment_signed_dup_monitor.1 = true;
+ if hold_post_reload_mon_update {
+ // When the A-B update is still InProgress, B-C monitor updates are blocked,
+ // so the responding commitment_signed is a duplicate that generates no update.
+ reconnect_args.pending_responding_commitment_signed_dup_monitor.1 = true;
+ }
reconnect_args.pending_raa.1 = true;
reconnect_nodes(reconnect_args);
}
- // Once the blocked `ChannelMonitorUpdate` *finally* completes, the pending
- // `PaymentForwarded` event will finally be released.
- let (_, ab_update_id) = nodes[1].chain_monitor.get_latest_mon_update_id(chan_id_ab);
- nodes[1].chain_monitor.chain_monitor.force_channel_monitor_updated(chan_id_ab, ab_update_id);
+ if hold_post_reload_mon_update {
+ // When the persister returned InProgress, we need to manually complete the
+ // A-B monitor update to unblock the PaymentForwarded completion action.
+ let (_, ab_update_id) = nodes[1].chain_monitor.get_latest_mon_update_id(chan_id_ab);
+ nodes[1]
+ .chain_monitor
+ .chain_monitor
+ .force_channel_monitor_updated(chan_id_ab, ab_update_id);
+ }
// If the A<->B channel was closed before we reload, we'll replay the claim against it on
// reload, causing the `PaymentForwarded` event to get replayed.
let evs = nodes[1].node.get_and_clear_pending_events();
- assert_eq!(evs.len(), if close_chans_before_reload { 2 } else { 1 });
- for ev in evs {
- if let Event::PaymentForwarded { claim_from_onchain_tx, next_htlcs, .. } = ev {
- if !claim_from_onchain_tx {
- // If the outbound channel is still open, the `next_user_channel_id` should be available.
- // This was previously broken.
- assert!(next_htlcs[0].user_channel_id.is_some())
+ if !close_chans_before_reload && !hold_post_reload_mon_update {
+ // PaymentForwarded already fired during get_and_clear_pending_events above.
+ assert!(evs.is_empty(), "{:?}", evs);
+ } else {
+ assert_eq!(evs.len(), if close_chans_before_reload { 2 } else { 1 }, "{:?}", evs);
+ for ev in evs {
+ if let Event::PaymentForwarded { claim_from_onchain_tx, next_htlcs, .. } = ev {
+ if !claim_from_onchain_tx {
+ assert!(next_htlcs[0].user_channel_id.is_some())
+ }
+ } else {
+ panic!("Unexpected event: {:?}", ev);
}
- } else {
- panic!();
}
}
if !close_chans_before_reload || close_only_a {
- // Once we call `process_pending_events` the final `ChannelMonitor` for the B<->C channel
- // will fly, removing the payment preimage from it.
- check_added_monitors(&nodes[1], 1);
+ if hold_post_reload_mon_update {
+ // The B-C monitor update from the completion action fires now.
+ check_added_monitors(&nodes[1], 1);
+ }
assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
send_payment(&nodes[1], &[&nodes[2]], 100_000);
}
@@ -5423,17 +5457,16 @@ fn test_late_counterparty_commitment_update_after_holder_commitment_spend_dust()
}
#[test]
-#[should_panic(
- expected = "Watch::update_channel returned Completed while prior updates are still InProgress"
-)]
-fn test_monitor_update_fail_after_funding_spend() {
- // When a counterparty commitment transaction confirms (funding spend), the
- // ChannelMonitor sets funding_spend_seen. If a commitment_signed from the
- // counterparty is then processed (a race between chain events and message
- // processing), update_monitor returns Err because no_further_updates_allowed()
- // is true. ChainMonitor overrides the result to InProgress, permanently
- // freezing the channel. A subsequent preimage claim returning Completed then
- // triggers the per-channel assertion.
+fn test_monitor_update_after_funding_spend() {
+ // Test that monitor updates still work after a funding spend is detected by the
+ // ChainMonitor but before ChannelManager has processed the corresponding block.
+ //
+ // When the counterparty commitment transaction confirms (funding spend), the
+ // ChannelMonitor sets funding_spend_seen and no_further_updates_allowed() returns
+ // true. ChainMonitor overrides all subsequent update_channel results to InProgress
+ // to freeze the channel. These overridden updates complete via deferred completions
+ // in release_pending_monitor_events, so that MonitorUpdateCompletionActions (like
+ // PaymentClaimed) can still fire.
let chanmon_cfgs = create_chanmon_cfgs(2);
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
@@ -5444,7 +5477,7 @@ fn test_monitor_update_fail_after_funding_spend() {
let (_, _, chan_id, _) = create_announced_chan_between_nodes(&nodes, 0, 1);
// Route payment 1 fully so B can claim it later.
- let (payment_preimage_1, _payment_hash_1, ..) =
+ let (payment_preimage_1, payment_hash_1, ..) =
route_payment(&nodes[0], &[&nodes[1]], 1_000_000);
// Get A's commitment tx (this is the "counterparty" commitment from B's perspective).
@@ -5453,10 +5486,14 @@ fn test_monitor_update_fail_after_funding_spend() {
// Confirm A's commitment tx on B's chain_monitor ONLY (not on B's ChannelManager).
// This sets funding_spend_seen in the monitor, making no_further_updates_allowed() true.
+ // We also update the best block on the chain_monitor so the broadcaster height is
+ // consistent when claiming HTLCs.
let (block_hash, height) = nodes[1].best_block_info();
let block = create_dummy_block(block_hash, height + 1, vec![as_commitment_tx[0].clone()]);
let txdata: Vec<_> = block.txdata.iter().enumerate().collect();
nodes[1].chain_monitor.chain_monitor.transactions_confirmed(&block.header, &txdata, height + 1);
+ nodes[1].chain_monitor.chain_monitor.best_block_updated(&block.header, height + 1);
+ nodes[1].blocks.lock().unwrap().push((block, height + 1));
// Send payment 2 from A to B.
let (route, payment_hash_2, _, payment_secret_2) =
@@ -5478,15 +5515,38 @@ fn test_monitor_update_fail_after_funding_spend() {
nodes[1].node.handle_update_add_htlc(node_a_id, &payment_event.msgs[0]);
- // B processes commitment_signed. The monitor's update_monitor succeeds on the
- // update steps, but returns Err at the end because no_further_updates_allowed()
- // is true (funding_spend_seen). ChainMonitor overrides the result to InProgress.
+ // B processes commitment_signed. The monitor applies the update but returns Err
+ // because no_further_updates_allowed() is true. ChainMonitor overrides to InProgress,
+ // freezing the channel.
nodes[1].node.handle_commitment_signed(node_a_id, &payment_event.commitment_msg[0]);
check_added_monitors(&nodes[1], 1);
- assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
- // B claims payment 1. The PaymentPreimage monitor update returns Completed
- // (update_monitor succeeds for preimage, and persister returns Completed),
- // but the prior InProgress from the commitment_signed is still pending.
+ // B claims payment 1. The preimage monitor update also returns InProgress (deferred),
+ // so no Completed-while-InProgress assertion fires.
nodes[1].node.claim_funds(payment_preimage_1);
+ check_added_monitors(&nodes[1], 1);
+
+ // First event cycle: the force-close MonitorEvent (CommitmentTxConfirmed) fires first,
+ // then the deferred completions resolve. The force-close generates a ChannelForceClosed
+ // update (also deferred), which blocks completion actions. So we only get ChannelClosed.
+ let events = nodes[1].node.get_and_clear_pending_events();
+ assert_eq!(events.len(), 1);
+ match &events[0] {
+ Event::ChannelClosed { reason: ClosureReason::CommitmentTxConfirmed, .. } => {},
+ _ => panic!("Unexpected event: {:?}", events[0]),
+ }
+ check_added_monitors(&nodes[1], 1);
+ nodes[1].node.get_and_clear_pending_msg_events();
+
+ // Second event cycle: the ChannelForceClosed deferred completion resolves, unblocking
+ // the PaymentClaimed completion action.
+ let events = nodes[1].node.get_and_clear_pending_events();
+ assert_eq!(events.len(), 1);
+ match &events[0] {
+ Event::PaymentClaimed { payment_hash, amount_msat, .. } => {
+ assert_eq!(payment_hash_1, *payment_hash);
+ assert_eq!(1_000_000, *amount_msat);
+ },
+ _ => panic!("Unexpected event: {:?}", events[0]),
+ }
}
Why this scored 53/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.