Update fee and dust handling for zero fee channels #3884

carlaKC · 2025-06-23T19:48:29Z

This PR completes the off-chain handling of V3 channels, as described in #3789.

ldk-reviews-bot · 2025-06-23T19:48:31Z

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

carlaKC · 2025-06-23T19:48:54Z

This still needs a few tests, opening up early for conceptual review.

carlaKC · 2025-06-23T19:50:39Z

lightning/src/ln/channel.rs

@@ -5078,6 +5092,7 @@ where
 	) -> u64 {
 		if funding.get_channel_type().supports_anchor_zero_fee_commitments() {
 			debug_assert_eq!(self.feerate_per_kw, 0);
+			debug_assert!(fee_spike_buffer_htlc.is_none());


Wondering whether it's worth having this assertion (which makes it necessary to check channel type before calling fee functions). The alternative would be to just ignore the parameters completely for zero fee channels (even though Some fee_spike_buffer_htlc doesn't make sense for the type).

Can't say I have a strong preference. The assertion seems fine to me.

TheBlueMatt

All LGTM, didn't carefully check every hunk, though.

TheBlueMatt · 2025-06-30T21:54:34Z

lightning/src/ln/channel.rs

@@ -133,6 +134,22 @@ enum FeeUpdateState {
 	Outbound,
 }

+/// Returns the fees for success and timeout second stage HTLC transactions.


nit: Probably belongs in chan_utils.rs given it kinda mirrors commit_and_htlc_tx_fees_sat and is also called from there?

TheBlueMatt · 2025-06-30T21:57:54Z

lightning/src/ln/channel.rs

@@ -5078,6 +5092,7 @@ where
 	) -> u64 {
 		if funding.get_channel_type().supports_anchor_zero_fee_commitments() {
 			debug_assert_eq!(self.feerate_per_kw, 0);
+			debug_assert!(fee_spike_buffer_htlc.is_none());


Can't say I have a strong preference. The assertion seems fine to me.

lightning/src/ln/channel.rs

lightning/src/ln/update_fee_tests.rs

ldk-reviews-bot · 2025-07-14T00:01:04Z

🔔 1st Reminder

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

lightning/src/ln/channel.rs

lightning/src/ln/chan_utils.rs

lightning/src/ln/channel.rs

lightning/src/ln/update_fee_tests.rs

lightning/src/ln/channel.rs

elnosh

Still grokking some of the changes here and the zero-fee commitment proposal but from my kind of limited context it LGTM (:

lightning/src/ln/chan_utils.rs

lightning/src/ln/channel.rs

carlaKC · 2025-07-15T19:35:35Z

Addressed review: only major change is moving responsibility for not triggering update_fee on zero fee channels to channelmanger (as there's been no change between new/old fee) and changing channel to just assert that zero fee channels don't hit update code.

tankyleo

LGTM mod two nits, feel free to squash / ping second reviewer :)

lightning/src/ln/functional_tests.rs

This fee rate is currently used in two scenarios: - To count any fees above what we consider to be a sane estimate towards our dust exposure. - To get a maximum dust exposure (when using MaxDustHTLCExposure::FeeEstimator strategy). When we have zero fee commitments: - Commitments are zero fee, so we don't need to count fees towards dust exposure. - The amount of dust we have is not dependent on fees, as everything is zero fee. - We still want to limit our total dust exposure. This commit updates get_dust_exposure_limiting_feerate to allow a None value to prepare for support for zero fee commitments. This clearly allows us to indicate when we don't care about fee rates for dust considerations. In get_max_dust_htlc_exposure_msat, we simply hardcode a value of 1 sat/vbyte if a feerate dependent strategy is being used.

Co-authored-by: Matt Corallo <[email protected]>

LDK does not support a channel type that supports both zero fee and nonzero fee anchors (as this is impossible), so we can drop the unnecessary check in build_htlc_output.

This commit pulls calculation of second stage fees into a helper function. A side effect of this refactor is that it fixes a rounding issue in commit_and_htlc_tx_fees_sat. Previously, rounding down of fees would happen after multiplying by the number of HTLCs. Now the fees will be rounded down before multiplying by the number of HTLCs. This wasn't a serious issue - it would just cause us very slightly over estimate our dust exposure at certain fee amounts that needed rounding. A hard-coded value in test_nondust_htlc_excess_fees_are_dust is updated to account for this rounding change.

Update second_stage_tx_fees_sat to return zero for zero fee commitments. As is the case for anchors_zero_fee_commitments, this changes ensures that we won't trim the second stage transaction fee off of the HTLC amount.

When we have zero fee commitments, we don't need to calculate our fee rate or check that it isn't stale because it is always zero. Co-authored-by: Matt Corallo <[email protected]>

Co-authored-by: Matt Corallo <[email protected]>

carlaKC · 2025-07-16T18:14:19Z

Squashed + addressed last 2x comments + rebased.

Edit: plus small silent rebase failure, new test on master needed an import this branch deleted.

Update test helper in preparation to test zero fee commitments, where the local balance will differ from the previously hardcoded value.

elnosh · 2025-07-16T19:27:43Z

lightning/src/ln/channel.rs

-		};
-		let real_dust_limit_success_sat = htlc_success_dust_limit + context.holder_dust_limit_satoshis;
-		let real_dust_limit_timeout_sat = htlc_timeout_dust_limit + context.holder_dust_limit_satoshis;
+		if funding.get_channel_type().supports_anchor_zero_fee_commitments() {


non-blocking - should the comment on this method be updated? the htlc is not an Option

TheBlueMatt

One nit and one thing maybe worth following up on, but not worth holding up on.

TheBlueMatt · 2025-07-17T11:14:40Z

lightning/src/ln/chan_utils.rs

+}
+
+/// Returns a fee estimate for the commitment transaction depending on channel type.
+pub(super) fn commitment_sat_per_1000_weight_for_type<F: Deref>(


Not a huge fan of the name because it hides which fee types we're using - we're using the "feerates that we'd want to assign to a channel" types, which are different from the "minimum/maximum we'd let our peer assign to a channel", but the name doesn't capture that.

Can you think of anything less atrociously long than holder_preferred_commitment_sat_per_1000_weight_for_type?

selected_commitment_sat_per_1000_weight?

TheBlueMatt · 2025-07-17T12:11:57Z

lightning/src/ln/channelmanager.rs

-								} else {
-									non_anchor_feerate
-								};
+								let new_feerate = commitment_sat_per_1000_weight_for_type(&self.fee_estimator, funded_chan.funding.get_channel_type());


Hmm, would be nice to keep the old behavior where we fetch the feerates outside the loop. In general implementations of the FeeEstimator trait should be pretty effecient, but if we can reduce the number of calls we probably should. Same above in maybe_update_chan_fees.

#3884 followups

carlaKC commented Jun 23, 2025

View reviewed changes

carlaKC mentioned this pull request Jun 23, 2025

[Custom Transactions] Add TxBuilder trait, support fixed additional outputs #3775

Merged

carlaKC added this to Weekly Goals Jun 26, 2025

carlaKC moved this to Goal: Open in Weekly Goals Jun 26, 2025

TheBlueMatt reviewed Jun 30, 2025

View reviewed changes

jkczyz assigned carlaKC Jul 3, 2025

carlaKC force-pushed the 3789-fees-and-dust branch from 001c8e9 to 4157d24 Compare July 10, 2025 13:16

carlaKC marked this pull request as ready for review July 10, 2025 13:32

ldk-reviews-bot requested a review from joostjager July 10, 2025 13:32

carlaKC removed the request for review from joostjager July 10, 2025 13:32

graphite-app bot reviewed Jul 10, 2025

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

carlaKC force-pushed the 3789-fees-and-dust branch from 4157d24 to 5fe6fa7 Compare July 10, 2025 13:40

graphite-app bot reviewed Jul 10, 2025

View reviewed changes

lightning/src/ln/update_fee_tests.rs Outdated Show resolved Hide resolved

carlaKC mentioned this pull request Jul 10, 2025

[Custom Transactions] Refactor channel validation #3921

Open

tankyleo self-requested a review July 11, 2025 16:49

tankyleo reviewed Jul 14, 2025

View reviewed changes

elnosh reviewed Jul 14, 2025

View reviewed changes

lightning/src/ln/chan_utils.rs Outdated Show resolved Hide resolved

lightning/src/ln/chan_utils.rs Outdated Show resolved Hide resolved

lightning/src/ln/channel.rs Show resolved Hide resolved

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

carlaKC force-pushed the 3789-fees-and-dust branch from 5fe6fa7 to f76a270 Compare July 15, 2025 19:33

tankyleo reviewed Jul 16, 2025

View reviewed changes

lightning/src/ln/functional_tests.rs Outdated Show resolved Hide resolved

lightning/src/ln/functional_tests.rs Outdated Show resolved Hide resolved

carlaKC and others added 7 commits July 16, 2025 14:05

ln/refactor: add utility to get commitment feerate by channel type

6ecacc4

ln: add channel type awareness to get_dust_exposure_limiting_feerate

b497c73

Co-authored-by: Matt Corallo <[email protected]>

ln: remove unnecessary nonzero anchor channel check in build_htlc_output

4a8a50c

LDK does not support a channel type that supports both zero fee and nonzero fee anchors (as this is impossible), so we can drop the unnecessary check in build_htlc_output.

ln: set zero second stage fees for zero fee commitments

2a9b187

Update second_stage_tx_fees_sat to return zero for zero fee commitments. As is the case for anchors_zero_fee_commitments, this changes ensures that we won't trim the second stage transaction fee off of the HTLC amount.

ln: return early and assert in fee calculation and checks

ccf3ef5

When we have zero fee commitments, we don't need to calculate our fee rate or check that it isn't stale because it is always zero. Co-authored-by: Matt Corallo <[email protected]>

ln: do not send and reject receive of update_fee for zero fee commits

ea0341f

Co-authored-by: Matt Corallo <[email protected]>

carlaKC force-pushed the 3789-fees-and-dust branch 2 times, most recently from b421d59 to 60064d1 Compare July 16, 2025 18:13

carlaKC added 2 commits July 16, 2025 14:28

ln: add test helper for fee spike buffer test and use TxBuilder

f12c191

Update test helper in preparation to test zero fee commitments, where the local balance will differ from the previously hardcoded value.

ln: do not maintain fee spike buffer for zero fee commitment channels

54559b3

carlaKC force-pushed the 3789-fees-and-dust branch from 60064d1 to 54559b3 Compare July 16, 2025 18:35

tankyleo approved these changes Jul 16, 2025

View reviewed changes

tankyleo requested a review from TheBlueMatt July 16, 2025 19:04

elnosh approved these changes Jul 16, 2025

View reviewed changes

TheBlueMatt approved these changes Jul 17, 2025

View reviewed changes

TheBlueMatt merged commit eae2bb1 into lightningdevkit:main Jul 17, 2025
25 of 26 checks passed

github-project-automation bot moved this from Goal: Open to Done in Weekly Goals Jul 17, 2025

carlaKC mentioned this pull request Jul 17, 2025

#3884 followups #3938

Merged

TheBlueMatt added a commit that referenced this pull request Jul 18, 2025

Merge pull request #3938 from carlaKC/3789-offchain-followups

1e34de0

#3884 followups

Update fee and dust handling for zero fee channels #3884

Update fee and dust handling for zero fee channels #3884

Uh oh!

Conversation

carlaKC commented Jun 23, 2025

Uh oh!

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

Uh oh!

Uh oh!

carlaKC commented Jun 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

ldk-reviews-bot commented Jul 14, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elnosh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carlaKC commented Jul 15, 2025

Uh oh!

tankyleo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

carlaKC commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

ldk-reviews-bot commented Jun 23, 2025 •

edited

Loading

carlaKC commented Jul 16, 2025 •

edited

Loading