channelmonitor: Persist force-close broadcast preference

Add allow_automated_broadcast flag to ChannelMonitor to prevent
automatic commitment transaction broadcasting when channel was
previously force-closed with should_broadcast=false.

Fixes unsafe broadcast on startup when ChannelManager finds
orphaned monitors that were intentionally closed without
broadcasting.

Author: martinsaposnic

lightning/src/chain/channelmonitor.rs

@@ -1166,6 +1166,14 @@ pub(crate) struct ChannelMonitorImpl<Signer: EcdsaChannelSigner> {
 	/// The node_id of our counterparty
 	counterparty_node_id: PublicKey,
 
+	/// Controls whether the monitor is allowed to automatically broadcast the latest holder commitment transaction.
+	///
+	/// This flag is set to `false` when a channel is force-closed with `should_broadcast: false`,
+	/// indicating that broadcasting the latest holder commitment transaction would be unsafe.
+	///
+	/// Default: `true`.
+	allow_automated_broadcast: bool,
+
 	/// Initial counterparty commmitment data needed to recreate the commitment tx
 	/// in the persistence pipeline for third-party watchtowers. This will only be present on
 	/// monitors created after 0.0.117.
@@ -1467,6 +1475,7 @@ impl<Signer: EcdsaChannelSigner> Writeable for ChannelMonitorImpl<Signer> {
 			(27, self.first_confirmed_funding_txo, required),
 			(29, self.initial_counterparty_commitment_tx, option),
 			(31, self.funding.channel_parameters, required),
+			(33, self.allow_automated_broadcast, required),
 		});
 
 		Ok(())
@@ -1684,6 +1693,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 
 			best_block,
 			counterparty_node_id: counterparty_node_id,
+			allow_automated_broadcast: true,
 			initial_counterparty_commitment_info: None,
 			initial_counterparty_commitment_tx: None,
 			balances_empty_height: None,
@@ -2038,7 +2048,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 	/// may be to contact the other node operator out-of-band to coordinate other options available
 	/// to you.
 	#[rustfmt::skip]
-	pub fn broadcast_latest_holder_commitment_txn<B: Deref, F: Deref, L: Deref>(
+	pub fn force_broadcast_latest_holder_commitment_txn_unsafe<B: Deref, F: Deref, L: Deref>(
 		&self, broadcaster: &B, fee_estimator: &F, logger: &L
 	)
 	where
@@ -3453,6 +3463,32 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		);
 	}
 
+	fn maybe_broadcast_latest_holder_commitment_txn<B: Deref, F: Deref, L: Deref>(
+		&mut self, broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>,
+		logger: &WithChannelMonitor<L>,
+	) where
+		B::Target: BroadcasterInterface,
+		F::Target: FeeEstimator,
+		L::Target: Logger,
+	{
+		if !self.allow_automated_broadcast {
+			return;
+		}
+		let detected_funding_spend = self.funding_spend_confirmed.is_some()
+			|| self
+				.onchain_events_awaiting_threshold_conf
+				.iter()
+				.any(|event| matches!(event.event, OnchainEvent::FundingSpendConfirmation { .. }));
+		if detected_funding_spend {
+			log_trace!(
+				logger,
+				"Avoiding commitment broadcast, already detected confirmed spend onchain"
+			);
+			return;
+		}
+		self.queue_latest_holder_commitment_txn_for_broadcast(broadcaster, fee_estimator, logger);
+	}
+
 	#[rustfmt::skip]
 	fn update_monitor<B: Deref, F: Deref, L: Deref>(
 		&mut self, updates: &ChannelMonitorUpdate, broadcaster: &B, fee_estimator: &F, logger: &WithChannelMonitor<L>
@@ -3535,28 +3571,16 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 				ChannelMonitorUpdateStep::ChannelForceClosed { should_broadcast } => {
 					log_trace!(logger, "Updating ChannelMonitor: channel force closed, should broadcast: {}", should_broadcast);
 					self.lockdown_from_offchain = true;
-					if *should_broadcast {
-						// There's no need to broadcast our commitment transaction if we've seen one
-						// confirmed (even with 1 confirmation) as it'll be rejected as
-						// duplicate/conflicting.
-						let detected_funding_spend = self.funding_spend_confirmed.is_some() ||
-							self.onchain_events_awaiting_threshold_conf.iter().any(
-								|event| matches!(event.event, OnchainEvent::FundingSpendConfirmation { .. }));
-						if detected_funding_spend {
-							log_trace!(logger, "Avoiding commitment broadcast, already detected confirmed spend onchain");
-							continue;
-						}
-						self.queue_latest_holder_commitment_txn_for_broadcast(broadcaster, &bounded_fee_estimator, logger);
-					} else if !self.holder_tx_signed {
-						log_error!(logger, "WARNING: You have a potentially-unsafe holder commitment transaction available to broadcast");
-						log_error!(logger, "    in channel monitor for channel {}!", &self.channel_id());
-						log_error!(logger, "    Read the docs for ChannelMonitor::broadcast_latest_holder_commitment_txn to take manual action!");
-					} else {
+					if self.allow_automated_broadcast {
+						self.allow_automated_broadcast = *should_broadcast;
+					}
+					if !*should_broadcast && self.holder_tx_signed {
 						// If we generated a MonitorEvent::HolderForceClosed, the ChannelManager
 						// will still give us a ChannelForceClosed event with !should_broadcast, but we
 						// shouldn't print the scary warning above.
 						log_info!(logger, "Channel off-chain state closed after we broadcasted our latest commitment transaction.");
 					}
+					self.maybe_broadcast_latest_holder_commitment_txn(broadcaster, &bounded_fee_estimator, logger);
 				},
 				ChannelMonitorUpdateStep::ShutdownScript { scriptpubkey } => {
 					log_trace!(logger, "Updating ChannelMonitor with shutdown script");
@@ -5438,6 +5462,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
 		let mut payment_preimages_with_info: Option<HashMap<_, _>> = None;
 		let mut first_confirmed_funding_txo = RequiredWrapper(None);
 		let mut channel_parameters = None;
+		let mut allow_automated_broadcast = None;
 		read_tlv_fields!(reader, {
 			(1, funding_spend_confirmed, option),
 			(3, htlcs_resolved_on_chain, optional_vec),
@@ -5455,6 +5480,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
 			(27, first_confirmed_funding_txo, (default_value, outpoint)),
 			(29, initial_counterparty_commitment_tx, option),
 			(31, channel_parameters, (option: ReadableArgs, None)),
+			(33, allow_automated_broadcast, option),
 		});
 		if let Some(payment_preimages_with_info) = payment_preimages_with_info {
 			if payment_preimages_with_info.len() != payment_preimages.len() {
@@ -5618,6 +5644,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
 
 			best_block,
 			counterparty_node_id: counterparty_node_id.unwrap(),
+			allow_automated_broadcast: allow_automated_broadcast.unwrap_or(true),
 			initial_counterparty_commitment_info,
 			initial_counterparty_commitment_tx,
 			balances_empty_height,

lightning/src/ln/channelmanager.rs

@@ -4467,7 +4467,7 @@ where
 	/// Fails if `channel_id` is unknown to the manager, or if the
 	/// `counterparty_node_id` isn't the counterparty of the corresponding channel.
 	/// You can always broadcast the latest local transaction(s) via
-	/// [`ChannelMonitor::broadcast_latest_holder_commitment_txn`].
+	/// [`ChannelMonitor::force_broadcast_latest_holder_commitment_txn_unsafe`].
 	pub fn force_close_without_broadcasting_txn(
 		&self, channel_id: &ChannelId, counterparty_node_id: &PublicKey, error_message: String,
 	) -> Result<(), APIError> {
@@ -7655,7 +7655,8 @@ where
 		ComplFunc: FnOnce(
 			Option<u64>,
 			bool,
-		) -> (Option<MonitorUpdateCompletionAction>, Option<RAAMonitorUpdateBlockingAction>),
+		)
+			-> (Option<MonitorUpdateCompletionAction>, Option<RAAMonitorUpdateBlockingAction>),
 	>(
 		&self, prev_hop: HTLCPreviousHopData, payment_preimage: PaymentPreimage,
 		payment_info: Option<PaymentClaimDetails>, completion_action: ComplFunc,
@@ -14144,7 +14145,7 @@ where
 		// LDK versions prior to 0.0.116 wrote the `pending_background_events`
 		// `MonitorUpdateRegeneratedOnStartup`s here, however there was never a reason to do so -
 		// the closing monitor updates were always effectively replayed on startup (either directly
-		// by calling `broadcast_latest_holder_commitment_txn` on a `ChannelMonitor` during
+		// by calling `force_broadcast_latest_holder_commitment_txn_unsafe` on a `ChannelMonitor` during
 		// deserialization or, in 0.0.115, by regenerating the monitor update itself).
 		0u64.write(writer)?;
 

lightning/src/ln/monitor_tests.rs

@@ -3151,7 +3151,7 @@ fn do_test_monitor_claims_with_random_signatures(anchors: bool, confirm_counterp
 		(&nodes[0], &nodes[1])
 	};
 
-	get_monitor!(closing_node, chan_id).broadcast_latest_holder_commitment_txn(
+	get_monitor!(closing_node, chan_id).force_broadcast_latest_holder_commitment_txn_unsafe(
 		&closing_node.tx_broadcaster, &closing_node.fee_estimator, &closing_node.logger
 	);
 

lightning/src/ln/reload_tests.rs

@@ -1368,3 +1368,124 @@ fn test_htlc_localremoved_persistence() {
 	let htlc_fail_msg_after_reload = msgs.2.unwrap().update_fail_htlcs[0].clone();
 	assert_eq!(htlc_fail_msg, htlc_fail_msg_after_reload);
 }
+
+#[test]
+fn test_deserialize_monitor_force_closed_without_broadcasting_txn() {
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let logger;
+	let fee_estimator;
+	let persister;
+	let new_chain_monitor;
+	let deserialized_chanmgr;
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+	let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	let (_, _, channel_id, funding_tx) = create_announced_chan_between_nodes(&nodes, 0, 1);
+
+	// send a ChannelMonitorUpdateStep::ChannelForceClosed with { should_broadcast: false } to the monitor.
+	// this should persist the { should_broadcast: false } on the monitor.
+	nodes[0]
+		.node
+		.force_close_without_broadcasting_txn(
+			&channel_id,
+			&nodes[1].node.get_our_node_id(),
+			"test".to_string(),
+		)
+		.unwrap();
+	check_closed_event!(
+		nodes[0],
+		1,
+		ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) },
+		[nodes[1].node.get_our_node_id()],
+		100000
+	);
+
+	// Serialize monitor and node
+	let mut mon_writer = test_utils::TestVecWriter(Vec::new());
+	get_monitor!(nodes[0], channel_id).write(&mut mon_writer).unwrap();
+	let monitor_bytes = mon_writer.0;
+	let manager_bytes = nodes[0].node.encode();
+
+	let keys_manager = &chanmon_cfgs[0].keys_manager;
+	logger = test_utils::TestLogger::new();
+	fee_estimator = test_utils::TestFeeEstimator::new(253);
+	persister = test_utils::TestPersister::new();
+	new_chain_monitor = test_utils::TestChainMonitor::new(
+		Some(nodes[0].chain_source),
+		nodes[0].tx_broadcaster,
+		&logger,
+		&fee_estimator,
+		&persister,
+		keys_manager,
+	);
+	nodes[0].chain_monitor = &new_chain_monitor;
+
+	// Deserialize
+	let mut mon_read = &monitor_bytes[..];
+	let (_, mut monitor) = <(BlockHash, ChannelMonitor<TestChannelSigner>)>::read(
+		&mut mon_read,
+		(keys_manager, keys_manager),
+	)
+	.unwrap();
+	assert!(mon_read.is_empty());
+
+	let mut mgr_read = &manager_bytes[..];
+	let mut channel_monitors = new_hash_map();
+
+	// insert a channel monitor without its corresponding channel (it was force-closed before)
+	// so when the channel manager deserializes, it replays the ChannelForceClosed with { should_broadcast: true }.
+	channel_monitors.insert(monitor.channel_id(), &monitor);
+	let (_, deserialized_chanmgr_tmp) = <(
+		BlockHash,
+		ChannelManager<
+			&test_utils::TestChainMonitor,
+			&test_utils::TestBroadcaster,
+			&test_utils::TestKeysInterface,
+			&test_utils::TestKeysInterface,
+			&test_utils::TestKeysInterface,
+			&test_utils::TestFeeEstimator,
+			&test_utils::TestRouter,
+			&test_utils::TestMessageRouter,
+			&test_utils::TestLogger,
+		>,
+	)>::read(
+		&mut mgr_read,
+		ChannelManagerReadArgs {
+			default_config: UserConfig::default(),
+			entropy_source: keys_manager,
+			node_signer: keys_manager,
+			signer_provider: keys_manager,
+			fee_estimator: &fee_estimator,
+			router: nodes[0].router,
+			message_router: &nodes[0].message_router,
+			chain_monitor: &new_chain_monitor,
+			tx_broadcaster: nodes[0].tx_broadcaster,
+			logger: &logger,
+			channel_monitors,
+		},
+	)
+	.unwrap();
+	deserialized_chanmgr = deserialized_chanmgr_tmp;
+	nodes[0].node = &deserialized_chanmgr;
+
+	{
+		let txs = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
+		assert!(txs.is_empty(), "Expected no transactions to be broadcasted after deserialization, because the should_broadcast was persisted as false");
+	}
+
+	monitor.force_broadcast_latest_holder_commitment_txn_unsafe(
+		&nodes[0].tx_broadcaster,
+		&&fee_estimator,
+		&&logger,
+	);
+	{
+		let txs = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
+		assert_eq!(txs.len(), 1, "Expected one transaction to be broadcasted after force_broadcast_latest_holder_commitment_txn_unsafe");
+		check_spends!(txs[0], funding_tx);
+		assert_eq!(txs[0].input[0].previous_output.txid, funding_tx.compute_txid());
+	}
+
+	assert!(nodes[0].chain_monitor.watch_channel(monitor.channel_id(), monitor).is_ok());
+	check_added_monitors!(nodes[0], 1);
+}