Always emit bump events, even when fees are sufficient

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