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

Witness multiplication optimization #70

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ryanleh
Copy link
Contributor

@ryanleh ryanleh commented Apr 3, 2021

Description

Currently, the witness polynomial multiplication in the second round of the AHP requires performing an FFT with a domain of size 4 * |H|. This change distributes the multiplication so that it only requires an FFT with a domain of size 2 * |H| along with some cheap multiplications/additions.

Running marlin-benches on a fairly powerful machine from master:

per-constraint proving time for Bls12_381: 51662 ns/constraint
per-constraint proving time for MNT4_298: 45649 ns/constraint
per-constraint proving time for MNT6_298: 55951 ns/constraint
per-constraint proving time for MNT4_753: 321626 ns/constraint

Running marlin-benches on the same machine from this PR:

per-constraint proving time for Bls12_381: 50834 ns/constraint
per-constraint proving time for MNT4_298: 46081 ns/constraint
per-constraint proving time for MNT6_298: 54172 ns/constraint
per-constraint proving time for MNT4_753: 320387 ns/constraint

So it seems to give an overall slight latency improvement and reduces the memory overhead of proving. I would guess that performance gains would be more noticeable on weaker machines.

This PR depends on arkworks-rs/algebra#258


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (master)
  • Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

@@ -356,14 +360,16 @@ impl<F: PrimeField> AHPForR1CS<F> {

let z_a_poly_time = start_timer!(|| "Computing z_A polynomial");
let z_a = state.z_a.clone().unwrap();
let z_a_poly = &EvaluationsOnDomain::from_vec_and_domain(z_a, domain_h).interpolate()
+ &(&DensePolynomial::from_coefficients_slice(&[F::rand(rng)]) * &v_H);
let r_a = F::rand(rng);
Copy link
Member

@ValarDragon ValarDragon Apr 3, 2021

Choose a reason for hiding this comment

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

Can there be a comment here explaining the operation these 3 lines are doing? (Ditto for z_b)

I believe its "go from z_a evaluations to z_a_poly as interpolate(z_a_evaluations over H) * (Z_I)) + rand_poly

Copy link
Member

Choose a reason for hiding this comment

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

It's computed as interpolate(z_a_evaluations_over_H) + v_H * r_a

src/ahp/prover.rs Outdated Show resolved Hide resolved
Comment on lines 299 to 301
mz_rands: None,
mz_polys: None,
mz_rand_polys: None,
Copy link
Member

@Pratyush Pratyush Apr 3, 2021

Choose a reason for hiding this comment

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

I don't think we need to store both mz_polys and mz_rand_polys; we can store just mz_rand_polys, and then later on, when we need to compute z_a * z_b, we just temporarily mutate mz_rand_polys in place by subtracting mz_rands * v_H.

This should reduce memory consumption.

Comment on lines +473 to +474
let v_H: DensePolynomial<F> = domain_h.vanishing_polynomial().into();
let z_a_poly = randomized_z_a_poly.polynomial() - &(&v_H * r_a);
Copy link
Member

Choose a reason for hiding this comment

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

Oh hm I meant that we should instead mutate randomized_z_a_poly in place temporarily. Also, we shouldn't convert v_H into a dense polynomial, as that wastes memory. Instead, we should add a way to subtract a sparse polynomial from a dense polynomial (if that doesn't already exist)

Copy link
Member

Choose a reason for hiding this comment

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

(and also a way to multiply a sparse polynomial by a scalar, mirroring the one for DensePolynomial

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, so the problem with that is randomized_z_a_poly and randomized_z_b_poly are inside an Rc that is borrowed by ProverFirstOracles here until the end of the protocol. So we could mutate in place, but I believe this would require unsafe to do (which I'm fine doing because we know the other reference isn't used until later) - or am I missing something?

I'll make the other changes mentioned for now though!

Copy link
Member

Choose a reason for hiding this comment

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

Hmm that’s annoying...

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