Skip to content
24 changes: 10 additions & 14 deletions lightning/src/ln/channel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4314,8 +4314,7 @@ where

#[rustfmt::skip]
fn can_send_update_fee<F: Deref, L: Deref>(
&self, funding: &FundingScope, holder_commitment_point: &HolderCommitmentPoint,
feerate_per_kw: u32, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L,
&self, funding: &FundingScope, feerate_per_kw: u32, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L,
) -> bool
where
F::Target: FeeEstimator,
Expand All @@ -4324,13 +4323,10 @@ where
// Before proposing a feerate update, check that we can actually afford the new fee.
let dust_exposure_limiting_feerate = self.get_dust_exposure_limiting_feerate(&fee_estimator);
let htlc_stats = self.get_pending_htlc_stats(funding, Some(feerate_per_kw), dust_exposure_limiting_feerate);
let commitment_data = self.build_commitment_transaction(
funding, holder_commitment_point.transaction_number(),
&holder_commitment_point.current_point(), true, true, logger,
);
let buffer_fee_msat = commit_tx_fee_sat(feerate_per_kw, commitment_data.tx.nondust_htlcs().len() + htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, funding.get_channel_type()) * 1000;
let holder_balance_msat = commitment_data.stats.local_balance_before_fee_msat - htlc_stats.outbound_holding_cell_msat;
if holder_balance_msat < buffer_fee_msat + funding.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 {
let stats = self.build_commitment_stats(funding, true, true, Some(feerate_per_kw), Some(htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Combining on_holder_tx_outbound_holding_cell_htlcs_count and CONCURRENT_INBOUND_HTLC_FEE_BUFFER in a param called fee_buffer_nondust_htlcs is a little confusing to me - one is a set of pending HTLCs we know we'll need to account for, the other part of fee spike buffer.

Perhaps this can just be num_htlcs like we have it in commit_tx_fee_sat? Or num_pending_htlcs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100% agreed on the bad name here - would you take something like extra_htlcs ? To emphasize that these are HTLCs added on top of the ones build_commitment_stats will count by default.

Copy link
Contributor Author

@tankyleo tankyleo Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the other part of fee spike buffer

A clarification here: as far as I see, this is meant to account for the case where the counterparty sends an update_add_htlc at the same time we send update_fee

let holder_balance_msat = stats.local_balance_before_fee_msat - htlc_stats.outbound_holding_cell_msat;
// Note that `stats.commit_tx_fee_sat` accounts for any HTLCs that transition from non-dust to dust under a higher feerate (in the case where HTLC-transactions pay endogenous fees).
if holder_balance_msat < stats.commit_tx_fee_sat * 1000 + funding.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 {
//TODO: auto-close after a number of failures?
log_debug!(logger, "Cannot afford to send new feerate at {}", feerate_per_kw);
return false;
Expand Down Expand Up @@ -4447,15 +4443,15 @@ where
/// commitment transaction. See `build_commitment_transaction` for further docs.
#[inline]
#[rustfmt::skip]
fn build_commitment_stats(&self, funding: &FundingScope, local: bool, generated_by_local: bool) -> CommitmentStats {
fn build_commitment_stats(&self, funding: &FundingScope, local: bool, generated_by_local: bool, feerate_per_kw: Option<u32>, fee_buffer_nondust_htlcs: Option<usize>) -> CommitmentStats {
let broadcaster_dust_limit_sat = if local { self.holder_dust_limit_satoshis } else { self.counterparty_dust_limit_satoshis };
let mut non_dust_htlc_count = 0;
let mut remote_htlc_total_msat = 0;
let mut local_htlc_total_msat = 0;
let mut value_to_self_claimed_msat = 0;
let mut value_to_remote_claimed_msat = 0;

let feerate_per_kw = self.get_commitment_feerate(funding, generated_by_local);
let feerate_per_kw = feerate_per_kw.unwrap_or_else(|| self.get_commitment_feerate(funding, generated_by_local));

for htlc in self.pending_inbound_htlcs.iter() {
if htlc.state.included_in_commitment(generated_by_local) {
Expand Down Expand Up @@ -4508,7 +4504,7 @@ where
broadcaster_max_commitment_tx_output.1 = cmp::max(broadcaster_max_commitment_tx_output.1, value_to_remote_msat);
}

let commit_tx_fee_sat = commit_tx_fee_sat(feerate_per_kw, non_dust_htlc_count, &funding.channel_transaction_parameters.channel_type_features);
let commit_tx_fee_sat = commit_tx_fee_sat(feerate_per_kw, non_dust_htlc_count + fee_buffer_nondust_htlcs.unwrap_or(0), &funding.channel_transaction_parameters.channel_type_features);
let total_anchors_sat = if funding.channel_transaction_parameters.channel_type_features.supports_anchors_zero_fee_htlc_tx() { ANCHOR_OUTPUT_VALUE_SATOSHI * 2 } else { 0 };

// We MUST use saturating subs here, as the funder's balance is not guaranteed to be greater
Expand Down Expand Up @@ -4552,7 +4548,7 @@ where
let broadcaster_dust_limit_sat = if local { self.holder_dust_limit_satoshis } else { self.counterparty_dust_limit_satoshis };
let feerate_per_kw = self.get_commitment_feerate(funding, generated_by_local);

let stats = self.build_commitment_stats(funding, local, generated_by_local);
let stats = self.build_commitment_stats(funding, local, generated_by_local, None, None);
let CommitmentStats {
commit_tx_fee_sat,
local_balance_before_fee_msat,
Expand Down Expand Up @@ -7659,7 +7655,7 @@ where

let can_send_update_fee = core::iter::once(&self.funding)
.chain(self.pending_funding.iter())
.all(|funding| self.context.can_send_update_fee(funding, &self.holder_commitment_point, feerate_per_kw, fee_estimator, logger));
.all(|funding| self.context.can_send_update_fee(funding, feerate_per_kw, fee_estimator, logger));
if !can_send_update_fee {
return None;
}
Expand Down
182 changes: 182 additions & 0 deletions lightning/src/ln/update_fee_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1193,3 +1193,185 @@ pub fn do_cannot_afford_on_holding_cell_release(
nodes[0].logger.assert_log("lightning::ln::channel", err, 1);
}
}

#[xtest(feature = "_externalize_tests")]
pub fn can_afford_given_trimmed_htlcs() {
do_can_afford_given_trimmed_htlcs(core::cmp::Ordering::Equal);
do_can_afford_given_trimmed_htlcs(core::cmp::Ordering::Greater);
do_can_afford_given_trimmed_htlcs(core::cmp::Ordering::Less);
}

pub fn do_can_afford_given_trimmed_htlcs(inequality_regions: core::cmp::Ordering) {
// Test that when we check whether we can afford a feerate update, we account for the
// decrease in the weight of the commitment transaction due to newly trimmed HTLCs at the higher feerate.
//
// Place a non-dust HTLC on the transaction, increase the feerate such that the HTLC
// gets trimmed, and finally check whether we were able to afford the new feerate.

let channel_type = ChannelTypeFeatures::only_static_remote_key();
let can_afford = match inequality_regions {
core::cmp::Ordering::Less => false,
core::cmp::Ordering::Equal => true,
core::cmp::Ordering::Greater => true,
};
let inequality_boundary_offset = match inequality_regions {
core::cmp::Ordering::Less => 0,
core::cmp::Ordering::Equal => 1,
core::cmp::Ordering::Greater => 2,
};

let chanmon_cfgs = create_chanmon_cfgs(2);

let mut default_config = test_default_channel_config();
default_config.channel_handshake_config.max_inbound_htlc_value_in_flight_percent_of_channel =
100;

let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
let node_chanmgrs =
create_node_chanmgrs(2, &node_cfgs, &[Some(default_config.clone()), Some(default_config)]);

let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);

let node_a_id = nodes[0].node.get_our_node_id();
let node_b_id = nodes[1].node.get_our_node_id();

// We will update the feerate from 253sat/kw to 1000sat/kw
let target_feerate = 1000;
// Set a HTLC amount that is non-dust at 253sat/kw and dust at 1000sat/kw
let node_0_inbound_htlc_amount_sat = 750;

// This is the number of HTLCs that `can_send_update_fee` will account for when checking
// whether node 0 can afford the target feerate. We do not include the inbound HTLC we will send,
// as that HTLC will be trimmed at the new feerate.
let buffer_tx_fee_sat = chan_utils::commit_tx_fee_sat(
target_feerate,
crate::ln::channel::CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize,
&channel_type,
);
let channel_reserve_satoshis = 1000;

let channel_value_sat = 100_000;
let node_0_balance_sat =
(buffer_tx_fee_sat + channel_reserve_satoshis) - 1 + inequality_boundary_offset;
let node_1_balance_sat = channel_value_sat - node_0_balance_sat;

let chan_id =
create_chan_between_nodes_with_value(&nodes[0], &nodes[1], channel_value_sat, 0).3;
{
// Double check the reserve here
let per_peer_state_lock;
let mut peer_state_lock;
let chan =
get_channel_ref!(nodes[1], nodes[0], per_peer_state_lock, peer_state_lock, chan_id);
assert_eq!(
chan.funding().holder_selected_channel_reserve_satoshis,
channel_reserve_satoshis
);
}

// Set node 0's balance at some offset from the inequality boundary
send_payment(&nodes[0], &[&nodes[1]], node_1_balance_sat * 1000);

// Route the HTLC from node 1 to node 0
route_payment(&nodes[1], &[&nodes[0]], node_0_inbound_htlc_amount_sat * 1000);

// Confirm the feerate on node 0's commitment transaction
{
let expected_tx_fee_sat = chan_utils::commit_tx_fee_sat(253, 1, &channel_type);
let commitment_tx = get_local_commitment_txn!(nodes[0], chan_id)[0].clone();

let mut actual_fee = commitment_tx
.output
.iter()
.map(|output| output.value.to_sat())
.reduce(|acc, value| acc + value)
.unwrap();
actual_fee = channel_value_sat - actual_fee;
assert_eq!(expected_tx_fee_sat, actual_fee);

// The HTLC is non-dust...
assert_eq!(commitment_tx.output.len(), 3);
}

{
// Bump the feerate
let mut feerate_lock = chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap();
*feerate_lock = target_feerate;
}
nodes[0].node.timer_tick_occurred();
check_added_monitors(&nodes[0], if can_afford { 1 } else { 0 });
let mut events = nodes[0].node.get_and_clear_pending_msg_events();

if can_afford {
// We could afford the target feerate, sanity check everything
assert_eq!(events.len(), 1);
if let MessageSendEvent::UpdateHTLCs { node_id, channel_id, updates } =
events.pop().unwrap()
{
assert_eq!(node_id, node_b_id);
assert_eq!(channel_id, chan_id);
assert_eq!(updates.commitment_signed.len(), 1);
// The HTLC is now trimmed!
assert_eq!(updates.commitment_signed[0].htlc_signatures.len(), 0);
assert_eq!(updates.update_add_htlcs.len(), 0);
assert_eq!(updates.update_fulfill_htlcs.len(), 0);
assert_eq!(updates.update_fail_htlcs.len(), 0);
assert_eq!(updates.update_fail_malformed_htlcs.len(), 0);
let update_fee = updates.update_fee.unwrap();
assert_eq!(update_fee.channel_id, chan_id);
assert_eq!(update_fee.feerate_per_kw, target_feerate);

nodes[1].node.handle_update_fee(node_a_id, &update_fee);
commitment_signed_dance!(nodes[1], nodes[0], updates.commitment_signed, false);

// Confirm the feerate on node 0's commitment transaction
{
// Also add the trimmed HTLC to the fees
let expected_tx_fee_sat =
chan_utils::commit_tx_fee_sat(target_feerate, 0, &channel_type)
+ node_0_inbound_htlc_amount_sat;
let commitment_tx = get_local_commitment_txn!(nodes[0], channel_id)[0].clone();

let mut actual_fee = commitment_tx
.output
.iter()
.map(|output| output.value.to_sat())
.reduce(|acc, value| acc + value)
.unwrap();
actual_fee = channel_value_sat - actual_fee;
assert_eq!(expected_tx_fee_sat, actual_fee);

// The HTLC is now trimmed!
assert_eq!(commitment_tx.output.len(), 2);
}

// Confirm the feerate on node 1's commitment transaction
{
// Also add the trimmed HTLC to the fees
let expected_tx_fee_sat =
chan_utils::commit_tx_fee_sat(target_feerate, 0, &channel_type)
+ node_0_inbound_htlc_amount_sat;
let commitment_tx = get_local_commitment_txn!(nodes[1], channel_id)[0].clone();

let mut actual_fee = commitment_tx
.output
.iter()
.map(|output| output.value.to_sat())
.reduce(|acc, value| acc + value)
.unwrap();
actual_fee = channel_value_sat - actual_fee;
assert_eq!(expected_tx_fee_sat, actual_fee);

// The HTLC is now trimmed!
assert_eq!(commitment_tx.output.len(), 2);
}
} else {
panic!();
}
} else {
// We could not afford the target feerate, no events should be generated
assert_eq!(events.len(), 0);
let err = format!("Cannot afford to send new feerate at {}", target_feerate);
nodes[0].logger.assert_log("lightning::ln::channel", err, 1);
}
}