Add Multilinear Polynomial KZG Commitments #6
Conversation
|
FWIW we have an implementation of the multilinear polynomial commitment also in poly-commit: https://github.com/arkworks-rs/poly-commit/tree/master/src/multilinear_pc |
I think this Implementation is for Lagrange basis multilinear polynomial commitment, while ours is for Coefficient basis. |
|
Ah I see, that's cool! If it makes sense for y'all, once your impl is done we can move it poly-commit also! |
src/misc.rs
Outdated
| { | ||
| let mut result = F::one(); | ||
| for (i, x_i) in x.iter().enumerate() { | ||
| if (idx >> i) & 1 == 1 { |
There was a problem hiding this comment.
we think it is correct but not necessarily efficient
src/misc.rs
Outdated
| } | ||
|
|
||
| /// Generate a random vector of field elements such that they're all different | ||
| pub fn random_unique_vector<F>(size: usize, rng: &mut impl RngCore) -> Vec<F> |
There was a problem hiding this comment.
I don't like this function to be pub because we end up using it only for testing, ditto for random_vector. Can you check #[cfg(test)] and add it above?
There was a problem hiding this comment.
It's also used to generate
| let dim = 12; | ||
| let rng = &mut ark_std::test_rng(); | ||
| let polynomial: Vec<Fr> = random_vector(1 << dim, rng); | ||
| let eval_point: Vec<Fr> = random_vector(dim, rng); |
There was a problem hiding this comment.
The variable dim sometimes refers the length of the vector, sometimes to the number of variables. this inconsistency is confusing
There was a problem hiding this comment.
I think we discussed this with Yuncong and decided that dim makes the most sense, since it is the number of variables. Whereas deg implies variables raised to powers greater than 1. Does that sound right?
| /// XXX TODO: This function is pub(crate) as in a previous version of this library, | ||
| /// Iterable: Copy and hence couldn't store vectors itself. | ||
| /// This is not anymore the case thus it can be moved inside init. | ||
| pub fn expand_tensor<F: Field>(elements: &[F]) -> PartialTensor<F> { |
There was a problem hiding this comment.
this is my code, why are you adding it here now? also what about XXX TODO? can you fix them?
There was a problem hiding this comment.
We talked about this on Wednesday - it appears as an addition due to moving around multikzg additions to misc.rs but should be unchanged from when you wrote it. It's used by psnark and snark and we are not sure if we should touch/edit this code to fix the TODO. What do you think?
multikzgmodule which implements polynomial commitments with multilinear polynomialskzgdocumentation