Use CLOSED_CHANNEL_UPDATE_ID in force closing ChannelMonitorUpdates

Currently, all that is required to force close a channel is to broadcast
either of the available commitment transactions, but this changes with
anchor outputs – commitment transactions may need to have
additional fees attached in order to confirm in a timely manner. While
we may be able to just queue a new update using the channel's next
available update ID, this may result in a violation of the
`ChannelMonitor` API (each update ID must strictly increase by 1) if the
channel had updates that were persisted by its `ChannelMonitor`, but not
the `ChannelManager`. Therefore, we choose to re-purpose the existing
`CLOSED_CHANNEL_UPDATE_ID` update ID to also apply to
`ChannelMonitorUpdate`s that will force close their respective channel
by broadcasting the holder's latest commitment transaction.

diff --git a/lightning-persister/src/lib.rs b/lightning-persister/src/lib.rs
index 488378576e5c24ca22bcc5b5de99514e94c8af6a..01cb4081ccc745d7775b86db079dcf25d49610ec 100644 (file)
--- a/lightning-persister/src/lib.rs
+++ b/lightning-persister/src/lib.rs
@@ -141,6 +141,7 @@ mod tests {
        use bitcoin::{Txid, TxMerkleNode};
        use lightning::chain::ChannelMonitorUpdateStatus;
        use lightning::chain::chainmonitor::Persist;
+       use lightning::chain::channelmonitor::CLOSED_CHANNEL_UPDATE_ID;
        use lightning::chain::transaction::OutPoint;
        use lightning::{check_closed_broadcast, check_closed_event, check_added_monitors};
        use lightning::ln::functional_test_utils::*;
@@ -253,7 +254,7 @@ mod tests {
                check_added_monitors!(nodes[1], 1);
 
                // Make sure everything is persisted as expected after close.
-               check_persisted_data!(11);
+               check_persisted_data!(CLOSED_CHANNEL_UPDATE_ID);
        }
 
        // Test that if the persister's path to channel data is read-only, writing a

diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index 1ebb65534b3f3d34af093e0c11260aa723bcc2ce..cd6b4c941a9e9b009992e5a37b1d34d210cc2c03 100644 (file)
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -76,27 +76,30 @@ pub struct ChannelMonitorUpdate {
        pub(crate) updates: Vec<ChannelMonitorUpdateStep>,
        /// The sequence number of this update. Updates *must* be replayed in-order according to this
        /// sequence number (and updates may panic if they are not). The update_id values are strictly
-       /// increasing and increase by one for each new update, with one exception specified below.
+       /// increasing and increase by one for each new update, with two exceptions specified below.
        ///
        /// This sequence number is also used to track up to which points updates which returned
        /// [`ChannelMonitorUpdateStatus::InProgress`] 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.
+       /// The only instances we allow where update_id values are not strictly increasing have a
+       /// special update ID of [`CLOSED_CHANNEL_UPDATE_ID`]. This update ID is used for updates that
+       /// will force close the channel by broadcasting the latest commitment transaction or
+       /// special post-force-close updates, like providing preimages necessary to claim outputs on the
+       /// broadcast commitment transaction. See its docs for more details.
        ///
        /// [`ChannelMonitorUpdateStatus::InProgress`]: super::ChannelMonitorUpdateStatus::InProgress
        pub update_id: u64,
 }
 
-/// If:
-///    (1) a channel has been force closed and
-///    (2) we receive a preimage from a forward link that allows us to spend an HTLC output on
-///        this channel's (the backward link's) broadcasted commitment transaction
-/// then we allow the `ChannelManager` to send a `ChannelMonitorUpdate` with this update ID,
-/// with the update providing said payment preimage. No other update types are allowed after
-/// force-close.
+/// The update ID used for a [`ChannelMonitorUpdate`] that is either:
+///
+///    (1) attempting to force close the channel by broadcasting our latest commitment transaction or
+///    (2) providing a preimage (after the channel has been force closed) from a forward link that
+///            allows us to spend an HTLC output on this channel's (the backward link's) broadcasted
+///            commitment transaction.
+///
+/// No other [`ChannelMonitorUpdate`]s are allowed after force-close.
 pub const CLOSED_CHANNEL_UPDATE_ID: u64 = core::u64::MAX;
 
 impl Writeable for ChannelMonitorUpdate {
@@ -2272,7 +2275,11 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
                if updates.update_id == CLOSED_CHANNEL_UPDATE_ID {
                        assert_eq!(updates.updates.len(), 1);
                        match updates.updates[0] {
-                               ChannelMonitorUpdateStep::PaymentPreimage { .. } => {},
+                               ChannelMonitorUpdateStep::ChannelForceClosed { .. } => {},
+                               // We should have already seen a `ChannelForceClosed` update if we're trying to
+                               // provide a preimage at this point.
+                               ChannelMonitorUpdateStep::PaymentPreimage { .. } =>
+                                       debug_assert_eq!(self.latest_update_id, CLOSED_CHANNEL_UPDATE_ID),
                                _ => {
                                        log_error!(logger, "Attempted to apply post-force-close ChannelMonitorUpdate of type {}", updates.updates[0].variant_name());
                                        panic!("Attempted to apply post-force-close ChannelMonitorUpdate that wasn't providing a payment preimage");

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 7a389b573e6862d2ead48613e95d8356b0f7dc1c..ae45f5e8fb61550880ee81336953cd7d9d69c078 100644 (file)
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -33,7 +33,7 @@ use crate::ln::chan_utils;
 use crate::ln::onion_utils::HTLCFailReason;
 use crate::chain::BestBlock;
 use crate::chain::chaininterface::{FeeEstimator, ConfirmationTarget, LowerBoundedFeeEstimator};
-use crate::chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateStep, LATENCY_GRACE_PERIOD_BLOCKS};
+use crate::chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateStep, LATENCY_GRACE_PERIOD_BLOCKS, CLOSED_CHANNEL_UPDATE_ID};
 use crate::chain::transaction::{OutPoint, TransactionData};
 use crate::chain::keysinterface::{WriteableEcdsaChannelSigner, EntropySource, ChannelSigner, SignerProvider, NodeSigner, Recipient};
 use crate::routing::gossip::NodeId;
@@ -6081,7 +6081,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
                        // monitor update to the user, even if we return one).
                        // See test_duplicate_chan_id and test_pre_lockin_no_chan_closed_update for more.
                        if self.channel_state & (ChannelState::FundingSent as u32 | ChannelState::ChannelReady as u32 | ChannelState::ShutdownComplete as u32) != 0 {
-                               self.latest_monitor_update_id += 1;
+                               self.latest_monitor_update_id = CLOSED_CHANNEL_UPDATE_ID;
                                Some((funding_txo, ChannelMonitorUpdate {
                                        update_id: self.latest_monitor_update_id,
                                        updates: vec![ChannelMonitorUpdateStep::ChannelForceClosed { should_broadcast }],