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

Fjord: secp256r1 curve support (RIP-7212) #168

Merged
merged 21 commits into from
Apr 23, 2024

Conversation

mdehoog
Copy link
Contributor

@mdehoog mdehoog commented Oct 25, 2023

Description

Same as ethereum/go-ethereum#27540, for early rollout for op-stack chains. Currently targeted to be enabled in the delta fjord hardfork.

Tests

Brought tests over from the go-ethereum PR.

Additional context

See https://github.com/ethereum/RIPs/blob/master/RIPS/rip-7212.md (and https://eips.ethereum.org/EIPS/eip-7212).

@tynes
Copy link
Contributor

tynes commented Oct 26, 2023

Currently targeted to be enabled in the delta hardfork.

Where is the discussion for the delta hardfork happening?

@mdehoog
Copy link
Contributor Author

mdehoog commented Oct 26, 2023

@tynes I'm using "delta" as a reference to the next hardfork post-canyon, but I don't believe discussions have started about it yet. I opened this PR to begin to build alignment to get this out in the next hardfork.

@tynes
Copy link
Contributor

tynes commented Oct 26, 2023

@tynes I'm using "delta" as a reference to the next hardfork post-canyon, but I don't believe discussions have started about it yet. I opened this PR to begin to build alignment to get this out in the next hardfork.

Got it, thanks! Just want to make sure twitter doesn't think this has already been scheduled for inclusion. I want to get this in but I also want to make sure that we do not act in a way that breaks assumptions about the EVM. We should discuss this at RollCall and get something into the RIPs so that we can get consensus between the L2s on what address we should include the predeploy at

@ulerdogan
Copy link
Contributor

Hello! I am happy to share that the specification of the proposal has been finalized by moving into the RIPs repository and the last changes will be applied with this PR.

@mdehoog mdehoog force-pushed the ulerdogan-secp256r1 branch 2 times, most recently from 76a04e9 to 9ff367a Compare January 3, 2024 21:08
@mdehoog
Copy link
Contributor Author

mdehoog commented Jan 3, 2024

Thank you for the ping @ulerdogan, I've pulled in your implementation address modifications from ethereum/go-ethereum@master...ulerdogan:go-ethereum:ulerdogan-secp256r1 and targeted the fjord hardfork (one after the 4844 ecotone hardfork)

@mdehoog mdehoog force-pushed the ulerdogan-secp256r1 branch 2 times, most recently from ea0e12d to b743dc1 Compare January 3, 2024 21:33
@ulerdogan
Copy link
Contributor

Thanks for the update @mdehoog! Also, I recommend to include more tests and review for my implementation. I would love to collaborate on this.

@mdehoog mdehoog changed the title [EIP-7212] secp256r1 curve support [Fjord] [EIP-7212] secp256r1 curve support Jan 17, 2024
@xiaoxiaff
Copy link

Hey @ulerdogan! Nice to e-meet you! I am a software engineer from Coinbase and I would love to work with you on more test cases to push this forward.

For the test cases, do you have anything top of your mind? I am thinking to bring some secp256r1 test vectors but not seeing that mentioned in the EIP7212. Let me know if you have any ideas or you are fine with some secp256r1 test vectors.

cc @mdehoog @angel-ding-cb

@ulerdogan
Copy link
Contributor

Hi @xiaoxiaff, nice to meet you too! I agree with you that it'd be good to apply the secp256r1 test vectors as the proposal specs are fully compatible with the original standard. How should we collaborate? Also, this repo by @daimo-eth team is a great example for the test vectors, applying for the Solidity verifier.

@xiaoxiaff
Copy link

@ulerdogan let me prepare some tests for this PR and feel free to left comments. If it looks good we can add those to your original code as well.

Double check does the EIP-7212 finalized? It looks like RIP-7212 is already finalized and if we need to keep it consistent with EIP then we probably won't be able to add the test vector to the RIP for reviews? 👀

@ulerdogan
Copy link
Contributor

Cool, thanks!

EIP-7212 will be removed from the EIPs repo and the precompile proposals will remain as only the RIP-7212. So, we don't need to consider the EIP or any other change.

Additionally, today in the RollCall Breakout #3, standardized tests for the RIPs, like Ethereum test repos, have been discussed to make sure that every rollup implementation behaves the same. Looks like this topic will be consulted with the L1 team and there will be another testing repo.

@xiaoxiaff
Copy link

@ulerdogan I have the PR ready which includes the test vectors from Google's wycheproof. Let me know if that looks good to you!

cc @mdehoog @angel-ding-cb

@ulerdogan
Copy link
Contributor

Thanks @xiaoxiaff, also commited to my branch.

@mdehoog mdehoog changed the title [Fjord] [EIP-7212] secp256r1 curve support [Fjord] [RIP-7212] secp256r1 curve support Feb 8, 2024
@mdehoog mdehoog marked this pull request as ready for review February 8, 2024 20:25
@sebastianst sebastianst changed the title [Fjord] [RIP-7212] secp256r1 curve support Fjord: secp256r1 curve support (RIP-7212) Apr 4, 2024
Copy link
Member

@sebastianst sebastianst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to go! Only a few open questions for my understanding.

Would be ideal if we could add the Fjord override and superchain config change from #249 to this PR, as we merge it first.

In the future, we should try to have a small separate PR just for adding the new fork fields, overrides etc...

We should also add an e2e test to the monorepo to confirm this is working as expected after Fjord activation.

crypto/secp256r1/verifier.go Show resolved Hide resolved
core/vm/contracts.go Show resolved Hide resolved
crypto/secp256r1/verifier.go Outdated Show resolved Hide resolved
core/vm/contracts.go Show resolved Hide resolved
core/vm/contracts.go Outdated Show resolved Hide resolved
@mdehoog mdehoog requested a review from a team as a code owner April 22, 2024 23:39
@mdehoog mdehoog requested review from tynes and removed request for a team April 22, 2024 23:39
Copy link
Member

@sebastianst sebastianst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thanks @danyalprout @mdehoog @ulerdogan @yukaitu-cb!

core/vm/contracts.go Show resolved Hide resolved
crypto/secp256r1/publickey.go Outdated Show resolved Hide resolved
crypto/secp256r1/verifier.go Show resolved Hide resolved
@sebastianst sebastianst merged commit 84b52df into ethereum-optimism:optimism Apr 23, 2024
4 of 5 checks passed
@danyalprout danyalprout deleted the ulerdogan-secp256r1 branch April 23, 2024 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants