From ebbdbebbc6b69a813fa303659149a4460e43d9ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erce=20Can=20Bekt=C3=BCre?= <47954181+ercecan@users.noreply.github.com> Date: Mon, 26 Aug 2024 17:14:24 +0300 Subject: [PATCH] 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 --- Cargo.lock | 1 + crates/evm/Cargo.toml | 1 + crates/evm/src/evm/db_commit.rs | 22 +++++++-- crates/evm/src/evm/handler.rs | 48 +++++++++++-------- crates/evm/src/tests/call_tests.rs | 34 +++++++------ crates/evm/src/tests/queries/basic_queries.rs | 8 ++-- .../src/tests/queries/estimate_gas_tests.rs | 2 +- crates/evm/src/tests/sys_tx_tests.rs | 12 ++--- 8 files changed, 79 insertions(+), 49 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 75b6f939b..3aee33997 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1700,6 +1700,7 @@ dependencies = [ "alloy-rlp", "alloy-sol-types", "anyhow", + "bcs", "borsh", "bytes", "citrea-primitives", diff --git a/crates/evm/Cargo.toml b/crates/evm/Cargo.toml index b26e08347..641a73007 100644 --- 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 index dfd4d38f4..e576b6de1 100644 --- 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::>(); // 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 index a4e24afd0..cf1c5124a 100644 --- 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( 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( 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( if account.destroyed { let account = &state[addr]; diff_size += slot_size * account.storage.len(); // Storage size - diff_size += size_of::(); // Nonces are u64 - diff_size += size_of::(); // Balances are U256 - diff_size += size_of::(); // 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( continue; } - // Apply size of changed nonce - if account.nonce_changed { - diff_size += size_of::(); // Nonces are u64 - } - - // Apply size of changed balances - if account.balance_changed { - diff_size += size_of::(); // 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( // Apply size of changed codes if account.code_changed { let account = &state[addr]; - diff_size += size_of::(); // 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 index 238a5a539..e787d8de1 100644 --- 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 index cd35049e6..8d6b2645b 100644 --- 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) { "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) { "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) { "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) { "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 index 055bb65ef..4058e3b3a 100644 --- 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::(json![{"gas":"0x6601","l1DiffSize":"0x34"}]) + serde_json::from_value::(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 index 3678b9b52..3e3ff762f 100644 --- 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(