Tweak htlc dust exposure due to excess fees #3572

tankyleo · 2025-01-29T20:30:57Z

    Remove extra sum of tx fee dust on the counterparty tx dust exposure
    
    Previously, `get_pending_htlc_stats` did not account for the inbound
    htlc because `can_accept_incoming_htlc` was called before the htlc was
    irrevocably committed. But after commit d8d9dc7,
    `can_accept_incoming_htlc` is called only when the htlc is irrevocably
    committed, hence `get_pending_htlc_stats` does account for the inbound
    htlc.
    
    Nonetheless, in the case of a non-dust htlc, our calculation of the
    counterparty tx dust exposure still assumed that
    `get_pending_htlc_stats` did not account for the inbound htlc, causing
    us to add the dust exposure due to that inbound htlc twice. This commit
    removes this extra sum.

and

    For the candidate outbound htlc, sum weights, then sum fees
    
    Previously, we calculated the fee of the commitment transaction with n
    htlcs, and the fee due to the candidate htlc, rounded the two fees to
    the lower satoshi, and then summed the fees. This is not equal to how
    fees of commitment transactions are calculated, which is to add up the
    total weight of the (n+1) htlc commitment transaction, convert to fee,
    then round to the lower satoshi.
    
    This commit corrects this delta by running the full fee calculation
    twice, once for the n htlc, and once for the (n+1) htlc counterparty
    commitment transactions.

Only the first commit(?) should be backported

tankyleo · 2025-01-29T20:32:17Z

See commit 51bf78d for further background.

wpaulino · 2025-01-30T23:54:44Z

lightning/src/ln/channel.rs

+				let htlc_dust_exposure_msat =
+					per_outbound_htlc_counterparty_commit_tx_fee_msat(excess_feerate, &self.context.channel_type);
+				let counterparty_tx_dust_exposure =
+					htlc_stats.on_counterparty_tx_dust_exposure_msat.saturating_add(htlc_dust_exposure_msat);


IIUC, since can_accept_incoming_htlc is now called once it's been irrevocably committed on both sides, I think the dust exposure for this HTLC is already being accounted for in get_pending_htlc_stats. I think we just need to enforce the check below? Having a test would be helpful here to confirm the behavior.

thanks for the review. I mistakenly assumed can_accept_incoming_htlc == "can we add this htlc to the commit tx?"

But at this point in the flow, the htlc is already on the commit tx !

Now I struggle to understand the point of checking here whether that htlc pushes us over the dust limit if it has already been added to the commit tx.

as i understand now, this is a limitation of the spec - you cannot really complain about dust limits in direct response to an update_add_htlc msg.

wpaulino · 2025-01-31T22:06:09Z

Thanks! Some test coverage here would still be nice if you're keen and it's not too much trouble.

TheBlueMatt

At least some of the commits here should be backported to 0.1, I think. Maybe just "Correct the units of counterparty htlc tx excess fees calculations"?

TheBlueMatt · 2025-02-10T14:05:40Z

lightning/src/ln/chan_utils.rs

-	// Note that we need to divide before multiplying to round properly,
-	// since the lowest denomination of bitcoin on-chain is the satoshi.
-	let commitment_tx_fee_msat = COMMITMENT_TX_WEIGHT_PER_HTLC * feerate_per_kw as u64 / 1000 * 1000;
+	// We avoid rounding here to make the returned value consistent with the slope (commit_tx_fee_sat/num_htlcs)


I'm really not sure what this means, and the commit text doesn't explain it either. What is the issue which is being solved by this change?

TLDR two reasons 1) we currently can get a 1 satoshi difference in the calculation of dust exposure between the inbound htlc case and the outbound htlc case for anchor channels when there should not be a difference; this commit tries to minimize that difference. 2) If someone later (currently not an issue) has the idea to multiply this value returned with num_htlcs there would be a significant difference between commit_tx_fee_sat(num_htlcs) and commit_tx_fee_sat(0) + num_htlcs * per_outbound_htlc_fee.

The tests demo these two points.

We can a) drop this commit, b) leave this commit in c) drop this commit and find a better solution.

========

Explanation for 1):

In the case of anchor channels, the cost of the single additional non-dust htlc should be the same in both directions, as we do not include the second level htlc transactions in the total weight.

For the inbound htlc, we do

commit_tx_fee_sat(feerate, 1, features)

For the outbound htlc, we do

commit_tx_fee_sat(feerate, 0, features) + per_outbound_htlc_counterparty_commit_tx_fee_msat(feerate, features)

In the test in f6c14a8 we try to cover this calculation.

On the main branch it would be:

anchors_zero_htlc_fee: 500_000 + 22 * (1_124 + 172) / 1000 * 1000 = 528_000

vs

anchors_zero_htlc_fee: 500_000 + 22 * 1124 / 1000 * 1000 + 22 * 172 / 1000 * 1000 = 527_000

With this commit, we now drop the last rounding on the calculation just above here, and we get

anchors_zero_htlc_fee: 500_000 + 22 * 1124 / 1000 * 1000 + 22 * 172 = 527_784

This commit does not solve the root cause (the inbound htlc is already on the commit tx, the outbound htlc is not), but tries to minimize the difference in the inbound and outbound cases for anchor channels.

In the non-anchor case, there is already a difference between the inbound and outbound case due to the differences in weights of the htlc success and the htlc timeout transactions, so this is less a concern.

Right, I don't disagree that it makes sense to do this, just that we should motivate it in the commit description. I'm not entirely sure I buy the inbound-vs-outbound argument, but I do think it makes sense here because we're ultimately calculating the dust exposure in total and ultimately may calculate it wrong because we round on the original transaction fee calculation and round on the per-HTLC transaction fee, but ultimately our real dust limit will be summing the transaction weights first then converting to fees. With that said...I'm not sure just removing the round here fixes it? We really need to add first, then convert to fees in the outbound case, no?

I'm not sure just removing the round here fixes it?

Right, it does not remove it, just reduces the difference.

We really need to add first, then convert to fees in the outbound case, no?

Yes ! I'm working on this now thank you.

Should be fixed in 5177311

TheBlueMatt · 2025-02-11T16:10:08Z

lightning/src/ln/channel.rs

-			let counterparty_tx_dust_exposure =
-				htlc_stats.on_counterparty_tx_dust_exposure_msat.saturating_add(htlc_dust_exposure_msat);
-			if counterparty_tx_dust_exposure > max_dust_htlc_exposure_msat {
+			let on_counterparty_tx_dust_htlc_exposure_msat = htlc_stats.on_counterparty_tx_dust_exposure_msat;


This is now equivalent to the amount < dust branch above, so presumably they should be combined?

Yes! Should the log message and the error message to be different depending on the dust, non-dust case ? That part I wasn't sure about.

Done in 988c33f

TheBlueMatt · 2025-02-11T16:10:54Z

lightning/src/ln/channel.rs

@@ -7333,13 +7333,10 @@ impl<SP: Deref> FundedChannel<SP> where
 				return Err(("Exceeded our dust exposure limit on counterparty commitment tx", 0x1000|7))
 			}
 		} else {
-			let htlc_dust_exposure_msat =


It may be worth adding a comment at the top of the method here noting that, when we're called, the HTLC is actually already added on the channel, thus we're trying to decide whether to fail or forward/claim.

Certainly will do

Done in 9bea4ce

TheBlueMatt · 2025-02-11T16:17:19Z

lightning/src/ln/chan_utils.rs

-	// Note that we need to divide before multiplying to round properly,
-	// since the lowest denomination of bitcoin on-chain is the satoshi.
-	let commitment_tx_fee_msat = COMMITMENT_TX_WEIGHT_PER_HTLC * feerate_per_kw as u64 / 1000 * 1000;
+	// We avoid rounding here to make the returned value consistent with the slope (commit_tx_fee_sat/num_htlcs)


Right, I don't disagree that it makes sense to do this, just that we should motivate it in the commit description. I'm not entirely sure I buy the inbound-vs-outbound argument, but I do think it makes sense here because we're ultimately calculating the dust exposure in total and ultimately may calculate it wrong because we round on the original transaction fee calculation and round on the per-HTLC transaction fee, but ultimately our real dust limit will be summing the transaction weights first then converting to fees. With that said...I'm not sure just removing the round here fixes it? We really need to add first, then convert to fees in the outbound case, no?

tankyleo · 2025-02-12T01:35:17Z

lightning/src/ln/functional_tests.rs

@@ -10300,7 +10300,7 @@ fn do_test_max_dust_htlc_exposure(dust_outbound_balance: bool, exposure_breach_e
 	} else { 0 };
 	let initial_feerate = if apply_excess_fee { 253 * 2 } else { 253 };
 	let expected_dust_buffer_feerate = initial_feerate + 2530;
-	let mut commitment_tx_cost = commit_tx_fee_msat(initial_feerate - 253, nondust_htlc_count_in_limit, &ChannelTypeFeatures::empty());
+	let mut commitment_tx_cost = chan_utils::commit_tx_fee_msat(initial_feerate - 253, nondust_htlc_count_in_limit as usize, &ChannelTypeFeatures::empty());


Further above in functional_tests, we define a fn commit_tx_fee_msat that rounds and returns msat precision. Here we want no rounding and msat precision. Let me know if this is ok.

TheBlueMatt

Sorry for the delay here. Feel free to squash, its a little hard to follow with fixup commits that are all at the end.

TheBlueMatt · 2025-02-13T21:50:33Z

lightning/src/ln/channel.rs

@@ -3665,14 +3665,14 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 			let on_counterparty_tx_nondust_htlcs =
 				on_counterparty_tx_accepted_nondust_htlcs + on_counterparty_tx_offered_nondust_htlcs;
 			on_counterparty_tx_dust_exposure_msat +=
-				commit_tx_fee_sat(excess_feerate, on_counterparty_tx_nondust_htlcs, &self.channel_type) * 1000;
+				commit_tx_fee_msat(excess_feerate, on_counterparty_tx_nondust_htlcs, &self.channel_type);


I'm not sure I agree that this is better. We can't have a dust exposure level that has msats in it, the whole point of dust exposure is that its the amount we lost when we went on chain. Now, maybe we need to always round up rather than rounding down, but not rounding doesn't seem any more "correct".

no problem on the delay, thank you for reviewing.

all the sums for on_holder_tx_dust_exposure_msat have msats precision,

all the sums besides this one (commit_tx_fee_sat) for on_counterparty_tx_dust_exposure_msat have msats precision.

MaxDustHTLCExposure::FixedLimitMsat has msat precision.

That's why I went this direction. Thank you let me know what you think.

The other entries are using msats because the balance being lost is actually msats (its the full HTLC value). In this case we're not actually going to lose value in msats, we're going to lose value to "excess fees" which are only really amounts in sats (we expected to pay fee X sats, we end up paying fee Y sats). Similar for the change below in the tx weight calculations - we're never going to pay the msat amount, we're going to pay the sat-rounded amount in fees.

TheBlueMatt

LGTM! Sadly, I guess we've merged a few things and now it doesn't make sense to backport the first commit. Do you think it makes sense to backport some of the fixes (that apply pre-HTLC-check-after-accept) by splitting commits back up or should we just leave 0.1 as-is?

TheBlueMatt · 2025-02-20T20:23:05Z

Oh, sadly this needs (trivial) rebase it seems.

Previously, `get_pending_htlc_stats` did not account for the inbound htlc because `can_accept_incoming_htlc` was called before the htlc was irrevocably committed. But after commit d8d9dc7, `can_accept_incoming_htlc` is called only when the htlc is irrevocably committed, hence `get_pending_htlc_stats` does account for the inbound htlc. Nonetheless, in the case of a non-dust htlc, our calculation of the counterparty tx dust exposure still assumed that `get_pending_htlc_stats` did not account for the inbound htlc, causing us to add the dust exposure due to that inbound htlc twice. This commit removes this extra sum.

Previously, we calculated the fee of the commitment transaction with n htlcs, and the fee due to the candidate htlc, rounded the two fees to the lower satoshi, and then summed the fees. This is not equal to how fees of commitment transactions are calculated, which is to add up the total weight of the (n+1) htlc commitment transaction, convert to fee, then round to the lower satoshi. This commit corrects this delta by running the full fee calculation twice, once for the n htlc, and once for the (n+1) htlc counterparty commitment transactions.

The payments in this test previously failed for reasons other than exhausting the dust exposure limit with excess fees. Upon payment failures, we now check the logs to assert failures due to dust exposure exhaustion.

This test checks to a 1msat precision the accounting of dust exposure due to excess fees on counterparty commmitment and htlc transactions, for both inbound and outbound htlcs.

codecov · 2025-02-21T01:50:34Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.61%. Comparing base (6cf270d) to head (f931db5).
Report is 13 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3572      +/-   ##
==========================================
- Coverage   88.61%   88.61%   -0.01%     
==========================================
  Files         149      149              
  Lines      117091   117280     +189     
  Branches   117091   117280     +189     
==========================================
+ Hits       103766   103927     +161     
- Misses      10816    10841      +25     
- Partials     2509     2512       +3

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

tankyleo · 2025-02-21T01:57:49Z

This rebase is just the trivial rebase of the four previous commits. No splitting done.

LGTM! Sadly, I guess we've merged a few things and now it doesn't make sense to backport the first commit. Do you think it makes sense to backport some of the fixes (that apply pre-HTLC-check-after-accept) by splitting commits back up or should we just leave 0.1 as-is?

Some thoughts:

The only part I'm uneasy about on 0.1 is the code that adds the full fees of the additional non-dust htlc to the dust exposure.
Given that this code has been shipped since 0.0.123, nearly 10 months ago, and we've had no complaints, the 0.1 backport does not seem critical.
If we do want a backport, (can definitely do it) I'd prefer creating a separate PR that pushes some small commits to the 0.1 branch - otherwise here in this PR, I'd be fixing some code, then deleting that same code in the same patch series.

wpaulino

Thanks for the test coverage! We probably don't need to backport since the 0.1 branch is currently enforcing a value below the expected max_dust_htlc_exposure, not above.

tankyleo · 2025-02-21T17:32:50Z

Thanks for the test coverage! We probably don't need to backport since the 0.1 branch is currently enforcing a value below the expected max_dust_htlc_exposure, not above.

This might mean that some payments fail when they should not right ? Though we've had no complaints so far.

wpaulino · 2025-02-21T17:40:02Z

Yeah because it makes the limit a bit stricter, which is certainly not worse than making it looser given the type of attack it's preventing.

TheBlueMatt

Thanks. I'm not sure "we haven't gotten any complaints" is quite the right benchmark given much of this logic shipped and has been in prod at a time when fees have slowly been coming down, where we expect it to not fire as often. That said, I'm also not too worried about getting this backported, really.

tankyleo changed the title ~~Tweak htlc dust exposure due to exceess fees~~ Tweak htlc dust exposure due to excess fees Jan 29, 2025

wpaulino reviewed Jan 30, 2025

View reviewed changes

tankyleo force-pushed the 25-01-htlc-dust-exposure branch from 133e661 to 188f299 Compare January 31, 2025 21:30

tankyleo force-pushed the 25-01-htlc-dust-exposure branch from 907c100 to 188f299 Compare February 6, 2025 19:23

tankyleo requested a review from wpaulino February 8, 2025 00:56

tankyleo force-pushed the 25-01-htlc-dust-exposure branch 4 times, most recently from eae77c0 to b63b336 Compare February 9, 2025 21:02

TheBlueMatt reviewed Feb 10, 2025

View reviewed changes

TheBlueMatt reviewed Feb 11, 2025

View reviewed changes

tankyleo force-pushed the 25-01-htlc-dust-exposure branch 2 times, most recently from 607543c to 988c33f Compare February 12, 2025 01:11

tankyleo commented Feb 12, 2025

View reviewed changes

TheBlueMatt reviewed Feb 13, 2025

View reviewed changes

TheBlueMatt added the backport 0.1 label Feb 14, 2025

tankyleo force-pushed the 25-01-htlc-dust-exposure branch from e64e7af to 7c4f6cf Compare February 15, 2025 19:41

tankyleo requested a review from TheBlueMatt February 15, 2025 20:13

TheBlueMatt previously approved these changes Feb 20, 2025

View reviewed changes

tankyleo added 4 commits February 21, 2025 01:28

Assert dust exposure exhaustion in the excess fees are dust test

8ed5a23

The payments in this test previously failed for reasons other than exhausting the dust exposure limit with excess fees. Upon payment failures, we now check the logs to assert failures due to dust exposure exhaustion.

Test the accounting of dust exposure due to excess fees

f931db5

This test checks to a 1msat precision the accounting of dust exposure due to excess fees on counterparty commmitment and htlc transactions, for both inbound and outbound htlcs.

tankyleo dismissed TheBlueMatt’s stale review via f931db5 February 21, 2025 01:42

tankyleo force-pushed the 25-01-htlc-dust-exposure branch from 7c4f6cf to f931db5 Compare February 21, 2025 01:42

tankyleo requested a review from TheBlueMatt February 21, 2025 17:16

wpaulino approved these changes Feb 21, 2025

View reviewed changes

TheBlueMatt approved these changes Feb 21, 2025

View reviewed changes

TheBlueMatt merged commit 81f8b67 into lightningdevkit:main Feb 21, 2025
24 of 26 checks passed

TheBlueMatt removed the backport 0.1 label Feb 21, 2025

Tweak htlc dust exposure due to excess fees #3572

Tweak htlc dust exposure due to excess fees #3572

Uh oh!

Conversation

tankyleo commented Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tankyleo commented Jan 29, 2025

Uh oh!

wpaulino Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

wpaulino commented Jan 31, 2025

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tankyleo Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Feb 20, 2025

Uh oh!

codecov bot commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tankyleo commented Feb 21, 2025

Uh oh!

wpaulino left a comment

Choose a reason for hiding this comment

Uh oh!

tankyleo commented Feb 21, 2025

Uh oh!

wpaulino commented Feb 21, 2025

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

tankyleo commented Jan 29, 2025 •

edited

Loading

wpaulino Jan 30, 2025 •

edited

Loading

tankyleo Feb 10, 2025 •

edited

Loading

codecov bot commented Feb 21, 2025 •

edited

Loading