Add support for EIP-7702 #323
Conversation
There was a problem hiding this comment.
It seems that there is several special cases that might not be handled correctly in this implementation:
- Delegation Chain Protection: EIP-7702 specifies that clients must retrieve only the first code and stop following delegation chains to prevent infinite loops.
- Delegation to a Precompile: EIP-7702 specifies that when a precompile address is the target of delegation, the retrieved code should be considered empty.
- Opcodes
CODESIZEandCODECOPYshould follows delegation, can you add tests for that?
See discussions above.
This seems to be correctly implemented. In case of delegating to precompile:
So this seems fine.
Right now from my review it looks correct. CODESIZE and CODECOPY returns in all cases the runtime code ( But nevertheless as @librelois has said, it'll always be better to have some more test cases! |
Addressed in 3134553 |
|
Added a comprehensive test suite in f5cc851 and currently investigating the failing tests. |
|
EIP-7702 states: "When multiple tuples from the same authority are present, set the code using the address in the last valid occurrence." It is my understanding that, when multiple authorizations from the same authorities are present in a list, for them to be valid they must have increasing nonces. I updated the corresponding test to reflect this in cc32e59 |
|
|
||
| // Should either succeed (infinite loop prevented) or fail gracefully | ||
| assert!( | ||
| matches!(call_exit_reason, ExitReason::Succeed(_)) |
There was a problem hiding this comment.
It should fail as the EOA can't have valid executable bytecode
|
You tested EXTCODECOPY and EXTCODESIZE, but I didn’t find tests for the corresponding behavior of CODECOPY and CODESIZE. It’s very important to test all affected opcodes. |
I did some research and it seems that geth increment the nonce for every valid tuple. So, if there is two authorizations for the same authority, the nonce must increase twice. |
| let metadata = evm::executor::stack::StackSubstateMetadata::new(1000000, &config); | ||
| let state = evm::executor::stack::MemoryStackState::new(metadata, &mut backend); | ||
| let precompiles = BTreeMap::new(); | ||
| let mut executor = StackExecutor::new_with_precompiles(state, &config, &precompiles); | ||
|
|
||
| let authorization = | ||
| create_authorization(U256::from(1), implementation, U256::zero(), authorizing); | ||
|
|
||
| // Provide insufficient gas (less than the 25000 needed for authorization) | ||
| let (exit_reason, _) = executor.transact_call( | ||
| caller, | ||
| target, | ||
| U256::zero(), | ||
| Vec::new(), | ||
| 20_000, // Insufficient gas | ||
| Vec::new(), | ||
| vec![authorization], | ||
| ); | ||
|
|
There was a problem hiding this comment.
This test fails because the gas recording validation compares against the gasometer's internal gas_limit field instead of the gas_limit parameter passed to executor.transact_call.
This behavior may be intentional - I'm not familiar enough with the codebase to determine the design intent. However, the current implementation appears to ignore the passed gas limit parameter entirely, allowing the test transaction to succeed when it might be expected to fail.
There was a problem hiding this comment.
It’s expected that in the EVM there are two gas limits: one for the transaction and one for each subcall.
The proper way to use the EVM is to create a new Executor instance for each transaction, then call transact_call for the root call and for every subcall. For example, in Frontier we create a new Executor instance at the start of each Ethereum transaction here: https://github.com/moonbeam-foundation/frontier/blob/moonbeam-polkadot-stable2412/frame/evm/src/runner/stack.rs#L296
You should refactor your tests so that the gas limit is placed in the executor’s metadata state, and ensure you create a fresh Executor instance for each transaction.
Description
Adds EIP-7702 support.