From e5bd7920bddb4f312741673e37a07cd859fd24fb Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 8 Sep 2023 00:05:56 +0000 Subject: [PATCH] Update `ChannelMonitorUpdateStatus` documentation with async support Since we now (almost) support async monitor update persistence, the documentation on `ChannelMonitorUpdateStatus` can be updated to no longer suggest users must keep a local copy that persists before returning. However, because there are still a few remaining issues, we note that async support is currently beta and explicily warn of potential for funds-loss. Fixes #1684 --- lightning/src/chain/chainmonitor.rs | 4 +-- lightning/src/chain/mod.rs | 44 ++++++++++++++++++----------- lightning/src/ln/channelmanager.rs | 14 +++++---- 3 files changed, 37 insertions(+), 25 deletions(-) diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs index 4e349799..4986e054 100644 --- a/lightning/src/chain/chainmonitor.rs +++ b/lightning/src/chain/chainmonitor.rs @@ -82,8 +82,8 @@ impl MonitorUpdateId { /// * If persistence (including any relevant `fsync()` calls) happens immediately, the /// implementation should return [`ChannelMonitorUpdateStatus::Completed`], indicating normal /// channel operation should continue. -/// * If persistence happens asynchronously, implementations should first ensure the -/// [`ChannelMonitor`] or [`ChannelMonitorUpdate`] are written durably to disk, and then return +/// +/// * If persistence happens asynchronously, implementations can return /// [`ChannelMonitorUpdateStatus::InProgress`] while the update continues in the background. /// Once the update completes, [`ChainMonitor::channel_monitor_updated`] should be called with /// the corresponding [`MonitorUpdateId`]. diff --git a/lightning/src/chain/mod.rs b/lightning/src/chain/mod.rs index 736da139..1abaa77f 100644 --- a/lightning/src/chain/mod.rs +++ b/lightning/src/chain/mod.rs @@ -176,6 +176,18 @@ pub trait Confirm { } /// An enum representing the status of a channel monitor update persistence. +/// +/// Note that there is no error variant - any failure to persist a [`ChannelMonitor`] should be +/// retried indefinitely, the node shut down (as if we cannot update stored data we can't do much +/// of anything useful). +/// +/// Note that channels should generally *not* be force-closed after a persistence failure. +/// Force-closing with the latest [`ChannelMonitorUpdate`] applied may result in a transaction +/// being broadcast which can only be spent by the latest [`ChannelMonitor`]! Thus, if the +/// latest [`ChannelMonitor`] is not durably persisted anywhere and exists only in memory, naively +/// calling [`ChannelManager::force_close_broadcasting_latest_txn`] *may result in loss of funds*! +/// +/// [`ChannelManager::force_close_broadcasting_latest_txn`]: crate::ln::channelmanager::ChannelManager::force_close_broadcasting_latest_txn #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum ChannelMonitorUpdateStatus { /// The update has been durably persisted and all copies of the relevant [`ChannelMonitor`] @@ -184,13 +196,13 @@ pub enum ChannelMonitorUpdateStatus { /// This includes performing any `fsync()` calls required to ensure the update is guaranteed to /// be available on restart even if the application crashes. Completed, - /// 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). + /// Indicates that the update will happen asynchronously in the background or that a transient + /// failure occurred which is being retried in the background and will eventually complete. /// - /// Such a failure will "freeze" a channel, preventing us from revoking old states or - /// submitting new commitment transactions to the counterparty. Once the update(s) which failed - /// have been successfully applied, a [`MonitorEvent::Completed`] can be used to restore the - /// channel to an operational state. + /// This will "freeze" a channel, preventing us from revoking old states or submitting a new + /// commitment transaction to the counterparty. Once the update(s) which are `InProgress` have + /// been completed, a [`MonitorEvent::Completed`] can be used to restore the channel to an + /// operational state. /// /// 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 @@ -204,6 +216,10 @@ 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. /// + /// Note that while fully asynchronous persistence of [`ChannelMonitor`] data is generally + /// reliable, this feature is considered beta, and a handful of edge-cases remain. Until the + /// remaining cases are fixed, in rare cases, *using this feature may lead to funds loss*. + /// /// [`InProgress`]: ChannelMonitorUpdateStatus::InProgress InProgress, } @@ -212,18 +228,12 @@ pub enum ChannelMonitorUpdateStatus { /// blocks are connected and disconnected. /// /// Each channel is associated with a [`ChannelMonitor`]. Implementations of this trait are -/// responsible for maintaining a set of monitors such that they can be updated accordingly as -/// channel state changes and HTLCs are resolved. See method documentation for specific -/// requirements. -/// -/// Implementations **must** ensure that updates are successfully applied and persisted upon method -/// completion. If an update will not succeed, then it must immediately shut down. +/// responsible for maintaining a set of monitors such that they can be updated as channel state +/// changes. On each update, *all copies* of a [`ChannelMonitor`] must be updated and the update +/// persisted to disk to ensure that the latest [`ChannelMonitor`] state can be reloaded if the +/// application crashes. /// -/// 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. +/// See method documentation and [`ChannelMonitorUpdateStatus`] for specific requirements. pub trait Watch { /// Watches a channel identified by `funding_txo` using `monitor`. /// diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 45873631..a7e7375f 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -923,12 +923,14 @@ where /// called [`funding_transaction_generated`] for outbound channels) being closed. /// /// Note that you can be a bit lazier about writing out `ChannelManager` than you can be with -/// [`ChannelMonitor`]. With [`ChannelMonitor`] you MUST write each monitor update out to disk before -/// returning from [`chain::Watch::watch_channel`]/[`update_channel`], with ChannelManagers, writing updates -/// happens out-of-band (and will prevent any other `ChannelManager` operations from occurring during -/// the serialization process). If the deserialized version is out-of-date compared to the -/// [`ChannelMonitor`] passed by reference to [`read`], those channels will be force-closed based on the -/// `ChannelMonitor` state and no funds will be lost (mod on-chain transaction fees). +/// [`ChannelMonitor`]. With [`ChannelMonitor`] you MUST durably write each +/// [`ChannelMonitorUpdate`] before returning from +/// [`chain::Watch::watch_channel`]/[`update_channel`] or before completing async writes. With +/// `ChannelManager`s, writing updates happens out-of-band (and will prevent any other +/// `ChannelManager` operations from occurring during the serialization process). If the +/// deserialized version is out-of-date compared to the [`ChannelMonitor`] passed by reference to +/// [`read`], those channels will be force-closed based on the `ChannelMonitor` state and no funds +/// will be lost (modulo on-chain transaction fees). /// /// Note that the deserializer is only implemented for `(`[`BlockHash`]`, `[`ChannelManager`]`)`, which /// tells you the last block hash which was connected. You should get the best block tip before using the manager. -- 2.30.2