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

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")
 		}
 

lightning/src/chain/chainmonitor.rs

@@ -69,14 +69,14 @@ pub struct ChannelMonitorUpdate {
 	/// increasing and increase by one for each new update, with one exception specified below.
 	///
 	/// This sequence number is also used to track up to which points updates which returned
-	/// [`ChannelMonitorUpdateErr::TemporaryFailure`] have been applied to all copies of a given
+	/// [`ChannelMonitorUpdateResult::UpdateInProgress`] have been applied to all copies of a given
 	/// ChannelMonitor when ChannelManager::channel_monitor_updated is called.
 	///
 	/// The only instance where update_id values are not strictly increasing is the case where we
 	/// allow post-force-close updates with a special update ID of [`CLOSED_CHANNEL_UPDATE_ID`]. See
 	/// its docs for more details.
 	///
-	/// [`ChannelMonitorUpdateErr::TemporaryFailure`]: super::ChannelMonitorUpdateErr::TemporaryFailure
+	/// [`ChannelMonitorUpdateResult::UpdateInProgress`]: super::ChannelMonitorUpdateResult::UpdateInProgress
 	pub update_id: u64,
 }
 
@@ -127,9 +127,9 @@ pub enum MonitorEvent {
 	CommitmentTxConfirmed(OutPoint),
 
 	/// Indicates a [`ChannelMonitor`] update has completed. See
-	/// [`ChannelMonitorUpdateErr::TemporaryFailure`] for more information on how this is used.
+	/// [`ChannelMonitorUpdateResult::UpdateInProgress`] for more information on how this is used.
 	///
-	/// [`ChannelMonitorUpdateErr::TemporaryFailure`]: super::ChannelMonitorUpdateErr::TemporaryFailure
+	/// [`ChannelMonitorUpdateResult::UpdateInProgress`]: super::ChannelMonitorUpdateResult::UpdateInProgress
 	UpdateCompleted {
 		/// The funding outpoint of the [`ChannelMonitor`] that was updated
 		funding_txo: OutPoint,
@@ -142,9 +142,9 @@ pub enum MonitorEvent {
 	},
 
 	/// Indicates a [`ChannelMonitor`] update has failed. See
-	/// [`ChannelMonitorUpdateErr::PermanentFailure`] for more information on how this is used.
+	/// [`ChannelMonitorUpdateResult::PermanentFailure`] for more information on how this is used.
 	///
-	/// [`ChannelMonitorUpdateErr::PermanentFailure`]: super::ChannelMonitorUpdateErr::PermanentFailure
+	/// [`ChannelMonitorUpdateResult::PermanentFailure`]: super::ChannelMonitorUpdateResult::PermanentFailure
 	UpdateFailed(OutPoint),
 }
 impl_writeable_tlv_based_enum_upgradable!(MonitorEvent,
@@ -1215,7 +1215,7 @@ impl<Signer: Sign> 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 [`ChannelMonitorUpdateErr::PermanentFailure`] or because we've
+	/// a monitor update failed with [`ChannelMonitorUpdateResult::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).
 	///
@@ -1225,7 +1225,7 @@ impl<Signer: Sign> ChannelMonitor<Signer> {
 	/// 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.
 	///
-	/// [`ChannelMonitorUpdateErr::PermanentFailure`]: super::ChannelMonitorUpdateErr::PermanentFailure
+	/// [`ChannelMonitorUpdateResult::PermanentFailure`]: super::ChannelMonitorUpdateResult::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)

lightning/src/chain/channelmonitor.rs

@@ -188,7 +188,13 @@ pub trait Confirm {
 
 /// An error enum representing a failure to persist a channel monitor update.
 #[derive(Clone, Copy, Debug, PartialEq)]
-pub enum ChannelMonitorUpdateErr {
+pub enum ChannelMonitorUpdateResult {
+	/// The update has been durably persisted and all copies of the relevant [`ChannelMonitor`]
+	/// have been updated.
+	///
+	/// This includes performing any `fsync()` calls required to ensure the update is guaranteed to
+	/// be available on restart even if the application crashes.
+	UpdateComplete,
 	/// Used to indicate a temporary failure (eg connection to a watchtower or remote backup of
 	/// our state failed, but is expected to succeed at some point in the future).
 	///
@@ -211,11 +217,12 @@ pub enum ChannelMonitorUpdateErr {
 	///
 	/// For deployments where a copy of ChannelMonitors and other local state are backed up in a
 	/// remote location (with local copies persisted immediately), it is anticipated that all
-	/// updates will return TemporaryFailure until the remote copies could be updated.
+	/// updates will return [`UpdateInProgress`] until the remote copies could be updated.
 	///
-	/// [`PermanentFailure`]: ChannelMonitorUpdateErr::PermanentFailure
+	/// [`PermanentFailure`]: ChannelMonitorUpdateResult::PermanentFailure
+	/// [`UpdateInProgress`]: ChannelMonitorUpdateResult::UpdateInProgress
 	/// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager
-	TemporaryFailure,
+	UpdateInProgress,
 	/// 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).
 	///
@@ -252,7 +259,7 @@ pub enum ChannelMonitorUpdateErr {
 	/// storage is used to claim outputs of rejected state confirmed onchain by another watchtower,
 	/// lagging behind on block processing.
 	///
-	/// [`PermanentFailure`]: ChannelMonitorUpdateErr::PermanentFailure
+	/// [`PermanentFailure`]: ChannelMonitorUpdateResult::PermanentFailure
 	/// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager
 	PermanentFailure,
 }
@@ -272,32 +279,32 @@ pub enum ChannelMonitorUpdateErr {
 /// 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 [`ChannelMonitorUpdateErr`] for more details about how to handle
+/// funds in the channel. See [`ChannelMonitorUpdateResult`] for more details about how to handle
 /// multiple instances.
 ///
-/// [`PermanentFailure`]: ChannelMonitorUpdateErr::PermanentFailure
+/// [`PermanentFailure`]: ChannelMonitorUpdateResult::PermanentFailure
 pub trait Watch<ChannelSigner: Sign> {
 	/// 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 [`ChannelMonitorUpdateErr::PermanentFailure`] if
+	/// Note: this interface MUST error with [`ChannelMonitorUpdateResult::PermanentFailure`] if
 	/// the given `funding_txo` has previously been registered via `watch_channel`.
 	///
 	/// [`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>) -> Result<(), ChannelMonitorUpdateErr>;
+	fn watch_channel(&self, funding_txo: OutPoint, monitor: ChannelMonitor<ChannelSigner>) -> ChannelMonitorUpdateResult;
 
 	/// Updates a channel identified by `funding_txo` by applying `update` to its monitor.
 	///
 	/// Implementations must call [`update_monitor`] with the given update. See
-	/// [`ChannelMonitorUpdateErr`] for invariants around returning an error.
+	/// [`ChannelMonitorUpdateResult`] for invariants around returning an error.
 	///
 	/// [`update_monitor`]: channelmonitor::ChannelMonitor::update_monitor
-	fn update_channel(&self, funding_txo: OutPoint, update: ChannelMonitorUpdate) -> Result<(), ChannelMonitorUpdateErr>;
+	fn update_channel(&self, funding_txo: OutPoint, update: ChannelMonitorUpdate) -> ChannelMonitorUpdateResult;
 
 	/// Returns any monitor events since the last call. Subsequent calls must only return new
 	/// events.
@@ -307,7 +314,7 @@ pub trait Watch<ChannelSigner: Sign> {
 	/// to disk.
 	///
 	/// For details on asynchronous [`ChannelMonitor`] updating and returning
-	/// [`MonitorEvent::UpdateCompleted`] here, see [`ChannelMonitorUpdateErr::TemporaryFailure`].
+	/// [`MonitorEvent::UpdateCompleted`] here, see [`ChannelMonitorUpdateResult::UpdateInProgress`].
 	fn release_pending_monitor_events(&self) -> Vec<(OutPoint, Vec<MonitorEvent>)>;
 }
 
@@ -326,9 +333,9 @@ pub trait Watch<ChannelSigner: Sign> {
 /// Note that use as part of a [`Watch`] implementation involves reentrancy. Therefore, the `Filter`
 /// should not block on I/O. Implementations should instead queue the newly monitored data to be
 /// processed later. Then, in order to block until the data has been processed, any [`Watch`]
-/// invocation that has called the `Filter` must return [`TemporaryFailure`].
+/// invocation that has called the `Filter` must return [`UpdateInProgress`].
 ///
-/// [`TemporaryFailure`]: ChannelMonitorUpdateErr::TemporaryFailure
+/// [`UpdateInProgress`]: ChannelMonitorUpdateResult::UpdateInProgress
 /// [BIP 157]: https://github.com/bitcoin/bips/blob/master/bip-0157.mediawiki
 /// [BIP 158]: https://github.com/bitcoin/bips/blob/master/bip-0158.mediawiki
 pub trait Filter {