Return Result from Watch::update_channel to separate monitor failure from persistence status

fuzz/src/chanmon_consistency.rs

@@ -320,7 +320,7 @@ impl chain::Watch<TestChannelSigner> for TestChainMonitor {
 
 	fn update_channel(
 		&self, channel_id: ChannelId, update: &channelmonitor::ChannelMonitorUpdate,
-	) -> chain::ChannelMonitorUpdateStatus {
+	) -> Result<chain::ChannelMonitorUpdateStatus, ()> {
 		let mut map_lock = self.latest_monitors.lock().unwrap();
 		let map_entry = map_lock.get_mut(&channel_id).expect("Didn't have monitor on update call");
 		let latest_monitor_data = map_entry
@@ -336,26 +336,29 @@ impl chain::Watch<TestChannelSigner> for TestChainMonitor {
 			)
 			.unwrap()
 			.1;
-		deserialized_monitor
-			.update_monitor(
-				update,
-				&&TestBroadcaster { txn_broadcasted: RefCell::new(Vec::new()) },
-				&&FuzzEstimator { ret_val: atomic::AtomicU32::new(253) },
-				&self.logger,
-			)
-			.unwrap();
+		let local_update_res = deserialized_monitor.update_monitor(
+			update,
+			&&TestBroadcaster { txn_broadcasted: RefCell::new(Vec::new()) },
+			&&FuzzEstimator { ret_val: atomic::AtomicU32::new(253) },
+			&self.logger,
+		);
 		let mut ser = VecWriter(Vec::new());
 		deserialized_monitor.write(&mut ser).unwrap();
 		let res = self.chain_monitor.update_channel(channel_id, update);
 		match res {
-			chain::ChannelMonitorUpdateStatus::Completed => {
+			Ok(chain::ChannelMonitorUpdateStatus::Completed) => {
+				assert!(local_update_res.is_ok());
 				map_entry.persisted_monitor_id = update.update_id;
 				map_entry.persisted_monitor = ser.0;
 			},
-			chain::ChannelMonitorUpdateStatus::InProgress => {
+			Ok(chain::ChannelMonitorUpdateStatus::InProgress) => {
+				assert!(local_update_res.is_ok());
 				map_entry.pending_monitors.push((update.update_id, ser.0));
 			},
-			chain::ChannelMonitorUpdateStatus::UnrecoverableError => panic!(),
+			Ok(chain::ChannelMonitorUpdateStatus::UnrecoverableError) => panic!(),
+			Err(()) => {
+				assert!(local_update_res.is_err());
+			},
 		}
 		res
 	}

lightning/src/chain/chainmonitor.rs

@@ -1311,7 +1311,7 @@ where
 
 	fn update_channel(
 		&self, channel_id: ChannelId, update: &ChannelMonitorUpdate,
-	) -> ChannelMonitorUpdateStatus {
+	) -> Result<ChannelMonitorUpdateStatus, ()> {
 		// `ChannelMonitorUpdate`'s `channel_id` is `None` prior to 0.0.121 and all channels in those
 		// versions are V1-established. For 0.0.121+ the `channel_id` fields is always `Some`.
 		debug_assert_eq!(update.channel_id.unwrap(), channel_id);
@@ -1328,7 +1328,7 @@ where
 				#[cfg(debug_assertions)]
 				panic!("ChannelManager generated a channel update for a channel that was not yet registered!");
 				#[cfg(not(debug_assertions))]
-				ChannelMonitorUpdateStatus::InProgress
+				Err(())
 			},
 			Some(monitor_state) => {
 				let monitor = &monitor_state.monitor;
@@ -1415,9 +1415,9 @@ where
 				}
 
 				if update_res.is_err() {
-					ChannelMonitorUpdateStatus::InProgress
+					Err(())
 				} else {
-					persist_res
+					Ok(persist_res)
 				}
 			},
 		}

lightning/src/chain/mod.rs

@@ -316,22 +316,23 @@ pub trait Watch<ChannelSigner: EcdsaChannelSigner> {
 
 	/// Updates a channel identified by `channel_id` by applying `update` to its monitor.
 	///
-	/// Implementations must call [`ChannelMonitor::update_monitor`] with the given update. This
-	/// may fail (returning an `Err(())`), in which case this should return
-	/// [`ChannelMonitorUpdateStatus::InProgress`] (and the update should never complete). This
+	/// Implementations must call [`ChannelMonitor::update_monitor`] with the given update. If
+	/// that call fails (returning an `Err(())`), this method should return `Err(())`. This
 	/// generally implies the channel has been closed (either by the funding outpoint being spent
 	/// on-chain or the [`ChannelMonitor`] having decided to do so and broadcasted a transaction),
 	/// and the [`ChannelManager`] state will be updated once it sees the funding spend on-chain.
+	/// Even when `update_monitor` fails, the updated monitor state should still be persisted.
 	///
-	/// In general, persistence failures should be retried after returning
-	/// [`ChannelMonitorUpdateStatus::InProgress`] and eventually complete. If a failure truly
-	/// cannot be retried, the node should shut down immediately after returning
-	/// [`ChannelMonitorUpdateStatus::UnrecoverableError`], see its documentation for more info.
+	/// On success, returns the persistence status. In general, persistence failures should be
+	/// retried after returning [`ChannelMonitorUpdateStatus::InProgress`] and eventually
+	/// complete. If a failure truly cannot be retried, the node should shut down immediately
+	/// after returning [`ChannelMonitorUpdateStatus::UnrecoverableError`], see its documentation
+	/// for more info.
 	///
 	/// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager
 	fn update_channel(
 		&self, channel_id: ChannelId, update: &ChannelMonitorUpdate,
-	) -> ChannelMonitorUpdateStatus;
+	) -> Result<ChannelMonitorUpdateStatus, ()>;
 
 	/// Returns any monitor events since the last call. Subsequent calls must only return new
 	/// events.
@@ -358,7 +359,7 @@ impl<ChannelSigner: EcdsaChannelSigner, T: Watch<ChannelSigner> + ?Sized, W: Der
 
 	fn update_channel(
 		&self, channel_id: ChannelId, update: &ChannelMonitorUpdate,
-	) -> ChannelMonitorUpdateStatus {
+	) -> Result<ChannelMonitorUpdateStatus, ()> {
 		self.deref().update_channel(channel_id, update)
 	}
 

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -136,14 +136,11 @@ fn test_monitor_and_persister_update_fail() {
 				&feeest,
 				&node_cfgs[0].logger,
 			) {
-				// Check that the persister returns InProgress (and will never actually complete)
-				// as the monitor update errors.
-				if let ChannelMonitorUpdateStatus::InProgress =
-					chain_mon.chain_monitor.update_channel(chan.2, &update)
-				{
-				} else {
-					panic!("Expected monitor paused");
-				}
+				// Check that the chain monitor returns Err as the monitor update errors.
+				assert!(
+					chain_mon.chain_monitor.update_channel(chan.2, &update).is_err(),
+					"Expected monitor update failure",
+				);
 				logger.assert_log_regex(
 					"lightning::chain::chainmonitor",
 					regex::Regex::new("Failed to update ChannelMonitor").unwrap(),
@@ -154,7 +151,7 @@ fn test_monitor_and_persister_update_fail() {
 				// ChannelManager and ChannelMonitor aren't out of sync.
 				assert_eq!(
 					nodes[0].chain_monitor.update_channel(chan.2, &update),
-					ChannelMonitorUpdateStatus::Completed
+					Ok(ChannelMonitorUpdateStatus::Completed)
 				);
 			} else {
 				assert!(false);
@@ -4967,10 +4964,10 @@ fn native_async_persist() {
 	// Now test two async `ChannelMonitorUpdate`s in flight at once, completing them in-order but
 	// separately.
 	let update_status = async_chain_monitor.update_channel(chan_id, &updates[0]);
-	assert_eq!(update_status, ChannelMonitorUpdateStatus::InProgress);
+	assert_eq!(update_status, Ok(ChannelMonitorUpdateStatus::InProgress));
 
 	let update_status = async_chain_monitor.update_channel(chan_id, &updates[1]);
-	assert_eq!(update_status, ChannelMonitorUpdateStatus::InProgress);
+	assert_eq!(update_status, Ok(ChannelMonitorUpdateStatus::InProgress));
 
 	persist_futures.poll_futures();
 	assert_eq!(async_chain_monitor.release_pending_monitor_events().len(), 0);
@@ -5016,10 +5013,10 @@ fn native_async_persist() {
 	// out-of-order and ensuring that no `MonitorEvent::Completed` is generated until they are both
 	// completed (and that it marks both as completed when it is generated).
 	let update_status = async_chain_monitor.update_channel(chan_id, &updates[2]);
-	assert_eq!(update_status, ChannelMonitorUpdateStatus::InProgress);
+	assert_eq!(update_status, Ok(ChannelMonitorUpdateStatus::InProgress));
 
 	let update_status = async_chain_monitor.update_channel(chan_id, &updates[3]);
-	assert_eq!(update_status, ChannelMonitorUpdateStatus::InProgress);
+	assert_eq!(update_status, Ok(ChannelMonitorUpdateStatus::InProgress));
 
 	persist_futures.poll_futures();
 	assert_eq!(async_chain_monitor.release_pending_monitor_events().len(), 0);

lightning/src/ln/channelmanager.rs

@@ -10058,7 +10058,20 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 				self.chain_monitor.update_channel(channel_id, &in_flight_updates[update_idx]);
 			let logger =
 				WithContext::from(&self.logger, Some(counterparty_node_id), Some(channel_id), None);
-			let update_completed = self.handle_monitor_update_res(update_res, logger);
+			// Map monitor update failure (Err) to InProgress to freeze the channel.
+			// This happens when ChannelMonitor::update_monitor fails internally,
+			// generally implying the channel has been closed on-chain.
+			let update_status = match update_res {
+				Ok(status) => status,
+				Err(()) => {
+					log_debug!(
+						logger,
+						"ChannelMonitor::update_monitor failed, treating as InProgress to freeze channel.",
+					);
+					ChannelMonitorUpdateStatus::InProgress
+				},
+			};
+			let update_completed = self.handle_monitor_update_res(update_status, logger);
 			if update_completed {
 				let _ = in_flight_updates.remove(update_idx);
 			}

lightning/src/ln/functional_tests.rs

@@ -7429,13 +7429,10 @@ pub fn test_update_err_monitor_lockdown() {
 				&feeest,
 				&node_cfgs[0].logger,
 			) {
-				assert_eq!(
-					watchtower.chain_monitor.update_channel(chan_1.2, &update),
-					ChannelMonitorUpdateStatus::InProgress
-				);
+				assert!(watchtower.chain_monitor.update_channel(chan_1.2, &update).is_err());
 				assert_eq!(
 					nodes[0].chain_monitor.update_channel(chan_1.2, &update),
-					ChannelMonitorUpdateStatus::Completed
+					Ok(ChannelMonitorUpdateStatus::Completed)
 				);
 			} else {
 				assert!(false);
@@ -7588,17 +7585,14 @@ pub fn test_concurrent_monitor_claim() {
 				&node_cfgs[0].logger,
 			) {
 				// Watchtower Alice should already have seen the block and reject the update
-				assert_eq!(
-					watchtower_alice.chain_monitor.update_channel(chan_1.2, &update),
-					ChannelMonitorUpdateStatus::InProgress
-				);
+				assert!(watchtower_alice.chain_monitor.update_channel(chan_1.2, &update).is_err());
 				assert_eq!(
 					watchtower_bob.chain_monitor.update_channel(chan_1.2, &update),
-					ChannelMonitorUpdateStatus::Completed
+					Ok(ChannelMonitorUpdateStatus::Completed)
 				);
 				assert_eq!(
 					nodes[0].chain_monitor.update_channel(chan_1.2, &update),
-					ChannelMonitorUpdateStatus::Completed
+					Ok(ChannelMonitorUpdateStatus::Completed)
 				);
 			} else {
 				assert!(false);

lightning/src/util/test_utils.rs

@@ -617,7 +617,7 @@ impl<'a> chain::Watch<TestChannelSigner> for TestChainMonitor<'a> {
 
 	fn update_channel(
 		&self, channel_id: ChannelId, update: &ChannelMonitorUpdate,
-	) -> chain::ChannelMonitorUpdateStatus {
+	) -> Result<chain::ChannelMonitorUpdateStatus, ()> {
 		#[cfg(feature = "std")]
 		if let Some(blocker) = &*self.write_blocker.lock().unwrap() {
 			blocker.recv().unwrap();