From 005ebd8511921c6a634b0af7117a528c737d6411 Mon Sep 17 00:00:00 2001 From: Roman Krasiuk Date: Mon, 1 May 2023 08:37:45 +0300 Subject: [PATCH] fix(poststate): introduce wiped counter (#2480) --- crates/revm/src/executor.rs | 12 +- crates/storage/provider/src/post_state.rs | 104 ++++++++++++++---- .../src/providers/post_state_provider.rs | 2 +- 3 files changed, 94 insertions(+), 24 deletions(-) diff --git a/crates/revm/src/executor.rs b/crates/revm/src/executor.rs index 9b4a05d2de..73ad299a4c 100644 --- a/crates/revm/src/executor.rs +++ b/crates/revm/src/executor.rs @@ -601,7 +601,8 @@ mod tests { Bytecode, Bytes, ChainSpecBuilder, ForkCondition, StorageKey, H256, MAINNET, U256, }; use reth_provider::{ - post_state::Storage, AccountProvider, BlockHashProvider, StateProvider, StateRootProvider, + post_state::{ChangedStorage, Storage}, + AccountProvider, BlockHashProvider, StateProvider, StateRootProvider, }; use reth_rlp::Decodable; use std::{collections::HashMap, str::FromStr}; @@ -819,7 +820,7 @@ mod tests { block.number, BTreeMap::from([( account1, - Storage { + ChangedStorage { wiped: false, // Slot 1 changed from 0 to 2 storage: BTreeMap::from([(U256::from(1), U256::ZERO)]) @@ -834,7 +835,10 @@ mod tests { post_state.storage(), &BTreeMap::from([( account1, - Storage { wiped: false, storage: BTreeMap::from([(U256::from(1), U256::from(2))]) } + Storage { + times_wiped: 0, + storage: BTreeMap::from([(U256::from(1), U256::from(2))]) + } )]), "Should have changed 1 storage slot" ); @@ -992,7 +996,7 @@ mod tests { "Selfdestructed account should have been deleted" ); assert!( - out.storage().get(&address_selfdestruct).unwrap().wiped, + out.storage().get(&address_selfdestruct).unwrap().wiped(), "Selfdestructed account should have its storage wiped" ); } diff --git a/crates/storage/provider/src/post_state.rs b/crates/storage/provider/src/post_state.rs index 241fd6502e..b6b8c560da 100644 --- a/crates/storage/provider/src/post_state.rs +++ b/crates/storage/provider/src/post_state.rs @@ -27,25 +27,45 @@ pub type AccountChanges = BTreeMap account -> slot -> old value` that represents what slots were changed, /// and what their values were prior to that change. -pub type StorageChanges = BTreeMap>; +pub type StorageChanges = BTreeMap>; -/// Storage for an account. +/// Changed storage state for the account. /// /// # Wiped Storage /// /// The field `wiped` denotes whether the pre-existing storage in the database should be cleared or /// not. -/// -/// If `wiped` is true, then the account was selfdestructed at some point, and the values contained -/// in `storage` should be the only values written to the database. #[derive(Debug, Default, Clone, Eq, PartialEq)] -pub struct Storage { +pub struct ChangedStorage { /// Whether the storage was wiped or not. pub wiped: bool, /// The storage slots. pub storage: BTreeMap, } +/// Latest storage state for the account. +/// +/// # Wiped Storage +/// +/// The `times_wiped` field indicates the number of times the storage was wiped in this poststate. +/// +/// If `times_wiped` is greater than 0, then the account was selfdestructed at some point, and the +/// values contained in `storage` should be the only values written to the database. +#[derive(Debug, Default, Clone, Eq, PartialEq)] +pub struct Storage { + /// The number of times the storage was wiped. + pub times_wiped: u64, + /// The storage slots. + pub storage: BTreeMap, +} + +impl Storage { + /// Returns `true` if the storage was wiped at any point. + pub fn wiped(&self) -> bool { + self.times_wiped > 0 + } +} + // todo: rewrite all the docs for this /// The state of accounts after execution of one or more transactions, including receipts and new /// bytecode. @@ -200,7 +220,7 @@ impl PostState { } storages.insert( keccak256(address), - HashedStorage { wiped: storage.wiped, storage: hashed_storage }, + HashedStorage { wiped: storage.wiped(), storage: hashed_storage }, ); } @@ -259,8 +279,8 @@ impl PostState { self.accounts.extend(other.accounts); for (address, their_storage) in other.storage { let our_storage = self.storage.entry(address).or_default(); - if their_storage.wiped { - our_storage.wiped = true; + if their_storage.wiped() { + our_storage.times_wiped += their_storage.times_wiped; our_storage.storage.clear(); } our_storage.storage.extend(their_storage.storage); @@ -328,7 +348,9 @@ impl PostState { for (_, storages) in storage_changes_to_revert.into_iter().rev() { for (address, storage) in storages { self.storage.entry(address).and_modify(|head_storage| { - head_storage.wiped = storage.wiped; + if storage.wiped { + head_storage.times_wiped -= 1; + } head_storage.storage.extend(storage.clone().storage); }); } @@ -405,7 +427,7 @@ impl PostState { .entry(address) .or_insert(Some(account)); let storage = self.storage.entry(address).or_default(); - storage.wiped = true; + storage.times_wiped += 1; storage.storage.clear(); let storage_changes = self.storage_changes.entry(block_number).or_default().entry(address).or_default(); @@ -493,7 +515,7 @@ impl PostState { // Write new storage state for (address, storage) in self.storage.into_iter() { // If the storage was wiped, remove all previous entries from the database. - if storage.wiped { + if storage.wiped() { tracing::trace!(target: "provider::post_state", ?address, "Wiping storage from plain state"); if storages_cursor.seek_exact(address)?.is_some() { storages_cursor.delete_current_duplicates()?; @@ -832,7 +854,7 @@ mod tests { // All the storage of account A has to be deleted in the database (wiped) assert!( - state.account_storage(&address_a).expect("Account A should have some storage").wiped, + state.account_storage(&address_a).expect("Account A should have some storage").wiped(), "The wiped flag should be set to discard all pre-existing storage from the database" ); // Then, we must ensure that *only* the storage from the last transition will be written @@ -870,6 +892,50 @@ mod tests { ); } + #[test] + fn wiped_revert() { + let address = Address::random(); + + let init_block_number = 1; + let init_account = Account { balance: U256::from(3), ..Default::default() }; + let init_slot = U256::from(1); + + // Create init state for demonstration purposes + // Block 1 + // Account: exists + // Storage: 0x01: 1 + let mut init_state = PostState::new(); + init_state.create_account(init_block_number, address, init_account); + init_state.change_storage( + init_block_number, + address, + BTreeMap::from([(init_slot, (U256::ZERO, U256::from(1)))]), + ); + + let mut post_state = PostState::new(); + // Block 2 + // Account: destroyed + // Storage: wiped + post_state.destroy_account(2, address, init_account); + assert!(post_state.storage.get(&address).unwrap().wiped()); + + // Block 3 + // Account: recreated + // Storage: wiped, then 0x01: 2 + let recreated_account = Account { balance: U256::from(4), ..Default::default() }; + post_state.create_account(3, address, recreated_account); + post_state.change_storage( + 3, + address, + BTreeMap::from([(init_slot, (U256::ZERO, U256::from(2)))]), + ); + assert!(post_state.storage.get(&address).unwrap().wiped()); + + // Revert to block 2 + post_state.revert_to(2); + assert!(post_state.storage.get(&address).unwrap().wiped()); + } + /// Checks that if an account is touched multiple times in the same block, /// then the old value from the first change is kept and not overwritten. /// @@ -983,7 +1049,7 @@ mod tests { block, BTreeMap::from([( address, - Storage { + ChangedStorage { storage: BTreeMap::from([ (U256::from(0), U256::from(0)), (U256::from(1), U256::from(3)) @@ -1008,7 +1074,7 @@ mod tests { (U256::from(0), U256::from(2)), (U256::from(1), U256::from(5)) ]), - wiped: false + times_wiped: 0, } )]), "The latest state of the storage is incorrect" @@ -1055,7 +1121,7 @@ mod tests { block, BTreeMap::from([( address, - Storage { + ChangedStorage { storage: BTreeMap::from([(U256::from(0), U256::from(0)),]), wiped: false, } @@ -1098,7 +1164,7 @@ mod tests { block, BTreeMap::from([( address, - Storage { + ChangedStorage { storage: BTreeMap::from([(U256::from(0), U256::from(1)),]), wiped: false, } @@ -1132,7 +1198,7 @@ mod tests { block, BTreeMap::from([( address, - Storage { + ChangedStorage { storage: BTreeMap::from([(U256::from(0), U256::from(0)),]), wiped: false, } @@ -1154,7 +1220,7 @@ mod tests { address, Storage { storage: BTreeMap::from([(U256::from(0), U256::from(2)),]), - wiped: false + times_wiped: 0, } )]), "The latest state of the storage is incorrect in the merged state" diff --git a/crates/storage/provider/src/providers/post_state_provider.rs b/crates/storage/provider/src/providers/post_state_provider.rs index 8d0aedd27b..b2c6e5e994 100644 --- a/crates/storage/provider/src/providers/post_state_provider.rs +++ b/crates/storage/provider/src/providers/post_state_provider.rs @@ -72,7 +72,7 @@ impl StateProvider for PostState storage.storage.get(&U256::from_be_bytes(storage_key.to_fixed_bytes())) { return Ok(Some(*value)) - } else if storage.wiped { + } else if storage.wiped() { return Ok(Some(U256::ZERO)) } }