Do not broadcast commitment txn on Permanent mon update failure

fuzz/src/chanmon_consistency.rs

@@ -32,7 +32,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, ChannelMonitorUpdateStatus, chainmonitor, channelmonitor, Confirm, Watch};
 use lightning::chain::channelmonitor::{ChannelMonitor, MonitorEvent};
 use lightning::chain::transaction::OutPoint;
 use lightning::chain::chaininterface::{BroadcasterInterface, ConfirmationTarget, FeeEstimator};
@@ -124,7 +124,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::ChannelMonitorUpdateStatus {
 		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)) {
@@ -134,7 +134,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::ChannelMonitorUpdateStatus {
 		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,
@@ -270,7 +270,7 @@ fn check_api_err(api_err: APIError) {
 				_ => panic!("{}", err),
 			}
 		},
-		APIError::MonitorUpdateFailed => {
+		APIError::MonitorUpdateInProgress => {
 			// We can (obviously) temp-fail a monitor update
 		},
 		APIError::IncompatibleShutdownScript { .. } => panic!("Cannot send an incompatible shutdown script"),
@@ -363,7 +363,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(ChannelMonitorUpdateStatus::Completed)
+				}), Arc::clone(&keys_manager)));
 
 			let mut config = UserConfig::default();
 			config.channel_config.forwarding_fee_proportional_millionths = 0;
@@ -383,7 +385,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(ChannelMonitorUpdateStatus::Completed)
+				}), Arc::clone(& $keys_manager)));
 
 			let mut config = UserConfig::default();
 			config.channel_config.forwarding_fee_proportional_millionths = 0;
@@ -412,7 +416,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),
+					ChannelMonitorUpdateStatus::Completed);
 			}
 			res
 		} }
@@ -889,12 +894,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() = ChannelMonitorUpdateStatus::InProgress,
+			0x01 => *monitor_b.persister.update_ret.lock().unwrap() = ChannelMonitorUpdateStatus::InProgress,
+			0x02 => *monitor_c.persister.update_ret.lock().unwrap() = ChannelMonitorUpdateStatus::InProgress,
+			0x04 => *monitor_a.persister.update_ret.lock().unwrap() = ChannelMonitorUpdateStatus::Completed,
+			0x05 => *monitor_b.persister.update_ret.lock().unwrap() = ChannelMonitorUpdateStatus::Completed,
+			0x06 => *monitor_c.persister.update_ret.lock().unwrap() = ChannelMonitorUpdateStatus::Completed,
 
 			0x08 => {
 				if let Some((id, _)) = monitor_a.latest_monitors.lock().unwrap().get(&chan_1_funding) {
@@ -1119,9 +1124,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() = ChannelMonitorUpdateStatus::Completed;
+				*monitor_b.persister.update_ret.lock().unwrap() = ChannelMonitorUpdateStatus::Completed;
+				*monitor_c.persister.update_ret.lock().unwrap() = ChannelMonitorUpdateStatus::Completed;
 
 				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

@@ -29,7 +29,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, ChannelMonitorUpdateStatus, Confirm, Listen};
 use lightning::chain::chaininterface::{BroadcasterInterface, ConfirmationTarget, FeeEstimator};
 use lightning::chain::chainmonitor;
 use lightning::chain::transaction::OutPoint;
@@ -389,7 +389,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(ChannelMonitorUpdateStatus::Completed) })));
 
 	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::ChannelMonitorUpdateStatus>,
 }
 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::ChannelMonitorUpdateStatus {
 		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::ChannelMonitorUpdateStatus {
 		self.update_ret.lock().unwrap().clone()
 	}
 }

lightning-invoice/src/payment.rs

@@ -513,11 +513,11 @@ where
 				},
 				PaymentSendFailure::PartialFailure { failed_paths_retry, payment_id, results } => {
 					// If a `PartialFailure` event returns a result that is an `Ok()`, it means that
-					// part of our payment is retried. When we receive `MonitorUpdateFailed`, it
+					// part of our payment is retried. When we receive `MonitorUpdateInProgress`, it
 					// means that we are still waiting for our channel monitor update to be completed.
 					for (result, path) in results.iter().zip(route.paths.into_iter()) {
 						match result {
-							Ok(_) | Err(APIError::MonitorUpdateFailed) => {
+							Ok(_) | Err(APIError::MonitorUpdateInProgress) => {
 								self.process_path_inflight_htlcs(payment_hash, path);
 							},
 							_ => {},
@@ -617,11 +617,11 @@ where
 			},
 			Err(PaymentSendFailure::PartialFailure { failed_paths_retry, results, .. }) => {
 				// If a `PartialFailure` error contains a result that is an `Ok()`, it means that
-				// part of our payment is retried. When we receive `MonitorUpdateFailed`, it
+				// part of our payment is retried. When we receive `MonitorUpdateInProgress`, it
 				// means that we are still waiting for our channel monitor update to complete.
 				for (result, path) in results.iter().zip(route.unwrap().paths.into_iter()) {
 					match result {
-						Ok(_) | Err(APIError::MonitorUpdateFailed) => {
+						Ok(_) | Err(APIError::MonitorUpdateInProgress) => {
 							self.process_path_inflight_htlcs(payment_hash, path);
 						},
 						_ => {},
@@ -796,7 +796,7 @@ mod tests {
 	use std::time::{SystemTime, Duration};
 	use time_utils::tests::SinceEpoch;
 	use DEFAULT_EXPIRY_TIME;
-	use lightning::util::errors::APIError::{ChannelUnavailable, MonitorUpdateFailed};
+	use lightning::util::errors::APIError::{ChannelUnavailable, MonitorUpdateInProgress};
 
 	fn invoice(payment_preimage: PaymentPreimage) -> Invoice {
 		let payment_hash = Sha256::hash(&payment_preimage.0);
@@ -1718,7 +1718,7 @@ mod tests {
 			.fails_with_partial_failure(
 				retry.clone(), OnAttempt(1),
 				Some(vec![
-					Err(ChannelUnavailable { err: "abc".to_string() }), Err(MonitorUpdateFailed)
+					Err(ChannelUnavailable { err: "abc".to_string() }), Err(MonitorUpdateInProgress)
 				]))
 			.expect_send(Amount::ForInvoice(final_value_msat));
 
@@ -1731,7 +1731,7 @@ mod tests {
 		invoice_payer.pay_invoice(&invoice_to_pay).unwrap();
 		let inflight_map = invoice_payer.create_inflight_map();
 
-		// Only the second path, which failed with `MonitorUpdateFailed` should be added to our
+		// Only the second path, which failed with `MonitorUpdateInProgress` should be added to our
 		// inflight map because retries are disabled.
 		assert_eq!(inflight_map.len(), 2);
 	}
@@ -1750,7 +1750,7 @@ mod tests {
 			.fails_with_partial_failure(
 				retry.clone(), OnAttempt(1),
 				Some(vec![
-					Ok(()), Err(MonitorUpdateFailed)
+					Ok(()), Err(MonitorUpdateInProgress)
 				]))
 			.expect_send(Amount::ForInvoice(final_value_msat));
 
@@ -2044,7 +2044,7 @@ mod tests {
 		}
 
 		fn fails_on_attempt(self, attempt: usize) -> Self {
-			let failure = PaymentSendFailure::ParameterError(APIError::MonitorUpdateFailed);
+			let failure = PaymentSendFailure::ParameterError(APIError::MonitorUpdateInProgress);
 			self.fails_with(failure, OnAttempt(attempt))
 		}
 

lightning-persister/src/lib.rs

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