building blocks for basefold pcs #133
Conversation
| /// Represents a leaf in the Merkle tree, containing 64 bytes of data stored in a BabyBearx16. | ||
| #[derive(Debug, Copy, Clone, PartialEq, Default)] | ||
| pub struct Leaf<F: Field + FieldSerde> { | ||
| pub data: F, | ||
| } | ||
|
|
There was a problem hiding this comment.
I have some thoughts here - as you mentioned here, the leaf contains 64 bytes of data, yet the type of leaf is F: Field + FieldSerde.
The reason why I wrote the octopos tree hash leaf from a chunk of bytes (https://github.com/cysic-labs/jolt-b/blob/master/octopos/src/hash.rs#L107) is that, I wanted to know explicitly how much data (oblivious of what type it is) can be written in the space, and given that we know what type the bytes should be deserialized to, it would be just fine to deserialize from the leaf.
Yet in this situation, though the field is generic, we somehow assume that the field serialize to 64 bytes, that has to be either Babybear_x16 or M31_x16.
What if we want to hash Orion codeword that works over GF(2) that is binary, then we probably need to construct some 512-length module of GF(2) to fit into the leaf, which seems a bit inconvenient here.
| /// Builds a tree with the given leaves. | ||
| #[inline] | ||
| pub fn new_with_leaves(leaves: Vec<Leaf<F>>) -> Self { | ||
| let tree_height = log2(leaves.len() + 1); |
There was a problem hiding this comment.
i felt like this can be wrong - consider 7 leaves, then you need 8 leaves capacity, while the height from 7 leaves is different from the one from 8 leaves
There was a problem hiding this comment.
maybe they are assuming that the leaves are always being po2, s.t., the height is correct in this case
|
|
||
| #[cfg(test)] | ||
| mod test {} |
| // append the prover's message to the transcript | ||
| // round_uni_poly. | ||
| // transcript.append_field_element(f); | ||
| println!("round_uni_poly: {:?}", round_uni_poly.coefficients.len()); |
There was a problem hiding this comment.
I guess you want to say something related to the degree of the univariate poly in the round of response in sumcheck?
| if poly.degree() != degree_bound { | ||
| panic!("poly degree incorrect") | ||
| } |
There was a problem hiding this comment.
can just assert_eq! without using panic?
| /// Returns (SumcheckInstanceProof, r_eval_point, final_evals) | ||
| /// - `r_eval_point`: Final random point of evaluation | ||
| /// - `final_evals`: Each of the polys evaluated at `r_eval_point` | ||
| pub fn prove_arbitrary<Func, T>( |
There was a problem hiding this comment.
a minor question regarding the naming - if I am guessing correctly, the naming suggests that it applies to arbitrary gate, i.e., the comb_func here. In a sense, this also suggests specialized sumchecks will be (or have been) implemented for certain cases, say some low degree common cases. Do these guesses sound reasonable to you?
| // eval 0: bound_func is A(low) | ||
| let params_zero: Vec<F> = | ||
| polys.iter().map(|poly| poly.coeffs[poly_term_i]).collect(); | ||
| accum[0] += comb_func(¶ms_zero); | ||
|
|
||
| // TODO(#28): Can be computed from prev_round_claim - eval_point_0 | ||
| let params_one: Vec<F> = polys | ||
| .iter() | ||
| .map(|poly| poly.coeffs[mle_half + poly_term_i]) | ||
| .collect(); | ||
| accum[1] += comb_func(¶ms_one); | ||
|
|
There was a problem hiding this comment.
it seems, the sumcheck summing sequence is from right to left, i.e., sumcheck reduces variables from the rightmost one to the leftmost one.
| // ... | ||
| let mut existing_term = params_one; | ||
| for eval_i in accum.iter_mut().take(combined_degree + 1).skip(2) { | ||
| let mut poly_evals = vec![F::zero(); polys.len()]; |
There was a problem hiding this comment.
move the declaration of poly_evals buffer outside the loop, eventually it gets overwritten each iteration from evaluation point being 2 to combine_degree + 1.
There was a problem hiding this comment.
actually, why not just directly existing_term[poly_i] += (poly.coeffs[mle_half + poly_term_i] - poly.coeffs[poly_term_i]?
also, you can take advantage of the prior stage_zero and stage_one, so you don't have to search into poly's coeffs for the prior searched coefficients, which may help perf a bit...
|
|
||
| let accum: Vec<Vec<F>> = (0..mle_half) | ||
| .map(|poly_term_i| { | ||
| let mut accum = vec![F::zero(); combined_degree + 1]; |
There was a problem hiding this comment.
hmmm, please consider another name, not conflicting with outside accum - I guess this variable is keeping track of the comb_func evaluations over current point, while the summing variable need to be from 0 to combine-degree, s.t., this allows verifier to sample a random challenge and reduce the claimed summation for next round of sumcheck.
back to what I have proposed, please do consider another naming for this variable.
| - poly.coeffs[poly_term_i]; | ||
| } | ||
|
|
||
| *eval_i += comb_func(&poly_evals); |
There was a problem hiding this comment.
please consider for syntax simplicity, that just use assign here? this is the same case with prior accum[0] and accum[1] that only visited once and assigned once, so please just assign rather than add to the value...
bi-kzg/src/lib.rs
Outdated
There was a problem hiding this comment.
a second thought after finishing code reading - can we take out the pcs trait to somewhere, s.t., pcs implementations afterwards can comply with the trait? #97 has 2 pcs traits in bi-kzg and basefold, should be just one pcs trait I guess?
|
|
||
| println!("creat proof"); | ||
| // Create the proof | ||
| let combined_degree = 2; // Sum of two linear polynomials has degree 2 |
| if (i >> j) & 1 == 1 { | ||
| Fr::one() | ||
| } else { | ||
| Fr::zero() | ||
| } |
There was a problem hiding this comment.
maybe just Fr::from((i >> j) & 1) as u64)?
this PR adds required building blocks for basefold. include the following: