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.

Author: wpaulino

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

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

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 }],