X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=blobdiff_plain;f=lightning%2Fsrc%2Fchain%2Fchainmonitor.rs;h=e87d082d9a7bb9061894ce26fb15da0087a7c173;hb=3f416bc24e0e804e0cae1c8c5650b19500122b6d;hp=a49b3b426872df0a7133e93a6c9eb1421c29a389;hpb=1ac53ed02bf26520d3b1a2c3c0e90c8691c83099;p=rust-lightning

diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs
index a49b3b42..e87d082d 100644
--- 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 {
@@ -98,7 +110,7 @@ impl MonitorUpdateId {
 ///    If at some point no further progress can be made towards persisting the pending updates, the
 ///    node should simply shut down.
 ///
-///  * If the persistence has failed and cannot be retried further (e.g. because of some timeout),
+///  * If the persistence has failed and cannot be retried further (e.g. because of an outage),
 ///    [`ChannelMonitorUpdateStatus::UnrecoverableError`] can be used, though this will result in
 ///    an immediate panic and future operations in LDK generally failing.
 ///
@@ -113,7 +125,10 @@ impl MonitorUpdateId {
 ///  [`ChainMonitor::channel_monitor_updated`] must be called once for *each* update which occurs.
 ///
 ///  If at some point no further progress can be made towards persisting a pending update, the node
-///  should simply shut down.
+///  should simply shut down. Until then, the background task should either loop indefinitely, or
+///  persistence should be regularly retried with [`ChainMonitor::list_pending_monitor_updates`]
+///  and [`ChainMonitor::get_monitor`] (note that if a full monitor is persisted all pending
+///  monitor updates may be marked completed).
 ///
 /// # Using remote watchtowers
 ///
@@ -152,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.
@@ -309,7 +324,6 @@ where C::Target: chain::Filter,
 				if self.update_monitor_with_chain_data(header, best_height, txdata, &process, funding_outpoint, &monitor_state).is_err() {
 					// Take the monitors lock for writing so that we poison it and any future
 					// operations going forward fail immediately.
-					core::mem::drop(monitor_state);
 					core::mem::drop(monitor_lock);
 					let _poison = self.monitors.write().unwrap();
 					log_error!(self.logger, "{}", err_str);
@@ -420,7 +434,8 @@ where C::Target: chain::Filter,
 	/// claims which are awaiting confirmation.
 	///
 	/// Includes the balances from each [`ChannelMonitor`] *except* those included in
-	/// `ignored_channels`.
+	/// `ignored_channels`, allowing you to filter out balances from channels which are still open
+	/// (and whose balance should likely be pulled from the [`ChannelDetails`]).
 	///
 	/// See [`ChannelMonitor::get_claimable_balances`] for more details on the exact criteria for
 	/// inclusion in the return value.
@@ -751,15 +766,21 @@ where C::Target: chain::Filter,
 			Some(monitor_state) => {
 				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_res = monitor.update_monitor(update, &self.broadcaster, &self.fee_estimator, &self.logger);
+
 				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);