Skip to content

Commit 4e6a960

Browse files
authored
fix(levm): don't read from parent block when processing withdrawals (#2556)
**Motivation** When processing withdrawals with levm, accounts that are not cached are fetched directly from the `Store` (aka our DB) using the block's parent hash instead of using the `StoreWrapper` api that already knows which block's state to read accounts from (as we do for all other DB reads). This works fine when executing one block at a time as the block that the StoreWrapper reads from is the block's parent. But when we execute blocks in batch, the StoreWrapper reads from the parent of the first block in the batch, as changes from the following blocks will be recorded in the cache, so when processing withdrawals we may not have the state of the current block's parent in the Store. This PR fixes this issue by using the `StoreWrapper` to read uncached accounts from the batch's parent block instead of looking for an accounts in a parent state that may not exist. It also removes the method `get_account_info_by_hash` so we don't run into the same issue in the future <!-- Why does this pull request exist? What are its goals? --> **Description** * Remove misleading method `get_account_info_by_hash` from levm Database trait (this can lead us to read state from a block that is not the designated parent block and which's state may not exist leading to Inconsistent Trie errors) * Remove the argument `parent_block_hash` from `process_withdrawals` <!-- A clear and concise general description of the changes this PR introduces --> <!-- Link to issues: Resolves #111, Resolves #222 --> Closes #issue_number
1 parent 5a7d759 commit 4e6a960

File tree

5 files changed

+10
-73
lines changed

5 files changed

+10
-73
lines changed

crates/blockchain/payload.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -294,7 +294,7 @@ impl Blockchain {
294294
.unwrap_or(&binding);
295295
context
296296
.vm
297-
.process_withdrawals(withdrawals, &context.payload.header)
297+
.process_withdrawals(withdrawals)
298298
.map_err(EvmError::from)
299299
}
300300

crates/vm/backends/levm/db.rs

-38
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
use ethrex_common::types::AccountInfo;
21
use ethrex_common::U256 as CoreU256;
32
use ethrex_common::{Address as CoreAddress, H256 as CoreH256};
43
use ethrex_levm::constants::EMPTY_CODE_HASH;
@@ -75,25 +74,6 @@ impl LevmDatabase for DatabaseLogger {
7574
self.store.get_chain_config()
7675
}
7776

78-
fn get_account_info_by_hash(
79-
&self,
80-
block_hash: ethrex_common::types::BlockHash,
81-
address: CoreAddress,
82-
) -> Result<Option<AccountInfo>, DatabaseError> {
83-
let account = self.store.get_account_info_by_hash(block_hash, address)?;
84-
{
85-
if let Some(acc) = account.clone() {
86-
let mut code_accessed = self
87-
.code_accessed
88-
.lock()
89-
.map_err(|_| DatabaseError::Custom("Could not lock mutex".to_string()))?;
90-
code_accessed.push(acc.code_hash);
91-
}
92-
}
93-
94-
Ok(account)
95-
}
96-
9777
fn get_account_code(&self, code_hash: CoreH256) -> Result<Option<bytes::Bytes>, DatabaseError> {
9878
{
9979
let mut code_accessed = self
@@ -163,16 +143,6 @@ impl LevmDatabase for StoreWrapper {
163143
self.store.get_chain_config().unwrap()
164144
}
165145

166-
fn get_account_info_by_hash(
167-
&self,
168-
block_hash: ethrex_common::types::BlockHash,
169-
address: CoreAddress,
170-
) -> Result<Option<AccountInfo>, DatabaseError> {
171-
self.store
172-
.get_account_info_by_hash(block_hash, address)
173-
.map_err(|e| DatabaseError::Custom(e.to_string()))
174-
}
175-
176146
fn get_account_code(&self, code_hash: CoreH256) -> Result<Option<bytes::Bytes>, DatabaseError> {
177147
self.store
178148
.get_account_code(code_hash)
@@ -230,14 +200,6 @@ impl LevmDatabase for ExecutionDB {
230200
self.get_chain_config()
231201
}
232202

233-
fn get_account_info_by_hash(
234-
&self,
235-
_block_hash: ethrex_common::types::BlockHash,
236-
address: CoreAddress,
237-
) -> Result<Option<ethrex_common::types::AccountInfo>, DatabaseError> {
238-
Ok(self.accounts.get(&address).cloned())
239-
}
240-
241203
fn get_account_code(&self, code_hash: CoreH256) -> Result<Option<bytes::Bytes>, DatabaseError> {
242204
Ok(self.code.get(&code_hash).cloned())
243205
}

crates/vm/backends/levm/mod.rs

+6-17
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ use ethrex_common::{
2020
use ethrex_levm::{
2121
errors::{ExecutionReport, TxResult, VMError},
2222
vm::{EVMConfig, GeneralizedDatabase, Substate, VM},
23-
Account, AccountInfo as LevmAccountInfo, Environment,
23+
Account, Environment,
2424
};
2525
use ethrex_storage::error::StoreError;
2626
use ethrex_storage::{hash_address, hash_key, AccountUpdate, Store};
@@ -79,7 +79,7 @@ impl LEVM {
7979
}
8080

8181
if let Some(withdrawals) = &block.body.withdrawals {
82-
Self::process_withdrawals(db, withdrawals, block.header.parent_hash)?;
82+
Self::process_withdrawals(db, withdrawals)?;
8383
}
8484

8585
cfg_if::cfg_if! {
@@ -243,7 +243,6 @@ impl LEVM {
243243
pub fn process_withdrawals(
244244
db: &mut GeneralizedDatabase,
245245
withdrawals: &[Withdrawal],
246-
parent_hash: H256,
247246
) -> Result<(), ethrex_storage::error::StoreError> {
248247
// For every withdrawal we increment the target account's balance
249248
for (address, increment) in withdrawals
@@ -253,23 +252,13 @@ impl LEVM {
253252
{
254253
// We check if it was in block_cache, if not, we get it from DB.
255254
let mut account = db.cache.get(&address).cloned().unwrap_or({
256-
let acc_info = db
255+
let info = db
257256
.store
258-
.get_account_info_by_hash(parent_hash, address)
259-
.map_err(|e| StoreError::Custom(e.to_string()))?
260-
.unwrap_or_default();
261-
let acc_code = db
262-
.store
263-
.get_account_code(acc_info.code_hash)
264-
.map_err(|e| StoreError::Custom(e.to_string()))?
265-
.unwrap_or_default();
257+
.get_account_info(address)
258+
.map_err(|e| StoreError::Custom(e.to_string()))?;
266259

267260
Account {
268-
info: LevmAccountInfo {
269-
balance: acc_info.balance,
270-
bytecode: acc_code,
271-
nonce: acc_info.nonce,
272-
},
261+
info,
273262
// This is the added_storage for the withdrawal.
274263
// If not involved in the TX, there won't be any updates in the storage
275264
storage: HashMap::new(),

crates/vm/backends/mod.rs

+2-8
Original file line numberDiff line numberDiff line change
@@ -201,16 +201,10 @@ impl Evm {
201201

202202
/// Wraps the [REVM::process_withdrawals] and [LEVM::process_withdrawals].
203203
/// Applies the withdrawals to the state or the block_chache if using [LEVM].
204-
pub fn process_withdrawals(
205-
&mut self,
206-
withdrawals: &[Withdrawal],
207-
block_header: &BlockHeader,
208-
) -> Result<(), StoreError> {
204+
pub fn process_withdrawals(&mut self, withdrawals: &[Withdrawal]) -> Result<(), StoreError> {
209205
match self {
210206
Evm::REVM { state } => REVM::process_withdrawals(state, withdrawals),
211-
Evm::LEVM { db } => {
212-
LEVM::process_withdrawals(db, withdrawals, block_header.parent_hash)
213-
}
207+
Evm::LEVM { db } => LEVM::process_withdrawals(db, withdrawals),
214208
}
215209
}
216210

crates/vm/levm/src/db/mod.rs

+1-9
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,7 @@
11
use crate::account::AccountInfo;
22
use bytes::Bytes;
33
use error::DatabaseError;
4-
use ethrex_common::{
5-
types::{BlockHash, ChainConfig},
6-
Address, H256, U256,
7-
};
4+
use ethrex_common::{types::ChainConfig, Address, H256, U256};
85

96
pub mod cache;
107
pub use cache::CacheDB;
@@ -16,10 +13,5 @@ pub trait Database: Send + Sync {
1613
fn get_block_hash(&self, block_number: u64) -> Result<Option<H256>, DatabaseError>;
1714
fn account_exists(&self, address: Address) -> bool;
1815
fn get_chain_config(&self) -> ChainConfig;
19-
fn get_account_info_by_hash(
20-
&self,
21-
block_hash: BlockHash,
22-
address: Address,
23-
) -> Result<Option<ethrex_common::types::AccountInfo>, DatabaseError>;
2416
fn get_account_code(&self, code_hash: H256) -> Result<Option<Bytes>, DatabaseError>;
2517
}

0 commit comments

Comments
 (0)