Detect commitment transaction confirmation in ChannelMonitor instead

Previously, the `ChannelManager` would assume a `Channel` was closed the
moment it saw a spend for its funding input. With splicing, this will no
longer be the case. Since the `ChannelMonitor` is already responsible
for reliably tracking each onchain transaction relevant to a channel, we
now produce a `MonitorEvent::CommitmentTxConfirmed` event to inform the
`ChannelManager` the channel can be considered closed and removed. Note
that this now makes it possible for a `Channel` to have an update while
the event has yet to be processed by the `ChannelManager`, but this is
still safe as the monitor should reject it, since it's seen a spending
transaction confirm, causing the channel to fail.

As a result of this change, many tests failed now that we rely on
handling the `MonitorEvent::CommitmentTxConfirmed` first before seeing
the `ChannelMonitorUpdateStep::ChannelForceClosed` go out.

Author: wpaulino

lightning/src/chain/chainmonitor.rs

@@ -1490,8 +1490,8 @@ mod tests {
 		assert_eq!(close_tx.len(), 1);
 
 		mine_transaction(&nodes[2], &close_tx[0]);
-		check_added_monitors(&nodes[2], 1);
 		check_closed_broadcast(&nodes[2], 1, true);
+		check_added_monitors(&nodes[2], 1);
 		let closure_reason = ClosureReason::CommitmentTxConfirmed;
 		check_closed_event!(&nodes[2], 1, closure_reason, false, [node_a_id], 1000000);
 

lightning/src/chain/channelmonitor.rs

@@ -205,6 +205,10 @@ pub enum MonitorEvent {
 	/// channel.
 	HolderForceClosed(OutPoint),
 
+	/// Indicates that we've detected a commitment transaction (either holder's or counterparty's)
+	/// be included in a block and should consider the channel closed.
+	CommitmentTxConfirmed(()),
+
 	/// Indicates a [`ChannelMonitor`] update has completed. See
 	/// [`ChannelMonitorUpdateStatus::InProgress`] for more information on how this is used.
 	///
@@ -236,6 +240,7 @@ impl_writeable_tlv_based_enum_upgradable_legacy!(MonitorEvent,
 		(4, channel_id, required),
 	},
 ;
+	(1, CommitmentTxConfirmed),
 	(2, HTLCEvent),
 	(4, HolderForceClosed),
 	// 6 was `UpdateFailed` until LDK 0.0.117
@@ -5088,6 +5093,9 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 					);
 					log_info!(logger, "Channel {} closed by funding output spend in txid {txid}",
 						self.channel_id());
+					if !self.funding_spend_seen {
+						self.pending_monitor_events.push(MonitorEvent::CommitmentTxConfirmed(()));
+					}
 					self.funding_spend_seen = true;
 
 					let mut balance_spendable_csv = None;
@@ -6369,6 +6377,7 @@ mod tests {
 	use bitcoin::{Sequence, Witness};
 
 	use crate::chain::chaininterface::LowerBoundedFeeEstimator;
+	use crate::events::ClosureReason;
 
 	use super::ChannelMonitorUpdateStep;
 	use crate::chain::channelmonitor::{ChannelMonitor, WithChannelMonitor};
@@ -6466,22 +6475,26 @@ mod tests {
 		// Build a new ChannelMonitorUpdate which contains both the failing commitment tx update
 		// and provides the claim preimages for the two pending HTLCs. The first update generates
 		// an error, but the point of this test is to ensure the later updates are still applied.
-		let monitor_updates = nodes[1].chain_monitor.monitor_updates.lock().unwrap();
-		let mut replay_update = monitor_updates.get(&channel.2).unwrap().iter().next_back().unwrap().clone();
-		assert_eq!(replay_update.updates.len(), 1);
-		if let ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo { .. } = replay_update.updates[0] {
-		} else { panic!(); }
-		replay_update.updates.push(ChannelMonitorUpdateStep::PaymentPreimage {
-			payment_preimage: payment_preimage_1, payment_info: None,
-		});
-		replay_update.updates.push(ChannelMonitorUpdateStep::PaymentPreimage {
-			payment_preimage: payment_preimage_2, payment_info: None,
-		});
+		let replay_update = {
+			let monitor_updates = nodes[1].chain_monitor.monitor_updates.lock().unwrap();
+			let mut replay_update = monitor_updates.get(&channel.2).unwrap().iter().next_back().unwrap().clone();
+			assert_eq!(replay_update.updates.len(), 1);
+			if let ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo { .. } = replay_update.updates[0] {
+			} else { panic!(); }
+			replay_update.updates.push(ChannelMonitorUpdateStep::PaymentPreimage {
+				payment_preimage: payment_preimage_1, payment_info: None,
+			});
+			replay_update.updates.push(ChannelMonitorUpdateStep::PaymentPreimage {
+				payment_preimage: payment_preimage_2, payment_info: None,
+			});
+			replay_update
+		};
 
 		let broadcaster = TestBroadcaster::with_blocks(Arc::clone(&nodes[1].blocks));
 		assert!(
 			pre_update_monitor.update_monitor(&replay_update, &&broadcaster, &&chanmon_cfgs[1].fee_estimator, &nodes[1].logger)
 			.is_err());
+
 		// Even though we error'd on the first update, we should still have generated an HTLC claim
 		// transaction
 		let txn_broadcasted = broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
@@ -6493,7 +6506,12 @@ mod tests {
 		assert_eq!(htlc_txn.len(), 2);
 		check_spends!(htlc_txn[0], broadcast_tx);
 		check_spends!(htlc_txn[1], broadcast_tx);
+
+		check_closed_broadcast(&nodes[1], 1, true);
+		check_closed_event(&nodes[1], 1, ClosureReason::CommitmentTxConfirmed, false, &[nodes[0].node.get_our_node_id()], 100000);
+		check_added_monitors(&nodes[1], 1);
 	}
+
 	#[test]
 	fn test_funding_spend_refuses_updates() {
 		do_test_funding_spend_refuses_updates(true);

lightning/src/ln/async_signer_tests.rs

@@ -1086,16 +1086,14 @@ fn do_test_async_holder_signatures(anchors: bool, remote_commitment: bool) {
 	nodes[0].disable_channel_signer_op(&node_b_id, &chan_id, sign_htlc_op);
 	mine_transaction(&nodes[0], &commitment_tx);
 
-	check_added_monitors(&nodes[0], 1);
 	check_closed_broadcast(&nodes[0], 1, true);
-	check_closed_event(
-		&nodes[0],
-		1,
-		ClosureReason::CommitmentTxConfirmed,
-		false,
-		&[node_b_id],
-		100_000,
-	);
+	check_added_monitors(&nodes[0], 1);
+	let closure_reason = if remote_commitment {
+		ClosureReason::CommitmentTxConfirmed
+	} else {
+		ClosureReason::HTLCsTimedOut
+	};
+	check_closed_event(&nodes[0], 1, closure_reason, false, &[node_b_id], 100_000);
 
 	// If the counterparty broadcast its latest commitment, we need to mine enough blocks for the
 	// HTLC timeout.

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -3895,6 +3895,11 @@ fn do_test_durable_preimages_on_closed_channel(
 			persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress);
 		}
 	}
+	if !close_chans_before_reload {
+		check_closed_broadcast(&nodes[1], 1, true);
+		let reason = ClosureReason::CommitmentTxConfirmed;
+		check_closed_event(&nodes[1], 1, reason, false, &[node_a_id], 100000);
+	}
 	nodes[1].node.timer_tick_occurred();
 	check_added_monitors(&nodes[1], mons_added);
 
@@ -3907,12 +3912,6 @@ fn do_test_durable_preimages_on_closed_channel(
 		.unwrap();
 	check_spends!(bs_preimage_tx, as_closing_tx[0]);
 
-	if !close_chans_before_reload {
-		check_closed_broadcast(&nodes[1], 1, true);
-		let reason = ClosureReason::CommitmentTxConfirmed;
-		check_closed_event(&nodes[1], 1, reason, false, &[node_a_id], 100000);
-	}
-
 	mine_transactions(&nodes[0], &[&as_closing_tx[0], bs_preimage_tx]);
 	check_closed_broadcast(&nodes[0], 1, true);
 	expect_payment_sent(&nodes[0], payment_preimage, None, true, true);
@@ -4048,7 +4047,7 @@ fn do_test_reload_mon_update_completion_actions(close_during_reload: bool) {
 	let mut events = nodes[1].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), if close_during_reload { 2 } else { 1 });
 	expect_payment_forwarded(
-		events.pop().unwrap(),
+		events.remove(0),
 		&nodes[1],
 		&nodes[0],
 		&nodes[2],
@@ -4491,9 +4490,9 @@ fn test_claim_to_closed_channel_blocks_forwarded_preimage_removal() {
 	assert_eq!(as_commit_tx.len(), 1);
 
 	mine_transaction(&nodes[1], &as_commit_tx[0]);
+	check_closed_broadcast!(nodes[1], true);
 	check_added_monitors!(nodes[1], 1);
 	check_closed_event!(nodes[1], 1, ClosureReason::CommitmentTxConfirmed, [node_a_id], 1000000);
-	check_closed_broadcast!(nodes[1], true);
 
 	// Now that B has a pending forwarded payment across it with the inbound edge on-chain, claim
 	// the payment on C and give B the preimage for it.
@@ -4567,9 +4566,9 @@ fn test_claim_to_closed_channel_blocks_claimed_event() {
 	assert_eq!(as_commit_tx.len(), 1);
 
 	mine_transaction(&nodes[1], &as_commit_tx[0]);
+	check_closed_broadcast!(nodes[1], true);
 	check_added_monitors!(nodes[1], 1);
 	check_closed_event!(nodes[1], 1, ClosureReason::CommitmentTxConfirmed, [node_a_id], 1000000);
-	check_closed_broadcast!(nodes[1], true);
 
 	// Now that B has a pending payment with the inbound HTLC on a closed channel, claim the
 	// payment on disk, but don't let the `ChannelMonitorUpdate` complete. This should prevent the

lightning/src/ln/channel.rs

@@ -5775,35 +5775,6 @@ where
 		Ok(false)
 	}
 
-	#[rustfmt::skip]
-	fn check_for_funding_tx_spent<L: Deref>(
-		&mut self, funding: &FundingScope, tx: &Transaction, logger: &L,
-	) -> Result<(), ClosureReason>
-	where
-		L::Target: Logger,
-	{
-		let funding_txo = match funding.get_funding_txo() {
-			Some(funding_txo) => funding_txo,
-			None => {
-				debug_assert!(false);
-				return Ok(());
-			},
-		};
-
-		for input in tx.input.iter() {
-			if input.previous_output == funding_txo.into_bitcoin_outpoint() {
-				log_info!(
-					logger, "Detected channel-closing tx {} spending {}:{}, closing channel {}",
-					tx.compute_txid(), input.previous_output.txid, input.previous_output.vout,
-					&self.channel_id(),
-				);
-				return Err(ClosureReason::CommitmentTxConfirmed);
-			}
-		}
-
-		Ok(())
-	}
-
 	/// Returns SCIDs that have been associated with the channel's funding transactions.
 	pub fn historical_scids(&self) -> &[u64] {
 		&self.historical_scids[..]
@@ -10005,12 +9976,6 @@ where
 				}
 
 				if let Some(channel_ready) = self.check_get_channel_ready(height, logger) {
-					for &(idx, tx) in txdata.iter() {
-						if idx > index_in_block {
-							self.context.check_for_funding_tx_spent(&self.funding, tx, logger)?;
-						}
-					}
-
 					log_info!(logger, "Sending a channel_ready to our peer for channel {}", &self.context.channel_id);
 					let announcement_sigs = self.get_announcement_sigs(node_signer, chain_hash, user_config, height, logger);
 					return Ok((Some(FundingConfirmedMessage::Establishment(channel_ready)), announcement_sigs));
@@ -10051,12 +10016,6 @@ where
 				let funding = self.pending_funding.get(confirmed_funding_index).unwrap();
 
 				if let Some(splice_locked) = pending_splice.check_get_splice_locked(&self.context, funding, height) {
-					for &(idx, tx) in txdata.iter() {
-						if idx > index_in_block {
-							self.context.check_for_funding_tx_spent(funding, tx, logger)?;
-						}
-					}
-
 					log_info!(
 						logger,
 						"Sending splice_locked txid {} to our peer for channel {}",
@@ -10076,13 +10035,6 @@ where
 					return Ok((Some(FundingConfirmedMessage::Splice(splice_locked, funding_txo, monitor_update)), announcement_sigs));
 				}
 			}
-
-			self.context.check_for_funding_tx_spent(&self.funding, tx, logger)?;
-			#[cfg(splicing)]
-			for funding in self.pending_funding.iter() {
-				self.context.check_for_funding_tx_spent(funding, tx, logger)?;
-			}
-
 		}
 
 		Ok((None, None))

lightning/src/ln/channelmanager.rs

@@ -11329,6 +11329,22 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 							}
 						}
 					},
+					MonitorEvent::CommitmentTxConfirmed(_) => {
+						let per_peer_state = self.per_peer_state.read().unwrap();
+						if let Some(peer_state_mutex) = per_peer_state.get(&counterparty_node_id) {
+							let mut peer_state_lock = peer_state_mutex.lock().unwrap();
+							let peer_state = &mut *peer_state_lock;
+							if let hash_map::Entry::Occupied(chan_entry) =
+								peer_state.channel_by_id.entry(channel_id)
+							{
+								let reason = ClosureReason::CommitmentTxConfirmed;
+								let err = ChannelError::Close((reason.to_string(), reason));
+								let mut chan = chan_entry.remove();
+								let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan);
+								failed_channels.push((Err(e), counterparty_node_id));
+							}
+						}
+					},
 					MonitorEvent::Completed { channel_id, monitor_update_id, .. } => {
 						self.channel_monitor_updated(
 							&channel_id,