Repository: bluealloy/revm (language: rust)

Issue:
Non-consuming conversion from `BundleState` to `StateChangeset`
Currently the API is `pub fn into_plain_state(self, is_value_known: OriginalValuesKnown) -> StateChangeset`. This means that reth DB interfaces must consume the bundlestate in order to write plain state to DB.

Given that all information in the `BundleState` is efficiently memcopy-able, we could make `pub fn to_plain_state(&self, is_value_known: OriginalValuesKnown) -> StateChangeset` with relatively little overhead (deref-and-copy instead of copy-to-stack-and-copy). This version would copy `AccountInfo` and rely on `bytes::Bytes` internal efficient `Clone` for contracts.




Discussion / hints:

Fix patch:
diff --git a/crates/primitives/src/state.rs b/crates/primitives/src/state.rs
index d6ddb3d215..d814450621 100644
--- a/crates/primitives/src/state.rs
+++ b/crates/primitives/src/state.rs
@@ -260,7 +260,33 @@ impl AccountInfo {
         }
     }
 
-    /// Returns account info without the code.
+    /// Returns a copy of this account with the [`Bytecode`] removed. This is
+    /// useful when creating journals or snapshots of the state, where it is
+    /// desirable to store the code blobs elsewhere.
+    ///
+    /// ## Note
+    ///
+    /// This is distinct from [`AccountInfo::without_code`] in that it returns
+    /// a new `AccountInfo` instance with the code removed.
+    /// [`AccountInfo::without_code`] will modify and return the same instance.
+    pub fn copy_without_code(&self) -> Self {
+        Self {
+            balance: self.balance,
+            nonce: self.nonce,
+            code_hash: self.code_hash,
+            code: None,
+        }
+    }
+
+    /// Strip the [`Bytecode`] from this account and drop it. This is
+    /// useful when creating journals or snapshots of the state, where it is
+    /// desirable to store the code blobs elsewhere.
+    ///
+    /// ## Note
+    ///
+    /// This is distinct from [`AccountInfo::copy_without_code`] in that it
+    /// modifies the account in place. [`AccountInfo::copy_without_code`]
+    /// will copy the non-code fields and return a new `AccountInfo` instance.
     pub fn without_code(mut self) -> Self {
         self.take_bytecode();
         self
diff --git a/crates/revm/src/db/states/bundle_state.rs b/crates/revm/src/db/states/bundle_state.rs
index b218e49c10..c2d27a3d58 100644
--- a/crates/revm/src/db/states/bundle_state.rs
+++ b/crates/revm/src/db/states/bundle_state.rs
@@ -581,19 +581,20 @@ impl BundleState {
         self.reverts.push(reverts);
     }
 
-    /// Consume the bundle state and return plain state.
-    pub fn into_plain_state(self, is_value_known: OriginalValuesKnown) -> StateChangeset {
+    /// Generate a [`StateChangeset`] from the bundle state without consuming
+    /// it.
+    pub fn to_plain_state(&self, is_value_known: OriginalValuesKnown) -> StateChangeset {
         // pessimistically pre-allocate assuming _all_ accounts changed.
         let state_len = self.state.len();
         let mut accounts = Vec::with_capacity(state_len);
         let mut storage = Vec::with_capacity(state_len);
 
-        for (address, account) in self.state {
+        for (address, account) in self.state.iter() {
             // append account info if it is changed.
             let was_destroyed = account.was_destroyed();
             if is_value_known.is_not_known() || account.is_info_changed() {
-                let info = account.info.map(AccountInfo::without_code);
-                accounts.push((address, info));
+                let info = account.info.as_ref().map(AccountInfo::copy_without_code);
+                accounts.push((*address, info));
             }
 
             // append storage changes
@@ -602,7 +603,7 @@ impl BundleState {
             // database so we can check if plain state was wiped or not.
             let mut account_storage_changed = Vec::with_capacity(account.storage.len());
 
-            for (key, slot) in account.storage {
+            for (key, slot) in account.storage.iter().map(|(k, v)| (*k, *v)) {
                 // If storage was destroyed that means that storage was wiped.
                 // In that case we need to check if present storage value is different then ZERO.
                 let destroyed_and_not_zero = was_destroyed && !slot.present_value.is_zero();
@@ -622,17 +623,19 @@ impl BundleState {
             if !account_storage_changed.is_empty() || was_destroyed {
                 // append storage changes to account.
                 storage.push(PlainStorageChangeset {
-                    address,
+                    address: *address,
                     wipe_storage: was_destroyed,
                     storage: account_storage_changed,
                 });
             }
         }
+
         let contracts = self
             .contracts
-            .into_iter()
+            .iter()
             // remove empty bytecodes
-            .filter(|(b, _)| *b != KECCAK_EMPTY)
+            .filter(|(b, _)| **b != KECCAK_EMPTY)
+            .map(|(b, code)| (*b, code.clone()))
             .collect::<Vec<_>>();
         StateChangeset {
             accounts,
@@ -641,14 +644,32 @@ impl BundleState {
         }
     }
 
-    /// Consume the bundle state and split it into reverts and plain state.
+    /// Convert the bundle state into a [`StateChangeset`].
+    #[deprecated = "Use `to_plain_state` instead"]
+    pub fn into_plain_state(self, is_value_known: OriginalValuesKnown) -> StateChangeset {
+        self.to_plain_state(is_value_known)
+    }
+
+    /// Generate a [`StateChangeset`] and [`PlainStateReverts`] from the bundle
+    /// state.
+    pub fn to_plain_state_and_reverts(
+        &self,
+        is_value_known: OriginalValuesKnown,
+    ) -> (StateChangeset, PlainStateReverts) {
+        (
+            self.to_plain_state(is_value_known),
+            self.reverts.to_plain_state_reverts(),
+        )
+    }
+
+    /// Consume the bundle state and split it into a [`StateChangeset`] and a
+    /// [`PlainStateReverts`].
+    #[deprecated = "Use `to_plain_state_and_reverts` instead"]
     pub fn into_plain_state_and_reverts(
-        mut self,
+        self,
         is_value_known: OriginalValuesKnown,
     ) -> (StateChangeset, PlainStateReverts) {
-        let reverts = self.take_all_reverts();
-        let plain_state = self.into_plain_state(is_value_known);
-        (plain_state, reverts.into_plain_state_reverts())
+        self.to_plain_state_and_reverts(is_value_known)
     }
 
     /// Extend the bundle with other state
diff --git a/crates/revm/src/db/states/plain_account.rs b/crates/revm/src/db/states/plain_account.rs
index 5aadfcc073..4f64c59629 100644
--- a/crates/revm/src/db/states/plain_account.rs
+++ b/crates/revm/src/db/states/plain_account.rs
@@ -21,7 +21,7 @@ impl PlainAccount {
 }
 
 /// This type keeps track of the current value of a storage slot.
-#[derive(Debug, Clone, Default, PartialEq, Eq, Hash)]
+#[derive(Debug, Copy, Clone, Default, PartialEq, Eq, Hash)]
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
 pub struct StorageSlot {
     /// The value of the storage slot before it was changed.
diff --git a/crates/revm/src/db/states/reverts.rs b/crates/revm/src/db/states/reverts.rs
index 4d8d3f402d..b6aa9dfff7 100644
--- a/crates/revm/src/db/states/reverts.rs
+++ b/crates/revm/src/db/states/reverts.rs
@@ -43,26 +43,34 @@ impl Reverts {
         self.0.extend(other.0);
     }
 
-    /// Consume reverts and create plain state reverts.
+    /// Generate a [`PlainStateReverts`].
     ///
     /// Note that account are sorted by address.
-    pub fn into_plain_state_reverts(mut self) -> PlainStateReverts {
+    pub fn to_plain_state_reverts(&self) -> PlainStateReverts {
         let mut state_reverts = PlainStateReverts::with_capacity(self.0.len());
-        for reverts in self.0.drain(..) {
+        for reverts in &self.0 {
             // pessimistically pre-allocate assuming _all_ accounts changed.
             let mut accounts = Vec::with_capacity(reverts.len());
             let mut storage = Vec::with_capacity(reverts.len());
-            for (address, revert_account) in reverts.into_iter() {
-                match revert_account.account {
-                    AccountInfoRevert::RevertTo(acc) => accounts.push((address, Some(acc))),
-                    AccountInfoRevert::DeleteIt => accounts.push((address, None)),
+            for (address, revert_account) in reverts {
+                match &revert_account.account {
+                    AccountInfoRevert::RevertTo(acc) => {
+                        // cloning is cheap, because account info has 3 small
+                        // fields and a Bytes
+                        accounts.push((*address, Some(acc.clone())))
+                    }
+                    AccountInfoRevert::DeleteIt => accounts.push((*address, None)),
                     AccountInfoRevert::DoNothing => (),
                 }
                 if revert_account.wipe_storage || !revert_account.storage.is_empty() {
                     storage.push(PlainStorageRevert {
-                        address,
+                        address: *address,
                         wiped: revert_account.wipe_storage,
-                        storage_revert: revert_account.storage.into_iter().collect::<Vec<_>>(),
+                        storage_revert: revert_account
+                            .storage
+                            .iter()
+                            .map(|(k, v)| (*k, *v))
+                            .collect::<Vec<_>>(),
                     });
                 }
             }
@@ -71,6 +79,14 @@ impl Reverts {
         }
         state_reverts
     }
+
+    /// Consume reverts and create [`PlainStateReverts`].
+    ///
+    /// Note that account are sorted by address.
+    #[deprecated = "Use `to_plain_state_reverts` instead"]
+    pub fn into_plain_state_reverts(self) -> PlainStateReverts {
+        self.to_plain_state_reverts()
+    }
 }
 
 /// Assumption is that Revert can return full state from any future state to any past state.


Test patch: