Rework chain::Watch return types to make async updates less scary

When a `chain::Watch` `ChannelMonitor` update method is called, the
user has three options:
 (a) persist the monitor update immediately and return success,
 (b) fail to persist the monitor update immediately and return
     failure,
 (c) return a flag indicating the monitor update is in progress and
     will complete in the future.

(c) is rather harmless, and in some deployments should be expected
to be the return value for all monitor update calls, but currently
requires returning `Err(ChannelMonitorUpdateErr::TemporaryFailure)`
which isn't very descriptive and sounds scarier than it is.

Instead, here, we change the return type used to be a single enum
(rather than a Result) and rename `TemporaryFailure`
`UpdateInProgress`.

Author: TheBlueMatt

fuzz/src/chanmon_consistency.rs

@@ -30,7 +30,7 @@ use bitcoin::hashes::sha256::Hash as Sha256;
 use bitcoin::hash_types::{BlockHash, WPubkeyHash};
 
 use lightning::chain;
-use lightning::chain::{BestBlock, ChannelMonitorUpdateErr, chainmonitor, channelmonitor, Confirm, Watch};
+use lightning::chain::{BestBlock, ChannelMonitorUpdateResult, chainmonitor, channelmonitor, Confirm, Watch};
 use lightning::chain::channelmonitor::{ChannelMonitor, MonitorEvent};
 use lightning::chain::transaction::OutPoint;
 use lightning::chain::chaininterface::{BroadcasterInterface, ConfirmationTarget, FeeEstimator};
@@ -122,7 +122,7 @@ impl TestChainMonitor {
 	}
 }
 impl chain::Watch<EnforcingSigner> for TestChainMonitor {
-	fn watch_channel(&self, funding_txo: OutPoint, monitor: channelmonitor::ChannelMonitor<EnforcingSigner>) -> Result<(), chain::ChannelMonitorUpdateErr> {
+	fn watch_channel(&self, funding_txo: OutPoint, monitor: channelmonitor::ChannelMonitor<EnforcingSigner>) -> chain::ChannelMonitorUpdateResult {
 		let mut ser = VecWriter(Vec::new());
 		monitor.write(&mut ser).unwrap();
 		if let Some(_) = self.latest_monitors.lock().unwrap().insert(funding_txo, (monitor.get_latest_update_id(), ser.0)) {
@@ -132,7 +132,7 @@ impl chain::Watch<EnforcingSigner> for TestChainMonitor {
 		self.chain_monitor.watch_channel(funding_txo, monitor)
 	}
 
-	fn update_channel(&self, funding_txo: OutPoint, update: channelmonitor::ChannelMonitorUpdate) -> Result<(), chain::ChannelMonitorUpdateErr> {
+	fn update_channel(&self, funding_txo: OutPoint, update: channelmonitor::ChannelMonitorUpdate) -> chain::ChannelMonitorUpdateResult {
 		let mut map_lock = self.latest_monitors.lock().unwrap();
 		let mut map_entry = match map_lock.entry(funding_txo) {
 			hash_map::Entry::Occupied(entry) => entry,
@@ -353,7 +353,9 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out) {
 			let logger: Arc<dyn Logger> = Arc::new(test_logger::TestLogger::new($node_id.to_string(), out.clone()));
 			let keys_manager = Arc::new(KeyProvider { node_id: $node_id, rand_bytes_id: atomic::AtomicU32::new(0), enforcement_states: Mutex::new(HashMap::new()) });
 			let monitor = Arc::new(TestChainMonitor::new(broadcast.clone(), logger.clone(), $fee_estimator.clone(),
-				Arc::new(TestPersister { update_ret: Mutex::new(Ok(())) }), Arc::clone(&keys_manager)));
+				Arc::new(TestPersister {
+					update_ret: Mutex::new(ChannelMonitorUpdateResult::UpdateComplete)
+				}), Arc::clone(&keys_manager)));
 
 			let mut config = UserConfig::default();
 			config.channel_config.forwarding_fee_proportional_millionths = 0;
@@ -373,7 +375,9 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out) {
 		    let keys_manager = Arc::clone(& $keys_manager);
 			let logger: Arc<dyn Logger> = Arc::new(test_logger::TestLogger::new($node_id.to_string(), out.clone()));
 			let chain_monitor = Arc::new(TestChainMonitor::new(broadcast.clone(), logger.clone(), $fee_estimator.clone(),
-				Arc::new(TestPersister { update_ret: Mutex::new(Ok(())) }), Arc::clone(& $keys_manager)));
+				Arc::new(TestPersister {
+					update_ret: Mutex::new(ChannelMonitorUpdateResult::UpdateComplete)
+				}), Arc::clone(& $keys_manager)));
 
 			let mut config = UserConfig::default();
 			config.channel_config.forwarding_fee_proportional_millionths = 0;
@@ -402,7 +406,8 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out) {
 
 			let res = (<(BlockHash, ChanMan)>::read(&mut Cursor::new(&$ser.0), read_args).expect("Failed to read manager").1, chain_monitor.clone());
 			for (funding_txo, mon) in monitors.drain() {
-				assert!(chain_monitor.chain_monitor.watch_channel(funding_txo, mon).is_ok());
+				assert_eq!(chain_monitor.chain_monitor.watch_channel(funding_txo, mon),
+					ChannelMonitorUpdateResult::UpdateComplete);
 			}
 			res
 		} }
@@ -878,12 +883,12 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out) {
 			// bit-twiddling mutations to have similar effects. This is probably overkill, but no
 			// harm in doing so.
 
-			0x00 => *monitor_a.persister.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure),
-			0x01 => *monitor_b.persister.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure),
-			0x02 => *monitor_c.persister.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure),
-			0x04 => *monitor_a.persister.update_ret.lock().unwrap() = Ok(()),
-			0x05 => *monitor_b.persister.update_ret.lock().unwrap() = Ok(()),
-			0x06 => *monitor_c.persister.update_ret.lock().unwrap() = Ok(()),
+			0x00 => *monitor_a.persister.update_ret.lock().unwrap() = ChannelMonitorUpdateResult::UpdateInProgress,
+			0x01 => *monitor_b.persister.update_ret.lock().unwrap() = ChannelMonitorUpdateResult::UpdateInProgress,
+			0x02 => *monitor_c.persister.update_ret.lock().unwrap() = ChannelMonitorUpdateResult::UpdateInProgress,
+			0x04 => *monitor_a.persister.update_ret.lock().unwrap() = ChannelMonitorUpdateResult::UpdateComplete,
+			0x05 => *monitor_b.persister.update_ret.lock().unwrap() = ChannelMonitorUpdateResult::UpdateComplete,
+			0x06 => *monitor_c.persister.update_ret.lock().unwrap() = ChannelMonitorUpdateResult::UpdateComplete,
 
 			0x08 => {
 				if let Some((id, _)) = monitor_a.latest_monitors.lock().unwrap().get(&chan_1_funding) {
@@ -1108,9 +1113,9 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out) {
 				// after we resolve all pending events.
 				// First make sure there are no pending monitor updates, resetting the error state
 				// and calling force_channel_monitor_updated for each monitor.
-				*monitor_a.persister.update_ret.lock().unwrap() = Ok(());
-				*monitor_b.persister.update_ret.lock().unwrap() = Ok(());
-				*monitor_c.persister.update_ret.lock().unwrap() = Ok(());
+				*monitor_a.persister.update_ret.lock().unwrap() = ChannelMonitorUpdateResult::UpdateComplete;
+				*monitor_b.persister.update_ret.lock().unwrap() = ChannelMonitorUpdateResult::UpdateComplete;
+				*monitor_c.persister.update_ret.lock().unwrap() = ChannelMonitorUpdateResult::UpdateComplete;
 
 				if let Some((id, _)) = monitor_a.latest_monitors.lock().unwrap().get(&chan_1_funding) {
 					monitor_a.chain_monitor.force_channel_monitor_updated(chan_1_funding, *id);

fuzz/src/full_stack.rs

@@ -27,7 +27,7 @@ use bitcoin::hashes::sha256::Hash as Sha256;
 use bitcoin::hash_types::{Txid, BlockHash, WPubkeyHash};
 
 use lightning::chain;
-use lightning::chain::{BestBlock, Confirm, Listen};
+use lightning::chain::{BestBlock, ChannelMonitorUpdateResult, Confirm, Listen};
 use lightning::chain::chaininterface::{BroadcasterInterface, ConfirmationTarget, FeeEstimator};
 use lightning::chain::chainmonitor;
 use lightning::chain::transaction::OutPoint;
@@ -378,7 +378,7 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
 
 	let broadcast = Arc::new(TestBroadcaster{ txn_broadcasted: Mutex::new(Vec::new()) });
 	let monitor = Arc::new(chainmonitor::ChainMonitor::new(None, broadcast.clone(), Arc::clone(&logger), fee_est.clone(),
-		Arc::new(TestPersister { update_ret: Mutex::new(Ok(())) })));
+		Arc::new(TestPersister { update_ret: Mutex::new(ChannelMonitorUpdateResult::UpdateComplete) })));
 
 	let keys_manager = Arc::new(KeyProvider { node_secret: our_network_key.clone(), inbound_payment_key: KeyMaterial(inbound_payment_key.try_into().unwrap()), counter: AtomicU64::new(0) });
 	let mut config = UserConfig::default();

fuzz/src/utils/test_persister.rs

@@ -7,14 +7,14 @@ use lightning::util::enforcing_trait_impls::EnforcingSigner;
 use std::sync::Mutex;
 
 pub struct TestPersister {
-	pub update_ret: Mutex<Result<(), chain::ChannelMonitorUpdateErr>>,
+	pub update_ret: Mutex<chain::ChannelMonitorUpdateResult>,
 }
 impl chainmonitor::Persist<EnforcingSigner> for TestPersister {
-	fn persist_new_channel(&self, _funding_txo: OutPoint, _data: &channelmonitor::ChannelMonitor<EnforcingSigner>, _update_id: MonitorUpdateId) -> Result<(), chain::ChannelMonitorUpdateErr> {
+	fn persist_new_channel(&self, _funding_txo: OutPoint, _data: &channelmonitor::ChannelMonitor<EnforcingSigner>, _update_id: MonitorUpdateId) -> chain::ChannelMonitorUpdateResult {
 		self.update_ret.lock().unwrap().clone()
 	}
 
-	fn update_persisted_channel(&self, _funding_txo: OutPoint, _update: &Option<channelmonitor::ChannelMonitorUpdate>, _data: &channelmonitor::ChannelMonitor<EnforcingSigner>, _update_id: MonitorUpdateId) -> Result<(), chain::ChannelMonitorUpdateErr> {
+	fn update_persisted_channel(&self, _funding_txo: OutPoint, _update: &Option<channelmonitor::ChannelMonitorUpdate>, _data: &channelmonitor::ChannelMonitor<EnforcingSigner>, _update_id: MonitorUpdateId) -> chain::ChannelMonitorUpdateResult {
 		self.update_ret.lock().unwrap().clone()
 	}
 }

lightning-persister/src/lib.rs

@@ -135,7 +135,7 @@ mod tests {
 	use bitcoin::blockdata::block::{Block, BlockHeader};
 	use bitcoin::hashes::hex::FromHex;
 	use bitcoin::Txid;
-	use lightning::chain::ChannelMonitorUpdateErr;
+	use lightning::chain::ChannelMonitorUpdateResult;
 	use lightning::chain::chainmonitor::Persist;
 	use lightning::chain::transaction::OutPoint;
 	use lightning::{check_closed_broadcast, check_closed_event, check_added_monitors};
@@ -266,7 +266,7 @@ mod tests {
 			index: 0
 		};
 		match persister.persist_new_channel(test_txo, &added_monitors[0].1, update_id.2) {
-			Err(ChannelMonitorUpdateErr::PermanentFailure) => {},
+			ChannelMonitorUpdateResult::PermanentFailure => {},
 			_ => panic!("unexpected result from persisting new channel")
 		}
 
@@ -303,7 +303,7 @@ mod tests {
 			index: 0
 		};
 		match persister.persist_new_channel(test_txo, &added_monitors[0].1, update_id.2) {
-			Err(ChannelMonitorUpdateErr::PermanentFailure) => {},
+			ChannelMonitorUpdateResult::PermanentFailure => {},
 			_ => panic!("unexpected result from persisting new channel")
 		}