From 4ce8474e02b3382bf8bf7a721fcb0c982d3d3bd1 Mon Sep 17 00:00:00 2001 From: Kien Nguyen Date: Fri, 1 Nov 2024 18:04:24 +0700 Subject: [PATCH 1/3] feat: fix nonce deadlock --- crates/pevm/src/vm.rs | 56 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 48 insertions(+), 8 deletions(-) diff --git a/crates/pevm/src/vm.rs b/crates/pevm/src/vm.rs index 19971252..fb0abb5b 100644 --- a/crates/pevm/src/vm.rs +++ b/crates/pevm/src/vm.rs @@ -1,6 +1,8 @@ +use std::cmp::Ordering; + use alloy_primitives::TxKind; use alloy_rpc_types::Receipt; -use hashbrown::HashMap; +use hashbrown::{hash_map::Entry, HashMap}; use revm::{ primitives::{ AccountInfo, Address, BlockEnv, Bytecode, CfgEnv, EVMError, Env, InvalidTransaction, @@ -370,19 +372,40 @@ impl<'a, S: Storage, C: PevmChain> Database for VmDb<'a, S, C> { *read_origins = new_origins; } + if location_hash == self.from_hash { + let mut current_parent: Option = + self.vm.parent_tx_idxs.get(&self.tx_idx).copied(); + for read_origin in read_origins.iter() { + match read_origin { + ReadOrigin::MvMemory(tx_version) => { + if let Some(parent) = current_parent { + match parent.cmp(&tx_version.tx_idx) { + Ordering::Less => {} + Ordering::Equal => { + current_parent = self.vm.parent_tx_idxs.get(&parent).copied(); + } + Ordering::Greater => { + return Err(ReadError::Blocking(parent)); + } + } + } + } + ReadOrigin::Storage => { + if let Some(parent) = current_parent { + return Err(ReadError::Blocking(parent)); + } + } + } + } + } + if let Some(mut account) = final_account { // Check sender nonce account.nonce += nonce_addition; if location_hash == self.from_hash && self.tx.nonce.is_some_and(|nonce| nonce != account.nonce) { - if self.tx_idx > 0 { - // TODO: Better retry strategy -- immediately, to the - // closest sender tx, to the missing sender tx, etc. - return Err(ReadError::Blocking(self.tx_idx - 1)); - } else { - return Err(ReadError::InvalidNonce(self.tx_idx)); - } + return Err(ReadError::InvalidNonce(self.tx_idx)); } // Fully evaluate the account and register it to read cache @@ -500,6 +523,7 @@ pub(crate) struct Vm<'a, S: Storage, C: PevmChain> { spec_id: SpecId, beneficiary_location_hash: MemoryLocationHash, reward_policy: RewardPolicy, + parent_tx_idxs: HashMap, } impl<'a, S: Storage, C: PevmChain> Vm<'a, S, C> { @@ -511,6 +535,21 @@ impl<'a, S: Storage, C: PevmChain> Vm<'a, S, C> { txs: &'a [TxEnv], spec_id: SpecId, ) -> Self { + let mut parent_tx_idxs = HashMap::new(); + let mut address_to_tx_idx = HashMap::new(); + + for (tx_idx, tx) in txs.iter().enumerate() { + match address_to_tx_idx.entry(tx.caller) { + Entry::Occupied(mut occupied_entry) => { + let old_value = occupied_entry.insert(tx_idx); + parent_tx_idxs.insert(tx_idx, old_value); + } + Entry::Vacant(vacant_entry) => { + vacant_entry.insert(tx_idx); + } + } + } + Self { storage, mv_memory, @@ -522,6 +561,7 @@ impl<'a, S: Storage, C: PevmChain> Vm<'a, S, C> { block_env.coinbase, )), reward_policy: chain.get_reward_policy(), + parent_tx_idxs, } } From 9590125c039476d8c503ff13092798b3a2a7c4b0 Mon Sep 17 00:00:00 2001 From: Kien Nguyen Date: Fri, 1 Nov 2024 18:04:24 +0700 Subject: [PATCH 2/3] feat: avoid HashSet for better performance --- crates/pevm/src/vm.rs | 71 +++++++++++++++++++++++++------------------ 1 file changed, 41 insertions(+), 30 deletions(-) diff --git a/crates/pevm/src/vm.rs b/crates/pevm/src/vm.rs index fb0abb5b..dd8fb76a 100644 --- a/crates/pevm/src/vm.rs +++ b/crates/pevm/src/vm.rs @@ -372,39 +372,47 @@ impl<'a, S: Storage, C: PevmChain> Database for VmDb<'a, S, C> { *read_origins = new_origins; } - if location_hash == self.from_hash { - let mut current_parent: Option = - self.vm.parent_tx_idxs.get(&self.tx_idx).copied(); - for read_origin in read_origins.iter() { - match read_origin { - ReadOrigin::MvMemory(tx_version) => { - if let Some(parent) = current_parent { - match parent.cmp(&tx_version.tx_idx) { - Ordering::Less => {} - Ordering::Equal => { - current_parent = self.vm.parent_tx_idxs.get(&parent).copied(); - } - Ordering::Greater => { - return Err(ReadError::Blocking(parent)); + if let Some(mut account) = final_account { + // Check sender nonce + account.nonce += nonce_addition; + if location_hash == self.from_hash + && self.tx.nonce.is_some_and(|nonce| nonce != account.nonce) + { + let get_parent = |tx_idx: TxIdx| { + let parent_tx_idx = unsafe { *self.vm.parent_tx_idxs.get_unchecked(tx_idx) }; + if parent_tx_idx == tx_idx { + None + } else { + Some(parent_tx_idx) + } + }; + + // BEGIN: nonce check + let mut current_parent: Option = get_parent(self.tx_idx); + for read_origin in read_origins.iter() { + match read_origin { + ReadOrigin::MvMemory(tx_version) => { + if let Some(parent) = current_parent { + match parent.cmp(&tx_version.tx_idx) { + Ordering::Less => {} + Ordering::Equal => { + current_parent = get_parent(parent); + } + Ordering::Greater => { + return Err(ReadError::Blocking(parent)); + } } } } - } - ReadOrigin::Storage => { - if let Some(parent) = current_parent { - return Err(ReadError::Blocking(parent)); + ReadOrigin::Storage => { + if let Some(parent) = current_parent { + return Err(ReadError::Blocking(parent)); + } } } } - } - } + // END: nonce check - if let Some(mut account) = final_account { - // Check sender nonce - account.nonce += nonce_addition; - if location_hash == self.from_hash - && self.tx.nonce.is_some_and(|nonce| nonce != account.nonce) - { return Err(ReadError::InvalidNonce(self.tx_idx)); } @@ -523,7 +531,7 @@ pub(crate) struct Vm<'a, S: Storage, C: PevmChain> { spec_id: SpecId, beneficiary_location_hash: MemoryLocationHash, reward_policy: RewardPolicy, - parent_tx_idxs: HashMap, + parent_tx_idxs: Vec, // parent_tx_idxs[i] = i if i does not have parent } impl<'a, S: Storage, C: PevmChain> Vm<'a, S, C> { @@ -535,14 +543,17 @@ impl<'a, S: Storage, C: PevmChain> Vm<'a, S, C> { txs: &'a [TxEnv], spec_id: SpecId, ) -> Self { - let mut parent_tx_idxs = HashMap::new(); - let mut address_to_tx_idx = HashMap::new(); + let mut parent_tx_idxs: Vec<_> = txs.iter().enumerate().map(|(index, _)| index).collect(); + let mut address_to_tx_idx = + HashMap::with_capacity_and_hasher(txs.len(), BuildSuffixHasher::default()); for (tx_idx, tx) in txs.iter().enumerate() { match address_to_tx_idx.entry(tx.caller) { Entry::Occupied(mut occupied_entry) => { let old_value = occupied_entry.insert(tx_idx); - parent_tx_idxs.insert(tx_idx, old_value); + unsafe { + *parent_tx_idxs.get_unchecked_mut(tx_idx) = old_value; + } } Entry::Vacant(vacant_entry) => { vacant_entry.insert(tx_idx); From 6f2860bbea0157bd15b623303df4dba128e0f63f Mon Sep 17 00:00:00 2001 From: Kien Nguyen Date: Fri, 1 Nov 2024 18:04:24 +0700 Subject: [PATCH 3/3] refactor: use HashMap::insert instead of HashMap::entry --- crates/pevm/src/vm.rs | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/crates/pevm/src/vm.rs b/crates/pevm/src/vm.rs index dd8fb76a..009b7fdc 100644 --- a/crates/pevm/src/vm.rs +++ b/crates/pevm/src/vm.rs @@ -2,7 +2,7 @@ use std::cmp::Ordering; use alloy_primitives::TxKind; use alloy_rpc_types::Receipt; -use hashbrown::{hash_map::Entry, HashMap}; +use hashbrown::HashMap; use revm::{ primitives::{ AccountInfo, Address, BlockEnv, Bytecode, CfgEnv, EVMError, Env, InvalidTransaction, @@ -548,16 +548,9 @@ impl<'a, S: Storage, C: PevmChain> Vm<'a, S, C> { HashMap::with_capacity_and_hasher(txs.len(), BuildSuffixHasher::default()); for (tx_idx, tx) in txs.iter().enumerate() { - match address_to_tx_idx.entry(tx.caller) { - Entry::Occupied(mut occupied_entry) => { - let old_value = occupied_entry.insert(tx_idx); - unsafe { - *parent_tx_idxs.get_unchecked_mut(tx_idx) = old_value; - } - } - Entry::Vacant(vacant_entry) => { - vacant_entry.insert(tx_idx); - } + // We use the fact that `insert` returns the previous value. + if let Some(parent_tx_idx) = address_to_tx_idx.insert(tx.caller, tx_idx) { + parent_tx_idxs[tx_idx] = parent_tx_idx; } }