← Watch feed
Improve prediction of commitment stats in `can_accept_incoming_htlc`
Public commit record
73/100 · Adequate
What the developer wrote
Authored by Leo Nash
Improve prediction of commitment stats in `can_accept_incoming_htlc`
`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`.
* Outbound HTLCs in the `RemoteRemoved` state, will not be present in
the next *local* commitment.
* Outbound HTLCs in the `LocalAnnounced` state have no guarantee that
they were yet received by the counterparty.
* Outbound `update_add_htlc`'s in the holding cell are certainly not
known by the counterparty, and we will reevaluate their addition to
the channel when freeing the holding cell.
* Inbound HTLCs in the `LocalRemoved` state will not be present in the
next *remote* commitment.
This commit stops using `get_pending_htlc_stats` in favor of the newly
added `ChannelContext::get_next_{local, remote}_commitment_stats`
methods, and fixes the issues described above.
`ChannelContext::next_remote_commit_tx_fee_msat` counts inbound HTLCs in
the `LocalRemoved` state, as well as outbound HTLCs in the
`LocalAnnounced` state. We now do not count them for the same reasons
described above.
Inbound `LocalRemoved` HTLCs that were **not** successful are now
credited to `remote_balance_before_fee_msat` as they will certainly not
be on the next remote commitment. We previously debited these from the
remote balance to arrive at `remote_balance_before_fee_msat`.
We now always check holder dust exposure, whereas we previously would
only do it if the incoming HTLC was dust on our own commitment
transaction.
Furthermore, dust exposure calculations now take a buffer from the
currently committed feerate, and ignore any fee updates in
`ChannelContext.pending_update_fee`.
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context
What changed, and why it matters
This commit fixes how a Lightning node predicts the contents of its next commitment transactions when deciding whether to accept an incoming payment (HTLC). Previously, the node overestimated which HTLCs would appear on the next commitment, which could cause it to wrongly reject valid incoming payments (a denial-of-service/availability issue) or apply incorrect fee and dust checks. The patch replaces the overestimate with more precise per-commitment statistics and tightens dust-exposure checks for the node's own commitment transaction.
Recommended action
Review the new get_next_local_commitment_stats and get_next_remote_commitment_stats implementations for off-by-one and state-filter correctness, and add regression tests covering the listed HTLC states (LocalAnnounced, RemoteRemoved, LocalRemoved, holding-cell adds) to ensure acceptance/rejection decisions match the intended next-commitment contents.
Security signals we found
01
Overestimated commitment HTLC set could cause incorrect rejection of incoming HTLCs (availability/fee-spike DoS)
02
Dust exposure check on holder commitment was conditional and ignored pending fee updates
03
Balance/fee calculations included HTLC states that cannot appear on the next relevant commitment
04
Patch removes direct use of uncommitted holding-cell adds and un-ACKed outbound adds in acceptance checks
Evidence from the diff
The change removes the use of ChannelContext::get_pending_htlc_stats in can_accept_incoming_htlc and introduces get_next_local_commitment_stats / get_next_remote_commitment_stats. It no longer counts outbound HTLCs that have been ACKed for removal, are RemoteRemoved, or are LocalAnnounced; it also excludes inbound LocalRemoved HTLCs from the next remote commitment and outbound holding-cell adds. next_remote_commit_tx_fee_msat no longer counts LocalRemoved inbound HTLCs or LocalAnnounced outbound HTLCs. Unsuccessful LocalRemoved inbound HTLCs are now credited back to remote_balance_before_fee_msat instead of debited. Dust exposure is now always checked on the holder commitment (not only when the incoming HTLC is dust), uses a buffer from the currently committed feerate, and ignores pending_update_fee. The UpdateAddHTLC argument is removed because the incoming HTLC is already committed by both sides.
Changed components
lightning/src/ln/channel.rslightning/src/ln/channelmanager.rsChannelContext::can_accept_incoming_htlcChannel::can_accept_incoming_htlcCommitment fee and dust-exposure prediction logicInspect captured patch +32 / −61
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index f87b312..0c2c7db 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -4483,79 +4483,50 @@ where
#[rustfmt::skip]
fn can_accept_incoming_htlc<L: Deref>(
- &self, funding: &FundingScope, msg: &msgs::UpdateAddHTLC,
+ &self, funding: &FundingScope,
dust_exposure_limiting_feerate: Option<u32>, logger: &L,
) -> Result<(), LocalHTLCFailureReason>
where
L::Target: Logger,
{
- let htlc_stats = self.get_pending_htlc_stats(funding, None, dust_exposure_limiting_feerate);
+ // The fee spike buffer (an additional nondust HTLC) we keep for the remote if the channel
+ // is not zero fee. This deviates from the spec because the fee spike buffer requirement
+ // doesn't exist on the receiver's side, only on the sender's.
+ let fee_spike_buffer_htlc = if funding.get_channel_type().supports_anchor_zero_fee_commitments() {
+ 0
+ } else {
+ 1
+ };
+ // Do not include outbound update_add_htlc's in the holding cell, or those which haven't yet been ACK'ed by the counterparty (ie. LocalAnnounced HTLCs)
+ let include_counterparty_unknown_htlcs = false;
+ // A `None` `HTLCCandidate` is used as in this case because we're already accounting for
+ // the incoming HTLC as it has been fully committed by both sides.
+ let next_local_commitment_stats = self.get_next_local_commitment_stats(funding, None, include_counterparty_unknown_htlcs, fee_spike_buffer_htlc, self.feerate_per_kw, dust_exposure_limiting_feerate);
+ let next_remote_commitment_stats = self.get_next_remote_commitment_stats(funding, None, include_counterparty_unknown_htlcs, fee_spike_buffer_htlc, self.feerate_per_kw, dust_exposure_limiting_feerate);
+
let max_dust_htlc_exposure_msat = self.get_max_dust_htlc_exposure_msat(dust_exposure_limiting_feerate);
- let on_counterparty_tx_dust_htlc_exposure_msat = htlc_stats.on_counterparty_tx_dust_exposure_msat;
- if on_counterparty_tx_dust_htlc_exposure_msat > max_dust_htlc_exposure_msat {
+ if next_remote_commitment_stats.dust_exposure_msat > max_dust_htlc_exposure_msat {
// Note that the total dust exposure includes both the dust HTLCs and the excess mining fees of the counterparty commitment transaction
log_info!(logger, "Cannot accept value that would put our total dust exposure at {} over the limit {} on counterparty commitment tx",
- on_counterparty_tx_dust_htlc_exposure_msat, max_dust_htlc_exposure_msat);
+ next_remote_commitment_stats.dust_exposure_msat, max_dust_htlc_exposure_msat);
return Err(LocalHTLCFailureReason::DustLimitCounterparty)
}
- let dust_buffer_feerate = self.get_dust_buffer_feerate(None);
- let (htlc_success_tx_fee_sat, _) = second_stage_tx_fees_sat(
- &funding.get_channel_type(), dust_buffer_feerate,
- );
- let exposure_dust_limit_success_sats = htlc_success_tx_fee_sat + self.holder_dust_limit_satoshis;
- if msg.amount_msat / 1000 < exposure_dust_limit_success_sats {
- let on_holder_tx_dust_htlc_exposure_msat = htlc_stats.on_holder_tx_dust_exposure_msat;
- if on_holder_tx_dust_htlc_exposure_msat > max_dust_htlc_exposure_msat {
- log_info!(logger, "Cannot accept value that would put our exposure to dust HTLCs at {} over the limit {} on holder commitment tx",
- on_holder_tx_dust_htlc_exposure_msat, max_dust_htlc_exposure_msat);
- return Err(LocalHTLCFailureReason::DustLimitHolder)
- }
+ if next_local_commitment_stats.dust_exposure_msat > max_dust_htlc_exposure_msat {
+ log_info!(logger, "Cannot accept value that would put our exposure to dust HTLCs at {} over the limit {} on holder commitment tx",
+ next_local_commitment_stats.dust_exposure_msat, max_dust_htlc_exposure_msat);
+ return Err(LocalHTLCFailureReason::DustLimitHolder)
}
if !funding.is_outbound() {
- let removed_outbound_total_msat: u64 = self.pending_outbound_htlcs
- .iter()
- .filter_map(|htlc| {
- matches!(
- htlc.state,
- OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(_, _))
- | OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(_, _))
- )
- .then_some(htlc.amount_msat)
- })
- .sum();
- let pending_value_to_self_msat =
- funding.value_to_self_msat + htlc_stats.pending_inbound_htlcs_value_msat - removed_outbound_total_msat;
- let pending_remote_value_msat =
- funding.get_value_satoshis() * 1000 - pending_value_to_self_msat;
- // Subtract any non-HTLC outputs from the local and remote balances
- let (_, remote_balance_before_fee_msat) = SpecTxBuilder {}.subtract_non_htlc_outputs(
- funding.is_outbound(),
- pending_value_to_self_msat,
- pending_remote_value_msat,
- funding.get_channel_type()
- );
-
- // `Some(())` is for the fee spike buffer we keep for the remote if the channel is
- // not zero fee. This deviates from the spec because the fee spike buffer requirement
- // doesn't exist on the receiver's side, only on the sender's. Note that with anchor
- // outputs we are no longer as sensitive to fee spikes, so we need to account for them.
- //
- // A `None` `HTLCCandidate` is used as in this case because we're already accounting for
- // the incoming HTLC as it has been fully committed by both sides.
- let fee_spike_buffer_htlc = if funding.get_channel_type().supports_anchor_zero_fee_commitments() {
- None
- } else {
- Some(())
- };
-
- let mut remote_fee_cost_incl_stuck_buffer_msat = self.next_remote_commit_tx_fee_msat(
- funding, None, fee_spike_buffer_htlc,
- );
+ let mut remote_fee_incl_fee_spike_buffer_htlc_msat = next_remote_commitment_stats.commit_tx_fee_sat * 1000;
+ // Note that with anchor outputs we are no longer as sensitive to fee spikes, so we don't need to account for them.
if !funding.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
- remote_fee_cost_incl_stuck_buffer_msat *= FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE;
+ remote_fee_incl_fee_spike_buffer_htlc_msat *= FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE;
}
- if remote_balance_before_fee_msat.saturating_sub(funding.holder_selected_channel_reserve_satoshis * 1000) < remote_fee_cost_incl_stuck_buffer_msat {
+ // We unwrap here; if the HTLC exhausts the counterparty's balance, we should have rejected it at `update_add_htlc`, here the HTLC is already
+ // irrevocably committed to the channel.
+ let remote_balance_before_fee_msat = next_remote_commitment_stats.counterparty_balance_before_fee_msat.expect("The counterparty's balance before fees should never underflow");
+ if remote_balance_before_fee_msat.saturating_sub(funding.holder_selected_channel_reserve_satoshis * 1000) < remote_fee_incl_fee_spike_buffer_htlc_msat {
log_info!(logger, "Attempting to fail HTLC due to fee spike buffer violation in channel {}. Rebalancing is required.", &self.channel_id());
return Err(LocalHTLCFailureReason::FeeSpikeBuffer);
}
@@ -9744,7 +9715,7 @@ where
/// this function determines whether to fail the HTLC, or forward / claim it.
#[rustfmt::skip]
pub fn can_accept_incoming_htlc<F: Deref, L: Deref>(
- &self, msg: &msgs::UpdateAddHTLC, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: L
+ &self, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: L
) -> Result<(), LocalHTLCFailureReason>
where
F::Target: FeeEstimator,
@@ -9760,7 +9731,7 @@ where
core::iter::once(&self.funding)
.chain(self.pending_funding.iter())
- .try_for_each(|funding| self.context.can_accept_incoming_htlc(funding, msg, dust_exposure_limiting_feerate, &logger))
+ .try_for_each(|funding| self.context.can_accept_incoming_htlc(funding, dust_exposure_limiting_feerate, &logger))
}
pub fn get_cur_holder_commitment_transaction_number(&self) -> u64 {
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index f42ea46..165dc29 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -6392,7 +6392,7 @@ where
&chan.context,
Some(update_add_htlc.payment_hash),
);
- chan.can_accept_incoming_htlc(update_add_htlc, &self.fee_estimator, &logger)
+ chan.can_accept_incoming_htlc(&self.fee_estimator, &logger)
},
) {
Some(Ok(_)) => {},
Risk score
Our methodology →Why this scored 63/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.