-
Notifications
You must be signed in to change notification settings - Fork 220
Open quotient polynomials at gz
#379
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
Conversation
adr1anh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just left very minor nits.
| /// - Assume each column value is an evaluation of a trace polynomial T_i(x). | ||
| /// - Assume each column value is an evaluation of a polynomial T_i(x), either trace or | ||
| /// constraint composition polynomial. | ||
| /// - For each T_i(x) compute T'_i(x) = (T_i(x) - T_i(z)) / (x - z) and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it doesn't make much sense to optimize the verifier here, but we could compute this more efficiently like the recursive verifier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about it but it seemed like a non-negligible amount of work for a marginal improvement in our current context.
We can come back to it if we need to optimize the verifier for whatever reason, if you agree.
cc90089 to
a636005
Compare
|
LGTM (but can't approve) |
plafer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Left a nit
prover/src/composer/mod.rs
Outdated
|
|
||
| // --- merge polynomials of the composition polynomial trace ------------------------------ | ||
| i = 0; | ||
| for poly in quotient_polys.into_columns() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of re-using i, I would use .into_columns().into_iter().enumerate() (as i is not needed in subsequent loops)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, changed it now
a636005 to
1db592b
Compare
irakliyk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you! I left a few small comments inline. Also, I'd like to clarify a couple of points:
My understanding is that we went from:
Where
To something like:
Where the first
Is this correct? If so, this seems like a bit more work (we also need to merge the evaluations at
Another thing that wasn't clear to me: for constraint evaluations at QuotientOodFrame are actual evaluations rather than some arbitrary data.
| pub struct QuotientOodFrame<E: FieldElement> { | ||
| current_row: Vec<E>, | ||
| next_row: Vec<E>, | ||
| num_quotients: usize, | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to have num_quotients as a separate filed here? Isn't basically the same as current_row.len()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, it is not needed. Removed now
Actually, if I am not missing something, this should be, algorithmically at least, better for the prover than before as we have removed the synthetic divisions, one per constraint composition polynomial, and just kept the original two divisions that were used for the trace columns, one for
There are, at least, two points I believe:
|
Makes sense!
Yes, makes sense that FRI guarantees that openings at It has also been a while since I thought about how this works - so, could be missing something simple. |
It has to be the same polynomial that was opened at |
Ah yes - makes sense! Thank you for explaining! |
|
Hopefully all feedback is addressed now :) |
irakliyk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good! Thank you!
This leads to a simplification of the code and has a also a simplifying effect on recursive verification.
Note: Ran some benchmarks and it seems that there is no noticeable performance changes.