-
Notifications
You must be signed in to change notification settings - Fork 58
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
Use Bn256/Grumpkin curves as a default setting #1039
Conversation
[Issue#1018] Reproducible example [DO NOT MERGE]
!gpu-benchmark |
Benchmark for 8046da0Click to view benchmark
|
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.
Thanks a lot! I think this is a great improvement, but I would like two issues open to merge this, just to track this milestone relative to future work:
- a list of pending items that still need to be converted from pasta to grumpkin,
- a specific issue to shift us from our
PastaEngine
(which implicitly comes with IPA on each side) toBn256EngineKZG
(which has MLKZG on Bn256, IPA on grumpkin), and bench, specifically, proof compression for that PR (we don't do it in general, just recursion).
Indeed, we've checked that the grumpkin cycle performs strictly better if we also switch the PCS, but that may not hold true if we stay on IPA. Does that sound fair?
@huitseeker , I have created an issue for the first item (#1078), but could you elaborate more on the second one? |
@storojs72 we have an issue ( #1018 ) for switching fields (pasta to grumpkin). I would like us to be conscious about switching PCSes (IPA to MLKZG), which in Nova terms means switching from : and that might mean merging this PR, but tracking the Engine switch in an issue, because I expect that switch to change the following code: |
@huitseeker, I have created issue for point 2: #1079 |
Why do benchmarks show improvement if no sensible part was touched (or was it?)? |
@arthurpaulino all our computations, including benches, depend on the speed of the implementation of the underlying field arithmetic. This is what changes here. |
@huitseeker but benches aren't changed so I believe they are still using Pasta curves |
@arthurpaulino ah, yes, I forgot (#1078 / #1079 should address that)! |
@arthurpaulino the bench comment at 8b4c10e shows no perf improvement and hence confirms the source of the bench result above is Arecibo. |
Partially fixes #1018
I think PR is finally ready for review.
LurkField
is changed to Bn256 in the majority of the unit-tests where it could be potentially useful for solidity-verifier to produce the test-vectors, so the field has not been changed in benches and examples - as they are more client-facing programs that somebody can already rely on, so this left for a future PR(s).Special thanks to @gabriel-barrett !