Don't trim HTLCs when calculating the commit tx fee including the fee spike multiple by tankyleo · Pull Request #4574 · lightningdevkit/rust-lightning

tankyleo · 2026-04-23T07:17:35Z

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.

ldk-reviews-bot · 2026-04-23T07:17:38Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

codecov · 2026-04-23T08:25:43Z

Codecov Report

❌ Patch coverage is 92.72727% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.40%. Comparing base (964a84f) to head (0a194ba).
⚠️ Report is 70 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	78.94%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4574      +/-   ##
==========================================
+ Coverage   86.18%   86.40%   +0.22%     
==========================================
  Files         156      158       +2     
  Lines      108528   109079     +551     
  Branches   108528   109079     +551     
==========================================
+ Hits        93532    94252     +720     
+ Misses      12386    12282     -104     
+ Partials     2610     2545      -65

Flag	Coverage Δ
fuzzing-fake-hashes	`5.09% <0.00%> (?)`
fuzzing-real-hashes	`22.79% <83.33%> (?)`
tests	`86.13% <92.72%> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ldk-claude-review-bot · 2026-05-06T06:27:59Z

+	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
-		},
-	);
+		});


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 (uses spiked_feerate for the has_output check)

Both adjust_boundaries_if_max_dust_htlc_produces_no_output calls (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.

ldk-claude-review-bot · 2026-05-06T06:28:03Z

+	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),
+		},
+	];


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.

ldk-claude-review-bot · 2026-05-06T06:28:15Z

Good — the else branch correctly passes false for include_fee_spike_multiple, ensuring PredictedNextFee stores values at the base feerate, which will match the actual commitment.

I've now completed a thorough review of the entire diff. The core logic changes are correct. My prior review comments cover all the issues I've found. No new bugs or security issues have been identified in this re-review.

Review Summary

Re-reviewed the entire PR diff thoroughly. The core logic change — using base-feerate nondust HTLC count with spiked feerate for fee reservation — is mathematically sound and correctly ensures the funder can afford any feerate between 1x and 2x (not just exactly 2x).

Prior inline comments (already posted, not repeated)

lightning/src/sign/tx_builder.rs:469 — Behavioral change: spiked feerate now applies to fundee channels in get_next_commitment_stats (the is_outbound_from_holder guard is no longer at the spiked feerate calculation point; callers control via include_fee_spike_multiple)
lightning/src/ln/htlc_reserve_unit_tests.rs:3738 — Fragile hardcoded payment hash first-byte matching
lightning/src/ln/htlc_reserve_unit_tests.rs:3501 — Comment corrections (880sat -> 688sat)

No new bugs or security issues found.

The PredictedNextFee test/fuzzing infrastructure is correctly updated — the else branch (when addl_nondust_htlc_count > 0) re-queries with include_fee_spike_multiple: false and addl=0, ensuring predicted fees match actual commitment fees. All call sites correctly thread the include_fee_spike_multiple parameter: true only in get_holder_counterparty_balances_floor_incl_fee and the fee spike buffer check in can_accept_incoming_htlc.

joostjager · 2026-05-08T12:40:03Z

Just noting that this is the last bigger PR that will make our fuzz tests green again

ldk-reviews-bot · 2026-05-09T00:00:56Z

🔔 1st Reminder

Hey @TheBlueMatt @carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-05-09T00:00:57Z

🔔 1st Reminder

Hey @TheBlueMatt @carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-05-11T00:02:11Z

🔔 2nd Reminder

Hey @TheBlueMatt @carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-05-11T00:02:13Z

🔔 2nd Reminder

Hey @TheBlueMatt @carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

TheBlueMatt

didn't get through this yesterday but here's one comment.

TheBlueMatt · 2026-05-08T20:54:27Z

-	let spiked_feerate = feerate_per_kw.saturating_mul(
-		if is_outbound_from_holder && !channel_type.supports_anchors_zero_fee_htlc_tx() {
+	let spiked_feerate =
+		feerate_per_kw.saturating_mul(if !channel_type.supports_anchors_zero_fee_htlc_tx() {


If the issue is just with legacy 0-reserve channels, should we not be restricting this more and maybe just banning 0-reserve non-anchor channels (seems kinda weird to do such a channel?).

I'd prefer to remove the if is_outbound_from_holder condition here rather than adding more checks to ban 0-reserve legacy channels.

I understand legacy 0-reserve channels are weird, but they still allow the fundee to spend all its balance so they do have their place I'm thinking.

I mean other nodes are gonna start requiring anchors for all new channels (or already have?) and I kinda feel like we should be moving towards the same. I see the reason for them (I guess someone who doesn't want to deal with anchors but wants to have an LSP open to a client?) but non-anchors are just so borderline insecure...

See below I delete the first commit, and will add it to a standalone PR to ban legacy 0-reserve channels.

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.

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.

TheBlueMatt

Okay, yea, I think I'm good with this now. Needs a second reviewer tho.

TheBlueMatt · 2026-05-12T13:27:22Z

 		&self, local: bool, is_outbound_from_holder: bool, channel_value_satoshis: u64,
 		value_to_holder_msat: u64, pending_htlcs: &[HTLCAmountDirection],
-		addl_nondust_htlc_count: usize, feerate_per_kw: u32,
+		addl_nondust_htlc_count: usize, feerate_per_kw: u32, include_fee_spike_multiple: bool,


nit: can we call this something more descriptive? Like assuming_future_increased_feerate? I think the "fee spike multiple" is a bit of an inside-LDK thing and we eventually want this API to be public.

tankyleo self-assigned this Apr 23, 2026

tankyleo added this to Weekly Goals Apr 23, 2026

tankyleo moved this to Goal: Merge in Weekly Goals Apr 23, 2026

tankyleo force-pushed the 2026-04-reserve-breach branch from 89ef026 to 059fb46 Compare April 24, 2026 17:31

tankyleo force-pushed the 2026-04-reserve-breach branch 2 times, most recently from 305a12d to 6d8b153 Compare May 6, 2026 05:39

tankyleo marked this pull request as ready for review May 6, 2026 06:18

ldk-reviews-bot requested a review from joostjager May 6, 2026 06:18

tankyleo requested review from TheBlueMatt and carlaKC and removed request for joostjager May 6, 2026 06:19

ldk-claude-review-bot reviewed May 6, 2026

View reviewed changes

tankyleo added this to the 0.3 milestone May 7, 2026

joostjager mentioned this pull request May 11, 2026

Clamp unsafe next outbound HTLC limit #4610

Draft

TheBlueMatt reviewed May 11, 2026

View reviewed changes

tankyleo requested a review from TheBlueMatt May 11, 2026 17:52

tankyleo force-pushed the 2026-04-reserve-breach branch from 6d8b153 to cc6965f Compare May 12, 2026 07:31

ldk-claude-review-bot reviewed May 12, 2026

View reviewed changes

Comment thread lightning/src/ln/htlc_reserve_unit_tests.rs Outdated

tankyleo force-pushed the 2026-04-reserve-breach branch 2 times, most recently from 0d8def8 to 6211878 Compare May 12, 2026 07:59

ldk-claude-review-bot reviewed May 12, 2026

View reviewed changes

Comment thread lightning/src/ln/htlc_reserve_unit_tests.rs Outdated

tankyleo added 3 commits May 12, 2026 08:17

tankyleo force-pushed the 2026-04-reserve-breach branch from 6211878 to 0a194ba Compare May 12, 2026 08:18

TheBlueMatt reviewed May 12, 2026

View reviewed changes

Conversation

tankyleo commented Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldk-claude-review-bot commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review Summary

Prior inline comments (already posted, not repeated)

No new bugs or security issues found.

Uh oh!

joostjager commented May 8, 2026

Uh oh!

ldk-reviews-bot commented May 9, 2026

Uh oh!

ldk-reviews-bot commented May 9, 2026

Uh oh!

ldk-reviews-bot commented May 11, 2026

Uh oh!

ldk-reviews-bot commented May 11, 2026

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tankyleo commented Apr 23, 2026 •

edited

Loading

ldk-reviews-bot commented Apr 23, 2026 •

edited

Loading

codecov Bot commented Apr 23, 2026 •

edited

Loading

ldk-claude-review-bot commented May 6, 2026 •

edited

Loading