-
Notifications
You must be signed in to change notification settings - Fork 100
test(levm,l1): run state tests as if they were blocks instead of transactions #4179
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Copilot <[email protected]>
Lines of code reportTotal lines added: Detailed view
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements the ability to run state tests as blocks instead of transactions to prepare for SP1 integration. The changes include transaction type refinements, system contract validation removal, and proper transaction signing.
- Refactors transaction creation to accurately handle different transaction types (Legacy, EIP2930, EIP1559, EIP4844, EIP7702)
- Removes system contract empty code validation checks that break state tests
- Implements proper transaction signing with sender's private key
Reviewed Changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
tooling/ef_tests/state_v2/src/modules/utils.rs | Updates load_initial_state to accept fork parameter |
tooling/ef_tests/state_v2/src/modules/types.rs | Adds chain config generation and genesis creation from test and fork |
tooling/ef_tests/state_v2/src/modules/runner.rs | Refactors transaction creation and adds proper signing |
tooling/ef_tests/state_v2/src/modules/block_runner.rs | New module for running tests as blocks |
tooling/ef_tests/state_v2/src/main.rs | Adds feature flag to switch between transaction and block runners |
crates/l2/networking/rpc/signer.rs | Fixes Legacy transaction signature recovery ID calculation |
crates/vm/backends/revm/mod.rs | Removes system contract empty code validation |
crates/vm/backends/levm/mod.rs | Removes system contract empty code validation |
crates/blockchain/blockchain.rs | Makes get_total_blob_gas function public |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
if *fork < Fork::Paris { | ||
panic!("We don't support pre Merge forks, what are you doing here?"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The panic message is unprofessional and unhelpful. Replace with a descriptive error message that explains the limitation and suggests alternatives.
if *fork < Fork::Paris { | |
panic!("We don't support pre Merge forks, what are you doing here?"); | |
panic!("Pre-Merge forks are not supported in this context. Please use a fork at or after Paris (the Merge)."); |
Copilot uses AI. Check for mistakes.
base_fee_per_gas: (test.env.current_base_fee.unwrap().as_u64() | ||
* BASE_FEE_MAX_CHANGE_DENOMINATOR as u64) | ||
.checked_div(7), // This was VERY carefully calculated so that the header passes validations in calculate_base_fee_per_gas |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment mentions "VERY carefully calculated" but doesn't explain the calculation rationale. Replace with a clear explanation of why this specific formula is needed for header validation.
base_fee_per_gas: (test.env.current_base_fee.unwrap().as_u64() | |
* BASE_FEE_MAX_CHANGE_DENOMINATOR as u64) | |
.checked_div(7), // This was VERY carefully calculated so that the header passes validations in calculate_base_fee_per_gas | |
.checked_div(7), // The base fee per gas in the genesis block must be set so that, after applying the base fee adjustment logic (as defined in EIP-1559 and implemented in `calculate_base_fee_per_gas`), the resulting base fee matches the expected value for the first block. The division by 7 is chosen to reverse the protocol's adjustment formula, ensuring the header passes validation according to the Ethereum specification. |
Copilot uses AI. Check for mistakes.
gas: test_case.gas, | ||
to: match to { | ||
TxKind::Call(to) => to, | ||
TxKind::Create => return Err(RunnerError::EIP7702ShouldNotBeCreateType), //TODO: See what to do with this. Maybe we want to get rid of the error and skip the test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This TODO comment indicates unresolved design decisions. Either implement the proper handling or create a tracking issue and reference it in the comment.
TxKind::Create => return Err(RunnerError::EIP7702ShouldNotBeCreateType), //TODO: See what to do with this. Maybe we want to get rid of the error and skip the test. | |
TxKind::Create => return Err(RunnerError::EIP7702ShouldNotBeCreateType), // TODO: See https://github.com/your-org/your-repo/issues/1234 for discussion on how to handle this case (skip test or error). |
Copilot uses AI. Check for mistakes.
} else if !test_case.access_list.is_empty() { | ||
// TODO: This will work, ideally Vec<something> should be Option<Vec<something>> so that we can tell if the field exists or not... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This TODO comment indicates a design limitation in the API. The current logic may incorrectly classify transactions with empty access lists. Consider implementing proper Optional handling or create a tracking issue.
Copilot uses AI. Check for mistakes.
receipts_root: compute_receipts_root(&receipts), | ||
logs_bloom: Default::default(), | ||
difficulty: U256::zero(), | ||
number: 1, // I think this is correct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment "I think this is correct" indicates uncertainty. Either verify this is the correct block number or document why this value is appropriate for state tests.
number: 1, // I think this is correct | |
number: 1, // In Ethereum state tests, the block being constructed is always the first block after genesis, which has block number 1. |
Copilot uses AI. Check for mistakes.
Motivation
Description
sign_inplace
for Legacy Transactions.HighGasPrice
EFTests because we need to fix the gas price for our Legacy and Type 1 transactions, it should be U256 L1: Use 256 bits for gas price in legacy transaction #3629Genesis
block. (Which wasn't very trivial I'd say)Closes #4183