Always emit bump events, even when fees are sufficient

Currently, the anchor commitment bump events are bypassed when the
commitment transaction has sufficient fees. However, this makes it
difficult for users to defer force-closures to a trusted party (such as
an LSP) while not maintaining reserves. Broadcasting a commitment
transaction without maintaining reserves would make HTLCs unclaimable
against that commitment transaction.

In this change, anchor commitment bump events will always be emitted so
users can capture and choose not to process them.

Author: wvanlint

lightning/src/chain/onchaintx.rs

@@ -23,7 +23,7 @@ use bitcoin::secp256k1::{ecdsa::Signature, Secp256k1};
 use bitcoin::transaction::OutPoint as BitcoinOutPoint;
 use bitcoin::transaction::Transaction;
 
-use crate::chain::chaininterface::{compute_feerate_sat_per_1000_weight, ConfirmationTarget};
+use crate::chain::chaininterface::ConfirmationTarget;
 use crate::chain::chaininterface::{BroadcasterInterface, FeeEstimator, LowerBoundedFeeEstimator};
 use crate::chain::channelmonitor::ANTI_REORG_DELAY;
 use crate::chain::package::{PackageSolvingData, PackageTemplate};
@@ -670,19 +670,8 @@ impl<ChannelSigner: EcdsaChannelSigner> OnchainTxHandler<ChannelSigner> {
 
 					let fee_sat = input_amount_sats - tx.output.iter()
 						.map(|output| output.value.to_sat()).sum::<u64>();
-					let commitment_tx_feerate_sat_per_1000_weight =
-						compute_feerate_sat_per_1000_weight(fee_sat, tx.weight().to_wu());
 					let package_target_feerate_sat_per_1000_weight = cached_request
 						.compute_package_feerate(fee_estimator, conf_target, feerate_strategy);
-					if commitment_tx_feerate_sat_per_1000_weight >= package_target_feerate_sat_per_1000_weight {
-						log_debug!(logger, "Pre-signed commitment {} already has feerate {} sat/kW above required {} sat/kW",
-							tx.compute_txid(), commitment_tx_feerate_sat_per_1000_weight,
-							package_target_feerate_sat_per_1000_weight);
-						// The commitment transaction already meets the required feerate and doesn't
-						// need a CPFP. We still want to return something other than the event to
-						// register the claim.
-						return Some((new_timer, 0, OnchainClaim::Tx(MaybeSignedTransaction(tx))));
-					}
 
 					// We'll locate an anchor output we can spend within the commitment transaction.
 					let channel_parameters = output.channel_parameters.as_ref()

lightning/src/events/bump_transaction/mod.rs

@@ -16,7 +16,9 @@ pub mod sync;
 use alloc::collections::BTreeMap;
 use core::ops::Deref;
 
-use crate::chain::chaininterface::{fee_for_weight, BroadcasterInterface};
+use crate::chain::chaininterface::{
+	compute_feerate_sat_per_1000_weight, fee_for_weight, BroadcasterInterface,
+};
 use crate::chain::ClaimId;
 use crate::io_extras::sink;
 use crate::ln::chan_utils;
@@ -123,7 +125,9 @@ pub enum BumpTransactionEvent {
 	/// and child anchor transactions), possibly resulting in a loss of funds. Once the transaction
 	/// is constructed, it must be fully signed for and broadcast by the consumer of the event
 	/// along with the `commitment_tx` enclosed. Note that the `commitment_tx` must always be
-	/// broadcast first, as the child anchor transaction depends on it.
+	/// broadcast first, as the child anchor transaction depends on it. It is also possible that the
+	/// feerate of the commitment transaction is already sufficient, in which case the child anchor
+	/// transaction is not needed and only the commitment transaction should be broadcast.
 	///
 	/// The consumer should be able to sign for any of the additional inputs included within the
 	/// child anchor transaction. To sign its anchor input, an [`EcdsaChannelSigner`] should be
@@ -658,6 +662,19 @@ where
 		commitment_tx: &Transaction, commitment_tx_fee_sat: u64,
 		anchor_descriptor: &AnchorDescriptor,
 	) -> Result<(), ()> {
+		// First, check if the commitment transaction has sufficient fees on its own.
+		let commitment_tx_feerate_sat_per_1000_weight = compute_feerate_sat_per_1000_weight(
+			commitment_tx_fee_sat,
+			commitment_tx.weight().to_wu(),
+		);
+		if commitment_tx_feerate_sat_per_1000_weight >= package_target_feerate_sat_per_1000_weight {
+			log_debug!(self.logger, "Pre-signed commitment {} already has feerate {} sat/kW above required {} sat/kW, broadcasting.",
+				commitment_tx.compute_txid(), commitment_tx_feerate_sat_per_1000_weight,
+				package_target_feerate_sat_per_1000_weight);
+			self.broadcaster.broadcast_transactions(&[&commitment_tx]);
+			return Ok(());
+		}
+
 		// Our commitment transaction already has fees allocated to it, so we should take them into
 		// account. We do so by pretending the commitment transaction's fee and weight are part of
 		// the anchor input.

lightning/src/ln/async_signer_tests.rs

@@ -1051,6 +1051,9 @@ fn do_test_async_holder_signatures(anchors: bool, remote_commitment: bool) {
 			&nodes[0].logger,
 		);
 	}
+	if anchors {
+		handle_bump_close_event(closing_node);
+	}
 
 	let commitment_tx = {
 		let mut txn = closing_node.tx_broadcaster.txn_broadcast();

lightning/src/ln/functional_test_utils.rs

@@ -2211,21 +2211,37 @@ macro_rules! check_closed_event {
 	};
 }
 
-pub fn handle_bump_htlc_event(node: &Node, count: usize) {
+pub fn handle_bump_events(node: &Node, expected_close: bool, expected_htlc_count: usize) {
 	let events = node.chain_monitor.chain_monitor.get_and_clear_pending_events();
-	assert_eq!(events.len(), count);
-	for event in events {
+	let mut close = false;
+	let mut htlc_count = 0;
+	for event in &events {
 		match event {
-			Event::BumpTransaction(bump_event) => {
-				if let BumpTransactionEvent::HTLCResolution { .. } = &bump_event {
-				} else {
-					panic!();
-				}
-				node.bump_tx_handler.handle_event(&bump_event);
+			Event::BumpTransaction(bump @ BumpTransactionEvent::ChannelClose { .. }) => {
+				close = true;
+				node.bump_tx_handler.handle_event(&bump);
 			},
-			_ => panic!(),
+			Event::BumpTransaction(bump @ BumpTransactionEvent::HTLCResolution { .. }) => {
+				htlc_count += 1;
+				node.bump_tx_handler.handle_event(&bump);
+			},
+			_ => panic!("Unexpected non-bump event: {:?}.", event),
 		}
 	}
+	assert_eq!(close, expected_close, "Expected a bump close event, found {:?}.", events);
+	assert_eq!(
+		htlc_count, expected_htlc_count,
+		"Expected {} bump HTLC events, found {:?}",
+		expected_htlc_count, events
+	);
+}
+
+pub fn handle_bump_close_event(node: &Node) {
+	handle_bump_events(node, true, 0);
+}
+
+pub fn handle_bump_htlc_event(node: &Node, count: usize) {
+	handle_bump_events(node, false, count);
 }
 
 pub fn close_channel<'a, 'b, 'c>(

lightning/src/ln/monitor_tests.rs

@@ -896,6 +896,9 @@ fn do_test_balances_on_local_commitment_htlcs(anchors: bool) {
 	check_closed_broadcast!(nodes[0], true);
 	let reason = ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true), message };
 	check_closed_event!(nodes[0], 1, reason, [nodes[1].node.get_our_node_id()], 1000000);
+	if anchors {
+		handle_bump_close_event(&nodes[0]);
+	}
 	let commitment_tx = {
 		let mut txn = nodes[0].tx_broadcaster.unique_txn_broadcast();
 		assert_eq!(txn.len(), 1);
@@ -905,9 +908,15 @@ fn do_test_balances_on_local_commitment_htlcs(anchors: bool) {
 	};
 	let commitment_tx_conf_height_a = block_from_scid(mine_transaction(&nodes[0], &commitment_tx));
 	if nodes[0].connect_style.borrow().updates_best_block_first() {
+		if anchors {
+			handle_bump_close_event(&nodes[0]);
+		}
 		let mut txn = nodes[0].tx_broadcaster.txn_broadcast();
-		assert_eq!(txn.len(), 1);
+		assert_eq!(txn.len(), if anchors { 2 } else { 1 });
 		assert_eq!(txn[0].compute_txid(), commitment_tx.compute_txid());
+		if anchors {
+			check_spends!(txn[1], txn[0]);  // Anchor output spend.
+		}
 	}
 
 	let htlc_balance_known_preimage = Balance::MaybeTimeoutClaimableHTLC {
@@ -2486,6 +2495,7 @@ fn do_test_yield_anchors_events(have_htlcs: bool) {
 		nodes[1].node.force_close_broadcasting_latest_txn(&chan_id, &nodes[0].node.get_our_node_id(), "".to_string()).unwrap();
 	}
 	{
+		handle_bump_close_event(&nodes[1]);
 		let txn = nodes[1].tx_broadcaster.txn_broadcast();
 		assert_eq!(txn.len(), 1);
 		check_spends!(txn[0], funding_tx);
@@ -2553,15 +2563,18 @@ fn do_test_yield_anchors_events(have_htlcs: bool) {
 	}
 
 	{
+		if nodes[1].connect_style.borrow().updates_best_block_first() {
+			handle_bump_close_event(&nodes[1]);
+		}
 		let mut txn = nodes[1].tx_broadcaster.unique_txn_broadcast();
 		// Both HTLC claims are pinnable at this point,
 		// and will be broadcast in a single transaction.
-		assert_eq!(txn.len(), if nodes[1].connect_style.borrow().updates_best_block_first() { 2 } else { 1 });
+		assert_eq!(txn.len(), if nodes[1].connect_style.borrow().updates_best_block_first() { 3 } else { 1 });
 		if nodes[1].connect_style.borrow().updates_best_block_first() {
-			let new_commitment_tx = txn.remove(0);
-			check_spends!(new_commitment_tx, funding_tx);
+			check_spends!(txn[1], funding_tx);
+			check_spends!(txn[2], txn[1]);  // Anchor output spend.
 		}
-		let htlc_claim_tx = txn.pop().unwrap();
+		let htlc_claim_tx = &txn[0];
 		assert_eq!(htlc_claim_tx.input.len(), 2);
 		assert_eq!(htlc_claim_tx.input[0].previous_output.vout, 2);
 		assert_eq!(htlc_claim_tx.input[1].previous_output.vout, 3);
@@ -2952,6 +2965,7 @@ fn do_test_anchors_monitor_fixes_counterparty_payment_script_on_reload(confirm_c
 	let reason = ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true), message };
 	check_closed_event!(&nodes[0], 1, reason, false,
 		 [nodes[1].node.get_our_node_id()], 100000);
+	handle_bump_close_event(&nodes[0]);
 
 	let commitment_tx = {
 		let mut txn = nodes[0].tx_broadcaster.unique_txn_broadcast();
@@ -3036,6 +3050,9 @@ fn do_test_monitor_claims_with_random_signatures(anchors: bool, confirm_counterp
 	get_monitor!(closing_node, chan_id).broadcast_latest_holder_commitment_txn(
 		&closing_node.tx_broadcaster, &closing_node.fee_estimator, &closing_node.logger
 	);
+	if anchors {
+		handle_bump_close_event(&closing_node);
+	}
 
 	// The commitment transaction comes first.
 	let commitment_tx = {

lightning/src/ln/reorg_tests.rs

@@ -844,6 +844,9 @@ fn do_test_retries_own_commitment_broadcast_after_reorg(anchors: bool, revoked_c
 	check_closed_broadcast(&nodes[0], 1, true);
 	check_added_monitors(&nodes[0], 1);
 	check_closed_event(&nodes[0], 1, ClosureReason::HTLCsTimedOut, false, &[nodes[1].node.get_our_node_id()], 100_000);
+	if anchors {
+		handle_bump_close_event(&nodes[0]);
+	}
 
 	{
 		let mut txn = nodes[0].tx_broadcaster.txn_broadcast();
@@ -870,6 +873,9 @@ fn do_test_retries_own_commitment_broadcast_after_reorg(anchors: bool, revoked_c
 	check_added_monitors(&nodes[1], 1);
 	let reason = ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true), message };
 	check_closed_event(&nodes[1], 1, reason, false, &[nodes[0].node.get_our_node_id()], 100_000);
+	if anchors {
+		handle_bump_close_event(&nodes[1]);
+	}
 
 	let commitment_b = {
 		let mut txn = nodes[1].tx_broadcaster.txn_broadcast();
@@ -882,13 +888,23 @@ fn do_test_retries_own_commitment_broadcast_after_reorg(anchors: bool, revoked_c
 	// Confirm B's commitment, A should now broadcast an HTLC timeout for commitment B.
 	mine_transaction(&nodes[0], &commitment_b);
 	{
-		let mut txn = nodes[0].tx_broadcaster.txn_broadcast();
 		if nodes[0].connect_style.borrow().updates_best_block_first() {
 			// `commitment_a` is rebroadcast because the best block was updated prior to seeing
 			// `commitment_b`.
-			assert_eq!(txn.len(), 2);
-			check_spends!(txn.last().unwrap(), commitment_b);
+			if anchors {
+				handle_bump_close_event(&nodes[0]);
+				let mut txn = nodes[0].tx_broadcaster.txn_broadcast();
+				assert_eq!(txn.len(), 3);
+				check_spends!(txn[0], commitment_b);
+				check_spends!(txn[1], funding_tx);
+				check_spends!(txn[2], txn[1]);  // Anchor output spend transaction.
+			} else {
+				let mut txn = nodes[0].tx_broadcaster.txn_broadcast();
+				assert_eq!(txn.len(), 2);
+				check_spends!(txn.last().unwrap(), commitment_b);
+			}
 		} else {
+			let mut txn = nodes[0].tx_broadcaster.txn_broadcast();
 			assert_eq!(txn.len(), 1);
 			check_spends!(txn[0], commitment_b);
 		}
@@ -898,11 +914,15 @@ fn do_test_retries_own_commitment_broadcast_after_reorg(anchors: bool, revoked_c
 	// blocks, one to get us back to the original height, and another to retry our pending claims.
 	disconnect_blocks(&nodes[0], 1);
 	connect_blocks(&nodes[0], 2);
+	if anchors {
+		handle_bump_close_event(&nodes[0]);
+	}
 	{
 		let mut txn = nodes[0].tx_broadcaster.unique_txn_broadcast();
 		if anchors {
-			assert_eq!(txn.len(), 1);
+			assert_eq!(txn.len(), 2);
 			check_spends!(txn[0], funding_tx);
+			check_spends!(txn[1], txn[0]);  // Anchor output spend.
 		} else {
 			assert_eq!(txn.len(), 2);
 			check_spends!(txn[0], txn[1]); // HTLC timeout A
@@ -977,6 +997,7 @@ fn do_test_split_htlc_expiry_tracking(use_third_htlc: bool, reorg_out: bool) {
 	let message = "Channel force-closed".to_owned();
 	let reason = ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true), message };
 	check_closed_event(&nodes[1], 1, reason, false, &[node_a_id], 10_000_000);
+	handle_bump_close_event(&nodes[1]);
 
 	let mut txn = nodes[1].tx_broadcaster.txn_broadcast();
 	assert_eq!(txn.len(), 1);
@@ -990,19 +1011,13 @@ fn do_test_split_htlc_expiry_tracking(use_third_htlc: bool, reorg_out: bool) {
 	check_added_monitors(&nodes[0], 1);
 
 	mine_transaction(&nodes[1], &commitment_tx);
-	let mut bump_events = nodes[1].chain_monitor.chain_monitor.get_and_clear_pending_events();
-	assert_eq!(bump_events.len(), 1);
-	match bump_events.pop().unwrap() {
-		Event::BumpTransaction(bump_event) => {
-			nodes[1].bump_tx_handler.handle_event(&bump_event);
-		},
-		ev => panic!("Unexpected event {ev:?}"),
-	}
+	handle_bump_events(&nodes[1], nodes[1].connect_style.borrow().updates_best_block_first(), 1);
 
 	let mut txn = nodes[1].tx_broadcaster.txn_broadcast();
 	if nodes[1].connect_style.borrow().updates_best_block_first() {
-		assert_eq!(txn.len(), 2, "{txn:?}");
+		assert_eq!(txn.len(), 3, "{txn:?}");
 		check_spends!(txn[0], funding_tx);
+		check_spends!(txn[1], txn[0]);  // Anchor output spend.
 	} else {
 		assert_eq!(txn.len(), 1, "{txn:?}");
 	}