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 @carlaKC 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 94.44444% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.40%. Comparing base (964a84f) to head (deb51ae).
⚠️ Report is 76 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	84.21%	2 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   109127     +599     
  Branches   108528   109127     +599     
==========================================
+ Hits        93532    94295     +763     
+ Misses      12386    12288      -98     
+ Partials     2610     2544      -66

Flag	Coverage Δ
fuzzing-fake-hashes	`5.09% <0.00%> (?)`
fuzzing-real-hashes	`22.78% <84.90%> (?)`
tests	`86.13% <94.44%> (-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

I've completed a thorough re-review of the entire PR diff, examining every changed file and hunk. I verified the core logic, all caller sites, parameter threading, and test correctness.

Review Summary

No new bugs or security issues found beyond what was flagged in prior review comments.

Prior inline comments (already posted)

lightning/src/sign/tx_builder.rs:469 — Behavioral change: spiked feerate now applies to inbound (fundee) channels in get_next_commitment_stats via assume_fee_spike, not just outbound channels
lightning/src/ln/htlc_reserve_unit_tests.rs:3738 — Fragile hardcoded payment hash first-byte matching (0x75, 0x64, 0x72, 0x66)
lightning/src/ln/htlc_reserve_unit_tests.rs:3446 — Test DUST_LIMIT_MSAT / scenario coverage question (may be resolved if the constant was corrected since the prior review)
lightning/src/ln/htlc_reserve_unit_tests.rs:3501 — Comment corrections

Verification of correctness

Two-step check in get_next_commitment_stats: Step 1 (has_output) correctly uses spiked_nondust_htlc_count with spiked_feerate. Step 2 (fee affordability) correctly uses nondust_htlc_count (base feerate) with spiked_feerate. The math is sound: since nondust_htlc_count >= spiked_nondust_htlc_count and spiked_feerate >= f for all f in [1x, 2x], commit_tx_fee_sat(spiked_feerate, nondust_htlc_count + addl, ...) is an upper bound on the fee at any intermediate feerate.
get_available_balances changes: local_nondust_htlc_count (base feerate) correctly used for fee computation (local_max_commit_tx_fee_sat, local_min_commit_tx_fee_sat). local_spiked_nondust_htlc_count correctly used for has_output checks and boundary adjustments. Parameter reordering in adjust_boundaries_if_max_dust_htlc_produces_no_output correctly applied at both call sites.
PredictedNextFee safety: All callers that pass assume_fee_spike: true also pass addl_nondust_htlc_count >= 1, so the else branch re-queries with false, ensuring predicted fees match actual commitment fees.
manually_trigger_update_fail_htlc refactoring: Dynamic commitment numbers from signer state replace hardcoded INITIAL_COMMITMENT_NUMBER. Parameter change from channel_value_sat to value_to_self_msat is correctly plumbed from both callers.

Minor latent concern (not a current bug)

The PredictedNextFee test/fuzz assertion in get_next_local_commitment_stats / get_next_remote_commitment_stats implicitly requires that assume_fee_spike: true is never called with addl_nondust_htlc_count == 0 for non-anchor channels. If a future caller violates this, the stored commit_tx_fee_sat (at spiked feerate) would mismatch the actual commitment fee (at base feerate), causing a confusing assertion failure in build_commitment_transaction. A debug_assert!(!assume_fee_spike || addl_nondust_htlc_count > 0) in the test-only block would prevent this.

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.

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.

ldk-reviews-bot · 2026-05-13T00:03:13Z

🔔 3rd Reminder

Hey @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.

carlaKC · 2026-05-13T18:26:43Z

+	// Note here we use the htlc count at the current feerate together with the spiked feerate;
+	// this makes sure that the holder can afford any fee bump between 1x to 2x from the current
+	// feerate.


For my understanding: This is an upper bound of the most HTLCs we can have and the highest feerate we're accounting for, right? If HTLCs are trimmed by the higher spiked feerate, we wouldn't actually end up with local_nondust_htlc_count on the commitment (it would be less).

Yes it's the upper bound. In the worst case, the HTLCs only get trimmed at exactly 2x the feerate, but we bump to eg 1.9x.

If HTLCs are trimmed by the higher spiked feerate, we wouldn't actually end up with local_nondust_htlc_count on the commitment (it would be less).

Yes correct

carlaKC · 2026-05-13T19:09:30Z

 		&self, funding: &FundingScope, htlc_candidate: Option<HTLCAmountDirection>,
 		include_counterparty_unknown_htlcs: bool, addl_nondust_htlc_count: usize,
-		feerate_per_kw: u32, dust_exposure_limiting_feerate: Option<u32>,
+		feerate_per_kw: u32, assuming_future_increased_feerate: bool,


nit: assume_fee_spike to use the same terminology that we have in the codebase about fee spike buffers?

Thanks addressed below

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.

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 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

tankyleo force-pushed the 2026-04-reserve-breach branch from 0a194ba to 76f5b32 Compare May 13, 2026 05:19

tankyleo mentioned this pull request May 13, 2026

Disallow opening and accepting 0-reserve legacy channels #4613

Draft

carlaKC previously approved these changes May 13, 2026

View reviewed changes

tankyleo added 2 commits May 13, 2026 20:41

tankyleo dismissed carlaKC’s stale review via deb51ae May 13, 2026 20:43

tankyleo force-pushed the 2026-04-reserve-breach branch from 76f5b32 to deb51ae Compare May 13, 2026 20:43

tankyleo requested review from TheBlueMatt and carlaKC May 13, 2026 20:44

carlaKC approved these changes May 13, 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)

Verification of correctness

Minor latent concern (not a current bug)

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!

ldk-reviews-bot commented May 13, 2026

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 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