Improve prediction of commitment stats in `can_send_update_fee`
What the developer wrote
Authored by Leo Nash
Improve prediction of commitment stats in `can_send_update_fee`
`ChannelContext::get_pending_htlc_stats` predicts that the set of HTLCs
on the next commitment will be all the HTLCs in
`ChannelContext.pending_inbound_htlcs`, and
`ChannelContext.pending_outbound_htlcs`, as well as all the outbound
HTLC adds in the holding cell.
This is an overestimate:
* Outbound HTLC removals which have been ACK'ed by the counterparty will
certainly not be present in any *next* commitment, even though they
remain in `pending_outbound_htlcs` (I refer to states
`AwaitingRemoteRevokeToRemove` and `AwaitingRemovedRemoteRevoke`).
* Outbound HTLCs in the `RemoteRemoved` state, will not be present in
the next *local* commitment.
* Inbound HTLCs in the `LocalRemoved` state will not be present in the
next *remote* commitment.
`ChannelContext::build_commitment_stats(funding, true, true, ..)` makes
these errors when predicting the HTLC count on the remote commitment:
* Inbound HTLCs in the state `RemoteAnnounced` are not included, but
they will be in the next remote commitment transaction if the local
ACK's the addition before producing the next remote commitment.
* Inbound HTLCs in the state `AwaitingRemoteRevokeToAnnounce` are not
included, even though the local has ACK'ed the addition.
* Outbound HTLCs in the state `AwaitingRemoteRevokeToRemove` are
counted, even though the local party has ACK'ed the removal.
This commit replaces these functions in favor of the newly added
`ChannelContext::get_next_{local, remote}_commitment_stats` methods,
and fixes the issues described above.
We now always calculate dust exposure using a buffer from
`msg.feerate_per_kw`, and not from
`max(feerate_per_kw, self.feerate_per_kw, self.pending_update_fee)`.
What changed, and why it matters
This commit fixes how a Lightning node predicts the contents of its next commitment transactions when deciding whether it can safely propose a fee update. Previously the code over- or under-counted pending HTLCs depending on their state, which could let the node accept a feerate that leaves it unable to pay the resulting commitment transaction fee or reserve, or that exceeds dust-exposure safety limits. The fix replaces the old prediction helpers with new state-aware methods that model the next local and remote commitments more accurately.
Review the new `get_next_local_commitment_stats` and `get_next_remote_commitment_stats` implementations (not shown in the diff) to confirm they correctly handle every HTLC state transition, and add regression tests covering fee updates at state boundaries such as `AwaitingRemoteRevokeToRemove`, `RemoteRemoved`, `LocalRemoved`, `RemoteAnnounced`, and `AwaitingRemoteRevokeToAnnounce`.
Security signals we found
Incorrect commitment-transaction HTLC prediction could allow accepting a feerate that makes the next commitment unaffordable (fee + reserve > balance).
Over/under-counting of dust HTLCs could allow breaching the configured max dust HTLC exposure.
State-aware HTLC lifecycle handling added to fee-update validation.
Removal of stale HTLCStats fields indicates the old prediction path is no longer used in this function.
Evidence from the diff
In ChannelContext::can_send_update_fee, the previous logic used get_pending_htlc_stats plus build_commitment_stats(..., true, true, ...) to estimate the next remote commitment. This combination mishandled several HTLC states: it counted outbound HTLCs that the counterparty had already ACK-removal of, omitted inbound HTLCs that would appear on the next remote commitment, and used a feerate buffer of max(feerate_per_kw, self.feerate_per_kw, self.pending_update_fee) for dust exposure. The patch removes those helpers from can_send_update_fee and instead calls new get_next_remote_commitment_stats and get_next_local_commitment_stats methods with explicit include_counterparty_unknown_htlcs and a fee buffer derived from msg.feerate_per_kw. It also removes the now-unused outbound_holding_cell_msat and on_holder_tx_outbound_holding_cell_htlcs_count fields from HTLCStats.
Changed components
lightning/src/ln/channel.rsChannelContext::can_send_update_feeHTLCStats structfee update / update_fee negotiationInspect captured patch +9 / −15
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 5aeafd5..a0d241a 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -1116,8 +1116,6 @@ struct HTLCStats {
// htlc on the counterparty's commitment transaction.
extra_nondust_htlc_on_counterparty_tx_dust_exposure_msat: Option<u64>,
on_holder_tx_dust_exposure_msat: u64,
- outbound_holding_cell_msat: u64,
- on_holder_tx_outbound_holding_cell_htlcs_count: u32, // dust HTLCs *non*-included
}
/// A struct gathering data on a commitment, either local or remote.
@@ -4462,11 +4460,12 @@ where
let dust_exposure_limiting_feerate = self.get_dust_exposure_limiting_feerate(
&fee_estimator, funding.get_channel_type(),
);
- let htlc_stats = self.get_pending_htlc_stats(funding, Some(feerate_per_kw), dust_exposure_limiting_feerate);
- 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));
- let holder_balance_msat = stats.local_balance_before_fee_msat - htlc_stats.outbound_holding_cell_msat;
+ // Include outbound update_add_htlc's in the holding cell, and those which haven't yet been ACK'ed by the counterparty (ie. LocalAnnounced HTLCs)
+ let include_counterparty_unknown_htlcs = true;
+ let next_remote_commitment_stats = self.get_next_remote_commitment_stats(funding, None, include_counterparty_unknown_htlcs, CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, feerate_per_kw, dust_exposure_limiting_feerate);
+ let holder_balance_msat = next_remote_commitment_stats.holder_balance_before_fee_msat.expect("The holder's balance before fees should never underflow.");
// 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 {
+ if holder_balance_msat < next_remote_commitment_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;
@@ -4474,11 +4473,13 @@ where
// Note, we evaluate pending htlc "preemptive" trimmed-to-dust threshold at the proposed `feerate_per_kw`.
let max_dust_htlc_exposure_msat = self.get_max_dust_htlc_exposure_msat(dust_exposure_limiting_feerate);
- if htlc_stats.on_holder_tx_dust_exposure_msat > max_dust_htlc_exposure_msat {
+ if next_remote_commitment_stats.dust_exposure_msat > max_dust_htlc_exposure_msat {
log_debug!(logger, "Cannot afford to send new feerate at {} without infringing max dust htlc exposure", feerate_per_kw);
return false;
}
- if htlc_stats.on_counterparty_tx_dust_exposure_msat > max_dust_htlc_exposure_msat {
+
+ let next_local_commitment_stats = self.get_next_local_commitment_stats(funding, None, include_counterparty_unknown_htlcs, CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, feerate_per_kw, dust_exposure_limiting_feerate);
+ if next_local_commitment_stats.dust_exposure_msat > max_dust_htlc_exposure_msat {
log_debug!(logger, "Cannot afford to send new feerate at {} without infringing max dust htlc exposure", feerate_per_kw);
return false;
}
@@ -4844,8 +4845,6 @@ where
}
let mut pending_outbound_htlcs_value_msat = 0;
- let mut outbound_holding_cell_msat = 0;
- let mut on_holder_tx_outbound_holding_cell_htlcs_count = 0;
let mut pending_outbound_htlcs = self.pending_outbound_htlcs.len();
{
let counterparty_dust_limit_success_sat = htlc_success_tx_fee_sat + context.counterparty_dust_limit_satoshis;
@@ -4866,7 +4865,6 @@ where
if let &HTLCUpdateAwaitingACK::AddHTLC { ref amount_msat, .. } = update {
pending_outbound_htlcs += 1;
pending_outbound_htlcs_value_msat += amount_msat;
- outbound_holding_cell_msat += amount_msat;
if *amount_msat / 1000 < counterparty_dust_limit_success_sat {
on_counterparty_tx_dust_exposure_msat += amount_msat;
} else {
@@ -4874,8 +4872,6 @@ where
}
if *amount_msat / 1000 < holder_dust_limit_timeout_sat {
on_holder_tx_dust_exposure_msat += amount_msat;
- } else {
- on_holder_tx_outbound_holding_cell_htlcs_count += 1;
}
}
}
@@ -4913,8 +4909,6 @@ where
on_counterparty_tx_dust_exposure_msat,
extra_nondust_htlc_on_counterparty_tx_dust_exposure_msat,
on_holder_tx_dust_exposure_msat,
- outbound_holding_cell_msat,
- on_holder_tx_outbound_holding_cell_htlcs_count,
}
}
Why this scored 63/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.