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

o1vm/pickles: absorb quotient polynomial and evaluate #2655

Closed
wants to merge 4 commits into from

Conversation

dannywillems
Copy link
Member

No description provided.

@dannywillems dannywillems marked this pull request as draft October 4, 2024 10:50
@dannywillems dannywillems force-pushed the dw/continue-quotient-polynomial branch from 0cd8e3f to aa2fdc6 Compare October 4, 2024 11:12
Copy link

codecov bot commented Oct 4, 2024

Codecov Report

Attention: Patch coverage is 6.45161% with 29 lines in your changes missing coverage. Please review.

Project coverage is 72.54%. Comparing base (96d42c1) to head (f573ef6).
Report is 72 commits behind head on master.

Files with missing lines Patch % Lines
o1vm/src/pickles/prover.rs 0.00% 29 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2655      +/-   ##
==========================================
- Coverage   72.57%   72.54%   -0.04%     
==========================================
  Files         246      246              
  Lines       58105    58132      +27     
==========================================
+ Hits        42171    42173       +2     
- Misses      15934    15959      +25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dannywillems dannywillems force-pushed the dw/continue-quotient-polynomial branch from aa2fdc6 to 57a6adb Compare October 4, 2024 17:29
It will be better to have them instead of having randomly the value popping up
in the code at different places.
@dannywillems dannywillems force-pushed the dw/continue-quotient-polynomial branch from 57a6adb to f573ef6 Compare October 4, 2024 19:03
/// have a degree 7 for the quotient polynomial.
/// Used to keep track of the number of chunks we do have when we commit to the
/// quotient polynomial.
pub const DEGREE_QUOTIENT_POLYNOMIAL: u64 = 7;
Copy link
Contributor

Choose a reason for hiding this comment

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

it's not because we evaluate on d8 that the degree is 8
it should be degree of the constraints - 1

@@ -225,7 +230,9 @@ where
quotient
};

let _t_comm = srs.commit_non_hiding(&quotient_poly, 7);
let t_comm = srs.commit_non_hiding(&quotient_poly, DEGREE_QUOTIENT_POLYNOMIAL as usize);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a part of this PR, but do we want to do chunking ?
We do not have any limitation on the SRS

Base automatically changed from o1vm/quotient-polynomial to master October 7, 2024 08:34
Comment on lines +291 to +309
// Compute ft(X) = \
// (1 - ζ^n) \
// (t_0(X) + ζ^n t_1(X) + ... + ζ^{kn} t_{k}(X))
// where Σ_i t_i(X) X^{i n} = t(X), and t(X) is the quotient polynomial.
// At the end, we get the (partial) evaluation of the constraint polynomial
// in ζ.
let ft: DensePolynomial<G::ScalarField> = {
let evaluation_point_to_domain_size = zeta.pow([domain.d1.size]);
// Compute Σ_i t_i(X) ζ^{i n}
// First we split t in t_i, and we reduce to degree (n - 1) after using `linearize`
let t_chunked: DensePolynomial<G::ScalarField> = quotient_poly
.to_chunked_polynomial(DEGREE_QUOTIENT_POLYNOMIAL as usize, domain.d1.size as usize)
.linearize(evaluation_point_to_domain_size);
// -Z_H = (1 - ζ^n)
let minus_vanishing_poly_at_zeta = -domain.d1.vanishing_polynomial().evaluate(&zeta);
// Multiply the polynomial Σ_i t_i(X) ζ^{i n} by -Z_H(ζ)
// (the evaluation in ζ of the vanishing polynomial)
t_chunked.scale(minus_vanishing_poly_at_zeta)
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I am confused, why are we evaluating this ?
We should evaluate the chunks of t, not scale by the vanishsing poly ?
Also, if we do chunking, at this point we should have access to the chunks (since we commited to them), so we should not call to chunk one more time
And if we want to evaluate the constraint poly, we should not go through t, we have a direct access to it

@dannywillems
Copy link
Member Author

Closing in favor of #2692

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.

2 participants