Merge pull request #2621 from G8XSU/dont-persist-erroneous-update

[rust-lightning] / lightning / src / chain / chainmonitor.rs

diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs
index cf51dbab72019d9eb87748e0817171c1c0abfb35..261c0471ca4089dfaa1d21ca0e8c8f810f90c7a5 100644 (file)
--- a/lightning/src/chain/chainmonitor.rs
+++ b/lightning/src/chain/chainmonitor.rs
@@ -47,23 +47,35 @@ use core::ops::Deref;
 use core::sync::atomic::{AtomicUsize, Ordering};
 use bitcoin::secp256k1::PublicKey;
 
-#[derive(Debug, Clone, Copy, Hash, PartialEq, Eq)]
-/// A specific update's ID stored in a `MonitorUpdateId`, separated out to make the contents
-/// entirely opaque.
-enum UpdateOrigin {
-       /// An update that was generated by the `ChannelManager` (via our `chain::Watch`
-       /// implementation). This corresponds to an actual [`ChannelMonitorUpdate::update_id`] field
-       /// and [`ChannelMonitor::get_latest_update_id`].
-       OffChain(u64),
-       /// An update that was generated during blockchain processing. The ID here is specific to the
-       /// generating [`ChainMonitor`] and does *not* correspond to any on-disk IDs.
-       ChainSync(u64),
+mod update_origin {
+       #[derive(Debug, Clone, Copy, Hash, PartialEq, Eq)]
+       /// A specific update's ID stored in a `MonitorUpdateId`, separated out to make the contents
+       /// entirely opaque.
+       pub(crate) enum UpdateOrigin {
+               /// An update that was generated by the `ChannelManager` (via our [`crate::chain::Watch`]
+               /// implementation). This corresponds to an actual [ChannelMonitorUpdate::update_id] field
+               /// and [ChannelMonitor::get_latest_update_id].
+               ///
+               /// [ChannelMonitor::get_latest_update_id]: crate::chain::channelmonitor::ChannelMonitor::get_latest_update_id
+               /// [ChannelMonitorUpdate::update_id]: crate::chain::channelmonitor::ChannelMonitorUpdate::update_id
+               OffChain(u64),
+               /// An update that was generated during blockchain processing. The ID here is specific to the
+               /// generating [ChannelMonitor] and does *not* correspond to any on-disk IDs.
+               ///
+               /// [ChannelMonitor]: crate::chain::channelmonitor::ChannelMonitor
+               ChainSync(u64),
+       }
 }
 
+#[cfg(any(feature = "_test_utils", test))]
+pub(crate) use update_origin::UpdateOrigin;
+#[cfg(not(any(feature = "_test_utils", test)))]
+use update_origin::UpdateOrigin;
+
 /// An opaque identifier describing a specific [`Persist`] method call.
 #[derive(Debug, Clone, Copy, Hash, PartialEq, Eq)]
 pub struct MonitorUpdateId {
-       contents: UpdateOrigin,
+       pub(crate) contents: UpdateOrigin,
 }
 
 impl MonitorUpdateId {
@@ -155,8 +167,8 @@ pub trait Persist<ChannelSigner: WriteableEcdsaChannelSigner> {
        /// updated monitor itself to disk/backups. See the [`Persist`] trait documentation for more
        /// details.
        ///
-       /// During blockchain synchronization operations, this may be called with no
-       /// [`ChannelMonitorUpdate`], in which case the full [`ChannelMonitor`] needs to be persisted.
+       /// During blockchain synchronization operations, and in some rare cases, this may be called with
+       /// no [`ChannelMonitorUpdate`], in which case the full [`ChannelMonitor`] needs to be persisted.
        /// Note that after the full [`ChannelMonitor`] is persisted any previous
        /// [`ChannelMonitorUpdate`]s which were persisted should be discarded - they can no longer be
        /// applied to the persisted [`ChannelMonitor`] as they were already applied.
@@ -756,14 +768,20 @@ where C::Target: chain::Filter,
                                let monitor = &monitor_state.monitor;
                                log_trace!(self.logger, "Updating ChannelMonitor for channel {}", log_funding_info!(monitor));
                                let update_res = monitor.update_monitor(update, &self.broadcaster, &*self.fee_estimator, &self.logger);
-                               if update_res.is_err() {
-                                       log_error!(self.logger, "Failed to update ChannelMonitor for channel {}.", log_funding_info!(monitor));
-                               }
-                               // Even if updating the monitor returns an error, the monitor's state will
-                               // still be changed. So, persist the updated monitor despite the error.
+
                                let update_id = MonitorUpdateId::from_monitor_update(update);
                                let mut pending_monitor_updates = monitor_state.pending_monitor_updates.lock().unwrap();
-                               let persist_res = self.persister.update_persisted_channel(funding_txo, Some(update), monitor, update_id);
+                               let persist_res = if update_res.is_err() {
+                                       // Even if updating the monitor returns an error, the monitor's state will
+                                       // still be changed. Therefore, we should persist the updated monitor despite the error.
+                                       // We don't want to persist a `monitor_update` which results in a failure to apply later
+                                       // while reading `channel_monitor` with updates from storage. Instead, we should persist
+                                       // the entire `channel_monitor` here.
+                                       log_warn!(self.logger, "Failed to update ChannelMonitor for channel {}. Going ahead and persisting the entire ChannelMonitor", log_funding_info!(monitor));
+                                       self.persister.update_persisted_channel(funding_txo, None, monitor, update_id)
+                               } else {
+                                       self.persister.update_persisted_channel(funding_txo, Some(update), monitor, update_id)
+                               };
                                match persist_res {
                                        ChannelMonitorUpdateStatus::InProgress => {
                                                pending_monitor_updates.push(update_id);