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

feat: add EVM precompiles #488

Merged
merged 8 commits into from Apr 6, 2023
Merged

feat: add EVM precompiles #488

merged 8 commits into from Apr 6, 2023

Conversation

ivokub
Copy link
Collaborator

@ivokub ivokub commented Feb 23, 2023

Closes #466.

Even if we have many primitives, then this PR is introducing tightly coupled circuits for using with EVM precompiles. The goal is that we can take verbatim precompile call arguments and return whatever implementations would return.

Still WIP, but starting a draft PR for discussions on interfaces etc. Need to fix #484 as with test engine right now get errors where shouldn't.

More specifically:

  • ECRecover - will have several variations with and without Keccak hashing the public key. Without hashing will rely on the prover to perform the hashing. But for consistency with the package promise will add variant with Keccak hashing. Mostly done, need to add a few in-circuit checks.
  • sha256 - lower priority right now.
  • identity - pointless
  • expmod - definitely needs rangechecking. I assume will be most complicated. Plan is to use Montgomery multiplication and textbook square-and-mul algorithm.
  • BN254 add - need to implement compatible interface.
  • BN254 scalar mul - need to implement compatible interface.
  • BN254 pairing - need to implement compatible interface.
  • Blake2

Tasks:

  • right now for testing ECRecover I have a local version of gnark-crypto which outputs compatible signatures (with v). Need to polish the implementation and merge in gnark-crypto for better maintainability.

@ivokub ivokub added this to the v0.9.0 milestone Feb 23, 2023
@ivokub ivokub self-assigned this Feb 23, 2023
@ivokub ivokub marked this pull request as ready for review March 17, 2023 14:44
@ivokub ivokub requested a review from yelhousni March 17, 2023 14:44
@ivokub
Copy link
Collaborator Author

ivokub commented Mar 17, 2023

Precompiles done. Will update the stats in the corresponding issues.

Will do rest of the precompiles later (modexp, SHA256, Blake, RipeMD)

@ivokub ivokub mentioned this pull request Mar 31, 2023
@ivokub
Copy link
Collaborator Author

ivokub commented Apr 5, 2023

@yelhousni, ping

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.

LGTM! apart from the 08-bnpairing.go where I think the precompile does not return the pairing value but just checks it is equal to 1 (there is no support of Fp12 arithmetic in EVM).

std/evmprecompiles/08-bnpairing.go Outdated Show resolved Hide resolved
std/evmprecompiles/01-ecrecover.go Outdated Show resolved Hide resolved
std/evmprecompiles/01-ecrecover.go Outdated Show resolved Hide resolved
std/evmprecompiles/01-ecrecover.go Show resolved Hide resolved
@ivokub
Copy link
Collaborator Author

ivokub commented Apr 5, 2023

Resolved the comments. Right now didn't refactor ECRecover into std/signature/ecdsa as it would have made the implementation a lot more messy.

If re-checking code, then I would particularly point to ECPair test. I tried to define a succeeding test.

@ivokub ivokub requested a review from yelhousni April 6, 2023 08:10
@yelhousni
Copy link
Contributor

yelhousni commented Apr 6, 2023

If re-checking code, then I would particularly point to ECPair test. I tried to define a succeeding test.

Suggested edit for ECPair test to use random points each time:

diff --git a/std/evmprecompiles/bn_test.go b/std/evmprecompiles/bn_test.go
index ed1cfb24..74815562 100644
--- a/std/evmprecompiles/bn_test.go
+++ b/std/evmprecompiles/bn_test.go
@@ -133,9 +133,8 @@ func TestECMulCircuitFull(t *testing.T) {
 }
 
 type ecpairCircuit struct {
-	P        [2]sw_bn254.G1Affine
-	Q        [2]sw_bn254.G2Affine
-	Expected sw_bn254.GTEl
+	P [2]sw_bn254.G1Affine
+	Q [2]sw_bn254.G2Affine
 }
 
 func (c *ecpairCircuit) Define(api frontend.API) error {
@@ -148,10 +147,19 @@ func (c *ecpairCircuit) Define(api frontend.API) error {
 func TestECPairCircuitShort(t *testing.T) {
 	assert := test.NewAssert(t)
 	_, _, p1, q1 := bn254.Generators()
+
+	var u, v fr.Element
+	u.SetRandom()
+	v.SetRandom()
+
+	p1.ScalarMultiplication(&p1, u.BigInt(new(big.Int)))
+	q1.ScalarMultiplication(&q1, v.BigInt(new(big.Int)))
+
 	var p2 bn254.G1Affine
 	var q2 bn254.G2Affine
 	p2.Neg(&p1)
 	q2.Set(&q1)
+
 	err := test.IsSolved(&ecpairCircuit{}, &ecpairCircuit{
 		P: [2]sw_bn254.G1Affine{sw_bn254.NewG1Affine(p1), sw_bn254.NewG1Affine(p2)},
 		Q: [2]sw_bn254.G2Affine{sw_bn254.NewG2Affine(q1), sw_bn254.NewG2Affine(q2)},

@yelhousni
Copy link
Contributor

@ivokub I applied the edit in the last commit + removed Expected sw_bn254.GTEl which is not used anymore after PairingCheck() use.

@yelhousni
Copy link
Contributor

Should we merge this PR or wait for the remaining precompiles?

@ivokub
Copy link
Collaborator Author

ivokub commented Apr 6, 2023

Should we merge this PR or wait for the remaining precompiles?

Looks good, I'll merge. Rest of the precompiles takes a bit time and is less important, so can do in another PR.

@ivokub ivokub merged commit 5c6f12d into develop Apr 6, 2023
4 checks passed
@ivokub ivokub deleted the feat/evm-precompiles branch April 6, 2023 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants