-
Notifications
You must be signed in to change notification settings - Fork 117
refactor(l1): refactor chainconfig #5233
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
Lines of code reportTotal lines added: Detailed view |
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
|
crates/common/types/genesis.rs
Outdated
| GrayGlacier = 14, | ||
| Paris = 15, | ||
| Shanghai = 16, | ||
| Homestead = 0, |
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.
why is this change of names?
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.
I changed the enum so the forks would be the ones mentioned in labels for the activation block numbers and timestamps. This was necessary ƒor the match statements in fork_activation_time_or_block and is_fork_activated to work properly. While I could have made it a separate enum, I think it makes more sense for the fork enums to reflect the forks as laid out in the chain config files (and in practice only the chain config struct methods and the tests use the fork enum, so it shouldn't create broader problems)
| GrayGlacier = 13, | ||
| Paris = 14, | ||
| Shanghai = 15, | ||
| #[default] |
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.
we probably shouldn't have defaults
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.
I tried to do this but it breaks other structs that have fork as a field and implement default, like parts of the LEVM runner or EVMConfig. Unless we want to remove those defaults too it might be better to leave this as is (so fork defaults aren't spread out throughout the code)
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.
it kinda sucks, but if you have to keep it, at least choose the (soon to be) current one, Osaka
crates/vm/levm/src/environment.rs
Outdated
|
|
||
| let blob_schedule = chain_config | ||
| .get_fork_blob_schedule(block_header.timestamp) | ||
| .get_current_blob_schedule(block_header.timestamp) |
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.
current is deceiving if its calculated based on a timestamp
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.
Sure. Addressed here (alongside some fixes for other things that were failing)
| // Gas cost for each storage key specified on access lists | ||
| pub const TX_ACCESS_LIST_STORAGE_KEY_GAS: u64 = 1900; | ||
|
|
||
| // Gas cost for each non zero byte on transaction data |
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 change seems unrelated to the PR
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.
Ah, I also took the opportunity to remove code that was only relevant for pre-Merge forks. This change is related to that. I should have clarified that in the PR description; will add it now.
| // TODO: maybe fetch hash too when filtering mempool so we don't have to compute it here (we can do this in the same refactor as adding timestamp) | ||
| let tx_hash = head_tx.tx.hash(); | ||
|
|
||
| // Check whether the tx is replay-protected |
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.
state tests don't use this?
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.
They aren't failing on the CI and they aren't failing locally either, so preseumably not.
Motivation
Improve the maintainability of our code with regards to future forks.
Description
Currently we have a lot of match statements spread throughout various functions to get the current fork, blobschedule, etc; and separate functions for checking the activation of each fork. This creates serious maintainability issues down the line. This PR centralized the match statements in two functions: one to get the activation timestamp/block for a fork, and one to get the blob schedule for a fork, and rewrites the rest of the functions accordingly. All the funcitons to check fork activation were also aggregated into a single
is_fork_activatedfunction that takes the fork as a parameter and checks its activation. The implementation ofget_blob_schedule_for_timealso fetches the most recent active blob schedule if the current fork specifies none, which is part of tackling #4849.Additionally, some code related to pre-Merge logic was removed (checks for EIP 155 and EIP 2028)
Closes #4720