perf: remove split_transactions (#1642)

src/eth/executor/evm_input.rs

@@ -91,7 +91,7 @@ impl EvmInput {
             gas_price: Wei::ZERO,
             nonce: Some(input.nonce),
             block_number: pending_block_number,
-            block_timestamp: UnixTime::now(),
+            block_timestamp: UnixTime::now(), // TODO: this should come from the pending block
             point_in_time: StoragePointInTime::Pending,
             chain_id: input.chain_id,
         }

src/eth/miner/miner.rs

@@ -4,7 +4,6 @@ use std::sync::Mutex;
 
 use itertools::Itertools;
 use keccak_hasher::KeccakHasher;
-use nonempty::NonEmpty;
 use tokio::sync::broadcast;
 use tracing::field;
 use tracing::info_span;
@@ -24,6 +23,7 @@ use crate::eth::primitives::Size;
 use crate::eth::primitives::StratusError;
 use crate::eth::primitives::TransactionExecution;
 use crate::eth::primitives::TransactionMined;
+use crate::eth::primitives::UnixTime;
 use crate::eth::storage::StratusStorage;
 use crate::ext::not;
 use crate::ext::spawn_thread;
@@ -136,33 +136,29 @@ impl Miner {
         // track
         #[cfg(feature = "tracing")]
         let _span = info_span!("miner::mine_external", block_number = field::Empty).entered();
-        tracing::debug!("mining external block");
 
         // lock
         let _mine_lock = self.locks.mine.lock().unwrap();
 
-        // mine
+        // mine block
         let block = self.storage.finish_pending_block()?;
-        let (local_txs, external_txs) = block.split_transactions();
-
-        // validate
+        Span::with(|s| s.rec_str("block_number", &block.number));
         let Some(external_block) = block.external_block else {
             return log_and_err!("failed to mine external block because there is no external block being reexecuted");
         };
-        if not(local_txs.is_empty()) {
-            return log_and_err!("failed to mine external block because one of the transactions is a local transaction");
-        }
 
-        // mine external transactions
-        let mined_txs = mine_external_transactions(block.number, external_txs)?;
-        let block = block_from_external(external_block, mined_txs);
-
-        #[cfg(feature = "tracing")]
-        if let Ok(ref block) = block {
-            Span::with(|s| s.rec_str("block_number", &block.number()));
+        // mine transactions
+        let mut external_txs = Vec::with_capacity(block.transactions.len());
+        for tx in block.transactions.into_values() {
+            if let TransactionExecution::External(tx) = tx {
+                external_txs.push(tx);
+            } else {
+                return log_and_err!("failed to mine external block because one of the transactions is not an external transaction");
+            }
         }
+        let mined_external_txs = mine_external_transactions(block.number, external_txs)?;
 
-        block
+        block_from_external(external_block, mined_external_txs)
     }
 
     /// Same as [`Self::mine_local`], but automatically commits the block instead of returning it.
@@ -177,29 +173,28 @@ impl Miner {
     /// Mines local transactions.
     ///
     /// External transactions are not allowed to be part of the block.
-    #[tracing::instrument(name = "miner::mine_local", skip_all, fields(block_number))]
     pub fn mine_local(&self) -> anyhow::Result<Block> {
-        tracing::debug!("mining local block");
+        #[cfg(feature = "tracing")]
+        let _span = info_span!("miner::mine_local", block_number = field::Empty).entered();
 
         // lock
         let _mine_lock = self.locks.mine.lock().unwrap();
 
-        // mine
+        // mine block
         let block = self.storage.finish_pending_block()?;
-        let (local_txs, external_txs) = block.split_transactions();
-
-        // validate
-        if not(external_txs.is_empty()) {
-            return log_and_err!("failed to mine local block because one of the transactions is an external transaction");
+        Span::with(|s| s.rec_str("block_number", &block.number));
+
+        // mine transactions
+        let mut local_txs = Vec::with_capacity(block.transactions.len());
+        for tx in block.transactions.into_values() {
+            if let TransactionExecution::Local(tx) = tx {
+                local_txs.push(tx);
+            } else {
+                return log_and_err!("failed to mine local block because one of the transactions is not a local transaction");
+            }
         }
 
-        // mine local transactions
-        let block = match NonEmpty::from_vec(local_txs) {
-            Some(local_txs) => block_from_local(block.number, local_txs),
-            None => Ok(Block::new_at_now(block.number)),
-        };
-
-        block.inspect(|block| Span::with(|s| s.rec_str("block_number", &block.number())))
+        block_from_local(block.number, local_txs)
     }
 
     /// Persists a mined block to permanent storage and prepares new block.
@@ -267,13 +262,12 @@ fn block_from_external(external_block: ExternalBlock, mined_txs: Vec<Transaction
     })
 }
 
-pub fn block_from_local(number: BlockNumber, txs: NonEmpty<LocalTransactionExecution>) -> anyhow::Result<Block> {
-    // init block
-    let block_timestamp = txs
-        .minimum_by(|tx1, tx2| tx1.result.execution.block_timestamp.cmp(&tx2.result.execution.block_timestamp))
-        .result
-        .execution
-        .block_timestamp;
+pub fn block_from_local(number: BlockNumber, txs: Vec<LocalTransactionExecution>) -> anyhow::Result<Block> {
+    // TODO: block timestamp should be set in the PendingBlock instead of being retrieved from the execution
+    let block_timestamp = match txs.first() {
+        Some(tx) => tx.result.execution.block_timestamp,
+        None => UnixTime::now(),
+    };
 
     let mut block = Block::new(number, block_timestamp);
     block.transactions.reserve(txs.len());

src/eth/primitives/block.rs

@@ -29,11 +29,6 @@ pub struct Block {
 }
 
 impl Block {
-    /// Creates a new block with the given number assuming the current system timestamp as the block timestamp.
-    pub fn new_at_now(number: BlockNumber) -> Self {
-        Self::new(number, UnixTime::now())
-    }
-
     /// Creates a new block with the given number and timestamp.
     pub fn new(number: BlockNumber, timestamp: UnixTime) -> Self {
         Self {

src/eth/primitives/pending_block.rs

@@ -3,16 +3,14 @@ use indexmap::IndexMap;
 
 use crate::eth::primitives::BlockNumber;
 use crate::eth::primitives::ExternalBlock;
-use crate::eth::primitives::ExternalTransactionExecution;
 use crate::eth::primitives::Hash;
-use crate::eth::primitives::LocalTransactionExecution;
 use crate::eth::primitives::TransactionExecution;
 
 /// Block that is being mined and receiving updates.
 #[derive(DebugAsJson, Clone, Default, serde::Serialize)]
 pub struct PendingBlock {
     pub number: BlockNumber,
-    pub tx_executions: IndexMap<Hash, TransactionExecution>,
+    pub transactions: IndexMap<Hash, TransactionExecution>,
     pub external_block: Option<ExternalBlock>,
 }
 
@@ -24,24 +22,6 @@ impl PendingBlock {
 
     /// Adds a transaction execution to the block.
     pub fn push_transaction(&mut self, tx: TransactionExecution) {
-        self.tx_executions.insert(tx.hash(), tx);
-    }
-
-    /// Splits transactions executions in local and external executions.
-    pub fn split_transactions(&self) -> (Vec<LocalTransactionExecution>, Vec<ExternalTransactionExecution>) {
-        let mut local_txs = Vec::with_capacity(self.tx_executions.len());
-        let mut external_txs = Vec::with_capacity(self.tx_executions.len());
-
-        for tx in self.tx_executions.values().cloned() {
-            match tx {
-                TransactionExecution::Local(tx) => {
-                    local_txs.push(tx);
-                }
-                TransactionExecution::External(tx) => {
-                    external_txs.push(tx);
-                }
-            }
-        }
-        (local_txs, external_txs)
+        self.transactions.insert(tx.hash(), tx);
     }
 }

src/eth/storage/inmemory/inmemory_temporary.rs

@@ -193,7 +193,7 @@ impl TemporaryStorage for InMemoryTemporaryStorage {
     fn pending_transactions(&self) -> anyhow::Result<Vec<TransactionExecution>> {
         let states = self.lock_read();
         let Some(ref pending_block) = states.head.block else { return Ok(vec![]) };
-        Ok(pending_block.tx_executions.clone().into_iter().map(|(_, tx)| tx.clone()).collect())
+        Ok(pending_block.transactions.clone().into_iter().map(|(_, tx)| tx.clone()).collect())
     }
 
     /// TODO: we cannot allow more than one pending block. Where to put this check?
@@ -216,7 +216,7 @@ impl TemporaryStorage for InMemoryTemporaryStorage {
     fn read_transaction(&self, hash: &Hash) -> anyhow::Result<Option<TransactionExecution>> {
         let states = self.lock_read();
         let Some(ref pending_block) = states.head.block else { return Ok(None) };
-        match pending_block.tx_executions.get(hash) {
+        match pending_block.transactions.get(hash) {
             Some(tx) => Ok(Some(tx.clone())),
             None => Ok(None),
         }