Skip to content
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

perf: bounded scalar multiplication #934

Merged
merged 13 commits into from
Nov 28, 2023
Merged

perf: bounded scalar multiplication #934

merged 13 commits into from
Nov 28, 2023

Conversation

ivokub
Copy link
Collaborator

@ivokub ivokub commented Nov 28, 2023

Description

This PR adds options for algebra methods to allow changing the behaviour of the curve operations. Particular options what I added are:

  • bounded scalar multiplication. Can be used to define the number of non-zero LSB bits of the scalar. This is useful if the scalar comes from a Fiat-Shamir challenge. Allows to reduce the number of iterations in the scalar multiplication.
  • multi scalar multiplication if doing folding. If we have a special case of the scalar multiplication where we do P_0 + r P_1 + r^2 P_2 + ... + r^n P_n then instead computes as P_0 + r(P_1 + r(P_2 + r(...)). This option becomes particularly useful when combined with WithNbDigits as we don't have to compute the powers of r and every scalar multiplication is bounded.

In PLONK verifier circuit for verifying BW6 in BN254 allows to save approximately 19% (12M constraints) as most of the cost is actually in MSMs not in pairing computation.

I implemented bounded scalar multiplication only for emulated scalar multiplication as we use GLV for 2-chains and I'm not sure the bound would help (imo we can only assume the scalars s1, s2 < lambda). Maybe @yelhousni can confirm?

Should be merged after #880.

Type of change

  • New feature (non-breaking change which adds functionality)

How has this been tested?

  • Added TestScalarMulBounded
  • PLONK verifier working

How has this been benchmarked?

Benchmarked PLONK verifier circuit size, going from 78M to 66M (with #749 and #925 included).

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I did not modify files generated from templates
  • golangci-lint does not output errors locally
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@ivokub ivokub added consolidate strengthen an existing feature zk-evm P1: High Issue priority: high labels Nov 28, 2023
@ivokub ivokub self-assigned this Nov 28, 2023
@ivokub
Copy link
Collaborator Author

ivokub commented Nov 28, 2023

I also tried implementing Wide methods in the commit chain, but in the end removed (here), as for the current implementation didn't find a good way to use existing algorithm (when omitting y coordinated computations) and when using simple double-and-add, then the overhead nulled all benefits for instances of width 2 (which is used in PLONK).

Maybe, if there is a way to make current scalar multiplication wide (by sharing some computation), then makes sense to revert the commit.

Copy link
Contributor

@yelhousni yelhousni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! I looked into WideScalarMul for instance size 2 and I didn't neither come with a nice algorithm given the algorithms we use in ScalarMul and ScalarMulBase. For 2-chains GLV, indeed I don't think it would save constraints because the loop size there is bounded by lambda the (fixed) endomorphism eigenvalue size. Maybe for later we can try the approach of using GLV only if the scalar is uniformly random in Fr and using the bounded scalar approach if the scalar are of a particular form e.g. smaller than half lambda. But this not crucial for now and it needs benchmarking to confirm.

@ivokub ivokub merged commit acb6e50 into master Nov 28, 2023
7 checks passed
@ivokub ivokub deleted the perf/multiscalarmul branch November 28, 2023 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consolidate strengthen an existing feature P1: High Issue priority: high zk-evm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants