Validate transparent inputs and outputs in V5 transactions #2302
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
reviewed
Jun 15, 2021
jvff
force-pushed
the
validate-sapling-and-transparent-v5-transactions
branch
from
168ff91 to
8d0194a
Compare
teor2345
reviewed
Jun 15, 2021
jvff
changed the title
teor2345
reviewed
Jun 15, 2021
jvff
force-pushed
the
validate-sapling-and-transparent-v5-transactions
branch
from
20a8508 to
ff76c70
Compare
jvff
force-pushed
the
validate-sapling-and-transparent-v5-transactions
branch
from
ff76c70 to
0b60f21
Compare
jvff
commented
Jun 15, 2021
jvff
force-pushed
the
validate-sapling-and-transparent-v5-transactions
branch
from
0b60f21 to
09fcfc9
Compare
teor2345
reviewed
Jun 16, 2021
teor2345
reviewed
Jun 16, 2021
teor2345
suggested changes
Jun 16, 2021
There was a problem hiding this comment.
I'd like to see some more doc comments in this PR, and the code that were added in the last PR.
A short, one-line comment is enough for most functions, types, and fields.
If the function is particularly complex, or has more than a few arguments, explain what each one does.
teor2345
reviewed
Jun 16, 2021
jvff
force-pushed
the
validate-sapling-and-transparent-v5-transactions
branch
from
1e57053 to
ad4cc1f
Compare
dconnolly
reviewed
Jun 17, 2021
dconnolly
reviewed
Jun 17, 2021
dconnolly
reviewed
Jun 17, 2021
dconnolly
added
A-rust
dconnolly
reviewed
Jun 17, 2021
teor2345
reviewed
Jun 18, 2021
jvff
force-pushed
the
validate-sapling-and-transparent-v5-transactions
branch
from
3111ec2 to
0b08aa0
Compare
teor2345
reviewed
Jun 22, 2021
teor2345
suggested changes
Jun 22, 2021
There was a problem hiding this comment.
There's just one confusing function doc comment here, otherwise I'm good to merge.
Feel free to merge if @dconnolly approves and you've fixed the comment.
Document methods to describe what they do and why.
Make it simpler to write the `FuturesUnordered` type with boxed futures. This will also end up being used more when refactoring to return the checks so that the `call` method can wait on them.
Refactors the verification of the transparent inputs and outputs into a separate method.
Instead of pushing the verifications into a stream of unordered futures, use the `ServiceExt::call_all` method to build an equivalent stream after building a stream of requests.
Make it more consistent with the rest of the code, and make sure that the `len()` method is available to use for tracing. Co-authored-by: teor <teor@riseup.net>
Allow the code snipped to be reused by other transaction version-specific check methods.
Use the script verifier to check the transparent inputs in a V5 transaction.
Check if a transaction has inputs and outputs, independently of the transaction version.
Refactor to move the repeated code into the `call` method. Now the validation methods return the set of asynchronous checks to wait for.
Creates a fake source UTXO, and then the input and output that represent spending that UTXO. The initial UTXO can be configured to have a script that either accepts or rejects any spend attempt.
Create a fake V4 transaction that includes a fake transparent transfer of funds. The transfer uses a script to allow any UTXO to spend it.
Create a fake transparent transfer where the source UTXO has a script that rejects spending. The script verifier should not accept this transaction.
Create a mock V5 transaction that includes a transparent transfer of funds. The transaction should be accepted by the verifier.
Create a fake transparent transfer where the source UTXO has a script that rejects spending. The script verifier should not accept this transaction.
Because this set of changes implement transparent validation. Co-authored-by: teor <teor@riseup.net>
Simplify it so that it won't become updated when ZcashFoundation#1683 is fixed. Co-authored-by: teor <teor@riseup.net>
Make it clearer, to avoid mixing up `Network` and `NetworkUpgrade` types. Co-authored-by: teor <teor@riseup.net>
jvff
force-pushed
the
validate-sapling-and-transparent-v5-transactions
branch
from
e162d90 to
d0ea656
Compare
teor2345
approved these changes
Jun 23, 2021
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
As part of NU5, there is a new transaction format and consensus rules (version 5). Zebra already had initial support for the new transaction, but the consensus rules aren't fully implemented. This includes another step in implementing the validation of those rules, by checking the transparent transfers in the V5 transaction.
This is the second PR that's part of #1981.
Specifications
The new transaction encoding and consensus rules are detailed in the specification.
Designs
Solution
The transparent validation code in
transaction::Verifier::verify_v4_transactionmethod was moved to a newtransaction::Verifier::verify_transparent_inputs_and_outputsmethod. This new method now returns aFuturesUnorderedof futures that send validation requests to thescript::Verifierand wait for their response.The new method is now also called by
transaction::Verifier::verify_v5_transaction. Theverify_v5_transactionalso calls thecheck::has_inputs_and_outputshelper.Both V4 and V5 transaction verification rely on asynchronous checks, so a helper method was created to share the code that awaits for the checks to complete and that trace their completion.
Review
@teor2345
Reviewer Checklist
Follow Up Work
There's still some work left for #1981 to be complete, which is validating the shielded parts of the transaction.