-
Notifications
You must be signed in to change notification settings - Fork 28
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: bls12-381 g1_add precompile #17
Conversation
f9369a8
to
34d1b60
Compare
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.
mostly have nitpicks, this is a great start! some of the logic that uses subtle types was initially confusing but it makes sense given the bls library. The tests are great
cool thanks! ok, all the comments are addressed PTAL. wrt to the submodule doc, do you think we could include |
Co-authored-by: Dan Cline <6798349+Rjected@users.noreply.github.com>
ab53503
to
21e3d06
Compare
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.
smol suggestion re blst precompile setup as a a function
crates/node/src/evm.rs
Outdated
precompiles.inner.insert(BLS12_G1ADD.0, BLS12_G1ADD.1); | ||
precompiles.inner.insert(BLS12_G1MUL.0, BLS12_G1MUL.1); | ||
precompiles.inner.insert(BLS12_G1MULTIEXP.0, BLS12_G1MULTIEXP.1); | ||
precompiles.inner.insert(BLS12_G2ADD.0, BLS12_G2ADD.1); | ||
precompiles.inner.insert(BLS12_G2MUL.0, BLS12_G2MUL.1); | ||
precompiles.inner.insert(BLS12_G2MULTIEXP.0, BLS12_G2MULTIEXP.1); | ||
precompiles.inner.insert(BLS12_PAIRING.0, BLS12_PAIRING.1); | ||
precompiles.inner.insert(BLS12_MAP_FP_TO_G1.0, BLS12_MAP_FP_TO_G1.1); | ||
precompiles.inner.insert(BLS12_MAP_FP2_TO_G2.0, BLS12_MAP_FP2_TO_G2.1); |
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.
should we extract this to a helper function, so this setup can be reused?
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.
good point, done here e008ab3 PTAL
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've taken a similar approach for instructions here https://github.com/paradigmxyz/alphanet/pull/17/files/e008ab32fe797b04cd4025307747840938fef295..0835e75b413d11182d6d5ab2cdc199af845d2164
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.
Just one typo comment but this looks good to me now!
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.
lgtm
Towards #11
Implements the
BLS12_G1ADD
precompile using thebls12_381
crate. This crate has been selected overblst
because it exposes a much more ergonomic interface to be used from Rust code.This implementation follows the spec at https://eips.ethereum.org/EIPS/eip-2537. Specifically, the logic takes into account the padding to specific lengths and the special encoding for the point of infinity as described here https://eips.ethereum.org/EIPS/eip-2537#point-of-infinity-encoding
It includes tests based on the test vectors defined for geth here https://github.com/ethereum/go-ethereum/blob/ab49f228ad6f37ba78be66b34aa5fee740245f57/core/vm/testdata/precompiles/blsG1Add.json for the happy path and here for the unhappy https://github.com/ethereum/go-ethereum/blob/ab49f228ad6f37ba78be66b34aa5fee740245f57/core/vm/testdata/precompiles/fail-blsG1Add.json (geth is one of the reference implementations cited in the spec https://eips.ethereum.org/EIPS/eip-2537#reference-implementation)
The PR also adds precompile functions for the rest of
BLS12-381
precompiles still unimplemented, and includes all of them in the node.