f: Hold deferred_monitor_update_completions lock to prevent races

Author: joostjager

lightning/src/chain/chainmonitor.rs

@@ -1345,6 +1345,12 @@ where
 				let logger = WithChannelMonitor::from(&self.logger, &monitor, None);
 				log_trace!(logger, "Updating ChannelMonitor to id {}", update.update_id,);
 
+				// We hold `deferred_monitor_update_completions` through the entire
+				// update process so that `release_pending_monitor_events` either
+				// sees both the `MonitorEvent`s generated by `update_monitor` and
+				// the corresponding deferral entry, or neither.
+				let mut deferred =
+					monitor_state.deferred_monitor_update_completions.lock().unwrap();
 				// We hold a `pending_monitor_updates` lock through `update_monitor` to ensure we
 				// have well-ordered updates from the users' point of view. See the
 				// `pending_monitor_updates` docs for more.
@@ -1396,6 +1402,7 @@ where
 					ChannelMonitorUpdateStatus::UnrecoverableError => {
 						// Take the monitors lock for writing so that we poison it and any future
 						// operations going forward fail immediately.
+						core::mem::drop(deferred);
 						core::mem::drop(pending_monitor_updates);
 						core::mem::drop(monitors);
 						let _poison = self.monitors.write().unwrap();
@@ -1434,11 +1441,7 @@ where
 					// 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);
+					deferred.push(update_id);
 					log_debug!(
 						logger,
 						"Deferring completion of ChannelMonitorUpdate id {:?} (channel is post-close)",
@@ -1461,6 +1464,10 @@ where
 		let monitors = self.monitors.read().unwrap();
 		let mut pending_monitor_events = self.pending_monitor_events.lock().unwrap().split_off(0);
 		for monitor_state in monitors.values() {
+			// Hold the deferred lock across both `get_and_clear_pending_monitor_events`
+			// and the deferred resolution so that `update_monitor` cannot insert a
+			// `MonitorEvent` and deferral entry between the two steps.
+			let mut deferred = monitor_state.deferred_monitor_update_completions.lock().unwrap();
 			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();
@@ -1473,16 +1480,12 @@ where
 					counterparty_node_id,
 				));
 			}
-		}
-		// Resolve any deferred monitor update completions after collecting regular monitor
-		// events. The regular events include the force-close (CommitmentTxConfirmed), which
-		// ChannelManager processes first. The deferred completions come after, so that
-		// completion actions resolve once the ChannelForceClosed update (generated during
-		// force-close processing) also gets deferred and resolved in the next event cycle.
-		for monitor_state in monitors.values() {
-			let deferred =
-				monitor_state.deferred_monitor_update_completions.lock().unwrap().split_off(0);
-			for update_id in deferred {
+			// Resolve any deferred monitor update completions after collecting regular monitor
+			// events. The regular events include the force-close (CommitmentTxConfirmed), which
+			// ChannelManager processes first. The deferred completions come after, so that
+			// completion actions resolve once the ChannelForceClosed update (generated during
+			// force-close processing) also gets deferred and resolved in the next event cycle.
+			for update_id in deferred.drain(..) {
 				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);