Implement Bitcoin Merkle trees #1386
Merged
Conversation
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
dconnolly
reviewed
Nov 25, 2020
teor2345
previously requested changes
Nov 25, 2020
There was a problem hiding this comment.
This looks good.
I'd be happy to merge this PR with the block test vector tests, and open a follow-up ticket for more testing.
I have a few suggestions for improvement:
- test mainnet and testnet block test vectors
- do the merkle root check immediately after the block difficulty check
And a question:
Would a transaction uniqueness check in the block verifier improve security until the full implementation is in place?
dconnolly
reviewed
Nov 25, 2020
dconnolly
reviewed
Nov 25, 2020
dconnolly
reviewed
Nov 25, 2020
hdevalence
force-pushed
the
block-merkle-tree
branch
from
ec89ae9 to
3d5d14e
Compare
hdevalence
dismissed
teor2345’s stale review
included changes in fixup commits
|
Updated the PR to test on all block test vectors, not just mainnet, and to include a comment explaining the |
hdevalence
force-pushed
the
block-merkle-tree
branch
from
3d5d14e to
3e5c37e
Compare
|
Updated issue description to reference #1392. |
|
Moved the checkpointer's Merkle checks into its existing |
|
Looks like the Merkle root computation is incorrect on blocks with a single transaction. |
|
|
hdevalence
force-pushed
the
block-merkle-tree
branch
from
dcf3901 to
d9ae5b3
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Ok, right, that was all pretty obvious stuff, I won't clutter the ticket with it. |
|
I pushed a commit that shows the transactions, block height, and block hash when the merkle root test fails. |
4 tasks
As a side effect of computing Merkle roots, we build a list of transaction hashes. Instead of discarding these, add them to PreparedBlock and FinalizedBlock so that they can be reused rather than recomputed. This commit adds Merkle root validation to: 1. the block verifier; 2. the checkpoint verifier. In the first case, Bitcoin Merkle tree malleability has no effect, because only a single Merkle tree in each malleablity set is valid (the others have duplicate transactions). In the second case, we need to check that the Merkle tree does not contain any duplicate transactions. Closes #1385 Closes #906
This moves it in with the existing `check_block` method and expands that method's contract to cover general block validation checks.
Change the Merkle root validation logic to also check that a block does not contain duplicate transactions. This check is redundant with later double-spend checks, but is a useful defense-in-depth.
Also show the block height and block hash.
hdevalence
force-pushed
the
block-merkle-tree
branch
from
874db1f to
a73bf22
Compare
The `CoinbaseData` parses the block height separately from the rest of the free-form coinbase data. However, it had two bugs: 1. It did not require that the height was canonically encoded; 2. Its canonical encoding was incorrect relative to the BIP34-inherited encoding. This meant that we computed some transaction hashes incorrectly, because when we re-serialized the coinbase transaction, we would canonically serialize the coinbase transaction (using the incorrect definition of canonical, bug 2). And we didn't notice that the wrong definition of canonical encoding was being used because we accepted what we thought were non-canonically encoded heights. The relevant rules are here: https://github.com/zcash/zcash/blob/877212414ad40e37cf8e49884a90767c54d59ba2/src/script/script.h#L307-L346 This commit changes the encoding to reject non-canonically encoded heights, and to match the correct encoding rules. We check that at least one non-canonically encoded height is correctly rejected using a new test vector. The database format increments because we saved a bunch of wrongly encoded blocks. This discrepancy was originally noticed by @teor2345, who pointed out that a previous version of the block 202 test vector (now preserved as "bad block 202") did not match the block from zcashd.
hdevalence
force-pushed
the
block-merkle-tree
branch
from
a73bf22 to
d4ddd6d
Compare
teor2345
approved these changes
Nov 30, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Motivation
Close #906, #1385.
Solution
As a side effect of computing Merkle roots, we build a list of
transaction hashes. Instead of discarding these, add them to
PreparedBlock and FinalizedBlock so that they can be reused rather than
recomputed.
This PR adds Merkle root validation to:
In the first case, Bitcoin Merkle tree malleability has no effect,
because only a single Merkle tree in each malleablity set is valid (the
others have duplicate transactions).
In the second case, we need to check that the Merkle tree does not contain any
duplicate transactions.
Closes #1385
Closes #906
The code in this pull request has:
Review
This should be done before the first alpha, but does not block anything.
Follow Up Work
We should have tests that exercise rejection behavior for both verifiers, but for the checkpoint verifier in particular. These are not done yet, and #1392 tracks them.