-
Notifications
You must be signed in to change notification settings - Fork 420
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
refactor: based on #515 generify groth16 MPC setup for all curves, flatten packages+ refactor #563
Conversation
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.
I think it looks good. Didn't go through the math exactly, hoping that @ThomasPiellard had a look.
Considering that the usage is more complex, I would recommend adding documentation example and a bit of package documentation. If sounds good, then can also do it myself.
Maybe would be good to add type-switched methods into backend/groth16
?
Based on #515 (cherry picked the commits from @HSG88 );
Did mostly stylistic edits; flattened the package structure, avoided couple of allocs, and generify for all curves.
To be merged after #561 ; MPC setup is now
gnark/backend/groth16/bn254/mpcsetup
.A future PR (cc @HSG88 ) would probably be nice to ensure APIs don't panic but return errors + struct and public methods are documented (you can test output with
godoc
) .On the perf note, the individual scalar mul on affine coordinates are a clear bottleneck, maybe we should use jacobian? (+ optimize the memory usage of the
mulGLV
path cc @yelhousni ingnark-crypto
) .