Merge pull request #4001 from wvanlint/wvanlint/always_emit_bump_event

Always emit bump events, even when fees are sufficient

Author: TheBlueMatt

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:?}");
 	}