refactor(levm): maintain cached state between executions in levm #2371
Conversation
Lines of code reportTotal lines added: Detailed view |
| store_wrapper, | ||
| block_cache, | ||
| } => LEVM::create_access_list(tx.clone(), header, store_wrapper.clone(), block_cache)?, | ||
| Evm::LEVM { db } => LEVM::create_access_list(tx.clone(), header, db)?, | ||
| }; | ||
| match result { | ||
| ( |
There was a problem hiding this comment.
❤️ all the changes in this file.
crates/l2/Makefile
Outdated
| @@ -151,7 +151,8 @@ init-l2-no-metrics: ## 🚀 Initializes an L2 Lambda ethrex Client | |||
| --http.addr 0.0.0.0 \ | |||
| --authrpc.port ${L2_AUTH_PORT} \ | |||
| --metrics.port ${L2_PROMETHEUS_METRICS_PORT} \ | |||
| --datadir ${ethrex_L2_DEV_LIBMDBX} | |||
| --datadir ${ethrex_L2_DEV_LIBMDBX} \ | |||
There was a problem hiding this comment.
Do we want to initialize with LEVM?
There was a problem hiding this comment.
I removed this levm default here 175dcf9 for now until we fixed the state reconstruct issues that arise on a separate PR
| InternalError::Custom("Error at LEVM::get_state_transitions()".to_owned()) | ||
| })?; | ||
| let mut db = load_initial_state_levm(test); | ||
| let levm_account_updates = backends::levm::LEVM::get_state_transitions(&mut db, *fork) |
There was a problem hiding this comment.
I think this may be wrong.
When loading the initial state now we are also clearing up the cache. So load_initial_state_levm doesn't do the same thing it used to do.
If I'm not mistaken, the execution of get_state_transitions in this scenario is going to result in no account updates because the Cache is empty.
Now, the thing gets messy here, because we don't have the new_state anymore in the LEVM's ExecutionReport, therefore, we lost the cache result.
Maybe now EFTestRunnerError::FailedToEnsurePostState(ExecutionReport, String) needs to contain the new state separately;
EFTestRunnerError::FailedToEnsurePostState(ExecutionReport, String, CacheDB) or something similar should work, so that in ensure_post_state() we return the updated cache with LEVM's changes to the state.
It would be something like:
return Err(EFTestRunnerError::FailedToEnsurePostState(
execution_report.clone(),
error_reason,
db.cache,
));That is just a suggestion, other alternatives can work too. Maybe I'm complicating things more than necessary!
There was a problem hiding this comment.
That makes sense, if I understand correctly this would mean adding the cache as a return value only to the errors used for EF tests; if that's so then the approach makes sense to me
| @@ -105,37 +81,8 @@ impl LEVM { | |||
| receipts.push(receipt); | |||
| } | |||
|
|
|||
| // Here we update block_cache with balance increments caused by withdrawals. | |||
| if let Some(withdrawals) = &block.body.withdrawals { | |||
There was a problem hiding this comment.
I have yet to take a closer look but I think that maybe we need to call process_withdrawals() here so that we update the cache with those changes.
If I'm not wrong this may be a possible culprit behind the failing of the Hive tests. (I hope so)
There was a problem hiding this comment.
Yeah that's correct, I readded withdrawal processing here f5f0c88 but there are still hive test regressions so there's more to it than this
| @@ -436,38 +362,35 @@ impl LEVM { | |||
| pub fn create_access_list( | |||
| mut tx: GenericTransaction, | |||
| header: &BlockHeader, | |||
| store: Arc<dyn LevmDatabase>, | |||
| block_cache: &CacheDB, | |||
| db: &mut GeneralizedDatabase, | |||
There was a problem hiding this comment.
Hmm, I think that mutating the Cache in these scenarios may be wrong. Here we execute with the VM just to get valuable information, but don't actually want to alter the state.
I am not 100% sure yet but I will look for cases like this, because they may cause some trouble. A quick fix would be to clone the database when we don't actually want to mutate it, and avoid sending it as mutable so that it's clear that this isn't our intention.
There was a problem hiding this comment.
This should be ok since the cache we are mutating is one that was instantiated just for the purpose of running the create_access_list flow from the RPC endpoint.
crates/vm/levm/src/vm.rs
Outdated
| // CREATE tx | ||
|
|
||
| let sender_nonce = get_account(&mut cache, db.clone(), env.origin).info.nonce; | ||
| let sender_nonce = get_account_no_push_cache(db, env.origin)?.info.nonce; |
There was a problem hiding this comment.
Is there a particular reason for changing get_account() for get_account_no_push_cache() here? Or is it just a mistake?
There was a problem hiding this comment.
Nice catch, this was indeed a mistake; restored the call to get_account here 67e52c6
Co-authored-by: Jeremías Salomón <[email protected]>
…a new PR when state reconstruct issues are fixed)
| for (address, pre_value) in &test.pre.0 { | ||
| let account = world_state.get_account_info(*address); | ||
| let account = world_state.get_account_info(*address).unwrap(); |
There was a problem hiding this comment.
Should these be catched? Either way I'd change it with expects
There was a problem hiding this comment.
Changed to a map_err here 0ec0b4b
crates/vm/backends/levm/db.rs
Outdated
| fn get_block_hash(&self, block_number: u64) -> Result<Option<CoreH256>, DatabaseError> { | ||
| let a = self | ||
| .store | ||
| .get_block_header(block_number) | ||
| .map_err(|e| DatabaseError::Custom(e.to_string()))?; | ||
|
|
||
| a.map(|a| CoreH256::from(a.compute_block_hash().0)) | ||
| Ok(a.map(|a| CoreH256::from(a.compute_block_hash().0))) |
There was a problem hiding this comment.
I think something like this could be done. Else I'd change the a name to something more descriptive
Ok(self
.store
.get_block_header(block_number)
.map_err(|e| DatabaseError::Custom(e.to_string()))?
.map(|header| CoreH256::from(header.compute_block_hash().0)))There was a problem hiding this comment.
Improved the code with your suggestion here fbbbf49
| .get_storage_at_hash(self.block_hash, address, key) | ||
| .unwrap() | ||
| .unwrap_or_default() | ||
| .map_err(|e| DatabaseError::Custom(e.to_string()))? |
There was a problem hiding this comment.
We might use more specific error types. Maybe in a later PR
**Motivation** - Fix hive tests that were failing (tested all RPC tests and they pass) <!-- Why does this pull request exist? What are its goals? --> **Description** - Created `stateless_execute()`, a wrapper of execute() that doesn't alter the state. It actually backs up the cache before executing and then restores it. I know cloning the cache is not ideal but optimizations to that can be performed later. We use this method when creating access list. - Clear the cache when instantiating a new VM: our storage slots have "original value" and "current value", this is for gas costs, but when executing a new transaction it should happen that all the original values and current values have to be the same. Before the database refactor we were doing this outside of LEVM, now it's embedded in the code! Update: I'm currently trying to fix the failing test from `ethereum/engine` suite. It is the test with name "Replace Blob Transactions (Cancun) (ethrex)" Closes #issue_number
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
|
…ambdaclass/ethrex into introduce-generalized-database-to-levm
**Motivation** Add support for hoodi testnet
**Motivation** Use the principle of least privilege and don't grand write permissions that are then forwarded to potentially malicious actions.
**Motivation** The codebase (mainly rpc) currently interacts with the synced by trying to acquire its lock, which works if we only need to know if the synced is busy, but no longer works if we need more precise information about the sync such as what is the mode of the current sync. This PR introduces the `SyncSupervisor` who is in charge of storing the latest fcu head, starting and restarting sync cycles and informing the current sync status at all times <!-- Why does this pull request exist? What are its goals? --> **Description** <!-- A clear and concise general description of the changes this PR introduces --> <!-- Link to issues: Resolves #111, Resolves #222 --> Closes #2282
**Motivation** Previous changes have sped up other components of the snap sync process, making faults in the byte code fetcher more evident. The byte code fetcher used the same batch size as storage requests, 300, which is far more than the byte codes normally returned by a peer request, causing the byte code fetcher to keep on fetching the last batches when all other fetchers have already finished. This PR reduces the batch size down to 70 so that it coincides with the amount of byte codes regularly returned by peers <!-- Why does this pull request exist? What are its goals? --> **Description** * Rename constant `BATCH_SIZE` -> `STORAGE_BATCH_SIZE` * Add constant `BYTECODE_BATCH_SIZE` <!-- A clear and concise general description of the changes this PR introduces --> <!-- Link to issues: Resolves #111, Resolves #222 --> Closes #issue_number
**Motivation** Avoid update to the cli code to end up in an outdated README **Description** - Added a job that checks that the help output in the ethrex command that is in the README is in sync with the code. Closes #2247
Motivation
This PR refactors levm introducing a
GeneralizedDatabase(the name is not the best, suggestions are welcome), which is simply the combination of the regular store (i.e. the on-disk database) and the cache (the in memory structure we use to keep track of changes done to accounts as execution happens).More importantly, it makes levm (the
VMstruct) hold a mutable reference to said database instead of a regular owned value. This is to bring the levm api more in line to what we need it to be for ethrex; the main problem we are solving is to be able to run multiple transactions while keeping the underlying cache changes from those transactions. Once we are done with execution (because we've finished executing or building the block or group of blocks), callingget_state_transitionswill drain the cachedb for all the account updates and return them.This change allowed, among other things, to implement for levm the previously uninplemented function
execute_block_without_clearing_state, which is used for syncing.Additionally, this PR introduces error handling for the levm
Databasetrait; previously its member functions did not return errors and we were just unwrapping on some of their implementations.Description
Closes #issue_number