Add V5 transparent and sapling to transaction::check, add missing coinbase PrevOut check #2070
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
teor2345
suggested changes
Apr 26, 2021
We only need itertools when the `proptest-impl` feature is enabled.
teor2345
changed the title
|
I pushed a fix that makes the |
teor2345
changed the title
…inputs This is a bugfix on V4 transaction validation. The PrevOut consensus rule was not explicitly stated in the Zcash spec until April 2021. (But it was implied by Bitcoin, and partially implemented by Zebra.) Also do the shielded sapling input check for V5 transactions.
Also make the variable names match the spec.
Move counts or iterators into `Transaction` methods, so we can remove duplicate code, and make the consensus rule logic clearer.
teor2345
changed the title
teor2345
changed the title
|
I added PRs in your repository with spec updates and refactors:
Here's what was missing from the PR:
While I was changing that code, I also:
|
- Quote from the spec - Explain why the function is redunant for v5 - Rename the function so it's clear that it is sapling-specific
Update sapling_balances_match for Transaction v5
Simplify transaction checks, add missing coinbase PrevOut check
2 tasks
9 tasks
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
We need some tests in the consensus side for the new sapling data in V5 transactions.
Solution
Add a test that converts the transactions from all test blocks we have(real past blocks from testnet and mainnet) into V5 transactions and validate them.
Also this PR moves the utility functions needed for the transaction conversion so they can be used by
zebra-chainandzebra-consensuscrates.Update the "check" functions for transaction v5, and simplify.
Add the missing "no transparent inputs with prevouts" coinbase check.
Simplify a bunch of the transaction checks, so the consensus rules are easier to review.
The code in this pull request has:
Review
Related Issues
Implement Transparent and Sapling validation for transaction v5 #1981 - "check" part
Hard-coded sapling-only transaction v5 test vectors #2047
Follow Up Work
Implement Transparent and Sapling validation for transaction v5 #1981 - transaction verifier part