-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feat/Switch Spartan Backend to Use Goldilocks Field #17
Conversation
Just delete unused code. We can get it back from the git history. At most you want to leave a comment that tells people what used to be there, and to check in the git history for details. |
We are cleaning up a lot in #17 Here we extract some of that cleanup, to make that other PR easier to review.
Currently, there are some compilation failures in |
* Delete all the things We are cleaning up a lot in #17 Here we extract some of that cleanup, to make that other PR easier to review. * Delete more
/// Inner Goldilocks extension field | ||
type InnerType: ExtensionField + Field; |
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.
It's pretty cumbersome to access its base field via <S::InnerType as ExtensionField>::BaseField
, right?
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.
We need to use BaseField in a lot of places for performance. For example, the type of each cell's value in the R1CS matrices must belong to the base field.
row: usize, | ||
col: usize, | ||
val: Scalar, | ||
val: S, |
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.
If S
itself extends the ExtensionField
trait, then val's type should be S::BaseField
. Otherwise, we need to use this cumbersome expression <S::InnerType as ExtensionField>::BaseField
.
let gens = DotProductProofGens::new(right.pow2(), label); | ||
PolyCommitmentGens { gens } | ||
} | ||
Z: Vec<S>, // evaluations of the polynomial in all the 2^num_vars Boolean inputs |
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.
Need be more flexible to allow DensePolynomial
to store either base elements or extension elements like https://github.com/scroll-tech/ceno/blob/master/multilinear_extensions/src/mle.rs#L163 did in ceno repo.
ts * r_hash_sqr + val * r_hash + addr | ||
}; | ||
let r_hash_sqr = *r_hash * *r_hash; | ||
let hash_func = |addr: &S, val: &S, ts: &S| -> S { *ts * r_hash_sqr + *val * *r_hash + *addr }; |
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.
This closure can be boosted by by changing the type of addr, ts
from S
to S::BaseField
as extension field element multiply by base field element e*b
(https://github.com/scroll-tech/ceno-Goldilocks/blob/master/src/extfield.rs#L24) is much faster than e * e
The features not included in this pr are tracked below |
88029e3
to
5f86ada
Compare
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.
Just looking through this briefly and got some questions. Not a full review. (And you can merge without addressing my questions / comments. I'm just curious.)
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 re-implement the field arithmetic here? What's the reason we can't re-use ceno-goldilocks or Plonky2 or so?
.chain(col.read_ts.iter()) | ||
.chain(val_vec.iter()), | ||
.into_iter() | ||
.chain(row.read_ts.into_iter()) |
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.
Itertool's chain
macro is a bit easier to read, IHMO. But that doesn't have to be in this PR.
@@ -436,25 +369,21 @@ impl SparseMatPolynomial { | |||
let row = self.M[i].row; | |||
let col = self.M[i].col; | |||
let val = &self.M[i].val; | |||
eval_table_rx[row] * eval_table_ry[col] * val | |||
eval_table_rx[row] * eval_table_ry[col] * *val |
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.
If we are implementing our own field element arithmetic anyway, we can add instances for arithmetic with references, too. See how we do that in Ceno.
(We don't need to do that in this PR.)
.fold(vec![Scalar::zero(); num_rows], |mut Mz, (r, v)| { | ||
Mz[r] += v; | ||
.fold(vec![S::field_zero(); num_rows], |mut Mz, (r, v)| { | ||
Mz[r] = Mz[r] + v; |
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.
We can implement AddAssign?
|
||
for i in 0..self.M.len() { | ||
let entry = &self.M[i]; | ||
M_evals[entry.col] += rx[entry.row] * entry.val; | ||
M_evals[entry.col] = M_evals[entry.col] + rx[entry.row] * entry.val; |
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 don't we implement AddAssign?
let derefs = { | ||
// combine all polynomials into a single polynomial (used below to produce a single commitment) | ||
let comb = DensePolynomial::merge(row_ops_val.iter().chain(col_ops_val.iter())); | ||
let comb = DensePolynomial::merge(row_ops_val.into_iter().chain(col_ops_val.into_iter())); |
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.
You could just clone here, then you don't need to change all the other lines. Makes the diff easier to review.
Completed in this PR:
ristretto255
scalar field definition. Use newgoldilocks
Scalar
impl.ceno-goldilocks
repo.Scalar
field struct to a generic.Scalar
operations and checks will be preserved.Scalar Checks Debugging progress:
[Done]
ProductCircuitEvalProofBatched
->fn verify
: 1 check[Done]
HashLayerProof
->fn verify_helper
: 4 checks[Done]
HashLayerProof
->fn verify
: 1 check