validation: add const for minimum witness commitment size #18780
Conversation
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
maflcko
approved these changes
Apr 27, 2020
Member
maflcko
left a comment
maflcko
left a comment
There was a problem hiding this comment.
Concept ACK. Thanks for adding the test.
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
This was referenced Apr 27, 2020
fanquake
force-pushed
the
minimum_witness_size
branch
from
54f8c48 to
a9db322
Compare
ajtowns
approved these changes
Apr 28, 2020
Contributor
ajtowns
left a comment
ajtowns
left a comment
There was a problem hiding this comment.
ACK a9db322e017281811e5039c80cd3114c5025ee27
maflcko
approved these changes
Apr 28, 2020
Member
maflcko
left a comment
maflcko
left a comment
There was a problem hiding this comment.
ACK, only style-nits. Feel free to ignore if you don't have to push for other reasons.
Per BIP 141, the witness commitment structure is atleast 38 bytes, OP_RETURN (0x6a) + 36 (0x24) + 4 byte header (0xaa21a9ed) + 32 byte SHA256 hash. It can be longer, however any additional data has no consensus meaning.
As per BIP 141, if there is more than 1 pubkey that matches the witness commitment structure, the one with the highest output index should be chosen. This adds a sanity check that we are doing that, which will fail if anyone trys to "optimise" GetWitnessCommitmentIndex() be returning early.
fanquake
force-pushed
the
minimum_witness_size
branch
from
a9db322 to
692f830
Compare
Member
|
ACK 692f830 |
Member
|
ACK 692f830 🌵 Show signature and timestampSignature: Timestamp of file with hash |
jonatack
reviewed
Apr 29, 2020
jnewbery
reviewed
Apr 29, 2020
Contributor
|
ACK 692f830 |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
May 2, 2020
…ment size 692f830 test: add test for witness commitment index (fanquake) 0644254 validation: Add minimum witness commitment size constant (fanquake) Pull request description: bitcoin@16101de: Per [BIP 141](https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#Commitment_structure), the witness commitment structure is at least 38 bytes, OP_RETURN (0x6a) + 36 (0x24) + 4 byte header (0xaa21a9ed) + 32 byte SHA256 hash. It can be longer, however any additional data has no consensus meaning. bitcoin@54f8c48: As per BIP 141, if there is more than 1 pubkey that matches the witness commitment structure, the one with the highest output index should be chosen. This adds a sanity check that we are doing that, which will fail if anyone tries to "optimize" GetWitnessCommitmentIndex() by returning early. ACKs for top commit: MarcoFalke: ACK 692f830 🌵 jonatack: Code review ACK 692f830 ajtowns: ACK 692f830 jnewbery: utACK 692f830 laanwj: ACK 692f830 Tree-SHA512: 7af3fe4b8a52fea2cdd0aec95f7bb935351a77b73d934bc88d6625a3503311b2a062cba5190b2228f97caa76840db3889032d910fc8e318ca8e7810a8afbafa0
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
8 participants
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.
16101de: Per BIP 141, the witness commitment structure is at least 38 bytes,
OP_RETURN (0x6a) + 36 (0x24) + 4 byte header (0xaa21a9ed) + 32 byte
SHA256 hash. It can be longer, however any additional data has no
consensus meaning.
54f8c48: As per BIP 141, if there is more than 1 pubkey that matches the witness
commitment structure, the one with the highest output index should be
chosen. This adds a sanity check that we are doing that, which will fail
if anyone tries to "optimize" GetWitnessCommitmentIndex() by returning
early.