feat(evm-ethereum): add BlockAccessListExecutor #10849
Conversation
Rjected
added
C-enhancement
Rjected
force-pushed
the
dan/add-block-access-list-executor
branch
from
6825a17
to
4e89479
Compare
crates/ethereum/evm/src/execute.rs
Outdated
| for (address, account) in self.executor.state.cache.accounts { | ||
| if let Entry::Vacant(entry) = bundle_state.state.entry(address) { |
There was a problem hiding this comment.
bundle state only stores changed slots. so if account or some slots were changed, accessed unchanged slots will not be written back to bundle state
There was a problem hiding this comment.
ah right, added a match here with the occupied branch adding unchanged slots, lmk what you think
| // NOTE: we need to merge keep the reverts for the bundle retention | ||
| self.executor.state.merge_transitions(BundleRetention::Reverts); | ||
|
|
||
| // now, ensure each account from the state is included in the bundle state |
There was a problem hiding this comment.
Below code basically need to be implemented again in crates/optimism/evm/src/execute.rs as well, not a big deal, but wonder if we can extract them somewhere?
There was a problem hiding this comment.
Hm, let me think about it, it might be possible to make this reusable across both
There was a problem hiding this comment.
Couldn't figure out something nice, will just copy the implementation since it's one method and it's simple
| @@ -384,6 +390,71 @@ where | |||
| } | |||
| } | |||
|
|
|||
| /// An executor that retains all cache state from execution in its bundle state. | |||
| #[derive(Debug)] | |||
| pub struct BlockAccessListExecutor<EvmConfig, DB> { | |||
There was a problem hiding this comment.
Currently we're reading from node components to get the block_executor_provider out, if we're gonna have another executor, are we ok with adding another method maybe called access_list_executor?
On the other side, here's the aforementioned version of code that changes fn execute(self, xxx) to fn execute(&mut self, xxx): https://github.com/paradigmxyz/reth/pull/10736/files#diff-eb5f2c91d277e8bdf71850182110f34c23bcb1d70a675f570572f18af090a1d1R33
It works, just need to change bunch of places to let mut executor which I'm not sure if we're ok with
There was a problem hiding this comment.
yes, I think we will add another associated type and method to BlockExecutorProvider
Rjected
force-pushed
the
dan/add-block-access-list-executor
branch
from
4e89479
to
df9a0e2
Compare
|
Would like review on this impl, before I copy it over to optimism as well |
Introduces a new executor type that includes the
CacheStatein the returnedBundleStateduring execution. This should make it easier to integrate the block executor into e.g.debug_executionWitnesswithout changing the normal execution code paths.