-
Notifications
You must be signed in to change notification settings - Fork 46
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
feat(levm): implement create_access_list #2244
Conversation
Lines of code reportTotal lines added: Detailed view
|
EF Tests ComparisonSame results between main branch and the current PR. |
Benchmark Results ComparisonPR ResultsBenchmark Results: Factorial
Benchmark Results: Factorial - Recursive
Benchmark Results: Fibonacci
Benchmark Results: ManyHashes
Benchmark Results: BubbleSort
Benchmark Results: ERC20 - Transfer
Benchmark Results: ERC20 - Mint
Benchmark Results: ERC20 - Approval
Main ResultsBenchmark Results: Factorial
Benchmark Results: Factorial - Recursive
Benchmark Results: Fibonacci
Benchmark Results: ManyHashes
Benchmark Results: BubbleSort
Benchmark Results: ERC20 - Transfer
Benchmark Results: ERC20 - Mint
Benchmark Results: ERC20 - Approval
|
d00597d
to
ec6a407
Compare
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.
Amazing!
8fb6b8a
to
7a25f44
Compare
crates/vm/backends/levm/mod.rs
Outdated
.map_err(|_| VMError::FatalError)?, | ||
header, | ||
); | ||
let mut env = Environment { |
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 imagine this is done more than once, maybe it should have an utility function or something?
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.
Refactored in 000f5f7!
crates/vm/backends/levm/mod.rs
Outdated
header: &BlockHeader, | ||
store: &StoreWrapper, | ||
chain_config: &ChainConfig, | ||
block_cache: &mut CacheDB, |
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.
Does block_cache
need to be mutable here? We are not updating the state, right?
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.
Addressed in 2848656!
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 think it looks good! Just left some suggestions but nothing important
crates/vm/levm/src/utils.rs
Outdated
let access_list: AccessList = substate | ||
.touched_storage_slots | ||
.iter() | ||
.map(|(k, v)| (*k, v.iter().cloned().collect::<Vec<H256>>())) |
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.
.map(|(k, v)| (*k, v.iter().cloned().collect::<Vec<H256>>())) | |
.map(|(address, slots)| (*address, slots.iter().cloned().collect::<Vec<H256>>())) |
Just for clarity
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.
Addressed in 000f5f7!
crates/vm/levm/src/utils.rs
Outdated
@@ -738,3 +738,13 @@ pub fn eip7702_get_code( | |||
|
|||
Ok((true, access_cost, auth_address, authorized_bytecode)) | |||
} | |||
|
|||
pub fn build_access_list(substate: &mut Substate) -> AccessList { |
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.
pub fn build_access_list(substate: &mut Substate) -> AccessList { | |
pub fn build_access_list(substate: &Substate) -> AccessList { |
It doesn't need to be mutable, right?
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.
Addressed in 000f5f7!
crates/vm/backends/mod.rs
Outdated
tx, | ||
header, | ||
store_wrapper, | ||
&store_wrapper.store.get_chain_config()?, |
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.
Maybe we can get the chain config inside the function itself so that the interface is simpler. I see that we are also calling get_chain_config()
inside of create_access_list
despite having it as a parameter. So maybe we could do a let chain_config = ...
at the beginning of the function and use that.
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.
Addressed in 000f5f7!
Motivation
Implement create_access_list for levm
Description
accrued_substate
an access list.Observation
Changes
touched_storage_slots
fromHashSet
toBTreeSet
to align with the expected output order of the addresses in the Hive tests.Hive Tests
These hive tests should be fixed with this PR
Closes #2183