fix: computer external_transaction metrics only after transaction is …

…persisted

src/eth/executor/evm.rs

@@ -6,8 +6,6 @@
 //! facilitates flexible EVM integrations, enabling the project to adapt to different blockchain environments
 //! or requirements while maintaining a consistent execution interface.
 
-use std::borrow::Cow;
-
 use display_json::DebugAsJson;
 
 use crate::eth::primitives::Address;
@@ -23,14 +21,11 @@ use crate::eth::primitives::ExternalReceipt;
 use crate::eth::primitives::ExternalTransaction;
 use crate::eth::primitives::Gas;
 use crate::eth::primitives::Nonce;
-use crate::eth::primitives::Signature;
-use crate::eth::primitives::SoliditySignature;
 use crate::eth::primitives::StratusError;
 use crate::eth::primitives::TransactionInput;
 use crate::eth::primitives::UnixTime;
 use crate::eth::primitives::Wei;
 use crate::eth::storage::StoragePointInTime;
-use crate::ext::not;
 use crate::ext::OptionExt;
 use crate::if_else;
 use crate::log_and_err;
@@ -121,18 +116,6 @@ pub struct EvmInput {
 }
 
 impl EvmInput {
-    fn is_contract_deployment(&self) -> bool {
-        self.to.is_none() && not(self.data.is_empty())
-    }
-
-    pub fn extract_function(&self) -> Option<SoliditySignature> {
-        if self.is_contract_deployment() {
-            return Some(Cow::from("contract_deployment"));
-        }
-        let sig = Signature::Function(self.data.get(..4)?.try_into().ok()?);
-        Some(sig.extract())
-    }
-
     /// Creates from a transaction that was sent directly to Stratus with `eth_sendRawTransaction`.
     pub fn from_eth_transaction(input: TransactionInput, pending_block_number: BlockNumber) -> Self {
         Self {

src/eth/executor/executor.rs

@@ -241,16 +241,13 @@ impl Executor {
         // determine how to execute each transaction
         for tx in &block.transactions {
             let receipt = receipts.try_get(&tx.hash())?;
-            let tx_execution = self.execute_external_transaction(tx, receipt, block)?;
-
-            // track block metrics
-            #[cfg(feature = "metrics")]
-            {
-                block_metrics += tx_execution.evm_execution.metrics;
-            }
-
-            // persist state
-            self.miner.save_execution(TransactionExecution::External(tx_execution))?;
+            self.execute_external_transaction(
+                tx,
+                receipt,
+                block,
+                #[cfg(feature = "metrics")]
+                &mut block_metrics,
+            )?;
         }
 
         // track block metrics
@@ -269,71 +266,88 @@ impl Executor {
     /// This function wraps `reexecute_external_tx_inner` and returns back the payload
     /// to facilitate re-execution of parallel transactions that failed
     #[tracing::instrument(name = "executor::external_transaction", skip_all, fields(tx_hash))]
-    fn execute_external_transaction<'a, 'b>(
-        &'a self,
-        tx: &'b ExternalTransaction,
-        receipt: &'b ExternalReceipt,
+    fn execute_external_transaction(
+        &self,
+        tx: &ExternalTransaction,
+        receipt: &ExternalReceipt,
         block: &ExternalBlock,
-    ) -> anyhow::Result<ExternalTransactionExecution> {
+        #[cfg(feature = "metrics")] block_metrics: &mut EvmExecutionMetrics,
+    ) -> anyhow::Result<()> {
+        #[cfg(feature = "metrics")]
+        let start = metrics::now();
+
+        // track
         Span::with(|s| {
             s.rec_str("tx_hash", &tx.hash);
         });
-
         tracing::info!(block_number = %block.number(), tx_hash = %tx.hash(), "reexecuting external transaction");
 
-        #[cfg(feature = "metrics")]
-        let start = metrics::now();
-
         // when transaction externally failed, create fake transaction instead of reexecuting
-        if receipt.is_failure() {
-            let sender = self.storage.read_account(&receipt.from.into(), &StoragePointInTime::Pending)?;
-            let execution = EvmExecution::from_failed_external_transaction(sender, receipt, block)?;
-            let evm_result = EvmExecutionResult {
-                execution,
-                metrics: EvmExecutionMetrics::default(),
-            };
-            return Ok(ExternalTransactionExecution::new(tx.clone(), receipt.clone(), evm_result));
-        }
+        let tx_execution = match receipt.is_success() {
+            //
+            // successful external transaction, re-execute locally
+            true => {
+                // re-execute transaction
+                let evm_input = EvmInput::from_external(tx, receipt, block)?;
+                let evm_execution = self.evms.execute(evm_input.clone(), EvmRoute::External);
+
+                // handle re-execution result
+                let mut evm_execution = match evm_execution {
+                    Ok(inner) => inner,
+                    Err(e) => {
+                        let json_tx = to_json_string(&tx);
+                        let json_receipt = to_json_string(&receipt);
+                        tracing::error!(reason = ?e, block_number = %block.number(), tx_hash = %tx.hash(), %json_tx, %json_receipt, "failed to reexecute external transaction");
+                        return Err(e.into());
+                    }
+                };
 
-        // re-execute transaction
-        let evm_input = EvmInput::from_external(tx, receipt, block)?;
-        let evm_execution = self.evms.execute(evm_input.clone(), EvmRoute::External);
-
-        // handle re-execution result
-        let mut evm_execution = match evm_execution {
-            Ok(inner) => inner,
-            Err(e) => {
-                let json_tx = to_json_string(&tx);
-                let json_receipt = to_json_string(&receipt);
-                tracing::error!(reason = ?e, block_number = %block.number(), tx_hash = %tx.hash(), %json_tx, %json_receipt, "failed to reexecute external transaction");
-                return Err(e.into());
+                // update execution with receipt
+                evm_execution.execution.apply_receipt(receipt)?;
+
+                // ensure it matches receipt before saving
+                if let Err(e) = evm_execution.execution.compare_with_receipt(receipt) {
+                    let json_tx = to_json_string(&tx);
+                    let json_receipt = to_json_string(&receipt);
+                    let json_execution_logs = to_json_string(&evm_execution.execution.logs);
+                    tracing::error!(reason = %"mismatch reexecuting transaction", block_number = %block.number(), tx_hash = %tx.hash(), %json_tx, %json_receipt, %json_execution_logs, "failed to reexecute external transaction");
+                    return Err(e);
+                };
+
+                ExternalTransactionExecution::new(tx.clone(), receipt.clone(), evm_execution)
+            }
+            //
+            // failed external transaction, re-cretea from receipt without re-executing
+            false => {
+                let sender = self.storage.read_account(&receipt.from.into(), &StoragePointInTime::Pending)?;
+                let execution = EvmExecution::from_failed_external_transaction(sender, receipt, block)?;
+                let evm_result = EvmExecutionResult {
+                    execution,
+                    metrics: EvmExecutionMetrics::default(),
+                };
+                ExternalTransactionExecution::new(tx.clone(), receipt.clone(), evm_result)
             }
         };
 
+        // persist state
+        let tx_execution = TransactionExecution::External(tx_execution);
+        self.miner.save_execution(tx_execution.clone())?;
+
         // track metrics
-        // FIX: these metrics are wrong because they are computed before saving the execution
         #[cfg(feature = "metrics")]
         {
-            let function = evm_input.extract_function();
+            let evm_execution = tx_execution.execution();
+            let evm_metrics = tx_execution.metrics();
+            *block_metrics += *evm_metrics;
+
+            let function = tx.solidity_signature();
             metrics::inc_executor_external_transaction(start.elapsed(), function.clone());
-            metrics::inc_executor_external_transaction_account_reads(evm_execution.metrics.account_reads, function.clone());
-            metrics::inc_executor_external_transaction_slot_reads(evm_execution.metrics.slot_reads, function.clone());
-            metrics::inc_executor_external_transaction_gas(evm_execution.execution.gas.as_u64() as usize, function);
+            metrics::inc_executor_external_transaction_account_reads(evm_metrics.account_reads, function.clone());
+            metrics::inc_executor_external_transaction_slot_reads(evm_metrics.slot_reads, function.clone());
+            metrics::inc_executor_external_transaction_gas(evm_execution.gas.as_u64() as usize, function);
         }
 
-        // update execution with receipt
-        evm_execution.execution.apply_receipt(receipt)?;
-
-        // ensure it matches receipt before saving
-        if let Err(e) = evm_execution.execution.compare_with_receipt(receipt) {
-            let json_tx = to_json_string(&tx);
-            let json_receipt = to_json_string(&receipt);
-            let json_execution_logs = to_json_string(&evm_execution.execution.logs);
-            tracing::error!(reason = %"mismatch reexecuting transaction", block_number = %block.number(), tx_hash = %tx.hash(), %json_tx, %json_receipt, %json_execution_logs, "failed to reexecute external transaction");
-            return Err(e);
-        };
-
-        Ok(ExternalTransactionExecution::new(tx.clone(), receipt.clone(), evm_execution))
+        Ok(())
     }
 
     // -------------------------------------------------------------------------
@@ -409,7 +423,7 @@ impl Executor {
         // track metrics
         #[cfg(feature = "metrics")]
         {
-            let function = tx_input.extract_function();
+            let function = tx_input.solidity_signature();
             match &tx_execution {
                 Ok(tx_execution) => {
                     metrics::inc_executor_local_transaction(start.elapsed(), true, function.clone());
@@ -537,7 +551,7 @@ impl Executor {
 
         #[cfg(feature = "metrics")]
         {
-            let function = call_input.extract_function();
+            let function = call_input.solidity_signature();
             match &evm_result {
                 Ok(evm_result) => {
                     metrics::inc_executor_local_call(start.elapsed(), true, function.clone());

src/eth/primitives/call_input.rs

@@ -17,7 +17,10 @@ pub struct CallInput {
 }
 
 impl CallInput {
-    pub fn extract_function(&self) -> Option<SoliditySignature> {
+    /// Parses the Solidity function being called.
+    ///
+    /// TODO: unify and remove duplicate implementations.
+    pub fn solidity_signature(&self) -> Option<SoliditySignature> {
         let sig = Signature::Function(self.data.get(..4)?.try_into().ok()?);
         Some(sig.extract())
     }

src/eth/primitives/external_transaction.rs

@@ -1,7 +1,12 @@
+use std::borrow::Cow;
+
 use ethers_core::types::Transaction as EthersTransaction;
 
 use crate::eth::primitives::BlockNumber;
 use crate::eth::primitives::Hash;
+use crate::eth::primitives::Signature;
+use crate::eth::primitives::SoliditySignature;
+use crate::ext::not;
 
 #[derive(Debug, Clone, Default, derive_more:: Deref, serde::Deserialize, serde::Serialize)]
 #[serde(transparent)]
@@ -17,6 +22,22 @@ impl ExternalTransaction {
     pub fn hash(&self) -> Hash {
         self.0.hash.into()
     }
+
+    /// Checks if the current transaction is a contract deployment.
+    pub fn is_contract_deployment(&self) -> bool {
+        self.to.is_none() && not(self.input.is_empty())
+    }
+
+    /// Parses the Solidity function being called.
+    ///
+    /// TODO: unify and remove duplicate implementations.
+    pub fn solidity_signature(&self) -> Option<SoliditySignature> {
+        if self.is_contract_deployment() {
+            return Some(Cow::from("contract_deployment"));
+        }
+        let sig = Signature::Function(self.input.get(..4)?.try_into().ok()?);
+        Some(sig.extract())
+    }
 }
 
 // -----------------------------------------------------------------------------

src/eth/primitives/transaction_input.rs

@@ -54,12 +54,15 @@ pub struct TransactionInput {
 }
 
 impl TransactionInput {
-    /// Checks if the current transaction is for a contract deployment.
+    /// Checks if the current transaction is a contract deployment.
     pub fn is_contract_deployment(&self) -> bool {
         self.to.is_none() && not(self.input.is_empty())
     }
 
-    pub fn extract_function(&self) -> Option<SoliditySignature> {
+    /// Parses the Solidity function being called.
+    ///
+    /// TODO: unify and remove duplicate implementations.
+    pub fn solidity_signature(&self) -> Option<SoliditySignature> {
         if self.is_contract_deployment() {
             return Some(Cow::from("contract_deployment"));
         }

src/eth/rpc/rpc_middleware.rs

@@ -303,7 +303,7 @@ impl TransactionTracingIdentifiers {
         let tx = parse_rpc_rlp::<TransactionInput>(&data)?;
         Ok(Self {
             hash: Some(tx.hash),
-            function: tx.extract_function(),
+            function: tx.solidity_signature(),
             from: Some(tx.signer),
             to: tx.to,
             nonce: Some(tx.nonce),
@@ -314,7 +314,7 @@ impl TransactionTracingIdentifiers {
         let (_, call) = next_rpc_param::<CallInput>(params.sequence())?;
         Ok(Self {
             hash: None,
-            function: call.extract_function(),
+            function: call.solidity_signature(),
             from: call.from,
             to: call.to,
             nonce: None,