Improve prediction of commitment stats in `validate_update_fee`
What the developer wrote
Authored by Leo Nash
Improve prediction of commitment stats in `validate_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`.
* 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 received by the counterparty before she sent the
`update_fee`.
* 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.
We now always calculate dust exposure using a buffer from
`msg.feerate_per_kw`, and not from
`max(self.feerate_per_kw, msg.feerate_per_kw)`.
What changed, and why it matters
This patch tightens how a Lightning node estimates which pending payments (HTLCs) will actually appear on the next commitment transaction when checking a peer's proposed fee rate. Previously the node counted HTLCs that might already be removed or not yet known to the peer, which could cause it to reject valid fee updates or, conversely, accept fee updates that expose it to more 'dust' HTLC risk than intended. The change makes the dust-exposure check more accurate by predicting the next local and next remote commitment separately and using only the new fee rate for the dust buffer.
Review the new get_next_local_commitment_stats and get_next_remote_commitment_stats implementations (not shown in this diff) to confirm they correctly exclude the HTLC states described, and ensure the change from max(current, proposed) to proposed-only feerate does not introduce an under-estimation edge case. Consider regression tests covering fee updates with HTLCs in each transitional state.
Security signals we found
Incorrect HTLC set used for fee-update dust-exposure check
Overestimation of pending HTLCs could cause spurious channel closes or missed dust-exposure risk
Dust-exposure buffer changed from max(current, proposed) feerate to proposed feerate only
Patch is defensive/hardening with no CVE or exploit code shown
Evidence from the diff
In ChannelContext::validate_update_fee, the code previously called get_pending_htlc_stats, which over-counted HTLCs: it included outbound HTLCs already ACK-removed by the counterparty, outbound HTLCs in RemoteRemoved state, LocalAnnounced outbound HTLCs not yet ACKed, outbound add_htlcs still in the holding cell, and inbound HTLCs in LocalRemoved state. The patch replaces that with two new methods, get_next_local_commitment_stats and get_next_remote_commitment_stats, with include_counterparty_unknown_htlcs=false, and uses msg.feerate_per_kw (not max(current, msg)) for the dust-exposure limiting feerate. This makes the dust-exposure validation match the actual upcoming commitments more closely.
Changed components
lightning/src/ln/channel.rsChannelContext::validate_update_feeDust HTLC exposure validation during update_feeInspect captured patch +10 / −5
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 0c2c7db..5aeafd5 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -4338,16 +4338,21 @@ 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, None, dust_exposure_limiting_feerate);
+ // 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;
+ let next_local_commitment_stats = self.get_next_local_commitment_stats(funding, None, include_counterparty_unknown_htlcs, 0, msg.feerate_per_kw, dust_exposure_limiting_feerate);
+ let next_remote_commitment_stats = self.get_next_remote_commitment_stats(funding, None, include_counterparty_unknown_htlcs, 0, msg.feerate_per_kw, dust_exposure_limiting_feerate);
+
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_local_commitment_stats.dust_exposure_msat > max_dust_htlc_exposure_msat {
return Err(ChannelError::close(format!("Peer sent update_fee with a feerate ({}) which may over-expose us to dust-in-flight on our own transactions (totaling {} msat)",
- msg.feerate_per_kw, htlc_stats.on_holder_tx_dust_exposure_msat)));
+ msg.feerate_per_kw, next_local_commitment_stats.dust_exposure_msat)));
}
- if htlc_stats.on_counterparty_tx_dust_exposure_msat > max_dust_htlc_exposure_msat {
+ if next_remote_commitment_stats.dust_exposure_msat > max_dust_htlc_exposure_msat {
return Err(ChannelError::close(format!("Peer sent update_fee with a feerate ({}) which may over-expose us to dust-in-flight on our counterparty's transactions (totaling {} msat)",
- msg.feerate_per_kw, htlc_stats.on_counterparty_tx_dust_exposure_msat)));
+ msg.feerate_per_kw, next_remote_commitment_stats.dust_exposure_msat)));
}
+
Ok(())
}
Why this scored 61/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.