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(optimism): Add secp256r1 precompile for Fjord #1436

Merged
merged 12 commits into from
May 26, 2024

Conversation

BrianBland
Copy link
Contributor

@BrianBland BrianBland commented May 17, 2024

  • Upstream the RIP-7212: secp256r1::P256VERIFY precompile from the alphanet implementation
  • Add new Optimism Fjord hardfork spec before Prague
  • Add the P256Verify to the chain precompiles post-fjord via the Optimism handle register

Note: Introduces new optional dependency on the p256 crate

Copy link
Member

@rakita rakita left a comment

Choose a reason for hiding this comment

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

Left few comments

crates/precompile/Cargo.toml Outdated Show resolved Hide resolved
crates/precompile/src/lib.rs Outdated Show resolved Hide resolved
crates/precompile/src/lib.rs Outdated Show resolved Hide resolved
crates/precompile/src/secp256r1.rs Outdated Show resolved Hide resolved
crates/precompile/src/secp256r1.rs Outdated Show resolved Hide resolved
crates/precompile/src/secp256r1.rs Outdated Show resolved Hide resolved
@BrianBland BrianBland requested a review from rakita May 24, 2024 18:24
Copy link
Member

@rakita rakita left a comment

Choose a reason for hiding this comment

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

lgtm!

// Can fail only if the input is not exact length.
let signature = Signature::from_slice(sig).unwrap();
// Can fail if the input is not valid, so we have to propagate the error.
let public_key = VerifyingKey::from_sec1_bytes(&uncompressed_pk).ok()?;
Copy link
Member

Choose a reason for hiding this comment

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

Have checked this part of the spec:
Note that many implementations use (0, 0) as the reference point at infinity, which is not on the curve and should therefore be rejected.

I have feed the VerifyingKey::from_sec1_bytes with pk = [0;64] and it fails to get key, this is expected behaviour, just wanting to note it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, added a test case for this

@rakita rakita merged commit 928883c into bluealloy:main May 26, 2024
25 checks passed
This was referenced May 25, 2024
This was referenced Jun 8, 2024
This was referenced Jun 17, 2024
j75689 added a commit to j75689/revm that referenced this pull request Aug 1, 2024
* feat(optimism): Add secp256r1 precompile for Fjord (bluealloy#1436)

* feat(optimism): Add secp256r1 precompile for Fjord

* Fix docs

* Fix nostd build

* Load fjord precompiles via optimism handler register

* Remove outdated fjord() precompile spec constructor

* Document the secp256r1 feature

* Address feedback

* Handle invalid signatures

* Update crates/precompile/src/secp256r1.rs

* Update crates/precompile/src/secp256r1.rs

* Blank return on failed signature verification

* Add test case for invalid (zero) pubkey

---------

Co-authored-by: rakita <rakita@users.noreply.github.com>

* feat: add Haber spec to opBNB

---------

Co-authored-by: Brian Bland <brian.t.bland@gmail.com>
Co-authored-by: rakita <rakita@users.noreply.github.com>
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.

2 participants