diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index 476362324ad..6fdf27af60d 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -320,7 +320,7 @@ impl chain::Watch for TestChainMonitor { fn update_channel( &self, channel_id: ChannelId, update: &channelmonitor::ChannelMonitorUpdate, - ) -> chain::ChannelMonitorUpdateStatus { + ) -> Result { 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 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 } diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs index 7db1b697c2b..088f3318807 100644 --- a/lightning/src/chain/chainmonitor.rs +++ b/lightning/src/chain/chainmonitor.rs @@ -1311,7 +1311,7 @@ where fn update_channel( &self, channel_id: ChannelId, update: &ChannelMonitorUpdate, - ) -> ChannelMonitorUpdateStatus { + ) -> Result { // `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) } }, } diff --git a/lightning/src/chain/mod.rs b/lightning/src/chain/mod.rs index bc47f1b1db6..cb487f009b6 100644 --- a/lightning/src/chain/mod.rs +++ b/lightning/src/chain/mod.rs @@ -316,22 +316,23 @@ pub trait Watch { /// 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; /// Returns any monitor events since the last call. Subsequent calls must only return new /// events. @@ -358,7 +359,7 @@ impl + ?Sized, W: Der fn update_channel( &self, channel_id: ChannelId, update: &ChannelMonitorUpdate, - ) -> ChannelMonitorUpdateStatus { + ) -> Result { self.deref().update_channel(channel_id, update) } diff --git a/lightning/src/ln/chanmon_update_fail_tests.rs b/lightning/src/ln/chanmon_update_fail_tests.rs index cd32d219b93..6d42dd15e8b 100644 --- a/lightning/src/ln/chanmon_update_fail_tests.rs +++ b/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); diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 6bf04cd62a4..b5a11365c5b 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/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); } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 09a87d93156..98d132912b1 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/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); diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index 22be4367c7a..a11a12063c5 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -617,7 +617,7 @@ impl<'a> chain::Watch for TestChainMonitor<'a> { fn update_channel( &self, channel_id: ChannelId, update: &ChannelMonitorUpdate, - ) -> chain::ChannelMonitorUpdateStatus { + ) -> Result { #[cfg(feature = "std")] if let Some(blocker) = &*self.write_blocker.lock().unwrap() { blocker.recv().unwrap();