Drop the ChannelMonitorUpdateStatus::PermanentFailure variant

When a `ChannelMonitorUpdate` fails to apply, it generally means
we cannot reach our storage backend. This, in general, is a
critical issue, but is often only a transient issue.

Sadly, users see the failure variant and return it on any I/O
error, resulting in channel force-closures due to transient issues.

Users don't generally expect force-closes in most cases, and
luckily with async `ChannelMonitorUpdate`s supported we don't take
any risk by "delaying" the `ChannelMonitorUpdate` indefinitely.

Thus, here we drop the `PermanentFailure` variant entirely, making
all failures instead be "the update is in progress, but won't ever
complete", which is equivalent if we do not close the channel
automatically.

Author: TheBlueMatt

lightning-persister/src/fs_store.rs

@@ -436,7 +436,7 @@ mod tests {
 	}
 
 	// Test that if the store's path to channel data is read-only, writing a
-	// monitor to it results in the store returning a PermanentFailure.
+	// monitor to it results in the store returning a InProgress.
 	// Windows ignores the read-only flag for folders, so this test is Unix-only.
 	#[cfg(not(target_os = "windows"))]
 	#[test]
@@ -470,7 +470,7 @@ mod tests {
 			index: 0
 		};
 		match store.persist_new_channel(test_txo, &added_monitors[0].1, update_id.2) {
-			ChannelMonitorUpdateStatus::PermanentFailure => {},
+			ChannelMonitorUpdateStatus::InProgress => {},
 			_ => panic!("unexpected result from persisting new channel")
 		}
 

lightning/src/chain/chainmonitor.rs

@@ -78,7 +78,7 @@ impl MonitorUpdateId {
 /// `Persist` defines behavior for persisting channel monitors: this could mean
 /// writing once to disk, and/or uploading to one or more backup services.
 ///
-/// Each method can return three possible values:
+/// Each method can return two possible values:
 ///  * If persistence (including any relevant `fsync()` calls) happens immediately, the
 ///    implementation should return [`ChannelMonitorUpdateStatus::Completed`], indicating normal
 ///    channel operation should continue.
@@ -91,10 +91,9 @@ impl MonitorUpdateId {
 ///    Note that unlike the direct [`chain::Watch`] interface,
 ///    [`ChainMonitor::channel_monitor_updated`] must be called once for *each* update which occurs.
 ///
-///  * If persistence fails for some reason, implementations should return
-///    [`ChannelMonitorUpdateStatus::PermanentFailure`], in which case the channel will likely be
-///    closed without broadcasting the latest state. See
-///    [`ChannelMonitorUpdateStatus::PermanentFailure`] for more details.
+///    If persistence fails for some reason, implementations should still return
+///    [`ChannelMonitorUpdateStatus::InProgress`], attempting to shut down or otherwise resolve the
+///    situation ASAP.
 ///
 /// Third-party watchtowers may be built as a part of an implementation of this trait, with the
 /// advantage that you can control whether to resume channel operation depending on if an update
@@ -335,11 +334,6 @@ where C::Target: chain::Filter,
 			match self.persister.update_persisted_channel(*funding_outpoint, None, monitor, update_id) {
 				ChannelMonitorUpdateStatus::Completed =>
 					log_trace!(self.logger, "Finished syncing Channel Monitor for channel {}", log_funding_info!(monitor)),
-				ChannelMonitorUpdateStatus::PermanentFailure => {
-					monitor_state.channel_perm_failed.store(true, Ordering::Release);
-					self.pending_monitor_events.lock().unwrap().push((*funding_outpoint, vec![MonitorEvent::UpdateFailed(*funding_outpoint)], monitor.get_counterparty_node_id()));
-					self.event_notifier.notify();
-				}
 				ChannelMonitorUpdateStatus::InProgress => {
 					log_debug!(self.logger, "Channel Monitor sync for channel {} in progress, holding events until completion!", log_funding_info!(monitor));
 					pending_monitor_updates.push(update_id);
@@ -673,12 +667,12 @@ where C::Target: chain::Filter,
 	///
 	/// Note that we persist the given `ChannelMonitor` while holding the `ChainMonitor`
 	/// monitors lock.
-	fn watch_channel(&self, funding_outpoint: OutPoint, monitor: ChannelMonitor<ChannelSigner>) -> ChannelMonitorUpdateStatus {
+	fn watch_channel(&self, funding_outpoint: OutPoint, monitor: ChannelMonitor<ChannelSigner>) -> Result<ChannelMonitorUpdateStatus, ()> {
 		let mut monitors = self.monitors.write().unwrap();
 		let entry = match monitors.entry(funding_outpoint) {
 			hash_map::Entry::Occupied(_) => {
 				log_error!(self.logger, "Failed to add new channel data: channel monitor for given outpoint is already present");
-				return ChannelMonitorUpdateStatus::PermanentFailure
+				return Err(());
 			},
 			hash_map::Entry::Vacant(e) => e,
 		};
@@ -691,10 +685,6 @@ where C::Target: chain::Filter,
 				log_info!(self.logger, "Persistence of new ChannelMonitor for channel {} in progress", log_funding_info!(monitor));
 				pending_monitor_updates.push(update_id);
 			},
-			ChannelMonitorUpdateStatus::PermanentFailure => {
-				log_error!(self.logger, "Persistence of new ChannelMonitor for channel {} failed", log_funding_info!(monitor));
-				return persist_res;
-			},
 			ChannelMonitorUpdateStatus::Completed => {
 				log_info!(self.logger, "Persistence of new ChannelMonitor for channel {} completed", log_funding_info!(monitor));
 			}
@@ -708,7 +698,7 @@ where C::Target: chain::Filter,
 			channel_perm_failed: AtomicBool::new(false),
 			last_chain_persist_height: AtomicUsize::new(self.highest_chain_height.load(Ordering::Acquire)),
 		});
-		persist_res
+		Ok(persist_res)
 	}
 
 	/// Note that we persist the given `ChannelMonitor` update while holding the
@@ -723,10 +713,10 @@ where C::Target: chain::Filter,
 				// We should never ever trigger this from within ChannelManager. Technically a
 				// user could use this object with some proxying in between which makes this
 				// possible, but in tests and fuzzing, this should be a panic.
-				#[cfg(any(test, fuzzing))]
+				#[cfg(debug_assertions)]
 				panic!("ChannelManager generated a channel update for a channel that was not yet registered!");
-				#[cfg(not(any(test, fuzzing)))]
-				ChannelMonitorUpdateStatus::PermanentFailure
+				#[cfg(not(debug_assertions))]
+				ChannelMonitorUpdateStatus::InProgress
 			},
 			Some(monitor_state) => {
 				let monitor = &monitor_state.monitor;
@@ -745,18 +735,14 @@ where C::Target: chain::Filter,
 						pending_monitor_updates.push(update_id);
 						log_debug!(self.logger, "Persistence of ChannelMonitorUpdate for channel {} in progress", log_funding_info!(monitor));
 					},
-					ChannelMonitorUpdateStatus::PermanentFailure => {
-						monitor_state.channel_perm_failed.store(true, Ordering::Release);
-						log_error!(self.logger, "Persistence of ChannelMonitorUpdate for channel {} failed", log_funding_info!(monitor));
-					},
 					ChannelMonitorUpdateStatus::Completed => {
 						log_debug!(self.logger, "Persistence of ChannelMonitorUpdate for channel {} completed", log_funding_info!(monitor));
 					},
 				}
 				if update_res.is_err() {
-					ChannelMonitorUpdateStatus::PermanentFailure
+					ChannelMonitorUpdateStatus::InProgress
 				} else if monitor_state.channel_perm_failed.load(Ordering::Acquire) {
-					ChannelMonitorUpdateStatus::PermanentFailure
+					ChannelMonitorUpdateStatus::InProgress
 				} else {
 					persist_res
 				}
@@ -988,12 +974,8 @@ mod tests {
 		chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::Completed);
 		unwrap_send_err!(nodes[0].node.send_payment_with_route(&route, second_payment_hash,
 				RecipientOnionFields::secret_only(second_payment_secret), PaymentId(second_payment_hash.0)
-			), true, APIError::ChannelUnavailable { ref err },
-			assert!(err.contains("ChannelMonitor storage failure")));
-		check_added_monitors!(nodes[0], 2); // After the failure we generate a close-channel monitor update
-		check_closed_broadcast!(nodes[0], true);
-		check_closed_event!(nodes[0], 1, ClosureReason::ProcessingError { err: "ChannelMonitor storage failure".to_string() },
-			[nodes[1].node.get_our_node_id()], 100000);
+			), false, APIError::MonitorUpdateInProgress, {});
+		check_added_monitors!(nodes[0], 1);
 
 		// However, as the ChainMonitor is still waiting for the original persistence to complete,
 		// it won't yet release the MonitorEvents.
@@ -1020,28 +1002,4 @@ mod tests {
 		do_chainsync_pauses_events(false);
 		do_chainsync_pauses_events(true);
 	}
-
-	#[test]
-	fn update_during_chainsync_fails_channel() {
-		let chanmon_cfgs = create_chanmon_cfgs(2);
-		let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
-		let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
-		let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
-		create_announced_chan_between_nodes(&nodes, 0, 1);
-
-		chanmon_cfgs[0].persister.chain_sync_monitor_persistences.lock().unwrap().clear();
-		chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::PermanentFailure);
-
-		connect_blocks(&nodes[0], 1);
-		// Before processing events, the ChannelManager will still think the Channel is open and
-		// there won't be any ChannelMonitorUpdates
-		assert_eq!(nodes[0].node.list_channels().len(), 1);
-		check_added_monitors!(nodes[0], 0);
-		// ... however once we get events once, the channel will close, creating a channel-closed
-		// ChannelMonitorUpdate.
-		check_closed_broadcast!(nodes[0], true);
-		check_closed_event!(nodes[0], 1, ClosureReason::ProcessingError { err: "Failed to persist ChannelMonitor update during chain sync".to_string() },
-			[nodes[1].node.get_our_node_id()], 100000);
-		check_added_monitors!(nodes[0], 1);
-	}
 }

lightning/src/chain/channelmonitor.rs

@@ -151,10 +151,7 @@ pub enum MonitorEvent {
 		monitor_update_id: u64,
 	},
 
-	/// Indicates a [`ChannelMonitor`] update has failed. See
-	/// [`ChannelMonitorUpdateStatus::PermanentFailure`] for more information on how this is used.
-	///
-	/// [`ChannelMonitorUpdateStatus::PermanentFailure`]: super::ChannelMonitorUpdateStatus::PermanentFailure
+	/// Indicates a [`ChannelMonitor`] update has failed.
 	UpdateFailed(OutPoint),
 }
 impl_writeable_tlv_based_enum_upgradable!(MonitorEvent,
@@ -1492,17 +1489,14 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {
 	/// the Channel was out-of-date.
 	///
 	/// You may also use this to broadcast the latest local commitment transaction, either because
-	/// a monitor update failed with [`ChannelMonitorUpdateStatus::PermanentFailure`] or because we've
-	/// fallen behind (i.e. we've received proof that our counterparty side knows a revocation
-	/// secret we gave them that they shouldn't know).
+	/// a monitor update failed or because we've fallen behind (i.e. we've received proof that our
+	/// counterparty side knows a revocation secret we gave them that they shouldn't know).
 	///
 	/// Broadcasting these transactions in the second case is UNSAFE, as they allow counterparty
 	/// side to punish you. Nevertheless you may want to broadcast them if counterparty doesn't
 	/// close channel with their commitment transaction after a substantial amount of time. Best
 	/// may be to contact the other node operator out-of-band to coordinate other options available
 	/// to you. In any-case, the choice is up to you.
-	///
-	/// [`ChannelMonitorUpdateStatus::PermanentFailure`]: super::ChannelMonitorUpdateStatus::PermanentFailure
 	pub fn get_latest_holder_commitment_txn<L: Deref>(&self, logger: &L) -> Vec<Transaction>
 	where L::Target: Logger {
 		self.inner.lock().unwrap().get_latest_holder_commitment_txn(logger)
@@ -2599,6 +2593,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 				ChannelMonitorUpdateStep::CommitmentSecret { idx, secret } => {
 					log_trace!(logger, "Updating ChannelMonitor with commitment secret");
 					if let Err(e) = self.provide_secret(*idx, *secret) {
+						debug_assert!(false, "Latest counterparty commitment secret was invalid");
 						log_error!(logger, "Providing latest counterparty commitment secret failed/was refused:");
 						log_error!(logger, "    {}", e);
 						ret = Err(());
@@ -4482,18 +4477,14 @@ mod tests {
 		let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[1], nodes[0], 100_000);
 		unwrap_send_err!(nodes[1].node.send_payment_with_route(&route, payment_hash,
 				RecipientOnionFields::secret_only(payment_secret), PaymentId(payment_hash.0)
-			), true, APIError::ChannelUnavailable { ref err },
-			assert!(err.contains("ChannelMonitor storage failure")));
-		check_added_monitors!(nodes[1], 2); // After the failure we generate a close-channel monitor update
-		check_closed_broadcast!(nodes[1], true);
-		check_closed_event!(nodes[1], 1, ClosureReason::ProcessingError { err: "ChannelMonitor storage failure".to_string() }, 
-			[nodes[0].node.get_our_node_id()], 100000);
+			), false, APIError::MonitorUpdateInProgress, {});
+		check_added_monitors!(nodes[1], 1);
 
 		// Build a new ChannelMonitorUpdate which contains both the failing commitment tx update
 		// and provides the claim preimages for the two pending HTLCs. The first update generates
 		// an error, but the point of this test is to ensure the later updates are still applied.
 		let monitor_updates = nodes[1].chain_monitor.monitor_updates.lock().unwrap();
-		let mut replay_update = monitor_updates.get(&channel.2).unwrap().iter().rev().skip(1).next().unwrap().clone();
+		let mut replay_update = monitor_updates.get(&channel.2).unwrap().iter().rev().next().unwrap().clone();
 		assert_eq!(replay_update.updates.len(), 1);
 		if let ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo { .. } = replay_update.updates[0] {
 		} else { panic!(); }

lightning/src/chain/mod.rs

@@ -192,10 +192,6 @@ pub enum ChannelMonitorUpdateStatus {
 	/// have been successfully applied, a [`MonitorEvent::Completed`] can be used to restore the
 	/// channel to an operational state.
 	///
-	/// Note that a given [`ChannelManager`] will *never* re-generate a [`ChannelMonitorUpdate`].
-	/// If you return this error you must ensure that it is written to disk safely before writing
-	/// the latest [`ChannelManager`] state, or you should return [`PermanentFailure`] instead.
-	///
 	/// Even when a channel has been "frozen", updates to the [`ChannelMonitor`] can continue to
 	/// occur (e.g. if an inbound HTLC which we forwarded was claimed upstream, resulting in us
 	/// attempting to claim it on this channel) and those updates must still be persisted.
@@ -208,49 +204,8 @@ pub enum ChannelMonitorUpdateStatus {
 	/// remote location (with local copies persisted immediately), it is anticipated that all
 	/// updates will return [`InProgress`] until the remote copies could be updated.
 	///
-	/// [`PermanentFailure`]: ChannelMonitorUpdateStatus::PermanentFailure
 	/// [`InProgress`]: ChannelMonitorUpdateStatus::InProgress
-	/// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager
 	InProgress,
-	/// Used to indicate no further channel monitor updates will be allowed (likely a disk failure
-	/// or a remote copy of this [`ChannelMonitor`] is no longer reachable and thus not updatable).
-	///
-	/// When this is returned, [`ChannelManager`] will force-close the channel but *not* broadcast
-	/// our current commitment transaction. This avoids a dangerous case where a local disk failure
-	/// (e.g. the Linux-default remounting of the disk as read-only) causes [`PermanentFailure`]s
-	/// for all monitor updates. If we were to broadcast our latest commitment transaction and then
-	/// restart, we could end up reading a previous [`ChannelMonitor`] and [`ChannelManager`],
-	/// revoking our now-broadcasted state before seeing it confirm and losing all our funds.
-	///
-	/// Note that this is somewhat of a tradeoff - if the disk is really gone and we may have lost
-	/// the data permanently, we really should broadcast immediately. If the data can be recovered
-	/// with manual intervention, we'd rather close the channel, rejecting future updates to it,
-	/// and broadcast the latest state only if we have HTLCs to claim which are timing out (which
-	/// we do as long as blocks are connected).
-	///
-	/// In order to broadcast the latest local commitment transaction, you'll need to call
-	/// [`ChannelMonitor::get_latest_holder_commitment_txn`] and broadcast the resulting
-	/// transactions once you've safely ensured no further channel updates can be generated by your
-	/// [`ChannelManager`].
-	///
-	/// Note that at least one final [`ChannelMonitorUpdate`] may still be provided, which must
-	/// still be processed by a running [`ChannelMonitor`]. This final update will mark the
-	/// [`ChannelMonitor`] as finalized, ensuring no further updates (e.g. revocation of the latest
-	/// commitment transaction) are allowed.
-	///
-	/// Note that even if you return a [`PermanentFailure`] due to unavailability of secondary
-	/// [`ChannelMonitor`] copies, you should still make an attempt to store the update where
-	/// possible to ensure you can claim HTLC outputs on the latest commitment transaction
-	/// broadcasted later.
-	///
-	/// In case of distributed watchtowers deployment, the new version must be written to disk, as
-	/// state may have been stored but rejected due to a block forcing a commitment broadcast. This
-	/// storage is used to claim outputs of rejected state confirmed onchain by another watchtower,
-	/// lagging behind on block processing.
-	///
-	/// [`PermanentFailure`]: ChannelMonitorUpdateStatus::PermanentFailure
-	/// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager
-	PermanentFailure,
 }
 
 /// The `Watch` trait defines behavior for watching on-chain activity pertaining to channels as
@@ -262,37 +217,44 @@ pub enum ChannelMonitorUpdateStatus {
 /// requirements.
 ///
 /// Implementations **must** ensure that updates are successfully applied and persisted upon method
-/// completion. If an update fails with a [`PermanentFailure`], then it must immediately shut down
-/// without taking any further action such as persisting the current state.
+/// completion. If an update will not succeed, then it must immediately shut down.
 ///
 /// If an implementation maintains multiple instances of a channel's monitor (e.g., by storing
 /// backup copies), then it must ensure that updates are applied across all instances. Otherwise, it
 /// could result in a revoked transaction being broadcast, allowing the counterparty to claim all
 /// funds in the channel. See [`ChannelMonitorUpdateStatus`] for more details about how to handle
 /// multiple instances.
-///
-/// [`PermanentFailure`]: ChannelMonitorUpdateStatus::PermanentFailure
 pub trait Watch<ChannelSigner: WriteableEcdsaChannelSigner> {
 	/// Watches a channel identified by `funding_txo` using `monitor`.
 	///
 	/// Implementations are responsible for watching the chain for the funding transaction along
 	/// with any spends of outputs returned by [`get_outputs_to_watch`]. In practice, this means
 	/// calling [`block_connected`] and [`block_disconnected`] on the monitor.
 	///
-	/// Note: this interface MUST error with [`ChannelMonitorUpdateStatus::PermanentFailure`] if
-	/// the given `funding_txo` has previously been registered via `watch_channel`.
+	/// A return of `Err(())` indicates that the channel should not open and the channel
+	/// immediately force-closed without broadcasting the funding transaction.
+	///
+	/// If the given `funding_txo` has previously been registered via `watch_channel`, `Err(())`
+	/// must be returned.
 	///
 	/// [`get_outputs_to_watch`]: channelmonitor::ChannelMonitor::get_outputs_to_watch
 	/// [`block_connected`]: channelmonitor::ChannelMonitor::block_connected
 	/// [`block_disconnected`]: channelmonitor::ChannelMonitor::block_disconnected
-	fn watch_channel(&self, funding_txo: OutPoint, monitor: ChannelMonitor<ChannelSigner>) -> ChannelMonitorUpdateStatus;
+	fn watch_channel(&self, funding_txo: OutPoint, monitor: ChannelMonitor<ChannelSigner>) -> Result<ChannelMonitorUpdateStatus, ()>;
 
 	/// Updates a channel identified by `funding_txo` by applying `update` to its monitor.
 	///
-	/// Implementations must call [`update_monitor`] with the given update. See
-	/// [`ChannelMonitorUpdateStatus`] for invariants around returning an error.
+	/// 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
+	/// 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.
+	///
+	/// If persistence fails, this should return [`ChannelMonitorUpdateStatus::InProgress`] and
+	/// the node should shut down immediately.
 	///
-	/// [`update_monitor`]: channelmonitor::ChannelMonitor::update_monitor
+	/// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager
 	fn update_channel(&self, funding_txo: OutPoint, update: &ChannelMonitorUpdate) -> ChannelMonitorUpdateStatus;
 
 	/// Returns any monitor events since the last call. Subsequent calls must only return new