-
Notifications
You must be signed in to change notification settings - Fork 492
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
Peers need to check each other's dust limit #894
Merged
Merged
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in f16ea99
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I agree with that. Both options have pros and cons:
dust_limit_satoshis = 546 sat
, we don't have issues at closing and Trim closing output below network dust threshold #896 isn't even neededdust_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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened an issue to discuss the
dust_limit_satoshis
lower bounds in #905I'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?