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.

Author: wpaulino

lightning/src/chain/channelmonitor.rs

@@ -1562,28 +1562,30 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {
 		self.inner.lock().unwrap().counterparty_node_id
 	}
 
-	/// Used by [`ChannelManager`] deserialization to broadcast the latest holder state if its copy
-	/// of the channel state was out-of-date.
-	///
-	/// You may also use this to broadcast the latest local commitment transaction, either because
+	/// You may use this to broadcast the latest local commitment transaction, either because
 	/// a monitor update failed or because we've fallen behind (i.e. we've received proof that our
 	/// counterparty side knows a revocation secret we gave them that they shouldn't know).
 	///
-	/// Broadcasting these transactions in the second case is UNSAFE, as they allow counterparty
+	/// Broadcasting these transactions in this manner is UNSAFE, as they allow counterparty
 	/// side to punish you. Nevertheless you may want to broadcast them if counterparty doesn't
 	/// 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.
-	///
-	/// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager
-	pub fn get_latest_holder_commitment_txn<L: Deref>(&self, logger: &L) -> Vec<Transaction>
-	where L::Target: Logger {
+	pub fn broadcast_latest_holder_commitment_txn<B: Deref, F: Deref, L: Deref>(
+		&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"))]
@@ -2855,7 +2857,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 					} 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::get_latest_holder_commitment_txn and take manual action!");
+						log_error!(logger, "    Read the docs for ChannelMonitor::broadcast_latest_holder_commitment_txn to take manual action!");
 					} else {
 						// If we generated a MonitorEvent::HolderForceClosed, the ChannelManager
 						// will still give us a ChannelForceClosed event with !should_broadcast, but we
@@ -3502,45 +3504,6 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		}
 	}
 
-	fn get_latest_holder_commitment_txn<L: Deref>(
-		&mut self, logger: &WithChannelMonitor<L>,
-	) -> Vec<Transaction> 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_fully_signed_holder_tx(&self.funding_redeemscript);
-		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 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_fully_signed_htlc_tx(
-					&::bitcoin::OutPoint { txid, vout }, &preimage) {
-					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.
-		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<L: Deref>(

lightning/src/ln/channelmanager.rs

@@ -3021,8 +3021,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)

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);