feat: block executor provider and ethereum + op impl #7594
Conversation
mattsse
changed the title
mattsse
force-pushed
the
matt/executor-trait
branch
from
365ccca
to
d194338
Compare
Co-authored-by: Oliver Nordbjerg <onbjerg@users.noreply.github.com>
Co-authored-by: Oliver Nordbjerg <onbjerg@users.noreply.github.com>
Co-authored-by: Oliver Nordbjerg <onbjerg@users.noreply.github.com>
Co-authored-by: Oliver Nordbjerg <onbjerg@users.noreply.github.com>
Co-authored-by: Oliver Nordbjerg <onbjerg@users.noreply.github.com>
Co-authored-by: Oliver Nordbjerg <onbjerg@users.noreply.github.com>
Co-authored-by: Oliver Nordbjerg <onbjerg@users.noreply.github.com>
Co-authored-by: Oliver Nordbjerg <onbjerg@users.noreply.github.com>
Co-authored-by: Oliver Nordbjerg <onbjerg@users.noreply.github.com>
I think we can eventually make this work I deliberately abstracted the output type so we can change the behaviour more easily |
There was a problem hiding this comment.
this is super nice. docs and comments are very helpful. code is clean, so even without reading docs it's easy to follow.
would be nice to group some of the code inside of the op execute function into smaller functions, for example group the checks related to gas into one function, although I'm aware the different checks are probably in this specific order for a reason, hence may not be able to group in that way.
yes ideally, we can come up with a reusable design that separates this into
but we're not there yet, so I want to keep it like this for now |
There was a problem hiding this comment.
As I understand this you need this refactor to continue being productive in the executor-related code and retire the previous code, so I am supportive of it from that sense.
It feels like we need to get to an Executor abstraction over the Runtime & its config that allows:
- Instantiate the Executor with an Ethereum-like header payload for the runtime
- Streaming inputs on a Transaction-level granularity into it over a channel (instead of over block level), so that we can do IO while we're executing
- Allow pre-execution and post-execution hooks, at the block-level granularity (for 4788 etc.), and perhaps transaction-level granularity for OP-style deposit transaction nonce caching thing?
- On executor-defined intervals, stream outputs to a cache and/or DB writer
| // apply pre execution changes | ||
| apply_beacon_root_contract_call( | ||
| &self.chain_spec, | ||
| block.timestamp, | ||
| block.number, | ||
| block.parent_beacon_block_root, | ||
| &mut evm, | ||
| )?; |
There was a problem hiding this comment.
This should be generalized to a hook exposed to the Node Builder for both pre and post exec cc @Rjected re: Electra EIPs
There was a problem hiding this comment.
this is the idea, but I just want to get rid of the EVMprocessor asap because this is blocking everything else
| fn execute_pre_and_transactions<Ext, DB>( | ||
| &mut self, | ||
| block: &BlockWithSenders, | ||
| mut evm: Evm<'_, Ext, &mut State<DB>>, |
There was a problem hiding this comment.
Is the &mut an issue for parallelism down the line?
There was a problem hiding this comment.
a paralle impl needs its own implementation
| error: err.into(), | ||
| } | ||
| })?; | ||
| evm.db_mut().commit(state); |
There was a problem hiding this comment.
Confirming this does no IO right, just inmem?
There was a problem hiding this comment.
just in mem
tbh I find the entire bundle state thing super confusing and actually not sure if this is need for the single block executor
There was a problem hiding this comment.
It's so that we can cache effectively across blocks while also keeping all the created/destroyed information in one place (vs clone'ing it / recalculating it like we did before with PostState)
| /// It does __not__ apply post-execution changes. | ||
| fn execute_pre_and_transactions<Ext, DB>( | ||
| &mut self, | ||
| block: &BlockWithSenders, |
There was a problem hiding this comment.
I hope that we can get to a world where we can stream reads from a TxReader service into an Executor one-by-one over a channel, vs having to load them all in mem and then pass on the entire batch to the executor, can we accommodate that?
| /// The output of an executed block in a batch. | ||
| #[derive(Debug, Clone, Copy)] | ||
| pub struct BatchBlockOutput { | ||
| /// The size hint of the batch's tracked state. | ||
| pub size_hint: Option<usize>, |
There was a problem hiding this comment.
a bit confusing type, I'd think BatchBlockOutput includes a Vec<Self::Output> above
There was a problem hiding this comment.
it does not because all changes are committed to the same bundlestate which then gets consumed and written to db
| /// A helper type for ethereum block inputs that consists of a block and the total difficulty. | ||
| #[derive(Debug)] | ||
| pub struct EthBlockExecutionInput<'a, Block> { | ||
| /// The block to execute. | ||
| pub block: &'a Block, | ||
| /// The total difficulty of the block. | ||
| pub total_difficulty: U256, | ||
| } |
There was a problem hiding this comment.
I like the Input being generic over the block, I'd think that we can make the total_difficulty an External generic such that we get rid of it as a term as it's no longer relevant Post-merge
| /// The total gas used by the block. | ||
| pub gas_used: u64, |
There was a problem hiding this comment.
I'm good with this being concrete, would encourage us to proactively think about what will multi dimensional pricing look like in the future, e.g. thinking about "local fee markets" in parevm
| // Cache the depositor account prior to the state transition for the deposit nonce. | ||
| // | ||
| // Note that this *only* needs to be done post-regolith hardfork, as deposit nonces | ||
| // were not introduced in Bedrock. In addition, regular transactions don't have deposit | ||
| // nonces, so we don't need to touch the DB for those. | ||
| let depositor = (is_regolith && transaction.is_deposit()) | ||
| .then(|| { | ||
| evm.db_mut() | ||
| .load_cache_account(*sender) | ||
| .map(|acc| acc.account_info().unwrap_or_default()) | ||
| }) | ||
| .transpose() | ||
| .map_err(|_| { | ||
| BlockExecutionError::OptimismBlockExecution( | ||
| OptimismBlockExecutionError::AccountLoadFailed(*sender), | ||
| ) | ||
| })?; |
There was a problem hiding this comment.
this feels hacky not sure how to pull out nicely
Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>
yes the EVMProcessor leaks into everything and is very bad because it is mutually exclusive with optimism/ethereum this needs to go asap. all of these suggestions are future work, but we need this separation now, then we can make changes more easily. |
the current BlockExecutor trait is intended for batch execution
we need one for single blocks, the batch executor can then simply use this.
This introduces a few new traits for execution
Batch and single block execution are now two separate parts
The OP and ETH currently has some duplicated code, but we want keep this because a lot will change wrt primitive types so unifying them at this point would introduce issues in future.
This only adds the types
replacing the EVMProcessor with this should be done in a separate pr
This will unlock a ton of cleanups wrt optimism types and project layout
Note: this does use GATs for the provider