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

Asynchronous Groth16 verification #830

Merged
merged 1 commit into from
Feb 5, 2021
Merged

Asynchronous Groth16 verification #830

merged 1 commit into from
Feb 5, 2021

Conversation

hdevalence
Copy link
Contributor

@hdevalence hdevalence commented Aug 5, 2020

This PR is the first step in getting a groth16 proving system fully integrated with the rest of zebra. This PR implements the initial async API, but none of the actual batching logic necessary for our eventual verifier design.

Once the batch verification API from bellman has been implemented we will need to swap out the "Batch" type defined in this crate with the new batch::Verifier defined in bellman.

Follow Up Work

@hdevalence hdevalence marked this pull request as draft August 5, 2020 10:17
@hdevalence hdevalence changed the base branch from main to static-redjubjub August 5, 2020 10:18
@hdevalence hdevalence force-pushed the static-redjubjub branch 2 times, most recently from 6098b17 to 03bec1d Compare August 6, 2020 04:22
Base automatically changed from static-redjubjub to main August 6, 2020 04:28
@hdevalence
Copy link
Contributor Author

Rebased onto main with changes from #821

@hdevalence
Copy link
Contributor Author

Using a WIP branch on the ZF fork of librustzcash for now. Unsure whether it's better to create an intermediate step of having the batch verification API do single verification internally or just go directly to what we want, but I'm leaning towards the latter, since we already know (from the Ed25519 and Redjubjub cases) that an async API with this shape will work.

Now leaning towards a middle option, to create a fake-batch branch that uses the batch verification interface currently defined but internally implements batch verification by calling single verification N times. This lets us reuse the interface, the item types, etc., and we can swap out the math later when we have more time.

@yaahc yaahc mentioned this pull request Aug 6, 2020
18 tasks
@yaahc yaahc added A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement labels Aug 11, 2020
@dconnolly dconnolly mentioned this pull request Aug 11, 2020
27 tasks
@dconnolly
Copy link
Contributor

It builds!

@dconnolly dconnolly marked this pull request as ready for review January 17, 2021 20:47
@dconnolly
Copy link
Contributor

dconnolly commented Jan 17, 2021

@yaahc I moved to 'ready for review' to get the other CI jobs running, you can move back if you want EDIT: i can trigger those jobs without moving from draft

@dconnolly dconnolly marked this pull request as draft January 17, 2021 20:48
@mpguerra mpguerra linked an issue Jan 18, 2021 that may be closed by this pull request
@mpguerra mpguerra added this to the 2021 Sprint 1 milestone Jan 18, 2021
@yaahc yaahc self-assigned this Jan 19, 2021
@yaahc yaahc marked this pull request as ready for review January 27, 2021 18:40
@yaahc yaahc requested a review from dconnolly January 27, 2021 18:40
@mpguerra mpguerra modified the milestones: 2021 Sprint 1, 2021 Sprint 2 Jan 29, 2021
@dconnolly
Copy link
Contributor

@yaahc I rebased because of a conflict related to a bls12 dependency update

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.

As far as I understand, this code looks good.

But it's hard to review without:

  • a PR description that explains the current purpose of this PR
  • doc comments on functions and types
  • tests

@teor2345 teor2345 dismissed their stale review February 4, 2021 00:24

I don't want to block this ticket

@teor2345
Copy link
Contributor

teor2345 commented Feb 4, 2021

If we're planning multiple PRs that change the same code, let's leave the doc comments until the later PRs, so we don't accidentally delete or revert any more comments.

@yaahc yaahc force-pushed the groth16-verifier branch 2 times, most recently from e438669 to df4d1e4 Compare February 4, 2021 00:31
@yaahc yaahc requested review from dconnolly and teor2345 February 4, 2021 00:32
This PR is the first step in getting a groth16 proving system fully
integrated with the rest of zebra. This PR implements the initial async
API, but none of the actual batching logic necessary for our eventual
verifier design.

Once the batch verification API from bellman has been implemented we
will need to swap out the "Batch" type defined in this crate with the
new `batch::Verifier` defined in bellman.
@yaahc
Copy link
Contributor

yaahc commented Feb 5, 2021

If we're planning multiple PRs that change the same code, let's leave the doc comments until the later PRs, so we don't accidentally delete or revert any more comments.

@teor2345 is this resolved btw? I assumed you meant "lets leave adding the doc comments until the later PR" but it never got approved after I did the rest of the cleanup so now I'm unsure if there's still something left for me to do on this PR.

Copy link
Contributor

@dconnolly dconnolly left a comment

Choose a reason for hiding this comment

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

I wonder if we should merge this now, knowing that when zkcrypto/bellman#59 is merged in there will be changes anyway, or try to support this pattern now

@yaahc
Copy link
Contributor

yaahc commented Feb 5, 2021

I wonder if we should merge this now, knowing that when zkcrypto/bellman#59 is merged in there will be changes anyway, or try to support this pattern now

I think merge personally, the PR is already setup to make it easy for the follow up bellman stuff, I don't see any reason to block this PR on it. As for the static verifiers, I think it might be good to add those with the PR that also adds the PreparedVerifyingKeys, since each static will need to hold a reference to a different key.

@dconnolly
Copy link
Contributor

Merging, tracking followup work in #1645

@dconnolly dconnolly merged commit 0ac2594 into main Feb 5, 2021
@dconnolly dconnolly deleted the groth16-verifier branch February 5, 2021 19:52
@teor2345
Copy link
Contributor

teor2345 commented Feb 7, 2021

If we're planning multiple PRs that change the same code, let's leave the doc comments until the later PRs, so we don't accidentally delete or revert any more comments.

@teor2345 is this resolved btw? I assumed you meant "lets leave adding the doc comments until the later PR" but it never got approved after I did the rest of the cleanup so now I'm unsure if there's still something left for me to do on this PR.

I've been focused on other work.

Last time I checked this PR, it was unclear how much work was remaining, so I couldn't really approve it. So the best I could do was dismiss my review so I wasn't blocking it.

@dconnolly dconnolly linked an issue Feb 8, 2021 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Groth16 batch verification optimizations Implement verifiers that accept Groth16 types
5 participants