Don't trim HTLCs when calculating the commit tx fee including the fee spike multiple#4574
Don't trim HTLCs when calculating the commit tx fee including the fee spike multiple#4574tankyleo wants to merge 6 commits intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @carlaKC as a reviewer! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4574 +/- ##
==========================================
+ Coverage 86.18% 86.43% +0.25%
==========================================
Files 156 158 +2
Lines 108528 108752 +224
Branches 108528 108752 +224
==========================================
+ Hits 93532 93998 +466
+ Misses 12386 12221 -165
+ Partials 2610 2533 -77
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tankyleo
force-pushed
the
2026-04-reserve-breach
branch
from
89ef026 to
059fb46
Compare
As a channel fundee, we previously could send HTLCs such that a fee spike in a legacy channel triggered the no-outputs case in zero-reserve channels. We now also maintain a buffer from the no-outputs case when we are the channel fundee.
We previously accounted for HTLC trims at the spiked feerate when calculating the reserved commitment transaction fees. This could cause an underestimate of the real current commitment fee at the current channel feerate. This is because a 2x increase in the feerate could trim enough HTLCs to result in a smaller commitment transaction fee. Also, the previous code only reserved the fee for an exact 2x increase in the feerate, instead of reserving the fee for any increase in the feerate between 1x to 2x. Fixes lightningdevkit#4563.
Focus on reserved commitment transaction fees, and available capacity in the 0-reserve, fundee case.
tankyleo
force-pushed
the
2026-04-reserve-breach
branch
from
059fb46 to
305a12d
Compare
In the next commit, we will make changes to how the fee spike buffer is calculated which require the real feerate to always be passed to `tx_builder::get_next_commitment_stats`, even in the case where we include a fee spike multiple.
We previously accounted for HTLC trims at the spiked feerate when calculating the commitment transaction fee including the fee spike multiple. This only ensured that the funder of the channel could afford the commitment transaction fee for an exact 2x increase in the feerate. Now, we check that the funder can cover any increase in the feerate between 1x to 2x.
Make sure it correctly does not trim HTLCs at the spiked feerate, see the previous commit for further details.
tankyleo
force-pushed
the
2026-04-reserve-breach
branch
from
305a12d to
6d8b153
Compare
tankyleo
requested review from
TheBlueMatt and
carlaKC
and removed request for
joostjager
| let spiked_feerate = | ||
| feerate_per_kw.saturating_mul(if !channel_type.supports_anchors_zero_fee_htlc_tx() { | ||
| crate::ln::channel::FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE as u32 | ||
| } else { | ||
| 1 | ||
| }, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
The is_outbound_from_holder guard has been removed from the spiked feerate calculation. Previously, only the channel funder (outbound) applied the 2x fee spike buffer here; now it applies to both funder and fundee channels.
This is a behavioral change beyond the stated fix: for non-anchor inbound (fundee) channels, spiked_feerate was previously just feerate_per_kw, and now it's feerate_per_kw * 2. This affects downstream calculations including:
local_max_commit_tx_fee_sat/local_min_commit_tx_fee_sat(though only used in the outbound branch)get_next_splice_out_maximum_sat(usesspiked_feeratefor thehas_outputcheck)- Both
adjust_boundaries_if_max_dust_htlc_produces_no_outputcalls (now use 2x feerate for inbound channels)
This makes capacity reporting more conservative for fundee channels, which may be intentional (aligning reported capacity with what can_accept_incoming_htlc actually accepts). If so, it would be good to mention this in the PR description / commit message as a deliberate secondary change.
| let htlcs_in_tx = vec![ | ||
| HTLCOutputInCommitment { | ||
| offered: false, | ||
| cltv_expiry: 81, | ||
| payment_hash: payment_hashes.iter().find(|hash| hash.0[0] == 0x75).unwrap().clone(), | ||
| amount_msat: 688_000, | ||
| transaction_output_index: Some(0), | ||
| }, | ||
| HTLCOutputInCommitment { | ||
| offered: false, | ||
| cltv_expiry: 81, | ||
| payment_hash: payment_hashes.iter().find(|hash| hash.0[0] == 0x64).unwrap().clone(), | ||
| amount_msat: 688_000, | ||
| transaction_output_index: Some(1), | ||
| }, | ||
| HTLCOutputInCommitment { | ||
| offered: false, | ||
| cltv_expiry: 81, | ||
| payment_hash, | ||
| amount_msat: 688_000, | ||
| transaction_output_index: Some(2), | ||
| }, | ||
| HTLCOutputInCommitment { | ||
| offered: false, | ||
| cltv_expiry: 81, | ||
| payment_hash: payment_hashes.iter().find(|hash| hash.0[0] == 0x72).unwrap().clone(), | ||
| amount_msat: 688_000, | ||
| transaction_output_index: Some(3), | ||
| }, | ||
| HTLCOutputInCommitment { | ||
| offered: false, | ||
| cltv_expiry: 81, | ||
| payment_hash: payment_hashes.iter().find(|hash| hash.0[0] == 0x66).unwrap().clone(), | ||
| amount_msat: 688_000, | ||
| transaction_output_index: Some(4), | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
Identifying HTLCs by hardcoded first-byte values of payment hashes (0x75, 0x64, 0x72, 0x66) is fragile. This depends on the internal deterministic hash generation of the test framework, and will panic at the unwrap() if the test infrastructure changes how payment hashes are derived.
Consider tracking payment hashes directly from the route_payment return values instead. For example, store them in a Vec as they are created, and reference them by index to build htlcs_in_tx.
Review SummaryThe core logic change — using the base-feerate nondust HTLC count with the spiked feerate for fee reservation — is correct. This properly ensures the funder can afford any feerate increase between 1x and 2x (not just exactly 2x), since Inline comments posted:
|
3 participants
We previously accounted for HTLC trims at the spiked feerate when calculating the reserved commitment transaction fees.
This could cause an underestimate of the real current commitment fee at the current channel feerate. This is because a 2x increase in the feerate could trim enough HTLCs to result in a smaller commitment transaction fee.
Also, the previous code only reserved the fee for an exact 2x increase in the feerate, instead of reserving the fee for any increase in the feerate between 1x to 2x.
Fixes #4563.