EIP-7951 (RIP-7212-compatible): secp256r1 P256VERIFY precompile#1708
EIP-7951 (RIP-7212-compatible): secp256r1 P256VERIFY precompile#1708snissn wants to merge 12 commits intofilecoin-project:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements the RIP-7212 secp256r1 (P-256) signature verification precompile at address 0x0100. Note: The PR title mentions "EIP-7951" but the implementation references "RIP-7212" throughout.
Key Changes:
- Adds secp256r1 ECDSA signature verification precompile using the p256 crate
- Implements special-case handling for address 0x0100 in precompile lookup logic
- Includes comprehensive test coverage with test vectors from daimo-eth/p256-verifier
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| actors/evm/src/interpreter/precompiles/secp256r1.rs | Core implementation of RIP-7212 precompile with signature verification logic and extensive unit tests |
| actors/evm/src/interpreter/precompiles/mod.rs | Registers the secp256r1 precompile at address 0x0100 with special-case lookup handling |
| actors/evm/tests/rip7212_precompile.rs | Integration tests for precompile functionality including value transfer scenarios |
| actors/evm/tests/contracts/Secp256r1Precompile.sol | Solidity test contract with test cases for valid/invalid signatures and edge cases |
| actors/evm/tests/contracts/Secp256r1Precompile.hex | Compiled bytecode for the Solidity test contract |
| actors/evm/tests/basic.rs | Integration test that invokes all Solidity test functions |
| actors/evm/Cargo.toml | Adds p256 crate dependency for secp256r1 support and rstest for parameterized testing |
| Cargo.lock | Lock file updates for new dependencies (p256, rstest, and transitive dependencies) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1708 +/- ##
==========================================
+ Coverage 91.23% 91.24% +0.01%
==========================================
Files 151 152 +1
Lines 32006 32060 +54
==========================================
+ Hits 29200 29254 +54
Misses 2806 2806
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,156 @@ | |||
| mod asm; | |||
There was a problem hiding this comment.
The filename 'rip7212_precompile.rs' may be misleading since this implementation follows EIP-7951 semantics, not RIP-7212 directly. While the precompile is interface-compatible with RIP-7212, the filename should reflect that this is an EIP-7951 implementation. Consider renaming to 'secp256r1_precompile.rs' or 'p256verify_precompile.rs' to better align with the actual implementation and avoid confusion.
| fn rip7212_call_success() { | ||
| let rt = util::construct_and_verify(p256_verify_contract_call()); | ||
|
|
||
| let input = p256_input(); | ||
| let result = util::invoke_contract(&rt, &input); | ||
| let mut expected = [0u8; 32]; | ||
| expected[31] = 1; | ||
| assert_eq!(result[0], util::PrecompileExit::Success as u8); | ||
| assert_eq!(&result[1..], &expected); | ||
| } | ||
|
|
||
| #[test] | ||
| fn rip7212_invalid_input_returns_empty() { | ||
| let rt = util::construct_and_verify(p256_verify_contract_call()); | ||
| let input = vec![0u8; 10]; | ||
| let result = util::invoke_contract(&rt, &input); | ||
| assert_eq!(result[0], util::PrecompileExit::Success as u8); | ||
| assert!(result[1..].is_empty()); | ||
| } | ||
|
|
||
| fn p256_verify_contract_call_value() -> Vec<u8> { | ||
| let init = ""; | ||
| let body = r#" | ||
|
|
||
| calldatasize | ||
| push1 0x00 | ||
| push1 0x00 | ||
| calldatacopy | ||
|
|
||
| # out size | ||
| push1 0x20 | ||
| # out off | ||
| push2 0xA000 | ||
|
|
||
| # in size (160) | ||
| push1 0xA0 | ||
| # in off | ||
| push1 0x00 | ||
|
|
||
| # value (1 atto) | ||
| push1 0x01 | ||
|
|
||
| # dst (0x0100) | ||
| push20 0x0000000000000000000000000000000000000100 | ||
|
|
||
| # gas | ||
| push1 0x00 | ||
|
|
||
| call | ||
|
|
||
| # write exit code memory | ||
| push1 0x00 # offset | ||
| mstore8 | ||
|
|
||
| returndatasize | ||
| push1 0x00 # input offset | ||
| push1 0x01 # dest offset | ||
| returndatacopy | ||
|
|
||
| returndatasize | ||
| push1 0x01 | ||
| add | ||
| push1 0x00 | ||
| return | ||
| "#; | ||
| asm::new_contract("rip7212-precompile-caller-value", init, body).unwrap() | ||
| } | ||
|
|
||
| #[test] | ||
| fn rip7212_call_with_value_transfers_on_success() { | ||
| let rt = util::construct_and_verify(p256_verify_contract_call_value()); | ||
| rt.set_balance(TokenAmount::from_atto(100)); | ||
|
|
||
| let input = p256_input(); | ||
| let mut expected = [0u8; 32]; | ||
| expected[31] = 1; | ||
|
|
||
| let addr = fil_actors_evm_shared::address::EthAddress(hex_literal::hex!( | ||
| "0000000000000000000000000000000000000100" | ||
| )); | ||
| let fil_addr = | ||
| FILAddress::new_delegated(fil_actors_runtime::EAM_ACTOR_ID, addr.as_ref()).unwrap(); | ||
| rt.expect_send_simple( | ||
| fil_addr, | ||
| METHOD_SEND, | ||
| None, | ||
| TokenAmount::from_atto(1), | ||
| None, | ||
| ExitCode::OK, | ||
| ); | ||
|
|
||
| let result = util::invoke_contract(&rt, &input); | ||
| assert_eq!(result[0], util::PrecompileExit::Success as u8); | ||
| assert_eq!(&result[1..], &expected); | ||
| } |
There was a problem hiding this comment.
The function names 'rip7212_call_success', 'rip7212_invalid_input_returns_empty', and 'rip7212_call_with_value_transfers_on_success' reference RIP-7212 but the implementation follows EIP-7951 semantics. While interface-compatible, consider renaming these functions to use 'secp256r1' or 'p256verify' prefix for clarity and to accurately reflect the underlying EIP-7951 implementation.
|
@LesnyRumcajs : could someone from Forest take a first pass? |
redpanda-f
left a comment
There was a problem hiding this comment.
Thank you!
I would like to understand this better: Why are we introducing tests/contracts/*.sol and *.hex at all? I feel secp256r1.rs should be enough? Am i missing something critical?
| } | ||
|
|
||
| #[test] | ||
| fn is_rip_7212_precompile() { |
There was a problem hiding this comment.
All mentions of RIP 7212 should be removed.
EIP-7951 is the formal Ethereum Improvement Proposal that supersedes RIP-7212.
References (https://eips.ethereum.org/EIPS/eip-7951):
This specification addresses critical security issues discovered in RIP-7212 while maintaining full interface compatibility with existing Layer 2 implementations.
This EIP supersedes RIP-7212 by implementing the same functionality with the same interface, but without the vulnerability.
There was a problem hiding this comment.
Not sure if this should be part of this PR: but something like addr!(0x100) expanding to the full addr hex here should be easier for maintenance long term.
| //! | ||
| //! This module implements secp256r1 (P-256) ECDSA signature verification for the FEVM precompile at `0x0100`. | ||
| //! | ||
| //! The precompile is specified by Ethereum's [EIP-7951](https://eips.ethereum.org/EIPS/eip-7951) and is interface-compatible |
There was a problem hiding this comment.
Again, this explicit description should be removed. EIP itself says it is interface compatible already.
| use rstest::rstest; | ||
|
|
||
| #[rstest] | ||
| // Test vectors from https://github.com/daimo-eth/p256-verifier/tree/master/test-vectors |
There was a problem hiding this comment.
Thank you so much for this reference!
| dependencies = [ | ||
| "data-encoding", | ||
| "syn 2.0.106", | ||
| "syn 1.0.109", |
There was a problem hiding this comment.
Why do we have syn downgrade? I understand lockfile commit given Cargo.toml additions however.
There was a problem hiding this comment.
I've seen this on other packages recently. It flip-flops but I suspect if you looked deeper you'd see both syn 1 and 2 included for different transitive reasons.
Insisted on going the other way here just last week: https://github.com/filecoin-project/filecoin-ffi/pull/563/changes
They're first-line integration tests. Can the evm actor even decode standard solidity that includes this new opcode and run it as expected. Being able to run it in Rust is one thing, being able to hit it from the Solidity path is another. See https://github.com/filecoin-project/builtin-actors/tree/master/actors/evm/tests/contracts for a bunch of others, and also #1720 that @ZenGround0 added today for the SectorStatus FIP as an integration test to exercise a feature that's supposed to be exposed to smart contracts. The feature might work, but does it work when you call it from a smart contract. For many of these we'll also do the same in Lotus to integration test at even higher level - it runs here in Rust, but does it run when compiled to WASM, executed in FVM and called from a message in Lotus. See here for recent EVM features that @snissn added (TSTORE and BLS precompiles): https://github.com/filecoin-project/lotus/blob/7741226198083e943a64d917e88a0a77d17aa30e/itests/fevm_test.go#L1831-L1908 -- @wjmelements this might also be interesting on your question of activation, our integration tests can be configured to tick over from one network version to another (see the top of the TSTORE test for example), so for EVM features like this we can say "feature doesn't exist and therefore reverts", then "now it works" within the space of an epoch. |
| # gas | ||
| push1 0x00 | ||
|
|
||
| call |
There was a problem hiding this comment.
it is good that we have test coverage for both call and staticcall
|
This has pretty good test coverage. I'd like additional testing for when there is excess or truncated calldata. |
Summary
0x0000000000000000000000000000000000000100(0x0100), implementing EIP-7951 semantics (interface-compatible with RIP-7212).0x0100in the precompile address space and wire it into precompile dispatch.ruintto addressRUSTSEC-2025-0137.Implementation
actors/evm/src/interpreter/precompiles/mod.rsP256VERIFYimplementation:actors/evm/src/interpreter/precompiles/secp256r1.rsmsg_hash(32) || r(32) || s(32) || pubkey_x(32) || pubkey_y(32)0x01; failure => empty bytesEIP-7951 security fixes
EIP-7951 fixes two edge cases present in RIP-7212 verification specs:
R'.r' ≡ r (mod n)) to handle cases where the x-coordinate ofR'exceeds the curve ordern.This implementation relies on the
p256/ecdsaverifier for correctness, and therefore follows the ECDSA verification procedure with these fixes.Tests
cargo test -p fil_actor_evmNotes / Risk
0x...0100is now a reserved precompile address; any code that previously treated it as a “normal” EVM address will now execute the precompile instead.