Combine coefficient polynomials #168
Conversation
There was a problem hiding this comment.
Nice! So we decrease key/index size (and I guess verifier runtime), but increase proof size (of ~1KB) IIUC.
I think this could probably reduced to making it so we only need to evaluate one additional polynomial with some more work if we want.
What do you have in mind?
| @@ -9,9 +9,11 @@ use crate::wires::{GateWires, COLUMNS, GENERICS}; | |||
| use ark_ff::FftField; | |||
| use array_init::array_init; | |||
|
|
|||
| pub const MUL_COEFF: usize = GENERICS; | |||
| pub const CONSTANT_COEFF: usize = GENERICS + 1; | |||
There was a problem hiding this comment.
this is great. Also "index" more than "coeff" no?
| @@ -28,7 +30,7 @@ impl<F: FftField> CircuitGate<F> { | |||
| pub fn create_generic_add(row: usize, wires: GateWires) -> Self { | |||
| let on = F::one(); | |||
| let off = F::zero(); | |||
| let qw: [F; COLUMNS] = array_init(|col| { | |||
| let qw: [F; GENERICS] = array_init(|col| { | |||
| if col < 2 { | |||
| on | |||
There was a problem hiding this comment.
btw this function is missing the fact that you can scale the operand freely, which I didn't think about back when I wrote that.
| impl<F: FftField> CircuitGate<F> { | ||
| // TODO(mimoo): why qw is length 15 if the polynomial side only uses 3? | ||
| pub fn create_generic(row: usize, wires: GateWires, qw: [F; COLUMNS], qm: F, qc: F) -> Self { | ||
| pub fn create_generic(row: usize, wires: GateWires, qw: [F; GENERICS], qm: F, qc: F) -> Self { |
There was a problem hiding this comment.
I had meant to look into why it was set to the full columns, thanks for fixing that!
|
|
||
| // return in lagrange and monomial form for optimization purpose | ||
| let eval_part = &multiplication + &wires; | ||
| let poly_part = &self.qc + public; | ||
| let poly_part = public.clone(); | ||
| (eval_part, poly_part) |
There was a problem hiding this comment.
if the poly part is now just the public polynomial, then let's not pass the public argument and return the poly_part in this function :o wdyt?
| } else { | ||
| F::zero() | ||
| } | ||
| }) |
There was a problem hiding this comment.
one-liner:
gate.c.get(i).unwrap_or(F::zero())There was a problem hiding this comment.
interesting, maybe it'd be good to document somewhere that if you need static coefficients for your gate, you should use the coefficient polynomials/commitments
There was a problem hiding this comment.
Actually the one liner doesn't work because you get a borrow from get. I guess you could make it work by mapping but seemed kind more awkward than this to me
There was a problem hiding this comment.
gate.c.get(i).copied().unwrap_or(F::zero()) ?
| /// coefficient commitment array | ||
| pub coefficients_comm: [PolyComm<G>; COLUMNS], | ||
| /// coefficient commitment array | ||
| pub generic_comm: PolyComm<G>, |
There was a problem hiding this comment.
this looks good, once this is in we need to update:
- mina stubs in [crypto][kimchi] stubs + backend for kimchi MinaProtocol/mina#9547 (specifically https://github.com/MinaProtocol/mina/blob/mimoo/backend/src/lib/crypto/kimchi_bindings/stubs/src/plonk_verifier_index.rs)
- verifierindex serialization in [ocaml] implementing missing code for kimchi stubs to work #170 (specifically https://github.com/o1-labs/proof-systems/blob/mimoo/serialization4/dlog/plonk-15-wires/src/index.rs#L65)
Codecov Report
@@ Coverage Diff @@
## master #168 +/- ##
==========================================
- Coverage 86.86% 86.84% -0.03%
==========================================
Files 74 74
Lines 15669 15639 -30
==========================================
- Hits 13611 13581 -30
Misses 2058 2058
Continue to review full report at Codecov.
|
Right now, for poseidon and generic gates, we have in the index
This PR changes this set-up to have
So overall, this eliminates 4 polynomials, and correspondingly removes 4 commitments from verification keys. On the other hand, the change necessitates the addition of evaluations for the poseidon and generic selector polynomials, adding (1 + 1)*(2 evaluation points) = 4 evaluations to the proofs. I think this could probably reduced to making it so we only need to evaluate one additional polynomial with some more work if we want.