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.

Sign up for GitHub

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

Jump to bottom

Async Halo2 verifier service #2645

Merged
merged 22 commits into from
Nov 17, 2021
Merged

Async Halo2 verifier service #2645

merged 22 commits into from
Nov 17, 2021

Conversation

dconnolly
Copy link
Contributor

@dconnolly dconnolly commented Aug 20, 2021 •
edited
Loading

Motivation

To match our verification patterns, we need a Halo2 proof async verifier service.

Solution

Implements a Halo2 verifier service that reproduces the batch api, to maintain the same API as out other verifier services and to be future-proof for batch optimizations for Halo2.

We don't have V5 transactions with verifiable Action proofs yet to use as test vectors, when we can mint those via zcashd they should be used to test this.

In the meantime, we produce test-specific Halo2 proofs. We should augment with test vectors as soon as they are available.

Resolves #2104

Review

Anyone but especially @jvff @conradoplg @upbqdn @teor2345

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

Follow Up Work

  • When finalized, encode the hash of the VerifyingKey and assert that it matches the ::build()'d one

@dconnolly dconnolly changed the title First pass at async Halo2 verification service [Blocked?] First pass at async Halo2 verification service Aug 20, 2021
@dconnolly dconnolly changed the title [Blocked?] First pass at async Halo2 verification service [Blocked?] Async Halo2 verifier service Aug 20, 2021
@dconnolly dconnolly changed the title [Blocked?] Async Halo2 verifier service Async Halo2 verifier service Oct 14, 2021
@dconnolly
Copy link
Contributor Author

Now unblocked as the orchard crate/circuit has been released

@teor2345 teor2345 added the P-Low label Oct 18, 2021
@dconnolly dconnolly marked this pull request as ready for review November 6, 2021 04:20
@dconnolly dconnolly added P-High A-consensus Area: Consensus rule updates A-dependencies Area: Dependency file updates A-rust Area: Updates to Rust code NU-5 Network Upgrade: NU5 specific tasks and removed P-Low labels Nov 6, 2021
teor2345
teor2345 reviewed Nov 8, 2021
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

I've skimmed this PR and I can't see anything particularly risky or concerning here.

I'd like to leave the detailed review to other people.

conradoplg
conradoplg requested changes Nov 9, 2021
View reviewed changes
Copy link
Collaborator

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

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

Looks good! I just left some suggestions

zebra-consensus/src/primitives/halo2.rs Outdated Show resolved Hide resolved
zebra-consensus/src/primitives/halo2.rs Outdated Show resolved Hide resolved
zebra-consensus/src/primitives/halo2/tests.rs Outdated Show resolved Hide resolved
jvff
jvff reviewed Nov 9, 2021
View reviewed changes
Copy link
Contributor

@jvff jvff left a comment

Choose a reason for hiding this comment

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

Haven't fully finished reviewing, but I wanted to post a question so I don't block you 😅

zebra-consensus/src/primitives/halo2.rs Outdated Show resolved Hide resolved
zebra-consensus/src/primitives/halo2.rs Show resolved Hide resolved
zebra-consensus/src/primitives/halo2.rs Outdated Show resolved Hide resolved
@dconnolly dconnolly mentioned this pull request Nov 9, 2021
Closed
jvff
jvff suggested changes Nov 10, 2021
View reviewed changes
Copy link
Contributor

@jvff jvff left a comment

Choose a reason for hiding this comment

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

Looks great! I added some small suggestions, but they're optional. The only thing I'd like to be sure before approving is if the Display implementation is correct.

zebra-consensus/src/primitives/halo2/tests.rs Outdated Show resolved Hide resolved
zebra-consensus/src/primitives/halo2/tests.rs Outdated Show resolved Hide resolved
teor2345
teor2345 reviewed Nov 12, 2021
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
@dconnolly dconnolly requested a review from jvff November 12, 2021 01:03
dconnolly and others added 22 commits November 16, 2021 18:49
@dconnolly
First pass at async Halo2 verification service
ecf1fad 
Stubs out a batch verifier for the future.
The dependencies for orchard, halo2, librustzcash, zcash_primitives, have
not been resolved.
@dconnolly
Halo2 verifier service and test
225300c
@dconnolly
Remove redundant conversion
1aff29d
@dconnolly
Test async halo2 verifier service with pre-computed Orchard shielded …
506f04a 
…data test vectors
@dconnolly @conradoplg
Fix typo
77fea70 
Co-authored-by: Conrado Gouvea <[email protected]>
@dconnolly @jvff
Assert future result is_ok() in Halo2 verifier test
820021f 
Co-authored-by: Janito Vaqueiro Ferreira Filho <[email protected]>
@dconnolly
Shorten tower::Service trait constraints for Halo2 verifier tests
a13956f
@dconnolly
Remove commented out trait constraints
57f4855
@dconnolly
.expect() vs .unwrap() to parse orchard::redpallas::VerificationKey
fac0b3e
@dconnolly
Use .to_vec() for some test vectors
3fc6c5d
@dconnolly
Fix self-referential Display impl
4ec0842
@dconnolly
Fix deps
93dbeee
@dconnolly
Distinguish orchard vs zebra_chain::orchard imports
804cfaf
@dconnolly
Add test that halo2 verifier fails with malformed proof inputs
e907547
@dconnolly
Use thiserror for Halo2Error
b50b8ce
@dconnolly
Use ZcashFoundation/orchard instead of dconnolly/orchard
ce93931
@dconnolly
Add a link to the issue to remove the zfnd fork of orchard crate
09dc2af
@dconnolly @teor2345
Update zebra-consensus/Cargo.toml
148fa71 
Co-authored-by: teor <[email protected]>
@dconnolly
Add note
6946e79
@dconnolly
Move artificial Orchard shielded data test vectors to zebra-test
f19cc64
@dconnolly
Align brackets
0ac71f7
@dconnolly @jvff
Tidy some trait constraints and debug statements
6607633 
Co-authored-by: Janito Vaqueiro Ferreira Filho <[email protected]>
@dconnolly dconnolly mentioned this pull request Nov 17, 2021
Merged
3 tasks
teor2345
teor2345 approved these changes Nov 17, 2021
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Looks good!

@teor2345 teor2345 merged commit eda83eb into main Nov 17, 2021
@teor2345 teor2345 deleted the halo2-verifier branch November 17, 2021 03:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teor2345 teor2345 teor2345 approved these changes

@upbqdn upbqdn Awaiting requested review from upbqdn

@jvff jvff Awaiting requested review from jvff

@conradoplg conradoplg Awaiting requested review from conradoplg

Assignees

@dconnolly dconnolly

Labels
A-consensus Area: Consensus rule updates A-dependencies Area: Dependency file updates A-rust Area: Updates to Rust code NU-5 Network Upgrade: NU5 specific tasks
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement a Halo2Verifier async service
5 participants
@dconnolly @conradoplg @jvff @teor2345 @mpguerra