Update fee and dust handling for zero fee channels #3884
Conversation
|
👋 Thanks for assigning @tankyleo as a reviewer! |
|
This still needs a few tests, opening up early for conceptual review. |
| @@ -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()); | |||
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Can't say I have a strong preference. The assertion seems fine to me.
lightning/src/ln/channel.rs
Outdated
| @@ -133,6 +134,22 @@ enum FeeUpdateState { | |||
| Outbound, | |||
| } | |||
|
|
|||
| /// Returns the fees for success and timeout second stage HTLC transactions. | |||
There was a problem hiding this comment.
nit: Probably belongs in chan_utils.rs given it kinda mirrors commit_and_htlc_tx_fees_sat and is also called from there?
| @@ -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()); | |||
There was a problem hiding this comment.
Can't say I have a strong preference. The assertion seems fine to me.
carlaKC
force-pushed
the
3789-fees-and-dust
branch
from
001c8e9 to
4157d24
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
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 <git@bluematt.me>
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 feerate would happen after multiplying by the number of HTLCs. Now the feerate 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 feerates 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 <git@bluematt.me>
Co-authored-by: Matt Corallo <git@bluematt.me>
Update test helper in preparation to test zero fee commitments, where the local balance will differ from the previously hardcoded value.
carlaKC
force-pushed
the
3789-fees-and-dust
branch
from
4157d24 to
5fe6fa7
Compare
| @@ -1375,3 +1375,59 @@ pub fn do_can_afford_given_trimmed_htlcs(inequality_regions: core::cmp::Ordering | |||
| nodes[0].logger.assert_log("lightning::ln::channel", err, 1); | |||
| } | |||
| } | |||
|
|
|||
| #[test] | |||
| pub fn test_zero_fee_commiments_no_update_fee() { | |||
There was a problem hiding this comment.
There's a typo in the function name: test_zero_fee_commiments_no_update_fee should be test_zero_fee_commitments_no_update_fee
| pub fn test_zero_fee_commiments_no_update_fee() { | |
| pub fn test_zero_fee_commitments_no_update_fee() { |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
This PR completes the off-chain handling of V3 channels, as described in #3789.