From 7496923f8a1d8fa0d48bbfb1335d80ac2388d6b6 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Thu, 25 Jan 2024 09:29:16 -0800 Subject: [PATCH] Rework get_latest_holder_commitment_txn to broadcast for users This method is meant to be used as a last resort when a user is forced to broadcast the current state, even if it is stale, in an attempt to claim their funds in the channel. Previously, we'd return the commitment and HTLC transactions such that they broadcast them themselves. Doing so required a different code path, one which was not tested, to obtain these transactions than our usual path when force closing. It's not worth maintaining both, and it's much simpler for us to broadcast instead. --- lightning/src/chain/channelmonitor.rs | 62 ++++++--------------------- lightning/src/ln/channelmanager.rs | 4 +- lightning/src/ln/monitor_tests.rs | 6 ++- 3 files changed, 18 insertions(+), 54 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index ce27c8baba9..c9a04019d9a 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -1551,14 +1551,21 @@ impl ChannelMonitor { /// close channel with their commitment transaction after a substantial amount of time. Best /// may be to contact the other node operator out-of-band to coordinate other options available /// to you. - pub fn get_latest_holder_commitment_txn(&self, logger: &L) -> Result, ()> - where L::Target: Logger { + pub fn broadcast_latest_holder_commitment_txn( + &self, broadcaster: &B, fee_estimator: &F, logger: &L + ) + where + B::Target: BroadcasterInterface, + F::Target: FeeEstimator, + L::Target: Logger + { let mut inner = self.inner.lock().unwrap(); + let fee_estimator = LowerBoundedFeeEstimator::new(&**fee_estimator); let logger = WithChannelMonitor::from_impl(logger, &*inner); - inner.get_latest_holder_commitment_txn(&logger) + inner.queue_latest_holder_commitment_txn_for_broadcast(broadcaster, &fee_estimator, &logger); } - /// Unsafe test-only version of get_latest_holder_commitment_txn used by our test framework + /// Unsafe test-only version of `broadcast_latest_holder_commitment_txn` used by our test framework /// to bypass HolderCommitmentTransaction state update lockdown after signature and generate /// revoked commitment transaction. #[cfg(any(test, feature = "unsafe_revoked_tx_signing"))] @@ -2836,7 +2843,7 @@ impl ChannelMonitorImpl { } 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.funding_info.0.to_channel_id()); - log_error!(logger, " Read the docs for ChannelMonitor::get_latest_holder_commitment_txn and take manual action!"); + log_error!(logger, " Read the docs for ChannelMonitor::broadcast_latest_holder_commitment_txn and take manual action!"); } else { // If we generated a MonitorEvent::HolderForceClosed, the ChannelManager // will still give us a ChannelForceClosed event with !should_broadcast, but we @@ -3461,51 +3468,6 @@ impl ChannelMonitorImpl { } } - fn get_latest_holder_commitment_txn( - &mut self, logger: &WithChannelMonitor, - ) -> Result, ()> where L::Target: Logger { - log_debug!(logger, "Getting signed latest holder commitment transaction!"); - self.holder_tx_signed = true; - let commitment_tx = self.onchain_tx_handler.get_maybe_signed_holder_tx(&self.funding_redeemscript); - if commitment_tx.input.iter().any(|input| input.witness.is_empty()) { - return Err(()); - } - let txid = commitment_tx.txid(); - let mut holder_transactions = vec![commitment_tx]; - // When anchor outputs are present, the HTLC transactions are only valid once the commitment - // transaction confirms. - if self.onchain_tx_handler.channel_type_features().supports_anchors_zero_fee_htlc_tx() { - return Ok(holder_transactions); - } - for htlc in self.current_holder_commitment_tx.htlc_outputs.iter() { - if let Some(vout) = htlc.0.transaction_output_index { - let preimage = if !htlc.0.offered { - if let Some(preimage) = self.payment_preimages.get(&htlc.0.payment_hash) { Some(preimage.clone()) } else { - // We can't build an HTLC-Success transaction without the preimage - continue; - } - } else if htlc.0.cltv_expiry > self.best_block.height() + 1 { - // Don't broadcast HTLC-Timeout transactions immediately as they don't meet the - // current locktime requirements on-chain. We will broadcast them in - // `block_confirmed` when `should_broadcast_holder_commitment_txn` returns true. - // Note that we add + 1 as transactions are broadcastable when they can be - // confirmed in the next block. - continue; - } else { None }; - if let Some(htlc_tx) = self.onchain_tx_handler.get_maybe_signed_htlc_tx( - &::bitcoin::OutPoint { txid, vout }, &preimage - ) { - if !htlc_tx.input.iter().any(|input| input.witness.is_empty()) { - holder_transactions.push(htlc_tx); - } - } - } - } - // We throw away the generated waiting_first_conf data as we aren't (yet) confirmed and we don't actually know what the caller wants to do. - // The data will be re-generated and tracked in check_spend_holder_transaction if we get a confirmation. - Ok(holder_transactions) - } - #[cfg(any(test,feature = "unsafe_revoked_tx_signing"))] /// Note that this includes possibly-locktimed-in-the-future transactions! fn unsafe_get_latest_holder_commitment_txn( diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 3dbbc9784bc..888f1884b63 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -2997,8 +2997,8 @@ where /// the latest local transaction(s). 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 get the latest local transaction(s) to broadcast from - /// [`ChannelMonitor::get_latest_holder_commitment_txn`]. + /// You can always broadcast the latest local transaction(s) via + /// [`ChannelMonitor::broadcast_latest_holder_commitment_txn`]. pub fn force_close_without_broadcasting_txn(&self, channel_id: &ChannelId, counterparty_node_id: &PublicKey) -> Result<(), APIError> { self.force_close_sending_error(channel_id, counterparty_node_id, false) diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index b6270181439..1ee98fdf349 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -2755,7 +2755,9 @@ fn do_test_monitor_claims_with_random_signatures(anchors: bool, confirm_counterp (&nodes[0], &nodes[1]) }; - closing_node.node.force_close_broadcasting_latest_txn(&chan_id, &other_node.node.get_our_node_id()).unwrap(); + get_monitor!(closing_node, chan_id).broadcast_latest_holder_commitment_txn( + &closing_node.tx_broadcaster, &closing_node.fee_estimator, &closing_node.logger + ); // The commitment transaction comes first. let commitment_tx = { @@ -2768,7 +2770,7 @@ fn do_test_monitor_claims_with_random_signatures(anchors: bool, confirm_counterp mine_transaction(closing_node, &commitment_tx); check_added_monitors!(closing_node, 1); check_closed_broadcast!(closing_node, true); - check_closed_event!(closing_node, 1, ClosureReason::HolderForceClosed, [other_node.node.get_our_node_id()], 1_000_000); + check_closed_event!(closing_node, 1, ClosureReason::CommitmentTxConfirmed, [other_node.node.get_our_node_id()], 1_000_000); mine_transaction(other_node, &commitment_tx); check_added_monitors!(other_node, 1);