chore(l1): fix contract deployment tests from EIP-7002 #2630
Conversation
There where 2 tests that had to do with EIP-7002 and its system contract deployments. A check on the contract code was missing, making the chain to deploy a contract without code.
Lines of code reportTotal lines added: Detailed view |
As with the missing check that is defined on EIP-7002, the same one was missing for the system contract defined on EIP-7251. With this bytecode verification another 2 tests pass.
There was a problem hiding this comment.
Good job identifying this!
I wanted to suggest a change if possible:
- I'd actually make
read_withdrawal_requests()anddequeue_consolidation_requests()return aResult<Option<ExecutionReport>, EvmError>, so that when calling them you can use?for propagating the errors. I think all the code will look cleaner.
crates/vm/backends/levm/mod.rs
Outdated
| @@ -322,6 +323,22 @@ impl LEVM { | |||
| ) | |||
| .ok()?; | |||
|
|
|||
| // According to EIP-7002 we need to check if the WITHDRAWAL_REQUEST_PREDEPLOY_ADDRESS | |||
| // has any code after being deployed. If not, the whole block becomes invalid. | |||
| let acc = db | |||
There was a problem hiding this comment.
I know this is usually in the cache but I'd use db.get_account() or db.get_account_info() (whichever exists, after #2629 it will be db.get_account()) instead just to be more careful.
crates/vm/backends/levm/mod.rs
Outdated
| .get(&*WITHDRAWAL_REQUEST_PREDEPLOY_ADDRESS) | ||
| .unwrap(); | ||
| if code_hash(&acc.info.bytecode) == EMPTY_CODE_HASH { | ||
| return Some(ExecutionReport { |
There was a problem hiding this comment.
This is a little bit hacky. It's best to just return an EvmError rather than making up an ExecutionReport
There was a problem hiding this comment.
Awesome! Sorry for the .unwrap(), totally forgot about it, my bad.
crates/vm/backends/levm/mod.rs
Outdated
| let acc = db | ||
| .cache | ||
| .get(&*CONSOLIDATION_REQUEST_PREDEPLOY_ADDRESS) | ||
| .unwrap(); | ||
| if code_hash(&acc.info.bytecode) == EMPTY_CODE_HASH { | ||
| return Some(ExecutionReport { | ||
| result: TxResult::Revert(VMError::FatalError), | ||
| gas_used: 0, | ||
| gas_refunded: 0, | ||
| output: Bytes::new(), | ||
| logs: vec![], | ||
| }); | ||
| } |
The bytecode of an address was being checked only on the cache, while it should also be checked on the store. Now it is being checked on both places. Also removed unwraps.
There was a problem hiding this comment.
Nice! It's looking better now.
I think there may be a mistake with ExecutionReport handling though.
The case in which read_withdrawals_requests or dequeue_consolidation_requests return an ExecutionReport with a transaction that reverts doesn't exist in this code, because the match report.result in those functions makes sure to return None if Tx reverted.
I would personally remove that match and return a Result<ExecutionReport, EvmError> instead of Result<Option<ExecutionReport>, EvmError>.
Then, we have to investigate what the correct behavior is when those kinds of transactions revert. Do we leave the data empty or do we return an error? Which one is correct? Are there any tests that test that behavior?
|
You are right! Regarding what should we do if the transaction reverts, the EIP-7002 says:
EIP-7251 handles this the same way for the About the tests, there is one that to my understanding tests a revert when using the system contract ( |
|
In the meantime i was debugging some more tests, and what was talked about before solves them. By changing the |
When doing the system calls defined in EIP-7002 and EIP-7251, execution could fail and return an ExecutionReport with a TxResult::Revert, this was not being handled. Now the block becomes invalid if this happens as defined in the EIPs.
A refactor was made on AccountInfo so the field bytecode no longer exists.
…into fix_eip7002_ef_tests
Motivation
On #2586 new tests were added and some of them failed on LEVM and REVM.
Description
8 new tests are now working and dont need to be skipped in each of the VMs. The tests were failing because we were not checking if the bytecode of the system contracts that the EIPs (7002 and 7251) define were empty or not. And also because the we were not handling the case were the system calls revert, leading to an invalidate block.
Closes #2598