Fix comparison in `get_dust_buffer_feerate` #2971

jbesraa · 2024-03-26T18:33:48Z

resolve #2815

We managed to fix the ChannelContext::get_dust_buffer_feerate implementation but the tests are still failing.

TheBlueMatt · 2024-03-26T21:24:39Z

Nice progress! Are you seeking help with the test changes here or just opening the WIP and will come back to it later this week?

oleonardolima · 2024-03-27T01:04:32Z

Nice progress! Are you seeking help with the test changes here or just opening the WIP and will come back to it later this week?

@TheBlueMatt We discussed during today's review-club session on leaving it open to anyone who would like to keep working on this, if nobody works on this we will revisit it on another session Thursday 28th [maybe?]

oleonardolima · 2024-03-27T01:05:38Z

--

@jbesraa I've not been able to solve the issue and the current failing test, but I did a little further investigation, here is broadly what I've discovered so far (in case someone also wants to move forward with this issue):

At least for now the failure that we are getting is:

node 0 ERROR [lightning::ln::channelmanager:3489] Got non-closing error: Cannot send less than our next-HTLC minimum - 2391000 msat

It happens here:

rust-lightning/lightning/src/ln/channel.rs

Lines 6843 to 6847 in 68d5e88

    
           let available_balances = self.context.get_available_balances(fee_estimator); 
        
           if amount_msat < available_balances.next_outbound_htlc_minimum_msat { 
        
           	return Err(ChannelError::Ignore(format!("Cannot send less than our next-HTLC minimum - {} msat", 
        
           		available_balances.next_outbound_htlc_minimum_msat))); 
        
           }

It fails because we have amount_msat == 2390000 and the available_balances.next_outbound_htlc_minimum_msat == 2391000

I still need to grasp how/why the calculation for dust is done, both during the test setup and here at channel::get_available_balances

But, it got my attention that the comment also does follow the code on the test setup, why are we have a -1 on open_channel.common_fields.dust_limit_satoshis - 1 ?

Is there any spec or previous discussion that all this dust logic is based on?

rust-lightning/lightning/src/ln/functional_tests.rs

Lines 9950 to 9968 in 68d5e88

    
           let dust_outbound_htlc_on_holder_tx_msat: u64 = (dust_buffer_feerate * htlc_timeout_tx_weight(&channel_type_features) / 1000 + open_channel.common_fields.dust_limit_satoshis - 1) * 1000; 
        
           let dust_outbound_htlc_on_holder_tx: u64 = max_dust_htlc_exposure_msat / dust_outbound_htlc_on_holder_tx_msat; 
        
           let dust_inbound_htlc_on_holder_tx_msat: u64 = (dust_buffer_feerate * htlc_success_tx_weight(&channel_type_features) / 1000 + open_channel.common_fields.dust_limit_satoshis - 1) * 1000; 
        
           let dust_inbound_htlc_on_holder_tx: u64 = max_dust_htlc_exposure_msat / dust_inbound_htlc_on_holder_tx_msat; 
        
           let dust_htlc_on_counterparty_tx: u64 = 4; 
        
           let dust_htlc_on_counterparty_tx_msat: u64 = max_dust_htlc_exposure_msat / dust_htlc_on_counterparty_tx; 
        
           if on_holder_tx { 
        
           	if dust_outbound_balance { 
        
           		// Outbound dust threshold: 2223 sats (`dust_buffer_feerate` * HTLC_TIMEOUT_TX_WEIGHT / 1000 + holder's `dust_limit_satoshis`) 
        
           		// Outbound dust balance: 4372 sats 
        
           		// Note, we need sent payment to be above outbound dust threshold on counterparty_tx of 2132 sats 
        
           		for _ in 0..dust_outbound_htlc_on_holder_tx { 
        
           			let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[1], dust_outbound_htlc_on_holder_tx_msat); 
        
           			nodes[0].node.send_payment_with_route(&route, payment_hash, 
        
           				RecipientOnionFields::secret_only(payment_secret), PaymentId(payment_hash.0)).unwrap(); 
        
           		}

jbesraa · 2024-03-27T12:37:48Z

Regarding the tests, what they are trying to do is basically "exhaust" the channel and then make a payment that should fail. basically adding alot of dust htlc until we meet some threshold and then make a payment that is expected to fail.

I think the main thing to look into is that we fixed the dust_feerate calculation and now the fee_rate for dust htlc should be a bit higher, ie its not meeting the requirements of the assertion in the tests where we try to send a payment with an htlc below the minimum. so now the payment we send is valid but the tests expect it to be invalid.

regarding the dust calculation, I am not entirely sure about the - 1/ + 1 but the main thing I got from this is that we have the fee_rate that is multiplied in the htlc weight(success/timeout htlcs have different weights) and then added to some fee the user can be set in different occasions.

codecov-commenter · 2024-03-27T13:10:42Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.02%. Comparing base (3a9fe20) to head (c01745e).
Report is 31 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2971      +/-   ##
==========================================
+ Coverage   89.33%   90.02%   +0.69%     
==========================================
  Files         117      117              
  Lines       95618    99768    +4150     
  Branches    95618    99768    +4150     
==========================================
+ Hits        85417    89817    +4400     
+ Misses       7952     7772     -180     
+ Partials     2249     2179      -70

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

TheBlueMatt · 2024-03-27T19:50:03Z

lightning/src/ln/functional_tests.rs

-		MaxDustHTLCExposure::FeeRateMultiplier(5_000_000 / 253)
-	} else { MaxDustHTLCExposure::FixedLimitMsat(5_000_000) }; // initial default setting value
+		MaxDustHTLCExposure::FeeRateMultiplier(6_000_000 / 253)
+	} else { MaxDustHTLCExposure::FixedLimitMsat(6_000_000) }; // initial default setting value


Looks like you fixed the tests? Care to update the comment here describing the new value selected and revert later changes in the test?

updated the comments and added how I did the calculations.
the 1500 here https://github.com/lightningdevkit/rust-lightning/pull/2971/files#diff-b30410f22a759d5e664e05938af7ef2edd244c8a7872e7ada376055ff130088bR9963 looks a bit off for me, is this the right calculation?

The comment doesn't really explain much for me? The update we made was adding 10 sat/vB, so reading the comment I'd guess we should actually be multiplying by 11 ((253 + 2530)/253). Can you add a bit more detail on what's going on?

TheBlueMatt · 2024-03-28T14:38:19Z

lightning/src/ln/functional_tests.rs

-		MaxDustHTLCExposure::FeeRateMultiplier(5_000_000 / 253)
-	} else { MaxDustHTLCExposure::FixedLimitMsat(5_000_000) }; // initial default setting value
+		MaxDustHTLCExposure::FeeRateMultiplier(6_000_000 / 253)
+	} else { MaxDustHTLCExposure::FixedLimitMsat(6_000_000) }; // initial default setting value


This is definitely not the default setting value anymore :)

TheBlueMatt · 2024-03-28T14:41:35Z

lightning/src/ln/functional_tests.rs

-		MaxDustHTLCExposure::FeeRateMultiplier(5_000_000 / 253)
-	} else { MaxDustHTLCExposure::FixedLimitMsat(5_000_000) }; // initial default setting value
+		MaxDustHTLCExposure::FeeRateMultiplier(6_000_000 / 253)
+	} else { MaxDustHTLCExposure::FixedLimitMsat(6_000_000) }; // initial default setting value


The comment doesn't really explain much for me? The update we made was adding 10 sat/vB, so reading the comment I'd guess we should actually be multiplying by 11 ((253 + 2530)/253). Can you add a bit more detail on what's going on?

TheBlueMatt · 2024-04-02T09:12:39Z

lightning/src/ln/functional_tests.rs

-		MaxDustHTLCExposure::FeeRateMultiplier(5_000_000 / 253)
-	} else { MaxDustHTLCExposure::FixedLimitMsat(5_000_000) }; // initial default setting value
+		// Default test fee estimator rate is 253 sat/kw. so we set the multiplier to 6_000_000 /
+		// 253 to accomdate 253 sat/kw plus an additional 2530 sat/kw for the dust buffer fee rate.


Still not sure where you got this number? 6_000_000/253 is 23715, which is quite a ways from 253+2530 (2783).

How would write this?
What I am trying to get to here is that the default test fee rate is 253 and in the dust_buffer_feerate we add to that 10sats/vb (2530 kw) and thats why we set the multiple to this number so we can accommodate whats happening in the tests while reaching dust limits

But that math doesn't work out? 253 + 2530 is not 6000000/253. Its not that this comment needs to be rewritten to be clearer, its that its currently completely wrong.

TheBlueMatt · 2024-04-02T09:13:01Z

lightning/src/ln/functional_tests.rs

@@ -10054,12 +10053,12 @@ fn do_test_max_dust_htlc_exposure(dust_outbound_balance: bool, exposure_breach_e
 fn do_test_max_dust_htlc_exposure_by_threshold_type(multiplier_dust_limit: bool) {
 	do_test_max_dust_htlc_exposure(true, ExposureEvent::AtHTLCForward, true, multiplier_dust_limit);
 	do_test_max_dust_htlc_exposure(false, ExposureEvent::AtHTLCForward, true, multiplier_dust_limit);
+	do_test_max_dust_htlc_exposure(false, ExposureEvent::AtHTLCForward, false, multiplier_dust_limit);


Why move these around?

ordering by ExposureEvent type ...

jbesraa · 2024-04-08T15:14:48Z

@TheBlueMatt may I ask how 5_000_000 was initially calculated?

TheBlueMatt · 2024-04-08T22:38:03Z

It was a bit of a rough estimate of where we thought that constant made sense. I intend to change it for the 0.0.123 release as a part of #2922, however. I don't think that should impact the test, though? The point of the changes to the test is just to keep a consistent value vs what it was when the test was written, not really to have a "good" value.

TheBlueMatt · 2024-04-08T23:06:13Z

lightning/src/ln/functional_tests.rs

+		// Default test fee estimator rate is 253 sat/kw plus 2530
+		// sat/kw buffer added by `get_dust_buffer_feerate`, so we
+		// set the multiplier to 5_002_500 / 253 to get roughly
+		// the same initial value as the default setting when this
+		// test was originally written.


I don't understand this, can you clarify the math? 5_002_500 / 253 is 19k, not 2783?

umm why do you expect it to be 2783? shouldn't our goal here be to calculate the multiplier to 2783?

My main motivation here was to raise the total dust limit taking into account the additional 1 sat/vb that is now added to the fee rate. The test below fills out the full dust capacity and then tries to route payments expecting them to fail but they were successful in some cases.
Maybe its getting tricky here because its not very clear to me how the initial 5_000_000 / 253 was decided on and passing the test ends up being fixing the numbers.

I'm expecting that based on your comment. I have no specific expectation for what it should be, but your comment seems to imply I should expect it to be 2783. Can you write the comment so the math checks out?

TheBlueMatt · 2024-04-08T23:11:07Z

lightning/src/ln/functional_tests.rs

@@ -10032,7 +10034,7 @@ fn do_test_max_dust_htlc_exposure(dust_outbound_balance: bool, exposure_breach_e
 		// For the multiplier dust exposure limit, since it scales with feerate,
 		// we need to add a lot of HTLCs that will become dust at the new feerate
 		// to cross the threshold.
-		for _ in 0..20 {
+		for _ in 0..30 {


Looks like this isn't needed anymore.

dunxen · 2024-04-12T14:03:03Z

lightning/src/ln/functional_tests.rs

@@ -9950,7 +9950,7 @@ fn do_test_max_dust_htlc_exposure(dust_outbound_balance: bool, exposure_breach_e
 	let dust_outbound_htlc_on_holder_tx_msat: u64 = (dust_buffer_feerate * htlc_timeout_tx_weight(&channel_type_features) / 1000 + open_channel.common_fields.dust_limit_satoshis - 1) * 1000;
 	let dust_outbound_htlc_on_holder_tx: u64 = max_dust_htlc_exposure_msat / dust_outbound_htlc_on_holder_tx_msat;

-	let dust_inbound_htlc_on_holder_tx_msat: u64 = (dust_buffer_feerate * htlc_success_tx_weight(&channel_type_features) / 1000 + open_channel.common_fields.dust_limit_satoshis - 1) * 1000;
+	let dust_inbound_htlc_on_holder_tx_msat: u64 = (dust_buffer_feerate * htlc_success_tx_weight(&channel_type_features) / 1000 + open_channel.common_fields.dust_limit_satoshis - if multiplier_dust_limit { 3 } else { 2 }) * 1000;


Could we comment about the choice of these values?

Added a comment

The current `cmp::max` doesnt align with the function comment, ie its comparing 2530 and `feerate_plus_quarter` instead of `feerate_per_kw + 2530` and `feerate_plus_quarter` which is fixed in this commit

jbesraa changed the title ~~[WIP] Review club/2815~~ [WIP] Fix comparison in get_dust_buffer_feerate Mar 26, 2024

TheBlueMatt added this to the 0.0.122 milestone Mar 26, 2024

jbesraa force-pushed the review-club/2815 branch from 395373c to 30e08c5 Compare March 27, 2024 12:38

jbesraa marked this pull request as ready for review March 27, 2024 12:38

jbesraa changed the title ~~[WIP] Fix comparison in get_dust_buffer_feerate~~ Fix comparison in get_dust_buffer_feerate Mar 27, 2024

TheBlueMatt reviewed Mar 27, 2024

View reviewed changes

jbesraa force-pushed the review-club/2815 branch 2 times, most recently from 97caa10 to 25ffe80 Compare March 28, 2024 08:43

TheBlueMatt reviewed Mar 28, 2024

View reviewed changes

jbesraa force-pushed the review-club/2815 branch from 25ffe80 to 21e1bf5 Compare April 1, 2024 16:29

TheBlueMatt reviewed Apr 2, 2024

View reviewed changes

jbesraa force-pushed the review-club/2815 branch from 21e1bf5 to 358a00b Compare April 8, 2024 14:20

TheBlueMatt reviewed Apr 8, 2024

View reviewed changes

jbesraa force-pushed the review-club/2815 branch from 358a00b to 87e9535 Compare April 11, 2024 13:16

TheBlueMatt previously approved these changes Apr 12, 2024

View reviewed changes

dunxen reviewed Apr 12, 2024

View reviewed changes

Fix cmp::max execution in ChannelContext::get_dust_buffer_feerate

c01745e

The current `cmp::max` doesnt align with the function comment, ie its comparing 2530 and `feerate_plus_quarter` instead of `feerate_per_kw + 2530` and `feerate_plus_quarter` which is fixed in this commit

jbesraa dismissed TheBlueMatt’s stale review via c01745e April 12, 2024 20:41

jbesraa force-pushed the review-club/2815 branch from 87e9535 to c01745e Compare April 12, 2024 20:41

dunxen approved these changes Apr 13, 2024

View reviewed changes

TheBlueMatt approved these changes Apr 13, 2024

View reviewed changes

TheBlueMatt merged commit bc4a5ea into lightningdevkit:main Apr 13, 2024
15 of 16 checks passed

valentinewallace mentioned this pull request May 1, 2024

Add draft release notes for 0.0.123 #3012

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix comparison in `get_dust_buffer_feerate` #2971

Fix comparison in `get_dust_buffer_feerate` #2971

jbesraa commented Mar 26, 2024

TheBlueMatt commented Mar 26, 2024

oleonardolima commented Mar 27, 2024

oleonardolima commented Mar 27, 2024 •

edited

jbesraa commented Mar 27, 2024

codecov-commenter commented Mar 27, 2024 •

edited

TheBlueMatt Mar 27, 2024

jbesraa Mar 28, 2024

TheBlueMatt Mar 28, 2024

TheBlueMatt Mar 28, 2024

TheBlueMatt Mar 28, 2024

TheBlueMatt Apr 2, 2024

jbesraa Apr 2, 2024

TheBlueMatt Apr 4, 2024

TheBlueMatt Apr 2, 2024

jbesraa Apr 2, 2024

jbesraa commented Apr 8, 2024

TheBlueMatt commented Apr 8, 2024 •

edited

TheBlueMatt Apr 8, 2024

jbesraa Apr 9, 2024

TheBlueMatt Apr 9, 2024

TheBlueMatt Apr 8, 2024

dunxen Apr 12, 2024

jbesraa Apr 13, 2024

Fix comparison in get_dust_buffer_feerate #2971

Fix comparison in get_dust_buffer_feerate #2971

Conversation

jbesraa commented Mar 26, 2024

TheBlueMatt commented Mar 26, 2024

oleonardolima commented Mar 27, 2024

oleonardolima commented Mar 27, 2024 • edited

jbesraa commented Mar 27, 2024

codecov-commenter commented Mar 27, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbesraa commented Apr 8, 2024

TheBlueMatt commented Apr 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fix comparison in `get_dust_buffer_feerate` #2971

Fix comparison in `get_dust_buffer_feerate` #2971

oleonardolima commented Mar 27, 2024 •

edited

codecov-commenter commented Mar 27, 2024 •

edited

TheBlueMatt commented Apr 8, 2024 •

edited