Defer monitor update completions after funding spend

When no_further_updates_allowed() is true and the persister returns
Completed, ChainMonitor now overrides the return to InProgress and
queues the update_id in deferred_monitor_update_completions. These
are resolved in release_pending_monitor_events, so ChannelManager
sees them complete together with the force-close MonitorEvents.

This eliminates phantom InProgress entries that would never complete:
previously, a rejected pre-close update (e.g. commitment_signed
arriving after funding spend) returned InProgress with no completion
path, blocking MonitorUpdateCompletionActions (PaymentClaimed,
PaymentForwarded) indefinitely. A subsequent post-close update
returning Completed would then violate the in-order completion
invariant.

AI tools were used in preparing this commit.

Author: joostjager

lightning/src/chain/chainmonitor.rs

@@ -227,6 +227,11 @@ struct MonitorHolder<ChannelSigner: EcdsaChannelSigner> {
 	/// [`ChannelMonitorUpdate`] which was already applied. While this isn't an issue for the
 	/// LDK-provided update-based [`Persist`], it is somewhat surprising for users so we avoid it.
 	pending_monitor_updates: Mutex<Vec<u64>>,
+	/// Monitor update IDs for which the persister returned `Completed` but we overrode the return
+	/// to `InProgress` because `no_further_updates_allowed()` was true. These are resolved in
+	/// [`ChainMonitor::release_pending_monitor_events`] so that `ChannelManager` sees them
+	/// complete together with the force-close `MonitorEvent`s.
+	deferred_monitor_update_completions: Mutex<Vec<u64>>,
 }
 
 impl<ChannelSigner: EcdsaChannelSigner> MonitorHolder<ChannelSigner> {
@@ -1054,7 +1059,11 @@ where
 		if let Some(ref chain_source) = self.chain_source {
 			monitor.load_outputs_to_watch(chain_source, &self.logger);
 		}
-		entry.insert(MonitorHolder { monitor, pending_monitor_updates: Mutex::new(Vec::new()) });
+		entry.insert(MonitorHolder {
+			monitor,
+			pending_monitor_updates: Mutex::new(Vec::new()),
+			deferred_monitor_update_completions: Mutex::new(Vec::new()),
+		});
 
 		Ok(ChannelMonitorUpdateStatus::Completed)
 	}
@@ -1305,6 +1314,7 @@ where
 		entry.insert(MonitorHolder {
 			monitor,
 			pending_monitor_updates: Mutex::new(pending_monitor_updates),
+			deferred_monitor_update_completions: Mutex::new(Vec::new()),
 		});
 		Ok(persist_res)
 	}
@@ -1414,7 +1424,26 @@ where
 					}
 				}
 
-				if update_res.is_err() {
+				if 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. We
+					// track this update_id in deferred_monitor_update_completions so it
+					// gets resolved during release_pending_monitor_events, together with
+					// those MonitorEvents.
+					pending_monitor_updates.push(update_id);
+					monitor_state
+						.deferred_monitor_update_completions
+						.lock()
+						.unwrap()
+						.push(update_id);
+					log_debug!(
+						logger,
+						"Deferring completion of ChannelMonitorUpdate id {:?} (channel is post-close)",
+						update_id,
+					);
 					ChannelMonitorUpdateStatus::InProgress
 				} else {
 					persist_res
@@ -1429,8 +1458,36 @@ where
 		for (channel_id, update_id) in self.persister.get_and_clear_completed_updates() {
 			let _ = self.channel_monitor_updated(channel_id, update_id);
 		}
+		// Resolve any deferred monitor update completions. These are updates where the
+		// persister returned Completed but we overrode the return to InProgress because
+		// no_further_updates_allowed() was true. We resolve them here so that ChannelManager
+		// sees them complete together with the force-close MonitorEvents.
+		let monitors = self.monitors.read().unwrap();
+		for monitor_state in monitors.values() {
+			let deferred =
+				monitor_state.deferred_monitor_update_completions.lock().unwrap().split_off(0);
+			for update_id in deferred {
+				let mut pending = monitor_state.pending_monitor_updates.lock().unwrap();
+				pending.retain(|id| *id != update_id);
+				let monitor_is_pending_updates = monitor_state.has_pending_updates(&pending);
+				if !monitor_is_pending_updates {
+					let funding_txo = monitor_state.monitor.get_funding_txo();
+					let channel_id = monitor_state.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_state.monitor.get_latest_update_id(),
+						}],
+						monitor_state.monitor.get_counterparty_node_id(),
+					));
+				}
+			}
+		}
 		let mut pending_monitor_events = self.pending_monitor_events.lock().unwrap().split_off(0);
-		for monitor_state in self.monitors.read().unwrap().values() {
+		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();

lightning/src/chain/channelmonitor.rs

@@ -6782,6 +6782,7 @@ mod tests {
 	};
 	use crate::ln::channelmanager::{HTLCSource, PaymentId};
 	use crate::ln::functional_test_utils::*;
+	use crate::ln::msgs::{BaseMessageHandler, MessageSendEvent};
 	use crate::ln::outbound_payment::RecipientOnionFields;
 	use crate::ln::script::ShutdownScript;
 	use crate::ln::types::ChannelId;
@@ -6890,7 +6891,16 @@ mod tests {
 		check_spends!(htlc_txn[0], broadcast_tx);
 		check_spends!(htlc_txn[1], broadcast_tx);
 
-		check_closed_broadcast(&nodes[1], 1, true);
+		// When get_and_clear_pending_msg_events triggers release_pending_monitor_events, the
+		// deferred completion from the rejected update resolves first, unfreezing the channel and
+		// generating a commitment update message. Then the force-close MonitorEvents are processed,
+		// generating BroadcastChannelUpdate + HandleError. So we expect 3 messages total.
+		let msg_events = nodes[1].node.get_and_clear_pending_msg_events();
+		assert_eq!(msg_events.len(), 3);
+		assert!(msg_events.iter().any(|e| matches!(e, MessageSendEvent::UpdateHTLCs { .. })));
+		assert!(msg_events.iter().any(|e| matches!(e, MessageSendEvent::BroadcastChannelUpdate { .. })));
+		assert!(msg_events.iter().any(|e| matches!(e, MessageSendEvent::HandleError { .. })));
+
 		check_closed_event(&nodes[1], 1, ClosureReason::CommitmentTxConfirmed, &[nodes[0].node.get_our_node_id()], 100000);
 		check_added_monitors(&nodes[1], 1);
 	}

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -3904,11 +3904,61 @@ 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);
+		if hold_post_reload_mon_update {
+			// When the persister returns InProgress, background events are replayed
+			// during check_closed_event but stay pending (InProgress). Only the
+			// ChannelClosed event fires.
+			check_closed_event(
+				&nodes[1],
+				1,
+				ClosureReason::CommitmentTxConfirmed,
+				&[node_a_id],
+				100000,
+			);
+		} else {
+			// When the persister returns Completed, check_closed_event triggers
+			// process_background_events (replaying the A-B preimage and force-close
+			// updates) AND process_pending_monitor_events (resolving the deferred
+			// completions). This fires both ChannelClosed and PaymentForwarded.
+			let evs = nodes[1].node.get_and_clear_pending_events();
+			assert_eq!(evs.len(), 2, "{:?}", evs);
+			let mut found_close = false;
+			let mut found_forward = false;
+			for ev in &evs {
+				match ev {
+					Event::ChannelClosed {
+						reason: ClosureReason::CommitmentTxConfirmed, ..
+					} => {
+						found_close = true;
+					},
+					Event::PaymentForwarded {
+						claim_from_onchain_tx, next_user_channel_id, ..
+					} => {
+						assert!(!claim_from_onchain_tx);
+						assert!(next_user_channel_id.is_some());
+						found_forward = true;
+					},
+					_ => panic!("Unexpected event: {:?}", ev),
+				}
+			}
+			assert!(found_close && found_forward);
+			// The background events replayed mons_added updates to chain_monitor.
+			check_added_monitors(&nodes[1], mons_added);
+		}
 	}
 	nodes[1].node.timer_tick_occurred();
-	check_added_monitors(&nodes[1], mons_added);
+	if hold_post_reload_mon_update {
+		check_added_monitors(&nodes[1], mons_added);
+	} else if close_chans_before_reload {
+		// For close_chans_before_reload with hold=false, the background events are
+		// replayed during timer_tick (first process_background_events call).
+		check_added_monitors(&nodes[1], mons_added);
+	} else {
+		// For !close_chans_before_reload with hold=false, the background events were
+		// already replayed during check_closed_event above. timer_tick is a no-op for
+		// monitor updates.
+		check_added_monitors(&nodes[1], 0);
+	}
 
 	// 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);
@@ -3923,44 +3973,64 @@ 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_user_channel_id, .. } = 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_user_channel_id.is_some())
+	if !close_chans_before_reload && !hold_post_reload_mon_update {
+		// PaymentForwarded already fired during check_closed_event 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_user_channel_id, .. } = 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_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);
 	}
@@ -5178,15 +5248,17 @@ fn test_mpp_claim_to_holding_cell() {
 }
 
 #[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 the next release_pending_monitor_events cycle, together with the force-close
+	// MonitorEvent, 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]);
@@ -5197,7 +5269,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).
@@ -5206,12 +5278,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.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) =
@@ -5233,15 +5307,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. This triggers process_pending_monitor_events internally, which
+	// processes the deferred completions and the force-close MonitorEvent together.
+	// ChainMonitor also overrides the preimage update to InProgress (channel is post-close),
+	// so no Completed-while-InProgress assertion fires.
 	nodes[1].node.claim_funds(payment_preimage_1);
+	check_added_monitors(&nodes[1], 1);
+
+	// Processing events resolves the deferred completions and force-close together.
+	// We get both a PaymentClaimed and a ChannelClosed event.
+	let events = nodes[1].node.get_and_clear_pending_events();
+	assert_eq!(events.len(), 2);
+	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]),
+	}
+	match &events[1] {
+		Event::ChannelClosed { reason: ClosureReason::CommitmentTxConfirmed, .. } => {},
+		_ => panic!("Unexpected event: {:?}", events[1]),
+	}
+
+	// The deferred completions unfreeze the channel, then the force-close MonitorEvent
+	// closes it. This generates a ChannelForceClosed monitor update and pending messages
+	// (revoke_and_ack, commitment update, error, channel update). Clear them.
+	check_added_monitors(&nodes[1], 1);
+	nodes[1].node.get_and_clear_pending_msg_events();
 }