-
Notifications
You must be signed in to change notification settings - Fork 5.9k
BIP-352: limit number of recipients to 2^32-1 #2055
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
Draft
theStack
wants to merge
1
commit into
bitcoin:master
Choose a base branch
from
theStack:bip352_limit_recipients
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
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 hidden or 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.
If we ignore the block size limits, and consider this restriction, a transaction with
4294967295 + N1 taproot outputs with only a single silent payment output wouldn't be scanned.I recommend to keep the reference for the recipient case but remove this constraint, because even if the case was possible, the limit would be something arbitrary.
Footnotes
N > 0 ↩
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 assume this is intended as humorous, or do you actually propose a change in the phrasing?
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 wasn't trying to. Maybe I'm going too far with the implications of the sentence, I just want to avoid someone thinking there is a magic number besides which you can ignore the transaction and don't have to scan more outputs, and later suggest we can avoid the worst performing scanning case by lowering this number again.
I'm saying
The number of taproot outputs in the transaction fits within an unsigned 32-bit integeris an arbitrary limit that can be dropped without harm, because is not there where the problem lies.From following the referenced comments above, I understood the original idea of this change was to drop checks on the recipients side (while creating sp outputs), not on the receiving side (while scanning sp outputs).
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 see. From my cursory following along here, it seemed like you were joking about the large numbers involved. I’ll leave it up to the author to reply then.
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.
@nymius: It took me a while to fully understand what you mean, but after reading your comment in the libsecp SP implementation PR (bitcoin-core/secp256k1#1765 (comment)) I think I got it. Trying to express in my own words: mandating a certain recipient limit at the sender side does not imply that we can safely skip txs that exceed that limit (in terms of number of taproot outputs) at the receiver side, as there could have been added an arbitrary number of other taproot outputs that are completely unrelated to the sender's created SP outputs; the spec doesn't prevent that, and doing it seems indeed not unreasonable (thinking e.g. of batched transactions at exchanges, combining SP and non-SP outputs). That's a fair point, and in fact something I overlooked.
So the two limits (while being absurdly high for practical purposes) are less strongly connected than I initially assumed. Will put this PR to draft state for now and think it through again.
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.
Yes, I was referring to this, thanks for the clearer wording!