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

Batch Verification API #59

Merged
merged 12 commits into from
Feb 11, 2021
Merged

Batch Verification API #59

merged 12 commits into from
Feb 11, 2021

Conversation

yaahc
Copy link
Contributor

@yaahc yaahc commented Jan 14, 2021

No description provided.

@dconnolly
Copy link
Contributor

The CI is pinned to 1.44.0 and build failing, on 1.49.0 (latest stable toolchain) it's passing.

Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

Untested ACK :-)


ml_terms.push(((proof.a * z).into(), (-proof.b).into()));

acc_Gammas[0] += &z; // a_0 is implicitly set to 1 (??)
Copy link
Contributor

Choose a reason for hiding this comment

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

That's correct.

Suggested change
acc_Gammas[0] += &z; // a_0 is implicitly set to 1 (??)
acc_Gammas[0] += &z; // a_0 is implicitly set to 1

//
// Y^acc_Y in multiplicative notation is [acc_Y]Y (scalar mul) in
// additive notation, which is the code convention here.
let Y = E::pairing(&vk.alpha_g1, &vk.beta_g2) * acc_Y;
Copy link
Contributor

Choose a reason for hiding this comment

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

This pairing could be precomputed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, calling the result Y could be a little confusing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@daira is right that it can be precomputed, but it's probably not more efficient in practice to do that. We do it in the groth16 verifier because we're just checking one proof and acc_Y = 1 more or less. In your case the multiplication by acc_Y is expensive -- it involves exponentiating by acc_Y because the result of the pairing is an element of a multiplicative subgroup of a large extension field. Instead, in practice it's probably just fine to do

ml_terms.push((E::G1Affine::from(vk.alpha_g1 * acc_Y), E::G2Prepared::from(vk.beta_g2)));

so you multiply the G1 element by acc_Y (because [z] e(A, B) = e([z] A, B) forall z, A, B) and then just add it to your multi-miller loop.

Copy link
Member

@str4d str4d left a comment

Choose a reason for hiding this comment

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

Please remove the last two commits.

toolchain: 1.44.0
toolchain: stable
Copy link
Member

Choose a reason for hiding this comment

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

This conflicts with #62, where the MSRV is set to 1.47.0.

Copy link
Contributor

Choose a reason for hiding this comment

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

Resolved to 1.47.0

toolchain: 1.44.0
toolchain: stable
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

Copy link
Contributor

Choose a reason for hiding this comment

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

Resolved to 1.47.0

src/lib.rs Outdated
#![deny(intra_doc_link_resolution_failure)]
#![deny(broken_intra_doc_links)]
Copy link
Member

Choose a reason for hiding this comment

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

We cannot change this until the MSRV is at least 1.48.0 (see zcash/librustzcash#279).

Copy link
Contributor

Choose a reason for hiding this comment

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

Reverted

@str4d
Copy link
Member

str4d commented Jan 26, 2021

@dconnolly

The CI is pinned to 1.44.0 and build failing, on 1.49.0 (latest stable toolchain) it's passing.

I believe that specific CI failure is related to #57 (the upstream rustc breakage landed in 1.49 IIRC). It should be possible to similarly work around the issues here.

Comment on lines +92 to +97
let Psi = vk
.ic
.iter()
.zip(acc_Gammas.iter())
.map(|(&Psi_i, acc_Gamma_i)| Psi_i * acc_Gamma_i)
.sum();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Constant factor improvement, but this can be done as a multiexp instead.

// additive notation, which is the code convention here.
let Y = E::pairing(&vk.alpha_g1, &vk.beta_g2) * acc_Y;

let mut terms = vec![];
Copy link
Collaborator

Choose a reason for hiding this comment

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

let ml_terms = ml_terms.iter().map(|(a, b)| (a, b)).collect::<Vec<_>>();

//
// Y^acc_Y in multiplicative notation is [acc_Y]Y (scalar mul) in
// additive notation, which is the code convention here.
let Y = E::pairing(&vk.alpha_g1, &vk.beta_g2) * acc_Y;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@daira is right that it can be precomputed, but it's probably not more efficient in practice to do that. We do it in the groth16 verifier because we're just checking one proof and acc_Y = 1 more or less. In your case the multiplication by acc_Y is expensive -- it involves exponentiating by acc_Y because the result of the pairing is an element of a multiplicative subgroup of a large extension field. Instead, in practice it's probably just fine to do

ml_terms.push((E::G1Affine::from(vk.alpha_g1 * acc_Y), E::G2Prepared::from(vk.beta_g2)));

so you multiply the G1 element by acc_Y (because [z] e(A, B) = e([z] A, B) forall z, A, B) and then just add it to your multi-miller loop.

let mut terms = vec![];
ml_terms.iter().for_each(|(a, b)| terms.push((a, b)));

if E::multi_miller_loop(terms.as_slice()).final_exponentiation() + Y == E::Gt::identity() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you fold the change in like I mentioned then this becomes

if E::multi_miller_loop(&terms).final_exponentiation().is_identity().into() {

I think.

@dconnolly
Copy link
Contributor

@ebfull @str4d I can't re-request a review from you in the github UI but 🙏

@str4d str4d requested review from str4d and ebfull February 2, 2021 01:11
@str4d str4d marked this pull request as ready for review February 2, 2021 01:11
benches/batch.rs Outdated Show resolved Hide resolved
src/groth16/verifier/batch.rs Outdated Show resolved Hide resolved
src/groth16/verifier/batch.rs Outdated Show resolved Hide resolved
src/groth16/verifier/batch.rs Show resolved Hide resolved
src/groth16/verifier/batch.rs Outdated Show resolved Hide resolved
src/groth16/verifier/batch.rs Show resolved Hide resolved
src/groth16/verifier/batch.rs Outdated Show resolved Hide resolved
src/groth16/verifier/batch.rs Outdated Show resolved Hide resolved
src/groth16/verifier/batch.rs Show resolved Hide resolved
src/groth16/verifier/batch.rs Outdated Show resolved Hide resolved
@dconnolly
Copy link
Contributor

dconnolly commented Feb 5, 2021

The clippy 'failure' if you do as it recommends it will not compile because:

‘z’: mismatched types
   expected reference `&<E as pairing::Engine>::Fr`
found associated type `<E as pairing::Engine>::Fr`
consider constraining the associated type `<E as pairing::Engine>::Fr` to `&<E as pairing::Engine>::Fr`
for more information, visit https://doc.rust-lang.org/book/ch19-03-advanced-traits.html [E0308]

@dconnolly
Copy link
Contributor

@str4d @ebfull one more look? 🙏

Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

Untested ACK.

Copy link
Member

@str4d str4d left a comment

Choose a reason for hiding this comment

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

utACK b9737fc

@dconnolly
Copy link
Contributor

@ebfull one more look por favor? 🙏

Copy link
Collaborator

@ebfull ebfull left a comment

Choose a reason for hiding this comment

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

Good job! :)

@ebfull ebfull merged commit bd4af09 into zkcrypto:main Feb 11, 2021
@dconnolly
Copy link
Contributor

Yay, thank you!

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.

5 participants