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

Improvements over plonk #671

Closed

Conversation

irfanbozkurt
Copy link
Contributor

Description

  • Satisfies change requested in Plonk Optimizations #655
  • Same domain (powers of omega) was being calculated 4 times. Reduces that to only once
  • Introduces successors to some places instead of instantiating a vector and pushing

@irfanbozkurt irfanbozkurt requested review from schouhy, ajgara and a team as code owners November 6, 2023 21:53
@codecov-commenter
Copy link

Codecov Report

Merging #671 (1aadcb8) into main (2d2f4e3) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #671   +/-   ##
=======================================
  Coverage   96.77%   96.77%           
=======================================
  Files         118      118           
  Lines       28454    28461    +7     
=======================================
+ Hits        27536    27543    +7     
  Misses        918      918           
Files Coverage Δ
provers/plonk/src/prover.rs 99.72% <100.00%> (+<0.01%) ⬆️
provers/plonk/src/setup.rs 99.29% <100.00%> (ø)
provers/plonk/src/test_utils/circuit_1.rs 100.00% <100.00%> (ø)
provers/plonk/src/test_utils/circuit_2.rs 100.00% <100.00%> (ø)
provers/plonk/src/test_utils/circuit_json.rs 100.00% <100.00%> (ø)
provers/plonk/src/test_utils/utils.rs 98.07% <100.00%> (+0.20%) ⬆️

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

let new_factor = num / den;
let new_term = coefficients.last().unwrap() * &new_factor;
coefficients.push(new_term);
z_evaluations.push(z_evaluations.last().unwrap() * (&z_nums[i] * &z_denoms[i]));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be done better using a scan iterator. That avoids pushing every evaluation into the vector, which is not super efficient

.collect();
let powers_main_group: Vec<G1Point> =
core::iter::successors(Some(FieldElement::from(1)), |acc| Some(acc * &s))
.take(n + 3)
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need for two maps; we can just fuse them into a single one with g1.operate_with_self(power.representative()).
Yet another version could use successors starting from the generator and having s directly as representative, so at every step we have s*previous

let z_nums: Vec<_> = (0..&cpi.n - 1)
.map(|i| {
lp(&witness.a[i], &cpi.domain[i])
* lp(&witness.b[i], &(&cpi.domain[i] * &cpi.k1))
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this could be more efficient just by just iterating over cpi.domain zipped with witness.a, witness.b, witness.c

result
let u_powers = [&FieldElement::one(), &u, &(u * u)];

(0..=2)
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need to start from zero if it is just multiplying with one

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