-
Notifications
You must be signed in to change notification settings - Fork 129
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
Implement Plonky3 frontend adaptor #306
Conversation
- Bring back Rc instead of Box in SymbolicExpression so that expressions built with folding avoid many clones - Rewrite Expression doulbe as `e * 2` instead of `e + e` to avoid exponential cloning in expressions built with folding - When a constraint doesn't use a location, change it to use a selector for usable columns to avoid failing in poisoned rows
I just made this PR ready for review to get some feedback. |
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.
Except for the TODOs you left, everything else LGTM.
Really cool to see the modularity of the front-back split in action :)
&|v| match v { | ||
VarMid::Query(q) => { | ||
let offset = offset as i32 + q.rotation.0; | ||
// TODO: Try to do mod n with a rust function |
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'm not sure this is the expected behavior.
I'd say its probably simpler and safer to disallow querying next
in the last row. I may be mistaken though and would be very interested in seeing a circuit that makes use of the "cycle" in the rows.
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 think I haven't seen a circuit that does this:
- querying next on the last row
- querying prev in the first row
But this is technically valid in Plonkish. If you have blinding rows, probably this doesn't make sense (but we support unblinded columns). This is a function that checks if the witness passes the constraints and my goal was to make this function accept all witnesses that would pass the constraints. So if the witness can be used to calculate a valid proof, I'd like this function to accept such a witness. It's like a simplified mock prover.
Also very important: this function receives CompiledCircuitV2
so it's not AIR specific!
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #306 +/- ##
==========================================
+ Coverage 80.86% 81.84% +0.97%
==========================================
Files 80 82 +2
Lines 16567 17000 +433
==========================================
+ Hits 13397 13913 +516
+ Misses 3170 3087 -83 ☔ View full report in Codecov by Sentry. |
I've added unit tests and increased the test coverage to ~90% |
let mut trace = | ||
RowMajorMatrix::new(vec![F::zero(); n * NUM_FIBONACCI_COLS], NUM_FIBONACCI_COLS); | ||
|
||
let (prefix, rows, suffix) = unsafe { trace.values.align_to_mut::<FibonacciRow<F>>() }; |
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.
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, it's nice to have a new frontend! :)
Plonky3 frontend implementation. Allows using a circuit defined with the Air trait from plonky3 to be proved with a halo2 backend.
Bump rust version from 1.73.0 to 1.75.0 because plonky3 requires it.