Implement Lagrange kernel constraints #247
Conversation
There was a problem hiding this comment.
Thank you! Looks good! I left some more comments inline - but these are mostly nits.
One thing I'm still trying to think through is whether we should change anything about how we handle OOD frame structs. But I think this is the last potential change (and I'm not sure if anything should be changed at all on this front).
@Al-Kindi-0 - could you give this another review?
There was a problem hiding this comment.
Looks great, thank you!
Left some small comments inline.
There is one thing which should conclude this PR and that is updating the portion related to the DEEP composition polynomial. I am not sure if this should go into this PR (as this one is already big enough and the remaining changes are quite substantial). I think a good improvement to discuss that will be useful for the remaining changes is to see if we can do away with using the coefficient form of the trace column polynomials. This will have some performance advantages, I believe, as well as make some of the changes required to accommodate the Lagrange kernel easy.
air/src/air/divisor.rs
Outdated
| pub fn from_transition(trace_length: usize, num_exemptions: usize) -> Self { | ||
| assert!( | ||
| num_exemptions > 0, | ||
| "invalid number of transition exemptions: must be greater than zero" | ||
| ); | ||
| let exemptions = (trace_length - num_exemptions..trace_length) | ||
| .map(|step| get_trace_domain_value_at::<B>(trace_length, step)) | ||
| .collect(); |
There was a problem hiding this comment.
I would suggest we rename trace_length and keep one constructor. We could rename it constraint_enforcement_domain_size or something better.
|
|
||
| let frame_length = self.trace_info.length().ilog2() as usize + 1; | ||
| for i in 0..frame_length - 1 { | ||
| let shift = self.blowup() * 2_u32.pow(i as u32) as usize; |
There was a problem hiding this comment.
minor nit: this could probably be optimized but probably not worth the degradation in readability
There was a problem hiding this comment.
Switched to (1 << i) - this is arguably more readable due to the removal of both as _.
The benchmarks indeed didn't improve though
There was a problem hiding this comment.
All looks good! Thank you! As the next steps, we should do the following:
- Make a small refactoring mentioned here and also remove the incomplete machinery related to supporting multiple auxiliary trace segments.
- Complete the ability to run the full GKR protocol after the main trace commitment is computed (discussed in #257. This may require refactoring how our prover/verifier channels work.
- Implement proper handling of DEEP composition for the Lagrange constraints.
Closes: #240
Builds on: #245
Done:
Airwhich uses a Lagrange kernel auxiliary columnLeft to do:
OodFramewith Lagrange kernel "frame"