Adjust diff size (#1039)

* Adjust diff size because of db account * Fix tests * Add key prefix size to state diff * Fix tests * Lint * Add noncechange event manually for call transactions * Fix tests * Add clarifying comment
chainwayxyz · Aug 26, 2024 · ebbdbeb · ebbdbeb
1 parent 3ed8fb7
commit ebbdbeb
Show file tree

Hide file tree

Showing 8 changed files with 79 additions and 49 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/crates/evm/Cargo.toml b/crates/evm/Cargo.toml
@@ -47,6 +47,7 @@ secp256k1 = { workspace = true, optional = true }
 
 [dev-dependencies]
 alloy = { workspace = true, features = ["consensus", "providers", "signers", "signer-local"] }
+bcs = { workspace = true }
 bytes = { workspace = true }
 lazy_static = "1.4.0"
 rand = { workspace = true }

diff --git a/crates/evm/src/evm/db_commit.rs b/crates/evm/src/evm/db_commit.rs
@@ -1,7 +1,8 @@
 use std::collections::BTreeMap;
 
+use alloy_primitives::Address;
 use reth_primitives::{KECCAK_EMPTY, U256};
-use revm::primitives::{Account, Address, HashMap};
+use revm::primitives::{Account, AccountInfo, HashMap};
 use revm::DatabaseCommit;
 use sov_modules_api::{StateMapAccessor, StateVecAccessor};
 
@@ -17,11 +18,15 @@ impl<'a, C: sov_modules_api::Context> DatabaseCommit for EvmDb<'a, C> {
                 continue;
             }
             let accounts_prefix = self.accounts.prefix();
+            let mut new_account_flag = false;
 
             let mut db_account = self
                 .accounts
                 .get(&address, self.working_set)
-                .unwrap_or_else(|| DbAccount::new(accounts_prefix, address));
+                .unwrap_or_else(|| {
+                    new_account_flag = true;
+                    DbAccount::new(accounts_prefix, address)
+                });
 
             // https://github.com/Sovereign-Labs/sovereign-sdk/issues/425
             if account.is_selfdestructed() {
@@ -51,8 +56,6 @@ impl<'a, C: sov_modules_api::Context> DatabaseCommit for EvmDb<'a, C> {
                 }
             }
 
-            db_account.info = account_info.into();
-
             let storage_slots = account.storage.into_iter().collect::<BTreeMap<_, _>>();
             // insert to StateVec keys must sorted -- or else nodes will have different state roots
             for (key, value) in storage_slots.into_iter() {
@@ -63,7 +66,16 @@ impl<'a, C: sov_modules_api::Context> DatabaseCommit for EvmDb<'a, C> {
                 db_account.storage.set(&key, &value, self.working_set);
             }
 
-            self.accounts.set(&address, &db_account, self.working_set)
+            if new_account_flag || check_account_info_changed(&db_account, &account_info) {
+                db_account.info = account_info.into();
+                self.accounts.set(&address, &db_account, self.working_set)
+            }
         }
     }
 }
+
+fn check_account_info_changed(db_account: &DbAccount, account_info: &AccountInfo) -> bool {
+    db_account.info.balance != account_info.balance
+        || db_account.info.code_hash != account_info.code_hash
+        || db_account.info.nonce != account_info.nonce
+}
diff --git a/crates/evm/src/evm/handler.rs b/crates/evm/src/evm/handler.rs
@@ -23,6 +23,12 @@ use tracing::instrument;
 use crate::system_events::SYSTEM_SIGNER;
 use crate::{BASE_FEE_VAULT, L1_FEE_VAULT};
 
+const DB_ACCOUNT_SIZE: usize = 256;
+
+// Normally db account key is: 24 bytes of prefix + 1 byte for size of remaining data + 20 bytes of address = 45 bytes
+// But we already add address size to diff size, so we don't need to add it here
+const DB_ACCOUNT_KEY_SIZE: usize = 25;
+
 #[derive(Copy, Clone, Default, Debug)]
 pub struct TxInfo {
     pub l1_diff_size: u64,
@@ -393,25 +399,29 @@ fn calc_diff_size<EXT, DB: Database>(
     struct AccountChange<'a> {
         created: bool,
         destroyed: bool,
-        nonce_changed: bool,
-        code_changed: bool,
-        balance_changed: bool,
         storage_changes: BTreeSet<&'a U256>,
+        code_changed: bool,         // implies code and code hash changed
+        account_info_changed: bool, // implies balance or nonce changed
     }
 
     let mut account_changes: BTreeMap<&Address, AccountChange<'_>> = BTreeMap::new();
 
+    // tx.from always has `account_info_changed` because its nonce is incremented
+    let from = account_changes.entry(&env.tx.caller).or_default();
+    from.account_info_changed = true;
+
     for entry in &journal {
         match entry {
             JournalEntry::NonceChange { address } => {
                 let account = account_changes.entry(address).or_default();
-                account.nonce_changed = true;
+                account.account_info_changed = true;
             }
             JournalEntry::BalanceTransfer { from, to, .. } => {
+                // No need to check balance for 0 value sent, revm does not add it to the journal
                 let from = account_changes.entry(from).or_default();
-                from.balance_changed = true;
+                from.account_info_changed = true;
                 let to = account_changes.entry(to).or_default();
-                to.balance_changed = true;
+                to.account_info_changed = true;
             }
             JournalEntry::StorageChanged { address, key, .. } => {
                 let account = account_changes.entry(address).or_default();
@@ -426,7 +436,7 @@ fn calc_diff_size<EXT, DB: Database>(
                 account.created = true;
                 // When account is created, there is a transfer to init its balance.
                 // So we need to only force the nonce change.
-                account.nonce_changed = true;
+                account.account_info_changed = true;
             }
             JournalEntry::AccountDestroyed { address, .. } => {
                 let account = account_changes.entry(address).or_default();
@@ -464,9 +474,10 @@ fn calc_diff_size<EXT, DB: Database>(
         if account.destroyed {
             let account = &state[addr];
             diff_size += slot_size * account.storage.len(); // Storage size
-            diff_size += size_of::<u64>(); // Nonces are u64
-            diff_size += size_of::<U256>(); // Balances are U256
-            diff_size += size_of::<B256>(); // Code hashes are B256
+
+            // All the nonce, balance and code_hash fields are updated and written to the state with DbAccount
+            diff_size += DB_ACCOUNT_SIZE; // DbAccount size
+            diff_size += DB_ACCOUNT_KEY_SIZE; // DbAccount key size
 
             // Retrieve code from DB and apply its size
             if let Some(info) = db.basic(*addr)? {
@@ -480,14 +491,13 @@ fn calc_diff_size<EXT, DB: Database>(
             continue;
         }
 
-        // Apply size of changed nonce
-        if account.nonce_changed {
-            diff_size += size_of::<u64>(); // Nonces are u64
-        }
-
-        // Apply size of changed balances
-        if account.balance_changed {
-            diff_size += size_of::<U256>(); // Balances are U256
+        // dev signer address 0x9e1abd37ec34bbc688b6a2b7d9387d9256cf1773
+        // we don't check `code_changed` bc account_info is changed always for code_changed
+        if account.account_info_changed || account.code_changed {
+            // DbAccount size is added because when any of those changes the db account is written to the state
+            // because these fields are part of the account info and not state values
+            diff_size += DB_ACCOUNT_SIZE;
+            diff_size += DB_ACCOUNT_KEY_SIZE; // DbAccount key size
         }
 
         // Apply size of changed slots
@@ -496,7 +506,7 @@ fn calc_diff_size<EXT, DB: Database>(
         // Apply size of changed codes
         if account.code_changed {
             let account = &state[addr];
-            diff_size += size_of::<B256>(); // Code hashes are B256
+
             if let Some(code) = account.info.code.as_ref() {
                 diff_size += code.len()
             } else {

diff --git a/crates/evm/src/tests/call_tests.rs b/crates/evm/src/tests/call_tests.rs
@@ -91,6 +91,12 @@ fn call_multiple_test() {
     evm.finalize_hook(&[99u8; 32].into(), &mut working_set.accessory_state());
 
     let db_account = evm.accounts.get(&contract_addr, &mut working_set).unwrap();
+
+    // Make sure the db account size is 256 bytes
+    let db_account_len = bcs::to_bytes(&db_account)
+        .expect("Failed to serialize value")
+        .len();
+    assert_eq!(db_account_len, 256);
     let storage_value = db_account
         .storage
         .get(&U256::ZERO, &mut working_set)
@@ -112,7 +118,7 @@ fn call_multiple_test() {
                 },
                 gas_used: 132943,
                 log_index_start: 0,
-                l1_diff_size: 565,
+                l1_diff_size: 1023,
             },
             Receipt {
                 receipt: reth_primitives::Receipt {
@@ -123,7 +129,7 @@ fn call_multiple_test() {
                 },
                 gas_used: 43730,
                 log_index_start: 0,
-                l1_diff_size: 136,
+                l1_diff_size: 437,
             },
             Receipt {
                 receipt: reth_primitives::Receipt {
@@ -134,7 +140,7 @@ fn call_multiple_test() {
                 },
                 gas_used: 26630,
                 log_index_start: 0,
-                l1_diff_size: 136,
+                l1_diff_size: 437,
             },
             Receipt {
                 receipt: reth_primitives::Receipt {
@@ -145,7 +151,7 @@ fn call_multiple_test() {
                 },
                 gas_used: 26630,
                 log_index_start: 0,
-                l1_diff_size: 136,
+                l1_diff_size: 437,
             }
         ]
     )
@@ -215,7 +221,7 @@ fn call_test() {
                 },
                 gas_used: 132943,
                 log_index_start: 0,
-                l1_diff_size: 565,
+                l1_diff_size: 1023,
             },
             Receipt {
                 receipt: reth_primitives::Receipt {
@@ -226,7 +232,7 @@ fn call_test() {
                 },
                 gas_used: 43730,
                 log_index_start: 0,
-                l1_diff_size: 136,
+                l1_diff_size: 437,
             }
         ]
     )
@@ -904,7 +910,7 @@ fn test_l1_fee_success() {
                 },
                 gas_used: 114235,
                 log_index_start: 0,
-                l1_diff_size: 477,
+                l1_diff_size: 935,
             },]
         )
     }
@@ -921,11 +927,11 @@ fn test_l1_fee_success() {
     );
     run_tx(
         1,
-        U256::from(100000000000000u64 - gas_fee_paid * 10000001 - 477),
+        U256::from(100000000000000u64 - gas_fee_paid * 10000001 - 935),
         // priority fee goes to coinbase
         U256::from(gas_fee_paid),
         U256::from(gas_fee_paid * 10000000),
-        U256::from(477),
+        U256::from(935),
     );
 }
 
@@ -1063,7 +1069,7 @@ fn test_l1_fee_halt() {
                 },
                 gas_used: 106947,
                 log_index_start: 0,
-                l1_diff_size: 445,
+                l1_diff_size: 903,
             },
             Receipt {
                 receipt: reth_primitives::Receipt {
@@ -1074,7 +1080,7 @@ fn test_l1_fee_halt() {
                 },
                 gas_used: 1000000,
                 log_index_start: 0,
-                l1_diff_size: 52,
+                l1_diff_size: 353,
             },
         ]
     );
@@ -1085,8 +1091,8 @@ fn test_l1_fee_halt() {
         .unwrap();
 
     let expenses = 1106947_u64 * 10000000 + // evm gas
-        445  + // l1 contract deploy fee
-        52; // l1 contract call fee
+        903  + // l1 contract deploy fee
+        353; // l1 contract call fee
     assert_eq!(
         db_account.info.balance,
         U256::from(
@@ -1102,5 +1108,5 @@ fn test_l1_fee_halt() {
         base_fee_valut.info.balance,
         U256::from(1106947_u64 * 10000000)
     );
-    assert_eq!(l1_fee_valut.info.balance, U256::from(445 + 52));
+    assert_eq!(l1_fee_valut.info.balance, U256::from(903 + 353));
 }
diff --git a/crates/evm/src/tests/queries/basic_queries.rs b/crates/evm/src/tests/queries/basic_queries.rs
@@ -453,7 +453,7 @@ fn check_against_third_block_receipts(receipts: Vec<AnyTransactionReceipt>) {
         "from": "0x9e1abd37ec34bbc688b6a2b7d9387d9256cf1773",
         "to": "0x819c5497b157177315e1204f52e588b393771719",
         "l1FeeRate": "0x1",
-        "l1DiffSize": "0x34",
+        "l1DiffSize": "0x161",
         "contractAddress": null,
         "logs": [
             {
@@ -504,7 +504,7 @@ fn check_against_third_block_receipts(receipts: Vec<AnyTransactionReceipt>) {
         "from": "0x9e1abd37ec34bbc688b6a2b7d9387d9256cf1773",
         "to": "0x819c5497b157177315e1204f52e588b393771719",
         "l1FeeRate": "0x1",
-        "l1DiffSize": "0x34",
+        "l1DiffSize": "0x161",
         "contractAddress": null,
         "logs": [
             {
@@ -555,7 +555,7 @@ fn check_against_third_block_receipts(receipts: Vec<AnyTransactionReceipt>) {
         "from": "0x9e1abd37ec34bbc688b6a2b7d9387d9256cf1773",
         "to": "0x819c5497b157177315e1204f52e588b393771719",
         "l1FeeRate": "0x1",
-        "l1DiffSize": "0x34",
+        "l1DiffSize": "0x161",
         "contractAddress": null,
         "logs": [
             {
@@ -606,7 +606,7 @@ fn check_against_third_block_receipts(receipts: Vec<AnyTransactionReceipt>) {
         "from": "0x9e1abd37ec34bbc688b6a2b7d9387d9256cf1773",
         "to": "0x819c5497b157177315e1204f52e588b393771719",
         "l1FeeRate": "0x1",
-        "l1DiffSize": "0x34",
+        "l1DiffSize": "0x161",
         "contractAddress": null,
         "logs": [
             {

diff --git a/crates/evm/src/tests/queries/estimate_gas_tests.rs b/crates/evm/src/tests/queries/estimate_gas_tests.rs
@@ -93,7 +93,7 @@ fn test_tx_request_fields_gas() {
     );
     assert_eq!(
         contract_diff_size.unwrap(),
-        serde_json::from_value::<EstimatedDiffSize>(json![{"gas":"0x6601","l1DiffSize":"0x34"}])
+        serde_json::from_value::<EstimatedDiffSize>(json![{"gas":"0x6601","l1DiffSize":"0x161"}])
             .unwrap()
     );
 

diff --git a/crates/evm/src/tests/sys_tx_tests.rs b/crates/evm/src/tests/sys_tx_tests.rs
@@ -50,7 +50,7 @@ fn test_sys_bitcoin_light_client() {
                 },
                 gas_used: 50751,
                 log_index_start: 0,
-                l1_diff_size: 136,
+                l1_diff_size: 437,
             },
             Receipt { // BitcoinLightClient::setBlockInfo(U256, U256)
                 receipt: reth_primitives::Receipt {
@@ -69,7 +69,7 @@ fn test_sys_bitcoin_light_client() {
                 },
                 gas_used: 80720,
                 log_index_start: 0,
-                l1_diff_size: 264,
+                l1_diff_size: 565,
             },
             Receipt {
                 receipt: reth_primitives::Receipt {
@@ -95,7 +95,7 @@ fn test_sys_bitcoin_light_client() {
                 },
                 gas_used: 261215,
                 log_index_start: 1,
-                l1_diff_size: 712,
+                l1_diff_size: 1013,
             }
         ]
     );
@@ -208,7 +208,7 @@ fn test_sys_bitcoin_light_client() {
                 },
                 gas_used: 80720,
                 log_index_start: 0,
-                l1_diff_size: 264,
+                l1_diff_size: 565,
             },
             Receipt {
                 receipt: reth_primitives::Receipt {
@@ -219,7 +219,7 @@ fn test_sys_bitcoin_light_client() {
                 },
                 gas_used: 114235,
                 log_index_start: 1,
-                l1_diff_size: 477,
+                l1_diff_size: 935,
             },
         ]
     );
@@ -230,7 +230,7 @@ fn test_sys_bitcoin_light_client() {
         base_fee_vault.info.balance,
         U256::from(114235u64 * 10000000)
     );
-    assert_eq!(l1_fee_vault.info.balance, U256::from(477));
+    assert_eq!(l1_fee_vault.info.balance, U256::from(935));
 
     let hash = evm
         .get_call(