Skip to content
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

Transaction verifier structure #1173

Merged
merged 15 commits into from
Oct 20, 2020
Merged

Transaction verifier structure #1173

merged 15 commits into from
Oct 20, 2020

Conversation

hdevalence
Copy link
Contributor

Supersedes #1100, as described here: #1100 (comment)

Rather than force-pushing the rebased branch on top of that one, I created a new PR (this one) to allow comparison.

This PR:

  • fills out the transaction::Verifier with enough structure to slot in the rest of the checks one-by-one;
  • adds some helper methods to zebra-chain to support future checks;
  • demonstrates async checks using the batch redjubjub code;
  • fills in the script::Verifier, which does plumbing to asynchronously connect the state with zebra-script;
  • connects the script, transaction, and block verifiers.

The extra checks added in #1100 are removed and will come in a follow-on PR. This way, this PR focuses on the structure and the later one on the consensus rules.

@hdevalence
Copy link
Contributor Author

One issue that we should fix is the duplication in error types. The zebra_consensus::error module already has error types that are supposed to exhaustively enumerate consensus rules (#1029), but these are currently duplicated by the new transaction code.

@hdevalence
Copy link
Contributor Author

Hmm, it looks like the error type duplication is pre-existing to this PR: there's VerifyBlockError and VerifyTransactionError that both shouldn't exist. We could either fix it now, or in a later PR. The downside of fixing it later is that implementing other consensus rules requires having the right error types, so all later changes would still block on the later PR.

@dconnolly dconnolly added this to the Transaction Validation milestone Oct 16, 2020
Comment on lines +8 to +31
//! Zebra's implementation of the Zcash consensus rules is oriented
//! around three telescoping notions of validity:
//!
//! 1. *Structural Validity*, or whether the format and structure of the
//! object are valid. For instance, Sprout-on-BCTV14 proofs are not
//! allowed in version 4 transactions, and a transaction with a spend
//! or output description must include a binding signature.
//!
//! 2. *Semantic Validity*, or whether the object could potentially be
//! valid, depending on the chain state. For instance, a transaction
//! that spends a UTXO must supply a valid unlock script; a shielded
//! transaction must have valid proofs, etc.
//!
//! 3. *Contextual Validity*, or whether a semantically valid
//! transaction is actually valid in the context of a particular
//! chain state. For instance, a transaction that spends a
//! UTXO is only valid if the UTXO remains unspent; a
//! shielded transaction spending some note must reveal a nullifier
//! not already in the nullifier set, etc.
//!
//! *Structural validity* is enforced by the definitions of data
//! structures in `zebra-chain`. *Semantic validity* is enforced by the
//! code in this crate. *Contextual validity* is enforced in
//! `zebra-state` when objects are committed to the chain state.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 💟 🤓

dconnolly and others added 14 commits October 20, 2020 01:32
Co-authored-by: Deirdre Connolly <deirdre@zfnd.org>
This squashes the previous sequence of commits to let us separate out
the structural skeleton (which unblocks other work and is not
consensus-critical) from the actual checks (which don't block other work
and are consensus-critical).

Co-authored-by: Deirdre Connolly <deirdre@zfnd.org>
- remove no longer accurate documentation about transaction verifier;
- add description of the role of the crate.
This reduces the API surface to the minimum required for functionality,
and cleans up module documentation.  The stub mempool module is deleted
entirely, since it will need to be redone later anyways.
@dconnolly dconnolly force-pushed the transaction-verifier-structure branch from 50182ad to dcd814d Compare October 20, 2020 05:36
@dconnolly
Copy link
Contributor

Hmm, it looks like the error type duplication is pre-existing to this PR: there's VerifyBlockError and VerifyTransactionError that both shouldn't exist. We could either fix it now, or in a later PR. The downside of fixing it later is that implementing other consensus rules requires having the right error types, so all later changes would still block on the later PR.

I've created #1186 so that we can merge this for now and let the checks one get rebased, and will dedupe the Errors separately but v soon.

@dconnolly dconnolly merged commit 9549e18 into main Oct 20, 2020
@dconnolly dconnolly deleted the transaction-verifier-structure branch October 20, 2020 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants