-
Notifications
You must be signed in to change notification settings - Fork 428
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
feat: add Pectra BLS12-381 elliptic curve precompiles #1447
base: master
Are you sure you want to change the base?
Conversation
…pectra/precompiles
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.
In principle looks good. But had a few comments about optimizing some computations and refactoring a bit.
Map to G1 is still ongoing. I'll rewrite a bit to have less duplication and better interfaces. Also need to add tests in the evmprecompiles packages.
return left, right | ||
} | ||
|
||
func (g1 *G1) AssertIsOnCurve(P *G1Affine) { |
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.
We have now two duplicate methods imo for AssertIsOnCurve
depending how we initialize BLS12 curve - if through the generic factory in std/algebra
, then we get the implementation of sw_emulated
and if we initialize G1
here then we get the current implementation. Imo it is not perfect and may have confusion and mismatch.
One approach we could take is to have AssertIsOnCurve
and AssertIsOnG1
in the std/algebra.Curve
interface, but this would require implementing it also for 2-chain curves. Maybe we can right now mark them TODO by panicking and then implementing later? Then in the precompile packages can use sw_emulated
for Curve ops instead of G1 here.
g1.curveF.AssertIsEqual(&p.Y, &q.Y) | ||
} | ||
|
||
func (g1 *G1) IsEqual(p, q *G1Affine) frontend.Variable { |
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.
Same for AssertIsOnCurve
and AssertIsOnG1
. It could be part of Curve
interface and we can implement in sw_emulated
instead
I added Consensys/gnark-crypto#674 - I'll go over G2 map to curve to see that it is also useful for #1040. Otherwise the map to curve also looks good. @yelhousni - please re-request review when you have addressed all the comments and I'll re-review. I had to merge master because there were some changes due to gnark-crypto API changes. |
Addressed all the points except the |
Yup - I think we could try to keep it simple for now. The only thing I'm worried about when having duplicate implementations (in sw_emulated and sw_bls12381) is that it may cause confusion when auditing or debugging bugs. I'll have another look, maybe I can come up with some idea while @ThomasPiellard is addressing the missing test comment. |
…pectra/precompiles
Description
This PR adds gnark circuits corresponding to precompiles for BLS12-381 curve operations https://eips.ethereum.org/EIPS/eip-2537.
TODO:
ScalarMul()
with GLVType of change
How has this been tested?
Tests are implemented in
std/evmprecompiles/bls_test.go
.How has this been benchmarked?
PLONK (on BLS12-377) number of constraints (scs):
Scalar Multiplication
Scalar Multiplication
Checklist:
golangci-lint
does not output errors locally