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

Implement recent spec changes regarding collab channel close outputs #7542

Merged
merged 3 commits into from
Oct 27, 2021

Conversation

bitromortac
Copy link
Contributor

@bitromortac bitromortac commented Oct 22, 2021

Fixes #7541

  • Implements option_shutdown_anysegwit, which lets peers close their channels to taproot scripts.
  • Constrains the allowed shutdown scriptpubkeys to only segwit types.
  • The dust limit is negotiated at channel opening. We currently reject dust limits that are lower than 546 sat, which leads to Dust limit too low when opening channels with latest lnd #7541

Fulfills spec changes:

Related:

Todo:

  • decide on a maximally accepted dust limit from the remote party
  • should we keep our dust limit at 546 sat?
  • decide on whether to force close upon dust limit violation at channel closure
  • a further improvement in the security when forwarding is to apply a limit on the allowed total value of dust HTLCs

electrum/lnutil.py Outdated Show resolved Hide resolved
@SomberNight
Copy link
Member

decide on a maximally accepted dust limit from the remote party

I guess 3000 sat as in current PR is unreasonable.
I would prefer an even lower number though.
Not sure what other implementations use; rust-lightning caps it at 660 sat.
Maybe use 1000 sat?

should we keep our dust limit at 546 sat?

Yes, let's keep that for now. We know from experience that others accept it during channel open, and I think ~500 sat outputs are hardly ever worth sweeping anyway (so not much point in lowering it IMO).

decide on whether to force close upon dust limit violation at channel closure

Hmm. Well I guess it's not critical, as the user can force-close manually if needed.
But yeah the many "dust limits" are becoming a bit confusing, seems to me we are not using the correct one either:

dust_limit_sat=self.config[LOCAL].dust_limit_sat)

it's ~harmless though.

electrum/lnpeer.py Outdated Show resolved Hide resolved
electrum/lnpeer.py Outdated Show resolved Hide resolved
electrum/transaction.py Outdated Show resolved Hide resolved
electrum/transaction.py Outdated Show resolved Hide resolved
electrum/lnpeer.py Outdated Show resolved Hide resolved
lightning/bolts#672

We check the received shutdown script against higher segwit versions and
accept closing to that script if option_shutdown_anysegwit has been
negotiated.
@bitromortac
Copy link
Contributor Author

Agree with your other points, thanks.

Hmm. Well I guess it's not critical, as the user can force-close manually if needed. But yeah the many "dust limits" are becoming a bit confusing, seems to me we are not using the correct one either:

dust_limit_sat=self.config[LOCAL].dust_limit_sat)

it's ~harmless though.

I think so, yes, very confusing 😕.

electrum/lnchannel.py Outdated Show resolved Hide resolved
electrum/lnpeer.py Outdated Show resolved Hide resolved
electrum/lnpeer.py Outdated Show resolved Hide resolved
Copy link
Member

@SomberNight SomberNight left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

EDIT: ah, one final thing; see comment below.

electrum/lnpeer.py Outdated Show resolved Hide resolved
* on channel opening we verify that the peer's dust limit is above 354
  sat, the limit for unknown segwit versions
* we constrain the allowed scriptpubkey types for channel closing
* we check that the remote's output is above the relay dust limit for
  the collaborative close case
@SomberNight SomberNight merged commit 5787f3a into spesmilo:master Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dust limit too low when opening channels with latest lnd
3 participants