feat(l1): reintroduce ecrecover implementation using secp256k1 for L1
#4924
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
edg-l
approved these changes
Oct 17, 2025
Lines of code reportTotal lines added: Detailed view |
There was a problem hiding this comment.
Pull Request Overview
Reintroduces the ECRECOVER precompile for L1 using the secp256k1 crate when not building with the sp1 feature, while retaining the k256-based implementation for sp1 builds. It also wires a new sp1 feature through the vm and guest crates and cleans up imports and feature flags accordingly.
- Add a cfg(not(feature = "sp1")) ECRECOVER implementation using secp256k1.
- Keep the k256-based ECRECOVER under cfg(feature = "sp1") and route features through vm and guest.
- Adjust Cargo features and remove the previous secp256k1 patch override.
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/vm/levm/src/precompiles.rs | Adds a new secp256k1-based ecrecover under cfg(not(feature = "sp1")), keeps k256-based path for sp1, and localizes the ECRECOVER_COST import. |
| crates/vm/levm/Cargo.toml | Introduces the sp1 feature and formatting tweaks; keeps secp256k1 and p256 deps. |
| crates/vm/Cargo.toml | Adds the sp1 feature and propagates it to ethrex-levm. |
| crates/l2/prover/src/guest_program/src/sp1/Cargo.toml | Enables the sp1 feature on ethrex-vm for the guest build. |
| Cargo.toml | Removes the secp256k1 crate patch override. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| }; | ||
|
|
||
| // We need to take the 64 bytes from the public key (discarding the first pos of the slice) | ||
| let public_key_hash = Keccak256::digest(&public_key.serialize_uncompressed()[1..]); |
There was a problem hiding this comment.
Keccak256::digest is a trait method from sha3::Digest and requires the Digest trait to be in scope; otherwise this won't compile. Add use sha3::Digest; alongside the Keccak256 import in this cfg block.
Copilot uses AI. Check for mistakes.
Comment on lines
+414
to
+418
| let Ok(recoverable_signature) = | ||
| secp256k1::ecdsa::RecoverableSignature::from_compact(raw_sig, recovery_id) | ||
| else { | ||
| return Ok(Bytes::new()); | ||
| }; |
There was a problem hiding this comment.
The ECRECOVER precompile must enforce EIP-2 (Homestead) to reject high-s signatures; otherwise malleable signatures will be accepted. After constructing the recoverable signature, convert to a standard signature and reject if normalization would change s: let mut standard_sig = recoverable_signature.to_standard(); if standard_sig.normalize_s() { return Ok(Bytes::new()); }.
Copilot uses AI. Check for mistakes.
rodrigo-o
requested changes
Oct 17, 2025
There was a problem hiding this comment.
We saw some issues with ecrecover in another PR:
025-10-17T06:06:12.673560Z INFO ethrex_p2p::sync: Executing 26 blocks for full sync. First block hash: 0xa375e446c5632ace5643b25e3929f074b88de3368dccac3bfcce56a80dd56b2c Last block hash: 0x894deba66c55ac3da824a5eefbb51236791961e0b56457fed672944fd797e986
2025-10-17T06:06:12.725721Z WARN ethrex_p2p::sync: Failed to add block during FullSync err=EVM error: Tried to slice non-existing data block=0xa375…6b2c
2025-10-17T06:06:12.725750Z ERROR ethrex_p2p::sync: Sync cycle failed due to EVM error: Tried to slice non-existing data, time elapsed: 0 secs
2025-10-17T06:06:14.642229Z ERROR ethrex_rpc::eth::gas_tip_estimator: Block body for block number 9428946 is missing but is below the latest known block!
Probably best to test this a lot and even wait for other PRs first
I clarified this internally; this is not the same |
Benchmark Results ComparisonNo significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: FibonacciRecursive
Benchmark Results: ManyHashes
Benchmark Results: MstoreBench
Benchmark Results: Push
Benchmark Results: SstoreBench_no_opt
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Caution
We are currently testing this PR with snapsync, do not merge yet.