Saving scalar multiplications while computing 'UVKZGPCSCommitment' #216
Conversation
storojs72
force-pushed
the
issue-184
branch
2 times, most recently
from
f5b4816 to
f93838f
Compare
|
I think for the context of this PR, Regarding the filtering approach, I think we would need some more experimentation to evaluate the performance. I'm fine with leaving the code in even if it unused for now. |
There was a problem hiding this comment.
The code looks fine on both the commit_offset and commit_filter approaches.
- This is a bit out of scope for this PR, but I think we'll need something like
commit_filter(irrespective of me not liking having to do this) across the code base, modifying thevartime_scalar_mulhere and there, - but before we commit to
commit_filterreplacing the MSM, I would like to see benchmarks (perhaps rebased on Test vectors & benchmark for PCS #222), showing that replacement does not decrease performance (e.g. through memory allocation), - in the meantime the
commit_offsetis certainly a valid improvement, and will remain so, so I'm inclined to approve a change that focuses on that today: it clearly shows our data structure representation for q_hat is too big, and reduces it to size1 << (num_vars - 1)from1 << num_vars🎉 1 - however I am not sure it reaps all the benefits, reading the paper p.38 2:
The prover computes a
$G_1$ Multi-Scalar Multiplication (MSM) of size$2^k$ for each commitment$C_k$ . It also computes an MSM of size$N/2$ to commit to$\hat{q}$ as the latter has at most$N/2 - 1$ non-zero coefficients between degree$2^{n-1} - 1 = 2^n - d_{n-1} - 1$ and degree$2^n - 1$ .
- note: I don't think the zeros within that degree range
$[2^{n-1} - 1, 2^n - 1]$ are expected to be consecutive, if N != 2^n - any cost reduction in committing to q_hat due to sparsity is expected to be yielded by the filtering optimization above whenever it's available anyway.
=> Requesting changes for a PR that focuses on commit_offset (great bug fix, thank you so much @storojs72 !)
Footnotes
src/provider/non_hiding_zeromorph.rs
Outdated
| @@ -378,6 +379,7 @@ fn batched_lifted_degree_quotient<F: PrimeField>( | |||
| }) | |||
| .collect::<Vec<F>>(); | |||
|
|
|||
| let mut offset = 0; | |||
There was a problem hiding this comment.
in this code block:
- we can replace
q_hat.len()by1 << num_vars, this quantity does not change, - we can notice that as we go through the loop, offset moves from
1 << num_varsto1 << num_vars - (1 << (num_vars - 1)) == 1 << (num_vars - 1)and directly output the finaloffsetresult rather than use a mutable variable to represent that
There was a problem hiding this comment.
Nit: the mut offset at the beginning is not very useful, as that usize is stack allocated - we may want to revert to the original, which declared offset as a local variable.
There was a problem hiding this comment.
Yeah, thanks. I've realised it recently (f5f0edd)
|
I will be opening up another branch/PR to further explore how |
storojs72
force-pushed
the
issue-184
branch
2 times, most recently
from
21ab251 to
5cf9186
Compare
src/provider/non_hiding_zeromorph.rs
Outdated
| @@ -378,6 +379,7 @@ fn batched_lifted_degree_quotient<F: PrimeField>( | |||
| }) | |||
| .collect::<Vec<F>>(); | |||
|
|
|||
| let mut offset = 0; | |||
There was a problem hiding this comment.
Nit: the mut offset at the beginning is not very useful, as that usize is stack allocated - we may want to revert to the original, which declared offset as a local variable.
src/provider/non_hiding_kzg.rs
Outdated
| #[allow(dead_code)] | ||
| pub fn commit_filtered( |
There was a problem hiding this comment.
It would be good to move this function to the test module of non_hiding_zeromorph, or at least make it pub (in crate::provider) and #[cfg(test)]
src/provider/non_hiding_zeromorph.rs
Outdated
| @@ -801,4 +811,47 @@ mod test { | |||
| assert_eq!(zeta_x_scalars[k], scalar); | |||
| } | |||
| } | |||
|
|
|||
| fn test_various_uvkzpcs_commit_template<E>() -> Result<(), NovaError> | |||
There was a problem hiding this comment.
This would benefit from a rename to test_ZM_commit_sparsity_template, which makes the goal of the test clear.
src/provider/non_hiding_kzg.rs
Outdated
| &scalars[(offset)..], | ||
| &bases[(offset)..scalars.len()], |
There was a problem hiding this comment.
Nit: I don't think you need the parentheses.
Fixes #184
We exploit the fact that
batched_lifted_degree_quotient(link) computesq_hatwith the first half of the coefficients equal to zero. During subsequent MSM computation inUVKZGPCS::commit(link), zero coefficients ofq_hat, which act as a scalars have no impact on the final value of 'UVKZGPCSCommitment'.Two alternative implementations of
UVKZGPCS::commitare actually proposed:UVKZGPCS::commit_offset- uses additional offset that points to the first non-zero value among scalars (and same offset is used to skip non-relevant bases)UVKZGPCS::commit_filter- uses specific preprocessing that excludes all non-zero values from scalars (and correspondent values from bases). It replaces theUVKZGPCS::commitinZMPCS::openwhileq_hat_commcomputing to save some scalar multiplications.@huitseeker, I remember that you didn't support the filtering approach while our discussion, so I left it unused in actual execution of
ZMPCS::openbut it is still there - since I believe that we in theory can have a situation, when scalars passed to MSM may have a lot of zero values not necessarily in the beginning of the slice (which is the case), but scattered randomly. If it is not a viable assumption, we can remove it.