Merge pull request #3619 from TheBlueMatt/2025-02-merge-chanmon-lockdown

Only generate a post-close lock ChannelMonitorUpdate if we need one

Author: valentinewallace

lightning/src/chain/channelmonitor.rs

@@ -1536,8 +1536,8 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 	fn provide_latest_holder_commitment_tx(
 		&self, holder_commitment_tx: HolderCommitmentTransaction,
 		htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<HTLCSource>)>,
-	) -> Result<(), ()> {
-		self.inner.lock().unwrap().provide_latest_holder_commitment_tx(holder_commitment_tx, htlc_outputs, &Vec::new(), Vec::new()).map_err(|_| ())
+	) {
+		self.inner.lock().unwrap().provide_latest_holder_commitment_tx(holder_commitment_tx, htlc_outputs, &Vec::new(), Vec::new())
 	}
 
 	/// This is used to provide payment preimage(s) out-of-band during startup without updating the
@@ -1774,10 +1774,14 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 		self.inner.lock().unwrap().get_cur_holder_commitment_number()
 	}
 
-	/// Gets whether we've been notified that this channel is closed by the `ChannelManager` (i.e.
-	/// via a [`ChannelMonitorUpdateStep::ChannelForceClosed`]).
-	pub(crate) fn offchain_closed(&self) -> bool {
-		self.inner.lock().unwrap().lockdown_from_offchain
+	/// Fetches whether this monitor has marked the channel as closed and will refuse any further
+	/// updates to the commitment transactions.
+	///
+	/// It can be marked closed in a few different ways, including via a
+	/// [`ChannelMonitorUpdateStep::ChannelForceClosed`] or if the channel has been closed
+	/// on-chain.
+	pub(crate) fn no_further_updates_allowed(&self) -> bool {
+		self.inner.lock().unwrap().no_further_updates_allowed()
 	}
 
 	/// Gets the `node_id` of the counterparty for this channel.
@@ -2938,7 +2942,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 	/// is important that any clones of this channel monitor (including remote clones) by kept
 	/// up-to-date as our holder commitment transaction is updated.
 	/// Panics if set_on_holder_tx_csv has never been called.
-	fn provide_latest_holder_commitment_tx(&mut self, holder_commitment_tx: HolderCommitmentTransaction, mut htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<HTLCSource>)>, claimed_htlcs: &[(SentHTLCId, PaymentPreimage)], nondust_htlc_sources: Vec<HTLCSource>) -> Result<(), &'static str> {
+	fn provide_latest_holder_commitment_tx(&mut self, holder_commitment_tx: HolderCommitmentTransaction, mut htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<HTLCSource>)>, claimed_htlcs: &[(SentHTLCId, PaymentPreimage)], nondust_htlc_sources: Vec<HTLCSource>) {
 		if htlc_outputs.iter().any(|(_, s, _)| s.is_some()) {
 			// If we have non-dust HTLCs in htlc_outputs, ensure they match the HTLCs in the
 			// `holder_commitment_tx`. In the future, we'll no longer provide the redundant data
@@ -3015,10 +3019,6 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 			}
 			self.counterparty_fulfilled_htlcs.insert(*claimed_htlc_id, *claimed_preimage);
 		}
-		if self.holder_tx_signed {
-			return Err("Latest holder commitment signed has already been signed, update is rejected");
-		}
-		Ok(())
 	}
 
 	/// Provides a payment_hash->payment_preimage mapping. Will be automatically pruned when all
@@ -3239,11 +3239,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 				ChannelMonitorUpdateStep::LatestHolderCommitmentTXInfo { commitment_tx, htlc_outputs, claimed_htlcs, nondust_htlc_sources } => {
 					log_trace!(logger, "Updating ChannelMonitor with latest holder commitment transaction info");
 					if self.lockdown_from_offchain { panic!(); }
-					if let Err(e) = self.provide_latest_holder_commitment_tx(commitment_tx.clone(), htlc_outputs.clone(), &claimed_htlcs, nondust_htlc_sources.clone()) {
-						log_error!(logger, "Providing latest holder commitment transaction failed/was refused:");
-						log_error!(logger, "    {}", e);
-						ret = Err(());
-					}
+					self.provide_latest_holder_commitment_tx(commitment_tx.clone(), htlc_outputs.clone(), &claimed_htlcs, nondust_htlc_sources.clone());
 				}
 				ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo { commitment_txid, htlc_outputs, commitment_number, their_per_commitment_point, .. } => {
 					log_trace!(logger, "Updating ChannelMonitor with latest counterparty commitment transaction info");
@@ -3323,12 +3319,16 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 			}
 		}
 
-		if ret.is_ok() && (self.funding_spend_seen || self.lockdown_from_offchain || self.holder_tx_signed) && is_pre_close_update {
+		if ret.is_ok() && self.no_further_updates_allowed() && is_pre_close_update {
 			log_error!(logger, "Refusing Channel Monitor Update as counterparty attempted to update commitment after funding was spent");
 			Err(())
 		} else { ret }
 	}
 
+	fn no_further_updates_allowed(&self) -> bool {
+		self.funding_spend_seen || self.lockdown_from_offchain || self.holder_tx_signed
+	}
+
 	fn get_latest_update_id(&self) -> u64 {
 		self.latest_update_id
 	}
@@ -3918,35 +3918,32 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 				}
 			}
 		}
-		if self.holder_tx_signed {
-			// If we've signed, we may have broadcast either commitment (prev or current), and
-			// attempted to claim from it immediately without waiting for a confirmation.
-			if self.current_holder_commitment_tx.txid != *confirmed_commitment_txid {
+		// Cancel any pending claims for any holder commitments in case they had previously
+		// confirmed or been signed (in which case we will start attempting to claim without
+		// waiting for confirmation).
+		if self.current_holder_commitment_tx.txid != *confirmed_commitment_txid {
+			log_trace!(logger, "Canceling claims for previously broadcast holder commitment {}",
+				self.current_holder_commitment_tx.txid);
+			let mut outpoint = BitcoinOutPoint { txid: self.current_holder_commitment_tx.txid, vout: 0 };
+			for (htlc, _, _) in &self.current_holder_commitment_tx.htlc_outputs {
+				if let Some(vout) = htlc.transaction_output_index {
+					outpoint.vout = vout;
+					self.onchain_tx_handler.abandon_claim(&outpoint);
+				}
+			}
+		}
+		if let Some(prev_holder_commitment_tx) = &self.prev_holder_signed_commitment_tx {
+			if prev_holder_commitment_tx.txid != *confirmed_commitment_txid {
 				log_trace!(logger, "Canceling claims for previously broadcast holder commitment {}",
-					self.current_holder_commitment_tx.txid);
-				let mut outpoint = BitcoinOutPoint { txid: self.current_holder_commitment_tx.txid, vout: 0 };
-				for (htlc, _, _) in &self.current_holder_commitment_tx.htlc_outputs {
+					prev_holder_commitment_tx.txid);
+				let mut outpoint = BitcoinOutPoint { txid: prev_holder_commitment_tx.txid, vout: 0 };
+				for (htlc, _, _) in &prev_holder_commitment_tx.htlc_outputs {
 					if let Some(vout) = htlc.transaction_output_index {
 						outpoint.vout = vout;
 						self.onchain_tx_handler.abandon_claim(&outpoint);
 					}
 				}
 			}
-			if let Some(prev_holder_commitment_tx) = &self.prev_holder_signed_commitment_tx {
-				if prev_holder_commitment_tx.txid != *confirmed_commitment_txid {
-					log_trace!(logger, "Canceling claims for previously broadcast holder commitment {}",
-						prev_holder_commitment_tx.txid);
-					let mut outpoint = BitcoinOutPoint { txid: prev_holder_commitment_tx.txid, vout: 0 };
-					for (htlc, _, _) in &prev_holder_commitment_tx.htlc_outputs {
-						if let Some(vout) = htlc.transaction_output_index {
-							outpoint.vout = vout;
-							self.onchain_tx_handler.abandon_claim(&outpoint);
-						}
-					}
-				}
-			}
-		} else {
-			// No previous claim.
 		}
 	}
 
@@ -4282,7 +4279,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 			}
 		}
 
-		if self.lockdown_from_offchain || self.funding_spend_seen || self.holder_tx_signed {
+		if self.no_further_updates_allowed() {
 			// Fail back HTLCs on backwards channels if they expire within
 			// `LATENCY_GRACE_PERIOD_BLOCKS` blocks and the channel is closed (i.e. we're at a
 			// point where no further off-chain updates will be accepted). If we haven't seen the
@@ -5440,7 +5437,7 @@ mod tests {
 		let dummy_commitment_tx = HolderCommitmentTransaction::dummy(0, &mut htlcs);
 
 		monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx.clone(),
-			htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), None)).collect()).unwrap();
+			htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), None)).collect());
 		monitor.provide_latest_counterparty_commitment_tx(Txid::from_byte_array(Sha256::hash(b"1").to_byte_array()),
 			preimages_slice_to_htlc_outputs!(preimages[5..15]), 281474976710655, dummy_key, &logger);
 		monitor.provide_latest_counterparty_commitment_tx(Txid::from_byte_array(Sha256::hash(b"2").to_byte_array()),
@@ -5478,7 +5475,7 @@ mod tests {
 		let mut htlcs = preimages_slice_to_htlcs!(preimages[0..5]);
 		let dummy_commitment_tx = HolderCommitmentTransaction::dummy(0, &mut htlcs);
 		monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx.clone(),
-			htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), None)).collect()).unwrap();
+			htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), None)).collect());
 		secret[0..32].clone_from_slice(&<Vec<u8>>::from_hex("2273e227a5b7449b6e70f1fb4652864038b1cbf9cd7c043a7d6456b7fc275ad8").unwrap());
 		monitor.provide_secret(281474976710653, secret.clone()).unwrap();
 		assert_eq!(monitor.inner.lock().unwrap().payment_preimages.len(), 12);
@@ -5489,7 +5486,7 @@ mod tests {
 		let mut htlcs = preimages_slice_to_htlcs!(preimages[0..3]);
 		let dummy_commitment_tx = HolderCommitmentTransaction::dummy(0, &mut htlcs);
 		monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx,
-			htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), None)).collect()).unwrap();
+			htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), None)).collect());
 		secret[0..32].clone_from_slice(&<Vec<u8>>::from_hex("27cddaa5624534cb6cb9d7da077cf2b22ab21e9b506fd4998a51d54502e99116").unwrap());
 		monitor.provide_secret(281474976710652, secret.clone()).unwrap();
 		assert_eq!(monitor.inner.lock().unwrap().payment_preimages.len(), 5);

lightning/src/ln/channelmanager.rs

@@ -13763,8 +13763,8 @@ where
 					// claim.
 					// Note that a `ChannelMonitor` is created with `update_id` 0 and after we
 					// provide it with a closure update its `update_id` will be at 1.
-					if !monitor.offchain_closed() || monitor.get_latest_update_id() > 1 {
-						should_queue_fc_update = !monitor.offchain_closed();
+					if !monitor.no_further_updates_allowed() || monitor.get_latest_update_id() > 1 {
+						should_queue_fc_update = !monitor.no_further_updates_allowed();
 						let mut latest_update_id = monitor.get_latest_update_id();
 						if should_queue_fc_update {
 							latest_update_id += 1;