Increase our default/minimum dust limit and decrease our max #1065

TheBlueMatt · 2021-09-02T17:47:57Z

CC @ariard can you take a look at the test I dropped here - its not documented how you came up with the constants used so its rather difficult to fix the test. Would be nice to replace all the constants with calculation based on the dust limits set.

codecov · 2021-09-02T18:07:52Z

Codecov Report

Merging #1065 (2da0d6c) into main (088daf7) will increase coverage by 1.17%.
The diff coverage is 81.25%.

@@            Coverage Diff             @@
##             main    #1065      +/-   ##
==========================================
+ Coverage   91.01%   92.19%   +1.17%     
==========================================
  Files          65       65              
  Lines       33767    41822    +8055     
==========================================
+ Hits        30732    38556    +7824     
- Misses       3035     3266     +231

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	`88.98% <77.77%> (+0.34%)`	⬆️
lightning/src/ln/functional_tests.rs	`98.51% <100.00%> (+1.00%)`	⬆️
lightning/src/ln/script.rs	`92.64% <100.00%> (-1.11%)`	⬇️
lightning-background-processor/src/lib.rs	`93.33% <0.00%> (-0.87%)`	⬇️
lightning/src/ln/onion_route_tests.rs	`97.08% <0.00%> (-0.01%)`	⬇️
lightning/src/ln/reorg_tests.rs	`100.00% <0.00%> (ø)`
lightning/src/ln/monitor_tests.rs	`100.00% <0.00%> (ø)`
lightning/src/ln/features.rs	`99.56% <0.00%> (+0.12%)`	⬆️
lightning/src/routing/router.rs	`96.81% <0.00%> (+0.55%)`	⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 088daf7...2da0d6c. Read the comment docs.

ariard

Concept ACK,

I can take #1065 on top of #1054 and fix the test there. Yeah values are not documented because...though the easy fix should be just to sub (660 - 543) to the loading HTLC amounts. I think.

lightning/src/ln/channel.rs

ariard · 2021-09-05T23:08:18Z

lightning/src/ln/channel.rs

@@ -3665,6 +3664,12 @@ impl<Signer: Sign> Channel<Signer> {
 			},
 		};

+		for outp in closing_tx.output.iter() {
+			if !outp.script_pubkey.is_witness_program() && outp.value < MAX_STD_OUTPUT_DUST_LIMIT_SATOSHIS {


Wait, the closing requirement implemented by this change is "Require cooperative close payout scripts to be Segwit" ? If so why also checking MAX_STD_OUTPUT_DUST_LIMIT_SATOSHIS, we want to reject non-segwit output no matter their output values ?

The new requirement is "our users have to use segwit scripts, but the counterparty can use something else, but if they do we may have to force close". I tweaked the commit message text to be more clear.

This should be reverted at some point, but the test is deficient and breaks on later changes that are important to land ASAP.

330 sat/vbyte, the current value, is not sufficient to ensure a future segwit script longer than 32 bytes meets the dust limit if used for a shutdown script. Thus, we can either check the value on shutdown or we can simply require segwit outputs and require a dust value of no less than 354 sat/vbyte. We swap the minimum dust value to 354 sat/vbyte here, requiring segwit scripts in a future commit. See lightning/bolts#905

TheBlueMatt · 2021-09-17T20:33:38Z

Rebased and squashed the fixup commits. I think we should go ahead and take this and just open an issue for the broken test to re-add it later - without this we're going to be unable to open any channels with other nodes very soon.

valentinewallace

One suggestion, otherwise looks good

valentinewallace · 2021-09-17T20:59:58Z

lightning/src/ln/channel.rs

+/// transaction non-standard and thus refuses to relay it.
+/// We also use this as the maximum counterparty `dust_limit_satoshis` allowed, given many
+/// implementations use this value for their dust limit today.
+pub const MAX_STD_OUTPUT_DUST_LIMIT_SATOSHIS: u64 = 546;


The MAX_STD part is throwing me off because from the comment, this constant seems to be the min std output value. Think diff might make it the clearest to me:

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 2d165adf..51963347 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -559,7 +559,9 @@ pub const MAX_FUNDING_SATOSHIS: u64 = 1 << 24; /// transaction non-standard and thus refuses to relay it. /// We also use this as the maximum counterparty `dust_limit_satoshis` allowed, given many /// implementations use this value for their dust limit today. -pub const MAX_STD_OUTPUT_DUST_LIMIT_SATOSHIS: u64 = 546; +pub const MIN_STD_OUTPUT_SATOSHIS: u64 = 546; + +pub const MAX_STD_OUTPUT_DUST_LIMIT_SATOSHIS: u64 = MIN_STD_OUTPUT_SATOSHIS; /// The dust limit is used for both the commitment transaction outputs as well as the closing /// transactions. For cooperative closing transactions, we require segwit outputs, though accept

Then MIN_STD could be used in fn closing_signed and MAX_DUST could be used everywhere else

Did something similar which I think captured your intent, lmk.

Oops, I meant in my original diff to rename the new const back to MAX_DUST_LIMIT_SATOSHIS (since it seems like an implementation detail what the actual value is)

Ah, ok, i see your point now. reverted the fixup, take a look now.

valentinewallace · 2021-09-20T23:54:22Z

lightning/src/ln/channel.rs

+pub const MAX_STD_OUTPUT_DUST_LIMIT_SATOSHIS: u64 = 546;
+
+/// The maximum channel dust limit we will accept from our counterparty.
+pub const MAX_CHAN_DUST_LIMIT_SATOSHIS: u64 = MAX_STD_OUTPUT_DUST_LIMIT_SATOSHIS;


For conciseness, why not MAX_DUST_LIMIT_SATOSHIS?

I find it a bit confusing, the distinction between the network's dust limit, and the channel's dust limit, both of which have the same name but mean different things.

Isn't MIN_DUST_LIMIT_SATOSHIS also a channel dust limit? Mainly the inconsistency between them I find confusing

Yes I agree MIN_DUST_LIMIT_SATOSHIS could be MIN_CHAN_DUST_LIMIT_SATOSHIS as it's applied at the minimal channel's dust_limit_satoshis accepted by our implementation.

Pushed a commit to rename that, too :)

ariard

Code Review ACK 88c0619

546 sat/vbyte is the current default dust limit on most implementations, matching the network dust limit for P2SH outputs. Implementations don't currently appear to send any larger dust limits, and allowing a larger dust limit implies higher payment failure risk, so we'd like to be as tight as we can here.

There is little reason for users to be paying out to non-Segwit scripts when closing channels at this point. Given we will soon, in rare cases, force-close during shutdown when a counterparty closes to a non-Segwit script, we should also require it of our own users.

If a counterparty (or an old channel of ours) uses a non-segwit script for their cooperative close payout, they may include an output which is unbroadcastable due to not meeting the network dust limit. Here we check for this condition, force-closing the channel instead if we find an output in the closing transaction which does not meet the limit.

While channel and P2P network dust limits are related, they're ultimately two different things, and thus their constant names should reference that.

TheBlueMatt · 2021-09-27T18:20:51Z

Squashed with no diff. Diff since ariard's ack is a trivial constant rename. Will land after CI.

$ git diff-tree -U0 88c0619 2da0d6c0
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 3faaafe8..f412bf2f 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -574 +574 @@ pub const MAX_CHAN_DUST_LIMIT_SATOSHIS: u64 = MAX_STD_OUTPUT_DUST_LIMIT_SATOSHIS
-pub const MIN_DUST_LIMIT_SATOSHIS: u64 = 354;
+pub const MIN_CHAN_DUST_LIMIT_SATOSHIS: u64 = 354;
@@ -641 +641 @@ impl<Signer: Sign> Channel<Signer> {
-		if holder_selected_channel_reserve_satoshis < MIN_DUST_LIMIT_SATOSHIS {
+		if holder_selected_channel_reserve_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS {
@@ -712 +712 @@ impl<Signer: Sign> Channel<Signer> {
-			holder_dust_limit_satoshis: MIN_DUST_LIMIT_SATOSHIS,
+			holder_dust_limit_satoshis: MIN_CHAN_DUST_LIMIT_SATOSHIS,
@@ -846,2 +846,2 @@ impl<Signer: Sign> Channel<Signer> {
-		if msg.dust_limit_satoshis < MIN_DUST_LIMIT_SATOSHIS {
-			return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is less than the implementation limit ({})", msg.dust_limit_satoshis, MIN_DUST_LIMIT_SATOSHIS)));
+		if msg.dust_limit_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS {
+			return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is less than the implementation limit ({})", msg.dust_limit_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS)));
@@ -867,2 +867,2 @@ impl<Signer: Sign> Channel<Signer> {
-		if holder_selected_channel_reserve_satoshis < MIN_DUST_LIMIT_SATOSHIS {
-			return Err(ChannelError::Close(format!("Suitable channel reserve not found. remote_channel_reserve was ({}). dust_limit_satoshis is ({}).", holder_selected_channel_reserve_satoshis, MIN_DUST_LIMIT_SATOSHIS)));
+		if holder_selected_channel_reserve_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS {
+			return Err(ChannelError::Close(format!("Suitable channel reserve not found. remote_channel_reserve was ({}). dust_limit_satoshis is ({}).", holder_selected_channel_reserve_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS)));
@@ -870,2 +870,2 @@ impl<Signer: Sign> Channel<Signer> {
-		if msg.channel_reserve_satoshis < MIN_DUST_LIMIT_SATOSHIS {
-			return Err(ChannelError::Close(format!("channel_reserve_satoshis ({}) is smaller than our dust limit ({})", msg.channel_reserve_satoshis, MIN_DUST_LIMIT_SATOSHIS)));
+		if msg.channel_reserve_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS {
+			return Err(ChannelError::Close(format!("channel_reserve_satoshis ({}) is smaller than our dust limit ({})", msg.channel_reserve_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS)));
@@ -976 +976 @@ impl<Signer: Sign> Channel<Signer> {
-			holder_dust_limit_satoshis: MIN_DUST_LIMIT_SATOSHIS,
+			holder_dust_limit_satoshis: MIN_CHAN_DUST_LIMIT_SATOSHIS,
@@ -1620,2 +1620,2 @@ impl<Signer: Sign> Channel<Signer> {
-		if msg.dust_limit_satoshis < MIN_DUST_LIMIT_SATOSHIS {
-			return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is less than the implementation limit ({})", msg.dust_limit_satoshis, MIN_DUST_LIMIT_SATOSHIS)));
+		if msg.dust_limit_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS {
+			return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is less than the implementation limit ({})", msg.dust_limit_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS)));
diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index 0991434c..e6529415 100644
--- a/lightning/src/ln/functional_tests.rs
+++ b/lightning/src/ln/functional_tests.rs
@@ -1483 +1483 @@ fn test_chan_reserve_dust_inbound_htlcs_outbound_chan() {
-	let dust_amt = crate::ln::channel::MIN_DUST_LIMIT_SATOSHIS * 1000
+	let dust_amt = crate::ln::channel::MIN_CHAN_DUST_LIMIT_SATOSHIS * 1000
$

TheBlueMatt force-pushed the 2021-08-bump-dust branch from 767370f to 0501354 Compare September 2, 2021 18:12

ariard reviewed Sep 5, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-08-bump-dust branch 3 times, most recently from f738691 to 3734846 Compare September 15, 2021 01:41

TheBlueMatt added 2 commits September 17, 2021 20:32

Drop broken test that is unfixable due to being undocumented

57f54bc

This should be reverted at some point, but the test is deficient and breaks on later changes that are important to land ASAP.

TheBlueMatt force-pushed the 2021-08-bump-dust branch from 3734846 to 5918d1c Compare September 17, 2021 20:33

TheBlueMatt added this to the 0.0.102 milestone Sep 17, 2021

valentinewallace reviewed Sep 17, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-08-bump-dust branch 2 times, most recently from 93e8971 to 88c0619 Compare September 20, 2021 23:11

valentinewallace reviewed Sep 20, 2021

View reviewed changes

ariard reviewed Sep 27, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-08-bump-dust branch from 2a96092 to 0f6e0c3 Compare September 27, 2021 17:57

valentinewallace approved these changes Sep 27, 2021

View reviewed changes

TheBlueMatt added 4 commits September 27, 2021 18:19

Rename MIN_DUST_LIMIT_SATOSHIS constant to disambiguate chan vs P2P

2da0d6c

While channel and P2P network dust limits are related, they're ultimately two different things, and thus their constant names should reference that.

TheBlueMatt force-pushed the 2021-08-bump-dust branch from 0f6e0c3 to 2da0d6c Compare September 27, 2021 18:20

TheBlueMatt merged commit ad819ea into lightningdevkit:main Sep 27, 2021

TheBlueMatt mentioned this pull request Sep 27, 2021

Re-add test_max_dust_htlc_exposure #1099

Closed

TheBlueMatt mentioned this pull request Oct 7, 2021

Force-close if closing transaction doesn't confirm in a timely manner #1046

Closed

Increase our default/minimum dust limit and decrease our max #1065

Increase our default/minimum dust limit and decrease our max #1065

Uh oh!

Conversation

TheBlueMatt commented Sep 2, 2021

Uh oh!

codecov bot commented Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Sep 17, 2021

Uh oh!

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

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!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Sep 27, 2021

Uh oh!

Uh oh!

codecov bot commented Sep 2, 2021 •

edited

Loading