Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
aa63238
Changed account updates to happen only if update info isnt empty
SDartayet May 6, 2025
4991f3a
Removed clone in account storage updating
SDartayet May 6, 2025
2030363
Refactored database functions to make them return references, removed…
SDartayet May 6, 2025
5d42563
Fixed warnings and ran cargo fmt
SDartayet May 6, 2025
115c9cd
Merge branch 'main' into remove-clones
SDartayet May 6, 2025
1a714e6
Ran cargo fmt
SDartayet May 6, 2025
f6efb76
Merge branch 'main' into remove-clones
SDartayet May 9, 2025
1c22fb4
Fixed some merge issues
SDartayet May 9, 2025
39aa796
Merge branch 'main' into remove-clones
SDartayet May 9, 2025
b1f9d96
Merge branch 'main' into remove-clones
SDartayet May 9, 2025
47d634a
Fixed some nits and removed another clone
SDartayet May 12, 2025
4f7025d
Fixed compiler errors
SDartayet May 12, 2025
b28a9fb
Reverted removing a clone
SDartayet May 12, 2025
bc7a9ea
Merge branch 'main' into remove-clones
SDartayet May 12, 2025
c10d204
Merge branch 'main' into remove-clones
SDartayet May 13, 2025
20f5ebf
Merge branch 'main' into remove-clones
SDartayet May 20, 2025
a607c4e
Fixed typo in environment.rs
SDartayet May 20, 2025
070df9c
Fixed typo in environment.rs
SDartayet May 20, 2025
5808bbc
Ran cargo fmt
SDartayet May 20, 2025
d554a10
Merge branch 'main' into remove-clones
SDartayet May 20, 2025
7293df3
Added perf changelog
SDartayet May 20, 2025
eee94f5
Removed unnecessary inner scope
SDartayet May 21, 2025
575dd42
Removed unnecessary inner scope
SDartayet May 21, 2025
bd4c997
Removed account info clone
SDartayet May 21, 2025
e98bcec
Replaced get account for access account
SDartayet May 21, 2025
9c6ce64
Merge branch 'main' into remove-clones
SDartayet May 21, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions crates/vm/backends/levm/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -247,14 +247,17 @@ impl LEVM {
.map(|w| (w.address, u128::from(w.amount) * u128::from(GWEI_TO_WEI)))
{
// We check if it was in block_cache, if not, we get it from DB.
let mut account = db.cache.get(&address).cloned().unwrap_or({
db.store
if let Some(account) = db.cache.get_mut(&address) {
account.info.balance += increment.into();
} else {
let mut account = db
.store
.get_account(address)
.map_err(|e| StoreError::Custom(e.to_string()))?
});

account.info.balance += increment.into();
db.cache.insert(address, account);
.clone();
account.info.balance += increment.into();
db.cache.insert(address, account);
}
}
Ok(())
}
Expand Down
55 changes: 32 additions & 23 deletions crates/vm/db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,39 +73,48 @@ impl ExecutionDB {
if update.removed {
self.accounts.remove(&update.address);
} else {
// Add or update AccountInfo
// Fetch current account_info or create a new one to be inserted
let mut account_info = match self.accounts.get(&update.address) {
Some(account_info) => account_info.clone(),
None => AccountInfo::default(),
};
// Check if account info needs to be updated
// If it is, create new struct
if let Some(info) = &update.info {
account_info.nonce = info.nonce;
account_info.balance = info.balance;
account_info.code_hash = info.code_hash;
// If the account already exists, we can just update its info in the array and avoid cloning it
if let Some(account) = self.accounts.get_mut(&update.address) {
account.nonce = info.nonce;
account.balance = info.balance;
account.code_hash = info.code_hash;
} else {
let account_info = AccountInfo {
nonce: info.nonce,
balance: info.balance,
code_hash: info.code_hash,
};

//Update the account info
self.accounts.insert(update.address, account_info);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly here I'd just do:
self.accounts.insert(update.address, info.clone());
It is way more simple and we are just cloning account info, which is pretty small. I think it is worth it for making the code simpler because the perfromance difference is negligible I'd say.

We should optimize for performance when cloning an entire Account that has storage in it. But these cases in which we clone AccountInfo are probably not significant. And we prefer simplicity in this scenario.
What do you think?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree given that it should be pretty small and we want to go for simplicity. Also it will be better for isolating the changes, if we want to test the performance impact of removing the clones of AccountInfo we can do it in another branch


// Store updated code
if let Some(code) = &update.code {
self.code.insert(info.code_hash, code.clone());
}
}
// Insert new AccountInfo
self.accounts.insert(update.address, account_info);

// Store the added storage
if !update.added_storage.is_empty() {
let mut storage = match self.storage.get(&update.address) {
Some(storage) => storage.clone(),
None => HashMap::default(),
};
for (storage_key, storage_value) in &update.added_storage {
if storage_value.is_zero() {
storage.remove(storage_key);
} else {
storage.insert(*storage_key, *storage_value);
let update_storage = |storage: &mut HashMap<H256, U256>| {
for (storage_key, storage_value) in &update.added_storage {
if storage_value.is_zero() {
storage.remove(storage_key);
} else {
storage.insert(*storage_key, *storage_value);
}
}
}
self.storage.insert(update.address, storage);
};
if let Some(storage) = self.storage.get_mut(&update.address) {
update_storage(storage);
} else {
let mut storage = HashMap::default();
update_storage(&mut storage);
self.storage.insert(update.address, storage);
};
}
}
}
Expand Down
4 changes: 4 additions & 0 deletions crates/vm/levm/src/db/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@ use std::collections::HashMap;

pub type CacheDB = HashMap<Address, Account>;

pub fn account_is_cached(cached_accounts: &CacheDB, address: &Address) -> bool {
cached_accounts.contains_key(address)
}

pub fn get_account<'cache>(
cached_accounts: &'cache CacheDB,
address: &Address,
Expand Down
16 changes: 7 additions & 9 deletions crates/vm/levm/src/db/gen_db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,13 @@ impl GeneralizedDatabase {
// ================== Account related functions =====================
/// Gets account, first checking the cache and then the database
/// (caching in the second case)
pub fn get_account(&mut self, address: Address) -> Result<Account, DatabaseError> {
match cache::get_account(&self.cache, &address) {
Some(acc) => Ok(acc.clone()),
None => {
let account = self.store.get_account(address)?;
cache::insert_account(&mut self.cache, address, account.clone());
Ok(account)
}
pub fn get_account(&mut self, address: Address) -> Result<&Account, DatabaseError> {
if !cache::account_is_cached(&self.cache, &address) {
let account = self.store.get_account(address)?.clone();
cache::insert_account(&mut self.cache, address, account);
}
cache::get_account(&self.cache, &address)
.ok_or(DatabaseError::Custom("Cache error".to_owned()))
}

/// **Accesses to an account's information.**
Expand All @@ -49,7 +47,7 @@ impl GeneralizedDatabase {
&mut self,
accrued_substate: &mut Substate,
address: Address,
) -> Result<(Account, bool), DatabaseError> {
) -> Result<(&Account, bool), DatabaseError> {
let address_was_cold = accrued_substate.touched_accounts.insert(address);
let account = self.get_account(address)?;

Expand Down
23 changes: 16 additions & 7 deletions crates/vm/levm/src/hooks/default_hook.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,13 @@ impl Hook for DefaultHook {
/// See 'docs' for more information about validations.
fn prepare_execution(&self, vm: &mut VM<'_>) -> Result<(), VMError> {
let sender_address = vm.env.origin;
let sender_account = vm.db.get_account(sender_address)?;
let sender_balance;
let sender_nonce;
{
let sender_account = vm.db.get_account(sender_address)?;
sender_balance = sender_account.info.balance;
sender_nonce = sender_account.info.nonce;
}
Comment thread
SDartayet marked this conversation as resolved.
Outdated

if vm.env.config.fork >= Fork::Prague {
validate_min_gas_limit(vm)?;
Expand All @@ -44,7 +50,7 @@ impl Hook for DefaultHook {
TxValidationError::GasLimitPriceProductOverflow,
))?;

validate_sender_balance(vm, &sender_account)?;
validate_sender_balance(vm, sender_balance)?;

// (2) INSUFFICIENT_MAX_FEE_PER_BLOB_GAS
if let Some(tx_max_fee_per_blob_gas) = vm.env.tx_max_fee_per_blob_gas {
Expand All @@ -70,9 +76,9 @@ impl Hook for DefaultHook {
.map_err(|_| VMError::TxValidation(TxValidationError::NonceIsMax))?;

// check for nonce mismatch
if sender_account.info.nonce != vm.env.tx_nonce {
if sender_nonce != vm.env.tx_nonce {
return Err(VMError::TxValidation(TxValidationError::NonceMismatch {
expected: sender_account.info.nonce,
expected: sender_nonce,
actual: vm.env.tx_nonce,
}));
}
Expand All @@ -90,7 +96,10 @@ impl Hook for DefaultHook {
}

// (9) SENDER_NOT_EOA
validate_sender(&sender_account)?;
{
Comment thread
SDartayet marked this conversation as resolved.
Outdated
let sender_account = vm.db.get_account(sender_address)?;
validate_sender(sender_account)?;
}

// (10) GAS_ALLOWANCE_EXCEEDED
validate_gas_allowance(vm)?;
Expand Down Expand Up @@ -408,7 +417,7 @@ pub fn validate_gas_allowance(vm: &mut VM<'_>) -> Result<(), VMError> {
Ok(())
}

pub fn validate_sender_balance(vm: &mut VM<'_>, sender_account: &Account) -> Result<(), VMError> {
pub fn validate_sender_balance(vm: &mut VM<'_>, sender_balance: U256) -> Result<(), VMError> {
// Up front cost is the maximum amount of wei that a user is willing to pay for. Gaslimit * gasprice + value + blob_gas_cost
let value = vm.current_call_frame()?.msg_value;

Expand Down Expand Up @@ -438,7 +447,7 @@ pub fn validate_sender_balance(vm: &mut VM<'_>, sender_account: &Account) -> Res
TxValidationError::InsufficientAccountFunds,
))?;

if sender_account.info.balance < balance_for_valid_tx {
if sender_balance < balance_for_valid_tx {
return Err(VMError::TxValidation(
TxValidationError::InsufficientAccountFunds,
));
Expand Down
19 changes: 14 additions & 5 deletions crates/vm/levm/src/hooks/l2_hook.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,13 @@ impl Hook for L2Hook {
}

let sender_address = vm.env.origin;
let sender_account = vm.db.get_account(sender_address)?;
let sender_balance;
let sender_nonce;
{
let sender_account = vm.db.get_account(sender_address)?;
sender_balance = sender_account.info.balance;
sender_nonce = sender_account.info.nonce;
}

Comment thread
SDartayet marked this conversation as resolved.
Outdated
if vm.env.config.fork >= Fork::Prague {
default_hook::validate_min_gas_limit(vm)?;
Expand All @@ -40,7 +46,7 @@ impl Hook for L2Hook {
TxValidationError::GasLimitPriceProductOverflow,
))?;

default_hook::validate_sender_balance(vm, &sender_account)?;
default_hook::validate_sender_balance(vm, sender_balance)?;

// (3) INSUFFICIENT_ACCOUNT_FUNDS
default_hook::deduct_caller(vm, gaslimit_price_product, sender_address)?;
Expand All @@ -50,15 +56,18 @@ impl Hook for L2Hook {
.map_err(|_| VMError::TxValidation(TxValidationError::NonceIsMax))?;

// check for nonce mismatch
if sender_account.info.nonce != vm.env.tx_nonce {
if sender_nonce != vm.env.tx_nonce {
return Err(VMError::TxValidation(TxValidationError::NonceMismatch {
expected: sender_account.info.nonce,
expected: sender_nonce,
actual: vm.env.tx_nonce,
}));
}

// (9) SENDER_NOT_EOA
default_hook::validate_sender(&sender_account)?;
{
Comment thread
SDartayet marked this conversation as resolved.
Outdated
let sender_account = vm.db.get_account(sender_address)?;
default_hook::validate_sender(sender_account)?;
}
}

// (2) INSUFFICIENT_MAX_FEE_PER_BLOB_GAS
Expand Down
30 changes: 20 additions & 10 deletions crates/vm/levm/src/opcode_handlers/environment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,13 @@ impl<'a> VM<'a> {
let (account, address_was_cold) = self
.db
.access_account(&mut self.accrued_substate, address)?;
let account_info = account.info.clone();
Comment thread
SDartayet marked this conversation as resolved.
Outdated

let current_call_frame = self.current_call_frame_mut()?;

current_call_frame.increase_consumed_gas(gas_cost::balance(address_was_cold)?)?;

current_call_frame.stack.push(account.info.balance)?;
current_call_frame.stack.push(account_info.balance)?;

Ok(OpcodeResult::Continue { pc_increment: 1 })
}
Expand Down Expand Up @@ -260,11 +261,13 @@ impl<'a> VM<'a> {
.db
.access_account(&mut self.accrued_substate, address)?;

let account_code_length = account.code.len().into();

let current_call_frame = self.current_call_frame_mut()?;

current_call_frame.increase_consumed_gas(gas_cost::extcodesize(address_was_cold)?)?;

current_call_frame.stack.push(account.code.len().into())?;
current_call_frame.stack.push(account_code_length)?;

Ok(OpcodeResult::Continue { pc_increment: 1 })
}
Expand All @@ -282,7 +285,7 @@ impl<'a> VM<'a> {
.map_err(|_| VMError::VeryLargeNumber)?;
let current_memory_size = self.current_call_frame()?.memory.len();

let (account, address_was_cold) = self
let (_, address_was_cold) = self
.db
.access_account(&mut self.accrued_substate, address)?;

Expand All @@ -302,7 +305,7 @@ impl<'a> VM<'a> {

// If the bytecode is a delegation designation, it will copy the marker (0xef0100) || address.
// https://eips.ethereum.org/EIPS/eip-7702#delegation-designation
let bytecode = account.code;
let bytecode = &self.db.get_account(address)?.code;

let mut data = vec![0u8; size];
if offset < bytecode.len().into() {
Expand Down Expand Up @@ -398,21 +401,28 @@ impl<'a> VM<'a> {
pub fn op_extcodehash(&mut self) -> Result<OpcodeResult, VMError> {
let address = word_to_address(self.current_call_frame_mut()?.stack.pop()?);

let (account, address_was_cold) = self
.db
.access_account(&mut self.accrued_substate, address)?;

let account_is_empty;
let account_code_hash;
let address_was_cold;
{
let (account, was_cold) = self
.db
.access_account(&mut self.accrued_substate, address)?;
address_was_cold = was_cold;
account_is_empty = account.is_empty();
account_code_hash = account.info.code_hash.0;
}
Comment thread
SDartayet marked this conversation as resolved.
Outdated
let current_call_frame = self.current_call_frame_mut()?;

current_call_frame.increase_consumed_gas(gas_cost::extcodehash(address_was_cold)?)?;

// An account is considered empty when it has no code and zero nonce and zero balance. [EIP-161]
if account.is_empty() {
if account_is_empty {
current_call_frame.stack.push(U256::zero())?;
return Ok(OpcodeResult::Continue { pc_increment: 1 });
}

let hash = U256::from_big_endian(&account.info.code_hash.0);
let hash = U256::from_big_endian(&account_code_hash);
current_call_frame.stack.push(hash)?;

Ok(OpcodeResult::Continue { pc_increment: 1 })
Expand Down
Loading