-
Notifications
You must be signed in to change notification settings - Fork 2.3k
perf(engine): do not invalidate accounts in cache post Cancun #20619
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,7 @@ use reth_trie::{ | |
| updates::TrieUpdates, AccountProof, HashedPostState, HashedStorage, MultiProof, | ||
| MultiProofTargets, StorageMultiProof, StorageProof, TrieInput, | ||
| }; | ||
| use revm_primitives::map::DefaultHashBuilder; | ||
| use revm_primitives::{hardfork::SpecId, map::DefaultHashBuilder}; | ||
| use std::{sync::Arc, time::Duration}; | ||
| use tracing::{debug_span, instrument, trace}; | ||
|
|
||
|
|
@@ -442,7 +442,11 @@ impl ExecutionCache { | |
| /// | ||
| /// Returns an error if the state updates are inconsistent and should be discarded. | ||
| #[instrument(level = "debug", target = "engine::caching", skip_all)] | ||
| pub(crate) fn insert_state(&self, state_updates: &BundleState) -> Result<(), ()> { | ||
| pub(crate) fn insert_state( | ||
| &self, | ||
| state_updates: &BundleState, | ||
| spec: &SpecId, | ||
| ) -> Result<(), ()> { | ||
| let _enter = | ||
| debug_span!(target: "engine::tree", "contracts", len = state_updates.contracts.len()) | ||
| .entered(); | ||
|
|
@@ -467,8 +471,13 @@ impl ExecutionCache { | |
| continue | ||
| } | ||
|
|
||
| // If the account was destroyed, invalidate from the account / storage caches | ||
| if account.was_destroyed() { | ||
| // If the account was destroyed, invalidate from the account / storage caches. | ||
| // | ||
| // Post-cancun when EIP-6780 is live, an account can be destroyed only when it's created | ||
| // in the same transaction. This guarantees that we will not have such accounts | ||
| // and storage slots in our cache, because Revm doesn't go through the | ||
| // Database for freshly created accounts. Hence we can safely ignore invalidating them. | ||
|
Comment on lines
+476
to
+479
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this doesnt mention state, can we elaborate why this is okay to not invalidate state |
||
| if account.was_destroyed() && !spec.is_enabled_in(revm_primitives::hardfork::CANCUN) { | ||
| // Invalidate the account cache entry if destroyed | ||
| self.account_cache.invalidate(addr); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,9 +14,10 @@ use alloy_consensus::transaction::Either; | |
| use alloy_eip7928::BlockAccessList; | ||
| use alloy_eips::{eip1898::BlockWithParent, NumHash}; | ||
| use alloy_evm::Evm; | ||
| use alloy_primitives::B256; | ||
| use alloy_primitives::{BlockTimestamp, B256}; | ||
| use rayon::prelude::*; | ||
| use reth_chain_state::{CanonicalInMemoryState, DeferredTrieData, ExecutedBlock}; | ||
| use reth_chainspec::EthereumHardforks; | ||
| use reth_consensus::{ConsensusError, FullConsensus}; | ||
| use reth_engine_primitives::{ | ||
| ConfigureEngineEvm, ExecutableTxIterator, ExecutionPayload, InvalidBlockHook, PayloadValidator, | ||
|
|
@@ -34,7 +35,7 @@ use reth_primitives_traits::{ | |
| SealedHeader, SignerRecoverable, | ||
| }; | ||
| use reth_provider::{ | ||
| providers::OverlayStateProviderFactory, BlockExecutionOutput, BlockReader, | ||
| providers::OverlayStateProviderFactory, BlockExecutionOutput, BlockReader, ChainSpecProvider, | ||
| DatabaseProviderFactory, DatabaseProviderROFactory, ExecutionOutcome, HashedPostStateProvider, | ||
| ProviderError, PruneCheckpointReader, StageCheckpointReader, StateProvider, | ||
| StateProviderFactory, StateReader, TrieReader, | ||
|
|
@@ -108,6 +109,7 @@ impl<'a, N: NodePrimitives> TreeCtx<'a, N> { | |
| pub struct BasicEngineValidator<P, Evm, V> | ||
| where | ||
| Evm: ConfigureEvm, | ||
| P: ChainSpecProvider<ChainSpec: EthereumHardforks>, | ||
| { | ||
| /// Provider for database access. | ||
| provider: P, | ||
|
|
@@ -118,7 +120,7 @@ where | |
| /// Configuration for the tree. | ||
| config: TreeConfig, | ||
| /// Payload processor for state root computation. | ||
| payload_processor: PayloadProcessor<Evm>, | ||
| payload_processor: PayloadProcessor<Evm, P::ChainSpec>, | ||
| /// Precompile cache map. | ||
| precompile_cache_map: PrecompileCacheMap<SpecFor<Evm>>, | ||
| /// Precompile cache metrics. | ||
|
|
@@ -141,6 +143,7 @@ where | |
| + StateProviderFactory | ||
| + StateReader | ||
| + HashedPostStateProvider | ||
| + ChainSpecProvider<ChainSpec: EthereumHardforks> | ||
| + Clone | ||
| + 'static, | ||
| Evm: ConfigureEvm<Primitives = N> + 'static, | ||
|
|
@@ -161,6 +164,7 @@ where | |
| evm_config.clone(), | ||
| &config, | ||
| precompile_cache_map.clone(), | ||
| provider.chain_spec(), | ||
| ); | ||
| Self { | ||
| provider, | ||
|
|
@@ -390,7 +394,14 @@ where | |
| .in_scope(|| self.evm_env_for(&input)) | ||
| .map_err(NewPayloadError::other)?; | ||
|
|
||
| let env = ExecutionEnv { evm_env, hash: input.hash(), parent_hash: input.parent_hash() }; | ||
| let spec_id = alloy_evm::spec_by_timestamp_and_block_number( | ||
| &provider_builder.provider_factory.chain_spec(), | ||
| input.timestamp(), | ||
| input.num_hash().number, | ||
| ); | ||
|
Comment on lines
+397
to
+401
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not really a fan of is this because this is super eth specific but this should work as long as everyone implements the eth fork activations correctly |
||
|
|
||
| let env = | ||
| ExecutionEnv { evm_env, hash: input.hash(), parent_hash: input.parent_hash(), spec_id }; | ||
|
|
||
| // Plan the strategy used for state root computation. | ||
| let strategy = self.plan_state_root_computation(); | ||
|
|
@@ -1185,6 +1196,7 @@ where | |
| + StateProviderFactory | ||
| + StateReader | ||
| + HashedPostStateProvider | ||
| + ChainSpecProvider<ChainSpec: EthereumHardforks> | ||
| + Clone | ||
| + 'static, | ||
| N: NodePrimitives, | ||
|
|
@@ -1226,6 +1238,7 @@ where | |
|
|
||
| fn on_inserted_executed_block(&self, block: ExecutedBlock<N>) { | ||
| self.payload_processor.on_inserted_executed_block( | ||
| block.recovered_block.header(), | ||
| block.recovered_block.block_with_parent(), | ||
| block.execution_output.state(), | ||
| ); | ||
|
|
@@ -1258,6 +1271,14 @@ impl<T: PayloadTypes> BlockOrPayload<T> { | |
| } | ||
| } | ||
|
|
||
| /// Returns the timestamp of the block. | ||
| pub fn timestamp(&self) -> BlockTimestamp { | ||
| match self { | ||
| Self::Payload(payload) => payload.timestamp(), | ||
| Self::Block(block) => block.timestamp(), | ||
| } | ||
| } | ||
|
|
||
| /// Returns the parent hash of the block. | ||
| pub fn parent_hash(&self) -> B256 { | ||
| match self { | ||
|
|
||
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 needs doc updates why this needs an evm specid