Peers need to check each other's dust limit #894

t-bast · 2021-08-11T07:35:25Z

Since HTLCs below this amount will not appear in the commitment tx, they are effectively converted to miner fees. The peer could use this to grief you by broadcasting its commitment once it contains a lot of dust HTLCs.

I just stumbled upon issue #696 which mentioned that a while ago, but we didn't update the spec accordingly.
However, implementations do already enforce that.

I also added a lower bound on dust_limit_satoshis that ensures that transactions will correctly propagate with default relay policies.

Fixes #696 and #905

TheBlueMatt · 2021-08-12T20:21:51Z

02-peer-protocol.md

@@ -239,6 +239,7 @@ The receiving node MAY fail the channel if:
  - it considers `channel_reserve_satoshis` too large.
  - it considers `max_accepted_htlcs` too small.
  - it considers `dust_limit_satoshis` too small and plans to rely on the sending node publishing its commitment transaction in the event of a data loss (see [message-retransmission](02-peer-protocol.md#message-retransmission)).


Should we drop this? I believe c-lightning and lnd both don't implement this at all today, and I'm not sure if we need to?

I think it's still useful, and I kept a lower bound on eclair, because you may want to avoid letting your peer create a commit tx that you think will not easily propagate through the bitcoin network because it doesn't satisfy default policy settings.

If you think you'll rely on their commit tx with option_dataloss_protect, you want to be sure their commit tx can get to miners' mempools, don't you?

If you think you'll rely on their commit tx with option_dataloss_protect, you want to be sure their commit tx can get to miners' mempools, don't you?

Yea, that's a good point. Should we clarify that a suggested lower-bound is 330 (which IIRC is the exact current network dust threshold)?

Done in f16ea99

Update: I instead added a list of the network dust threshold in 40cfede which implementers can refer to when deciding what lower bound they want to enforce.

I don't think that's really sufficient - there needs to be guidance in what implementations should be allowed to set the dust limit to, as otherwise we end up back where we are today where some nodes arbitrarily force-closed based on dust limit and there isn't a consensus. I'd strongly prefer if we either agree to set it to the max value for a standard output, or we set it to 330 and then fix the close negotiation logic via #896.

Right, I agree with that. Both options have pros and cons:

if we all decide to enforce a minimum dust_limit_satoshis = 546 sat, we don't have issues at closing and Trim closing output below network dust threshold #896 isn't even needed

otherwise if we enforce a minimum dust_limit_satoshis = 546 sat, this allows smaller outputs on the commit tx but we need to do something at closing time (Trim closing output below network dust threshold #896 or some other solution).

Note that an alternative to #896 which would also fix the issue would be to disallow non-segwit scripts in shutdown. Now that segwit is widely supported, this would fix a few headaches.

I'd like feedback from other implementers on this choice, @rustyrussell @Roasbeef what do you think?

Yea, if we can get agreement on it, I'd love to just say "segwit-only shutdown", drop 896 and then set dust limit to be the segwit lower bound (354, I think? check my math). Do any implementations in practice do non-segwit closing scripts today? It would mean you can't open a channel with that output which seems incredibly restrictive.

You can close to cold storage, I guess, which may be old skool. But simplicity FTW: you can always unilaterally close so I've never considered it a big deal whatever we do here.

IOW, I'm happy with adding "mutual quickclose must be segwit" in #847

I opened an issue to discuss the dust_limit_satoshis lower bounds in #905

I'd like to treat this in a separate PR than this one, which fixes a different issue that we should have fixed a long time ago (adding a higher bound on our peer's dust_limit_satoshis).

If my calculations of the various dust thresholds imposed by Bitcoin Core are correct, I believe we can merge this PR as the need to impose a higher bound on remote dust should be obvious to everyone?

t-bast · 2021-08-16T09:49:45Z

There can be issues with dust limits below 546, as pointed out here.

This is also a concern with future segwit versions, for which we don't know the bitcoin network dust threshold (when using option_shutdown_anysegwit). I believe we need a new mechanism to handle that kind of issues.

TheBlueMatt · 2021-08-16T17:23:20Z

There can be issues with dust limits below 546, as pointed out here.

That's a good point, I'd like to hear from lnd and c-lightning folks whether they think this matters given they currently don't enforce this at all. I presume they force-close channels if the closing transaction doesn't confirm in some time?

This is also a concern with future segwit versions, for which we don't know the bitcoin network dust threshold (when using option_shutdown_anysegwit). I believe we need a new mechanism to handle that kind of issues.

Right, this seems to be a general problem - the only thing you can realistically do here is force-close if nothing confirms in some time because at least you know all the scripts used in that transaction.

t-bast · 2021-08-16T17:34:05Z

AFAICT the only place where peers are free to use different types of scripts is during mutual close.

The spec could list in the mutual close requirements the dust threshold for each type of script (e.g. 546 sats for non-segwit, 330 sats for segwit, ??? for taproot) and both nodes should enforce these static checks and prune outputs accordingly. If one of the peers doesn't, this will automatically result in a force-close since the closing signatures won't match.

TheBlueMatt · 2021-08-16T18:59:58Z

??? for taproot

I tried to ask about this on #bitcoin-core-dev and basically got told to fuck off and not rely on relay dust limits for anything, so I guess for now just assume it won't change cause Core doesn't change things and hope for the best.

The spec could list in the mutual close requirements the dust threshold for each type of script (e.g. 546 sats for non-segwit, 330 sats for segwit, ??? for taproot) and both nodes should enforce these static checks and prune outputs accordingly. If one of the peers doesn't, this will automatically result in a force-close since the closing signatures won't match.

That is an incompatibility, I think - one issue with closing_signed today is that it talks about how nodes can unilaterally drop their own output (which would be required for this to work), but then talks about how if the fee_satoshis field is the same as the one it sent it's done, even though the transaction may be different. Maybe we should fix it in #847?

If we fix the spec to actually allow unilateral dropping of a node's output, we could reasonably just say that nodes can send a dust threshold down to 330, but have to enforce that counterparties have removed their output during closing if its lower than the scripts real dust threshold.

Crypt-iQ · 2021-08-16T19:57:40Z

In case anybody was wondering, I believe p2tr outputs will have a dust limit defined by Core's GetDustThreshold func as 330 sats, which is the same as p2wsh outputs. It may be a good idea to place the different output type's dust values calculated at dust_relay_fee of 3000sat/kvb somewhere?

Roasbeef

LGTM ⛷

02-peer-protocol.md

t-bast · 2021-09-14T14:36:39Z

As discussed during yesterday's spec meeting, I added option 2 of #905 in 3e7771e

we explicitly reject dust limits below 354 sats to ensure there won't be propagation issues with default relay policies
we drop support for non-segwit scripts in shutdown
if at closing time we realize that an output will prevent propagation, we force-close

TheBlueMatt · 2021-09-15T01:38:40Z

Looks good to me. Is it worth mentioning in the rationale section that older nodes will send non-segwit shutdown scripts and you may wish to allow them for backwards-compatibility today, taking note of the force-close requirement at shutdown.

Since HTLCs below this amount will not appear in the commitment tx, they are effectively converted to miner fees. The peer could use this to grief you by broadcasting its commitment once it contains a lot of dust HTLCs. Fixes #696

As implemented in Bitcoin Core's default relay policy.

This allows dust limit to go as low as 354 sats without creating relay issues with default node policies. We add a requirement that dust limit cannot be lower than 354 sats. This ensures implementers don't have to figure this subtlety on their own.

t-bast · 2021-09-15T07:37:27Z

Is it worth mentioning in the rationale section that older nodes will send non-segwit shutdown scripts and you may wish to allow them for backwards-compatibility today, taking note of the force-close requirement at shutdown.

You're right, it's hard to understand without any context. I rebased and added some rationale in 9ff1387

We are slowly dropping support for non-segwit outputs, as proposed in lightning/bolts#894 We can thus safely allow dust limits all the way down to 354 satoshis.

We are slowly dropping support for non-segwit outputs, as proposed in lightning/bolts#894 We can thus safely allow dust limits all the way down to 354 satoshis. In very rare cases where dust_limit_satoshis is negotiated to a low value, our peer may generate closing txs that will not correctly relay on the bitcoin network due to dust relay policies. When that happens, we detect it and force-close instead of completing the mutual close flow.

03-transactions.md

Crypt-iQ · 2021-09-21T19:32:01Z

02-peer-protocol.md

@@ -270,6 +269,7 @@ The receiving node MUST fail the channel if:
  - `funding_pubkey`, `revocation_basepoint`, `htlc_basepoint`, `payment_basepoint`, or `delayed_payment_basepoint`
 are not valid secp256k1 pubkeys in compressed format.
  - `dust_limit_satoshis` is greater than `channel_reserve_satoshis`.
+  - `dust_limit_satoshis` is smaller than `354 satoshis` (see [BOLT 3](03-transactions.md#dust-limits)).


I think this should only apply for option_shutdown_anysegwit and otherwise allow a lower bound of 330 satoshis.

correction: I see eclair's reasoning for not allowing lower than 546 unless segwit-only shutdown. Maybe this should only apply if the node has option_shutdown_anysegwit & segwit-only shutdown. I know the latter isn't a feature-bit, but it could be.

I think this should only apply for option_shutdown_anysegwit and otherwise allow a lower bound of 330 satoshis.

I disagree, because option_shutdown_anysegwit can be activated after the channel is opened.
For example, Alice and Bob open a channel without option_shutdown_anysegwit and set dust_limit to 330 sats.
Then later they both activate option_shutdown_anysegwit (because why not?).
If they didn't use an upfront_shutdown_script, they're allowed to use a segwit v1+ script when closing the channel, which can be an issue for dust limits below 354 sats.
So let's be safe and use 354 sats which is future-proof.

I see, that makes sense.

We are slowly dropping support for non-segwit outputs, as proposed in lightning/bolts#894 We can thus safely allow dust limits all the way down to 354 satoshis. In very rare cases where dust_limit_satoshis is negotiated to a low value, our peer may generate closing txs that will not correctly relay on the bitcoin network due to dust relay policies. When that happens, we detect it and force-close instead of completing the mutual close flow.

TheBlueMatt

I didn't check all the dust limits, but I believe you :)

We are slowly dropping support for non-segwit outputs, as proposed in lightning/bolts#894 We can thus safely allow dust limits all the way down to 354 satoshis.

We are slowly dropping support for non-segwit outputs, as proposed in lightning/bolts#894 We can thus safely allow dust limits all the way down to 354 satoshis. In very rare cases where dust_limit_satoshis is negotiated to a low value, our peer may generate closing txs that will not correctly relay on the bitcoin network due to dust relay policies. When that happens, we detect it and force-close instead of completing the mutual close flow.

t-bast · 2021-10-06T07:39:22Z

Merging this PR as discussed during the last spec meetings and explicitly approved since.

We are slowly dropping support for non-segwit outputs, as proposed in lightning/bolts#894 We can thus safely allow dust limits all the way down to 354 satoshis. In very rare cases where dust_limit_satoshis is negotiated to a low value, our peer may generate closing txs that will not correctly relay on the bitcoin network due to dust relay policies. When that happens, we detect it and force-close instead of completing the mutual close flow.

@t-bast

P2SH outputs have larger dust requirements than native SegWit (and Taproot) scripts; we disallow the creation of them on funding txs as a they're problematic for relay when allowing dust limits below 546 sats (see lightning#894) Suggested-by: @t-bast

@t-bast

P2SH outputs have larger dust requirements than native SegWit (and Taproot) scripts; we disallow the creation of them on funding txs as a they're problematic for relay when allowing dust limits below 546 sats (see lightning#894) Suggested-by: @t-bast

t-bast requested a review from rustyrussell August 11, 2021 07:35

t-bast mentioned this pull request Aug 11, 2021

Lightning Specification Meeting 2021/08/16 #893

Closed

18 tasks

TheBlueMatt reviewed Aug 12, 2021

View reviewed changes

TheBlueMatt approved these changes Aug 12, 2021

View reviewed changes

TheBlueMatt mentioned this pull request Aug 16, 2021

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

Closed

Roasbeef approved these changes Aug 17, 2021

View reviewed changes

Roasbeef reviewed Aug 17, 2021

View reviewed changes

02-peer-protocol.md Outdated Show resolved Hide resolved

t-bast force-pushed the check-remote-dust-limit branch from f16ea99 to 40cfede Compare August 18, 2021 12:13

t-bast mentioned this pull request Aug 18, 2021

Trim closing output below network dust threshold #896

Closed

t-bast mentioned this pull request Aug 27, 2021

Lightning Specification Meeting 2021/08/30 #901

Closed

15 tasks

t-bast force-pushed the check-remote-dust-limit branch from 40cfede to 0e39482 Compare August 31, 2021 14:41

t-bast mentioned this pull request Aug 31, 2021

Allowing dust limit below 546 sats #905

Closed

t-bast mentioned this pull request Sep 8, 2021

Lightning Specification Meeting 2021/09/13 #909

Closed

19 tasks

t-bast added 3 commits September 15, 2021 09:36

Peers need to check each other's dust limit

a19d5fb

Since HTLCs below this amount will not appear in the commitment tx, they are effectively converted to miner fees. The peer could use this to grief you by broadcasting its commitment once it contains a lot of dust HTLCs. Fixes #696

Add network dust thresholds

7e3dce4

As implemented in Bitcoin Core's default relay policy.

Drop non-segwit support in shutdown

9ff1387

This allows dust limit to go as low as 354 sats without creating relay issues with default node policies. We add a requirement that dust limit cannot be lower than 354 sats. This ensures implementers don't have to figure this subtlety on their own.

t-bast force-pushed the check-remote-dust-limit branch from 342ab55 to 9ff1387 Compare September 15, 2021 07:37

t-bast added a commit to ACINQ/eclair that referenced this pull request Sep 15, 2021

Lower minimum remote dust limit

5613aa8

We are slowly dropping support for non-segwit outputs, as proposed in lightning/bolts#894 We can thus safely allow dust limits all the way down to 354 satoshis.

t-bast mentioned this pull request Sep 15, 2021

Lower minimum remote dust limit ACINQ/eclair#1900

Merged

This was referenced Sep 15, 2021

Set remote dust limit lower bound ACINQ/lightning-kmp#294

Merged

interactive-tx: Add dual-funding flow, using the interactive tx protocol (feature 28/29) #851

Merged

Crypt-iQ mentioned this pull request Sep 20, 2021

lnwallet+htlcswitch: make Switch dust-aware lightningnetwork/lnd#5770

Merged

Crypt-iQ reviewed Sep 21, 2021

View reviewed changes

fixup! Add network dust thresholds

dff76b2

t-bast mentioned this pull request Sep 23, 2021

Lightning Specification Meeting 2021/09/27 #916

Closed

19 tasks

TheBlueMatt approved these changes Sep 27, 2021

View reviewed changes

rustyrussell approved these changes Sep 27, 2021

View reviewed changes

t-bast added a commit to ACINQ/eclair that referenced this pull request Sep 28, 2021

Lower minimum remote dust limit

7f2e484

We are slowly dropping support for non-segwit outputs, as proposed in lightning/bolts#894 We can thus safely allow dust limits all the way down to 354 satoshis.

t-bast merged commit 8f2104e into master Oct 6, 2021

t-bast deleted the check-remote-dust-limit branch October 6, 2021 07:40

bitromortac mentioned this pull request Oct 22, 2021

Implement recent spec changes regarding collab channel close outputs spesmilo/electrum#7542

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Peers need to check each other's dust limit #894

Peers need to check each other's dust limit #894

t-bast commented Aug 11, 2021 •

edited

TheBlueMatt Aug 12, 2021

t-bast Aug 13, 2021

TheBlueMatt Aug 13, 2021

t-bast Aug 13, 2021

t-bast Aug 18, 2021

TheBlueMatt Aug 24, 2021

t-bast Aug 25, 2021

TheBlueMatt Aug 27, 2021 •

edited

rustyrussell Aug 27, 2021

t-bast Aug 31, 2021 •

edited

t-bast commented Aug 16, 2021

TheBlueMatt commented Aug 16, 2021

t-bast commented Aug 16, 2021 •

edited

TheBlueMatt commented Aug 16, 2021

Crypt-iQ commented Aug 16, 2021

Roasbeef left a comment

t-bast commented Sep 14, 2021

TheBlueMatt commented Sep 15, 2021

t-bast commented Sep 15, 2021

Crypt-iQ Sep 21, 2021

Crypt-iQ Sep 21, 2021 •

edited

t-bast Sep 22, 2021

Crypt-iQ Sep 22, 2021

TheBlueMatt left a comment

t-bast commented Oct 6, 2021

Peers need to check each other's dust limit #894

Peers need to check each other's dust limit #894

Conversation

t-bast commented Aug 11, 2021 • 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

Choose a reason for hiding this comment

TheBlueMatt Aug 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-bast Aug 31, 2021 • edited

Choose a reason for hiding this comment

t-bast commented Aug 16, 2021

TheBlueMatt commented Aug 16, 2021

t-bast commented Aug 16, 2021 • edited

TheBlueMatt commented Aug 16, 2021

Crypt-iQ commented Aug 16, 2021

Roasbeef left a comment

Choose a reason for hiding this comment

t-bast commented Sep 14, 2021

TheBlueMatt commented Sep 15, 2021

t-bast commented Sep 15, 2021

Choose a reason for hiding this comment

Crypt-iQ Sep 21, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

t-bast commented Oct 6, 2021

t-bast commented Aug 11, 2021 •

edited

TheBlueMatt Aug 27, 2021 •

edited

t-bast Aug 31, 2021 •

edited

t-bast commented Aug 16, 2021 •

edited

Crypt-iQ Sep 21, 2021 •

edited