Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lower minimum remote dust limit #1900

Merged
merged 4 commits into from
Sep 29, 2021
Merged

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented Aug 9, 2021

Implement lightning/bolts#894

We can safely accept dust limits as small as 354 sats, as long as we explicitly handle the case where a closing tx is generated with an output below dust (when that happens, we force-close to get our funds back).

The spec PR received broad agreement during yesterday's spec meeting, so we can safely merge this change now.

@t-bast t-bast marked this pull request as ready for review August 9, 2021 15:00
@t-bast t-bast requested a review from sstone August 9, 2021 15:00
@codecov-commenter
Copy link

codecov-commenter commented Aug 9, 2021

Codecov Report

Merging #1900 (ca2c87f) into master (c846781) will decrease coverage by 0.01%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master    #1900      +/-   ##
==========================================
- Coverage   87.67%   87.65%   -0.02%     
==========================================
  Files         158      158              
  Lines       12407    12420      +13     
  Branches      519      512       -7     
==========================================
+ Hits        10878    10887       +9     
- Misses       1529     1533       +4     
Impacted Files Coverage Δ
...c/main/scala/fr/acinq/eclair/channel/Helpers.scala 95.84% <87.50%> (-0.33%) ⬇️
...re/src/main/scala/fr/acinq/eclair/NodeParams.scala 93.48% <100.00%> (ø)
...c/main/scala/fr/acinq/eclair/channel/Channel.scala 88.27% <100.00%> (ø)
...ala/fr/acinq/eclair/balance/ChannelsListener.scala 93.10% <0.00%> (-3.45%) ⬇️
...r-core/src/main/scala/fr/acinq/eclair/Eclair.scala 49.03% <0.00%> (-1.93%) ⬇️
...r/acinq/eclair/payment/send/PaymentLifecycle.scala 86.70% <0.00%> (-1.53%) ⬇️
...clair/channel/publish/ReplaceableTxPublisher.scala 85.88% <0.00%> (-1.18%) ⬇️
...-core/src/main/scala/fr/acinq/eclair/io/Peer.scala 89.94% <0.00%> (-0.11%) ⬇️
...src/main/scala/fr/acinq/eclair/router/Router.scala 93.78% <0.00%> (-0.10%) ⬇️
...cala/fr/acinq/eclair/channel/ChannelFeatures.scala 100.00% <0.00%> (ø)
... and 5 more

@sstone
Copy link
Member

sstone commented Aug 16, 2021

If our peer uses non-segwit shutdown scripts and we lower the dust limit below what is acceptable for non-segwit outputs (546 instead of 330), then we may end up creating mutual close transactions with dust outputs ?

@t-bast
Copy link
Member Author

t-bast commented Aug 31, 2021

I started a discussion on the spec regarding this issue: lightning/bolts#905
Let's see what direction we want to go in and I'll update this PR accordingly.

@t-bast t-bast force-pushed the remote-low-dust-limit-threshold branch from 00d7b2f to 705cc91 Compare September 15, 2021 08:40
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.
We shouldn't use non-segwit scripts anymore as they can in theory mess
with the dust limits (and we should encourage migration to segwit).
@t-bast t-bast force-pushed the remote-low-dust-limit-threshold branch from 705cc91 to ca2c87f Compare September 28, 2021 06:44
@t-bast t-bast requested a review from pm47 September 28, 2021 06:45
Copy link
Member

@pm47 pm47 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was expecting a check on the script with a force close branch here:

case Event(remoteShutdown@Shutdown(_, remoteScriptPubKey, _), d: DATA_NORMAL) =>
but you are doing it differently. You let the mutual close process reach the point where a sig is exchanged, an only then do we check amounts and maybe force-close. Is that correct?

I was also expecting a check there for the case where we initiate the mutual close:

case Event(c: CMD_CLOSE, d: DATA_NORMAL) =>
and I understand why you may want to fail earlier in the API to return a proper error, but that doesn't remove the need for proper handling of CMD_CLOSE to prevent unnecessary force closes if used directly (e.g. from a plugin).

@t-bast
Copy link
Member Author

t-bast commented Sep 28, 2021

You let the mutual close process reach the point where a sig is exchanged, an only then do we check amounts and maybe force-close. Is that correct?

Yes, that was the least invasive option, because we need to have decided on a fee before we can check whether we need to force-close (since the fee impacts the output amounts). But it's mostly because it was easiest to do here than elsewhere.

I was also expecting a check there for the case where we initiate the mutual close:

I don't think it's worth explicitly rejecting here.
In almost all cases, it's actually ok if non-segwit is used.
The edge case where we need to force-close is close to impossible to get into (see details here: lightning/bolts#905).
I even hesitated to handle it at all, but since it was a very small amount of code I found it was worth it.

But this was a good opportunity to promote segwit-only at the API level to encourage our users to migrate (if not already done), because there's just no reason to use non-segwit today.

@t-bast t-bast merged commit 5fc980c into master Sep 29, 2021
@t-bast t-bast deleted the remote-low-dust-limit-threshold branch September 29, 2021 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants