Add flag to disable broadcasting when it's dangerous due to information loss

lightning/src/chain/channelmonitor.rs

@@ -783,6 +783,10 @@ pub(crate) struct ChannelMonitorImpl<Signer: Sign> {
 	counterparty_node_id: Option<PublicKey>,
 
 	secp_ctx: Secp256k1<secp256k1::All>, //TODO: dedup this a bit...
+
+	// Used to track that the channel was updated with ChannelForceClosed {should_broadcast: false}
+	// implying that it's unsafe to broadcast the latest holder commitment transaction.
+	allow_automated_broadcast: bool,
 }
 
 /// Transaction outputs to watch for on-chain spends.
@@ -1017,6 +1021,7 @@ impl<Signer: Sign> Writeable for ChannelMonitorImpl<Signer> {
 			(7, self.funding_spend_seen, required),
 			(9, self.counterparty_node_id, option),
 			(11, self.confirmed_commitment_tx_counterparty_output, option),
+			(13, self.allow_automated_broadcast, required),
 		});
 
 		Ok(())
@@ -1130,6 +1135,8 @@ impl<Signer: Sign> ChannelMonitor<Signer> {
 			counterparty_node_id: Some(counterparty_node_id),
 
 			secp_ctx,
+
+			allow_automated_broadcast: true,
 		})
 	}
 
@@ -1180,15 +1187,29 @@ impl<Signer: Sign> ChannelMonitor<Signer> {
 			payment_hash, payment_preimage, broadcaster, fee_estimator, logger)
 	}
 
-	pub(crate) fn broadcast_latest_holder_commitment_txn<B: Deref, L: Deref>(
+	/// Broadcasts the latest commitment transaction only if it's safe to do so.
+	pub(crate) fn maybe_broadcast_latest_holder_commitment_txn<B: Deref, L: Deref>(
+		&self,
+		broadcaster: &B,
+		logger: &L,
+	) where
+		B::Target: BroadcasterInterface,
+		L::Target: Logger,
+	{
+		self.inner.lock().unwrap().maybe_broadcast_latest_holder_commitment_txn(broadcaster, logger)
+	}
+
+	/// Broadcasts the latest commitment transaction, even if we can't ensure it's safe to do so
+	/// due to missing information.
+	pub fn force_broadcast_latest_holder_commitment_txn_unsafe<B: Deref, L: Deref>(
 		&self,
 		broadcaster: &B,
 		logger: &L,
 	) where
 		B::Target: BroadcasterInterface,
 		L::Target: Logger,
 	{
-		self.inner.lock().unwrap().broadcast_latest_holder_commitment_txn(broadcaster, logger)
+		self.inner.lock().unwrap().force_broadcast_latest_holder_commitment_txn_unsafe(broadcaster, logger)
 	}
 
 	/// Updates a ChannelMonitor on the basis of some new information provided by the Channel
@@ -2148,7 +2169,25 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 		}
 	}
 
-	pub(crate) fn broadcast_latest_holder_commitment_txn<B: Deref, L: Deref>(&mut self, broadcaster: &B, logger: &L)
+	/// Broadcasts the latest commitment transaction only if it's safe to do so.
+	pub fn maybe_broadcast_latest_holder_commitment_txn<B: Deref, L: Deref>(&mut self, broadcaster: &B, logger: &L)
+		where B::Target: BroadcasterInterface,
+					L::Target: Logger,
+	{
+		if self.allow_automated_broadcast {
+			for tx in self.get_latest_holder_commitment_txn(logger).iter() {
+				log_info!(logger, "Broadcasting local {}", log_tx!(tx));
+				broadcaster.broadcast_transaction(tx);
+			}
+			self.pending_monitor_events.push(MonitorEvent::CommitmentTxConfirmed(self.funding_info.0));
+		} else {
+			log_info!(logger, "Not broadcasting local commitment txn. Automated broadcasting is disabled.");
+		}
+	}
+
+	/// Broadcasts the latest commitment transaction, even if we can't ensure it's safe to do so
+	/// due to missing information.
+	pub fn force_broadcast_latest_holder_commitment_txn_unsafe<B: Deref, L: Deref>(&mut self, broadcaster: &B, logger: &L)
 		where B::Target: BroadcasterInterface,
 					L::Target: Logger,
 	{
@@ -2214,16 +2253,11 @@ impl<Signer: Sign> 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 {
-						self.broadcast_latest_holder_commitment_txn(broadcaster, logger);
-					} else if !self.holder_tx_signed {
-						log_error!(logger, "You have a toxic holder commitment transaction avaible in channel monitor, read comment in ChannelMonitor::get_latest_holder_commitment_txn to be informed of manual action to take");
-					} else {
-						// If we generated a MonitorEvent::CommitmentTxConfirmed, the ChannelManager
-						// will still give us a ChannelForceClosed event with !should_broadcast, but we
-						// shouldn't print the scary warning above.
+					self.allow_automated_broadcast = *should_broadcast;
+					if !*should_broadcast && self.holder_tx_signed {
 						log_info!(logger, "Channel off-chain state closed after we broadcasted our latest commitment transaction.");
 					}
+					self.maybe_broadcast_latest_holder_commitment_txn(broadcaster, logger);
 				},
 				ChannelMonitorUpdateStep::ShutdownScript { scriptpubkey } => {
 					log_trace!(logger, "Updating ChannelMonitor with shutdown script");
@@ -3628,13 +3662,15 @@ impl<'a, Signer: Sign, K: KeysInterface<Signer = Signer>> ReadableArgs<&'a K>
 		let mut funding_spend_seen = Some(false);
 		let mut counterparty_node_id = None;
 		let mut confirmed_commitment_tx_counterparty_output = None;
+		let mut allow_automated_broadcast =  None;
 		read_tlv_fields!(reader, {
 			(1, funding_spend_confirmed, option),
 			(3, htlcs_resolved_on_chain, vec_type),
 			(5, pending_monitor_events, vec_type),
 			(7, funding_spend_seen, option),
 			(9, counterparty_node_id, option),
 			(11, confirmed_commitment_tx_counterparty_output, option),
+			(13, allow_automated_broadcast, option),
 		});
 
 		let mut secp_ctx = Secp256k1::new();
@@ -3692,6 +3728,8 @@ impl<'a, Signer: Sign, K: KeysInterface<Signer = Signer>> ReadableArgs<&'a K>
 			counterparty_node_id,
 
 			secp_ctx,
+
+			allow_automated_broadcast: allow_automated_broadcast.unwrap(),
 		})))
 	}
 }

lightning/src/ln/channelmanager.rs

@@ -5409,7 +5409,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 	/// Channel object.
 	fn handle_init_event_channel_failures(&self, mut failed_channels: Vec<ShutdownResult>) {
 		for mut failure in failed_channels.drain(..) {
-			// Either a commitment transactions has been confirmed on-chain or
+			// Either a commitment transaction has been confirmed on-chain or
 			// Channel::block_disconnected detected that the funding transaction has been
 			// reorganized out of the main chain.
 			// We cannot broadcast our latest local state via monitor update (as
@@ -6951,7 +6951,7 @@ impl<'a, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 						log_bytes!(channel.channel_id()), monitor.get_latest_update_id(), channel.get_latest_monitor_update_id());
 					let (_, mut new_failed_htlcs) = channel.force_shutdown(true);
 					failed_htlcs.append(&mut new_failed_htlcs);
-					monitor.broadcast_latest_holder_commitment_txn(&args.tx_broadcaster, &args.logger);
+					monitor.maybe_broadcast_latest_holder_commitment_txn(&args.tx_broadcaster, &args.logger);
 					channel_closures.push(events::Event::ChannelClosed {
 						channel_id: channel.channel_id(),
 						user_channel_id: channel.get_user_id(),
@@ -6980,7 +6980,7 @@ impl<'a, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 		for (ref funding_txo, ref mut monitor) in args.channel_monitors.iter_mut() {
 			if !funding_txo_set.contains(funding_txo) {
 				log_info!(args.logger, "Broadcasting latest holder commitment transaction for closed channel {}", log_bytes!(funding_txo.to_channel_id()));
-				monitor.broadcast_latest_holder_commitment_txn(&args.tx_broadcaster, &args.logger);
+				monitor.maybe_broadcast_latest_holder_commitment_txn(&args.tx_broadcaster, &args.logger);
 			}
 		}
 

lightning/src/ln/functional_tests.rs

@@ -2471,7 +2471,7 @@ fn revoked_output_claim() {
 	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
 	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
 	let chan_1 = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
-	// node[0] is gonna to revoke an old state thus node[1] should be able to claim the revoked output
+	// node[0] is going to revoke an old state thus node[1] should be able to claim the revoked output
 	let revoked_local_txn = get_local_commitment_txn!(nodes[0], chan_1.2);
 	assert_eq!(revoked_local_txn.len(), 1);
 	// Only output is the full channel value back to nodes[0]:
@@ -4752,6 +4752,74 @@ fn test_manager_serialize_deserialize_inconsistent_monitor() {
 	}
 }
 
+#[test]
+fn test_deserialize_monitor_force_closed_without_broadcasting_txn() {
+	// Test deserializing a manager with a monitor that was force closed with {should_broadcast: false}.
+	// We expect not to broadcast the txn during deseriliazization, but to still be able to force-broadcast it.
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let logger;
+	let fee_estimator;
+	let persister: test_utils::TestPersister;
+	let new_chain_monitor: test_utils::TestChainMonitor;
+	let deserialized_chanmgr: ChannelManager<EnforcingSigner, &test_utils::TestChainMonitor, &test_utils::TestBroadcaster, &test_utils::TestKeysInterface, &test_utils::TestFeeEstimator, &test_utils::TestLogger>;
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None, 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, InitFeatures::known(), InitFeatures::known());
+
+	node_chanmgrs[0].force_close_without_broadcasting_txn(&channel_id, &nodes[1].node.get_our_node_id()).unwrap();
+
+	// Serialize the monitor and node.
+	let mut chanmon_writer = test_utils::TestVecWriter(Vec::new());
+	get_monitor!(nodes[0], channel_id).write(&mut chanmon_writer).unwrap();
+	let serialized_chanmon = chanmon_writer.0;
+	let serialized_node = nodes[0].node.encode();
+
+	logger = test_utils::TestLogger::new();
+	fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) };
+	persister = test_utils::TestPersister::new();
+	let keys_manager = &chanmon_cfgs[0].keys_manager;
+	new_chain_monitor = test_utils::TestChainMonitor::new(Some(nodes[0].chain_source), nodes[0].tx_broadcaster.clone(), &logger, &fee_estimator, &persister, keys_manager);
+	nodes[0].chain_monitor = &new_chain_monitor;
+
+	// Deserialize the result.
+	let mut chanmon_read = &serialized_chanmon[..];
+	let (_, mut deserialized_chanmon) = <(BlockHash, ChannelMonitor<EnforcingSigner>)>::read(&mut chanmon_read, keys_manager).unwrap();
+	assert!(chanmon_read.is_empty());
+	let mut node_read = &serialized_node[..];
+	let tx_broadcaster = nodes[0].tx_broadcaster.clone();
+	let channel_monitors = HashMap::from([(deserialized_chanmon.get_funding_txo().0, &mut deserialized_chanmon)]);
+	let (_, deserialized_chanmgr_temp) =
+		<(BlockHash, ChannelManager<EnforcingSigner, &test_utils::TestChainMonitor, &test_utils::TestBroadcaster, &test_utils::TestKeysInterface, &test_utils::TestFeeEstimator, &test_utils::TestLogger>)>::read(&mut node_read, ChannelManagerReadArgs {
+		default_config: UserConfig::default(),
+		keys_manager,
+		fee_estimator: &fee_estimator,
+		chain_monitor: &new_chain_monitor,
+		tx_broadcaster,
+		logger: &logger,
+		channel_monitors,
+	}).unwrap();
+	deserialized_chanmgr = deserialized_chanmgr_temp;
+	nodes[0].node = &deserialized_chanmgr;
+
+	{ // Assert that the latest commitment txn hasn't been broadcasted.
+		let txn = tx_broadcaster.txn_broadcasted.lock().unwrap();
+		assert_eq!(txn.len(), 0);
+	}
+
+	{ // Assert that we can still force-broadcast the latest commitment txn.
+		deserialized_chanmon.force_broadcast_latest_holder_commitment_txn_unsafe(&tx_broadcaster, &&logger); 
+		let txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
+		assert_eq!(txn.len(), 1);
+		check_spends!(txn[0], funding_tx);
+		assert_eq!(txn[0].input[0].previous_output.txid, funding_tx.txid());
+	}
+
+	assert!(nodes[0].chain_monitor.watch_channel(deserialized_chanmon.get_funding_txo().0, deserialized_chanmon).is_ok());
+	check_added_monitors!(nodes[0], 1);
+	check_closed_event!(nodes[0], 1, ClosureReason::HolderForceClosed);
+}
+
 macro_rules! check_spendable_outputs {
 	($node: expr, $keysinterface: expr) => {
 		{
@@ -7578,7 +7646,7 @@ fn test_force_close_without_broadcast() {
 #[test]
 fn test_check_htlc_underpaying() {
 	// Send payment through A -> B but A is maliciously
-	// sending a probe payment (i.e less than expected value0
+	// sending a probe payment (i.e less than expected value)
 	// to B, B should refuse payment.
 
 	let chanmon_cfgs = create_chanmon_cfgs(2);

lightning/src/ln/reorg_tests.rs

@@ -368,7 +368,7 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_
 		}
 	}
 	// With expect_channel_force_closed set the TestChainMonitor will enforce that the next update
-	// is a ChannelForcClosed on the right channel with should_broadcast set.
+	// is a ChannelForceClosed on the right channel with should_broadcast set.
 	*nodes[0].chain_monitor.expect_channel_force_closed.lock().unwrap() = Some((chan.2, true));
 	nodes[0].node.test_process_background_events(); // Required to free the pending background monitor update
 	check_added_monitors!(nodes[0], 1);