From 1eefa3756efa8176257500a83d2c2b187c012597 Mon Sep 17 00:00:00 2001 From: Martin Saposnic Date: Thu, 26 Jun 2025 13:34:22 -0300 Subject: [PATCH] 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. --- lightning/src/chain/channelmonitor.rs | 61 +++++++++---- lightning/src/ln/channelmanager.rs | 7 +- lightning/src/ln/monitor_tests.rs | 2 +- lightning/src/ln/reload_tests.rs | 121 ++++++++++++++++++++++++++ 4 files changed, 169 insertions(+), 22 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 5a4a94b1641..760acbe06e0 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -1268,6 +1268,14 @@ pub(crate) struct ChannelMonitorImpl { /// 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. @@ -1570,6 +1578,7 @@ impl Writeable for ChannelMonitorImpl { (29, self.initial_counterparty_commitment_tx, option), (31, self.funding.channel_parameters, required), (32, self.pending_funding, optional_vec), + (33, self.allow_automated_broadcast, required), }); Ok(()) @@ -1788,6 +1797,7 @@ impl ChannelMonitor { 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, @@ -2144,7 +2154,7 @@ impl ChannelMonitor { /// 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( + pub fn force_broadcast_latest_holder_commitment_txn_unsafe( &self, broadcaster: &B, fee_estimator: &F, logger: &L ) where @@ -3681,6 +3691,32 @@ impl ChannelMonitorImpl { Ok(()) } + fn maybe_broadcast_latest_holder_commitment_txn( + &mut self, broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator, + logger: &WithChannelMonitor, + ) 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( &mut self, updates: &ChannelMonitorUpdate, broadcaster: &B, fee_estimator: &F, logger: &WithChannelMonitor @@ -3774,28 +3810,14 @@ impl ChannelMonitorImpl { 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 { + 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"); @@ -5682,6 +5704,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP let mut first_confirmed_funding_txo = RequiredWrapper(None); let mut channel_parameters = None; let mut pending_funding = None; + let mut allow_automated_broadcast = None; read_tlv_fields!(reader, { (1, funding_spend_confirmed, option), (3, htlcs_resolved_on_chain, optional_vec), @@ -5700,6 +5723,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP (29, initial_counterparty_commitment_tx, option), (31, channel_parameters, (option: ReadableArgs, None)), (32, pending_funding, optional_vec), + (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() { @@ -5864,6 +5888,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, diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 5c39d503a6b..94b9a7b72b3 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/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, bool, - ) -> (Option, Option), + ) + -> (Option, Option), >( &self, prev_hop: HTLCPreviousHopData, payment_preimage: PaymentPreimage, payment_info: Option, 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)?; diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index 0a89899f118..91a4b48dcfd 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/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 ); diff --git a/lightning/src/ln/reload_tests.rs b/lightning/src/ln/reload_tests.rs index 17028b36ae5..23e7ef9e802 100644 --- a/lightning/src/ln/reload_tests.rs +++ b/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)>::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); +}