diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 52cfe9972f7..89f9d2aa359 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -783,6 +783,10 @@ pub(crate) struct ChannelMonitorImpl { counterparty_node_id: Option, secp_ctx: Secp256k1, //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 Writeable for ChannelMonitorImpl { (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 ChannelMonitor { counterparty_node_id: Some(counterparty_node_id), secp_ctx, + + allow_automated_broadcast: true, }) } @@ -1180,7 +1187,21 @@ impl ChannelMonitor { payment_hash, payment_preimage, broadcaster, fee_estimator, logger) } - pub(crate) fn broadcast_latest_holder_commitment_txn( + /// Broadcasts the latest commitment transaction only if it's safe to do so. + pub(crate) fn maybe_broadcast_latest_holder_commitment_txn( + &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( &self, broadcaster: &B, logger: &L, @@ -1188,7 +1209,7 @@ impl ChannelMonitor { 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 ChannelMonitorImpl { } } - pub(crate) fn broadcast_latest_holder_commitment_txn(&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(&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(&mut self, broadcaster: &B, logger: &L) where B::Target: BroadcasterInterface, L::Target: Logger, { @@ -2214,16 +2253,11 @@ 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 { - 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,6 +3662,7 @@ impl<'a, Signer: Sign, K: KeysInterface> 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), @@ -3635,6 +3670,7 @@ impl<'a, Signer: Sign, K: KeysInterface> ReadableArgs<&'a K> (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> ReadableArgs<&'a K> counterparty_node_id, secp_ctx, + + allow_automated_broadcast: allow_automated_broadcast.unwrap(), }))) } } diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index e8830ab5ffe..d049791d221 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -5409,7 +5409,7 @@ impl ChannelMana /// Channel object. fn handle_init_event_channel_failures(&self, mut failed_channels: Vec) { 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); } } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index c41d5402ff3..03444ae8ec0 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/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; + 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)>::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)>::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); diff --git a/lightning/src/ln/reorg_tests.rs b/lightning/src/ln/reorg_tests.rs index e4b916c9345..9fd395f9d2a 100644 --- a/lightning/src/ln/reorg_tests.rs +++ b/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);