fix: re execute txs instead of simulate for txn receipts (#1577)

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 ## Next release
 
+- fix: re-execute txs instead of simulating for txn receipts
 - chore: rebase on latest blockifier
 - refactoring : Removed Redundant logs in madara
 - fix: transaction receipt fails for txs in the middle of a block

crates/client/rpc/src/lib.rs

@@ -16,7 +16,7 @@ use std::marker::PhantomData;
 use std::sync::Arc;
 
 use blockifier::transaction::account_transaction::AccountTransaction;
-use blockifier::transaction::objects::ResourcesMapping;
+use blockifier::transaction::objects::{ResourcesMapping, TransactionExecutionInfo};
 use blockifier::transaction::transactions::L1HandlerTransaction;
 use errors::StarknetRpcApiError;
 use jsonrpsee::core::{async_trait, RpcResult};
@@ -28,6 +28,7 @@ pub use mc_rpc_core::{
     StarknetWriteRpcApiServer,
 };
 use mc_storage::OverrideHandle;
+use mp_block::BlockTransactions;
 use mp_felt::Felt252Wrapper;
 use mp_hashers::HasherT;
 use mp_simulations::SimulationFlags;
@@ -1483,9 +1484,6 @@ where
 
         let messages_sent = messages.into_iter().map(starknet_api_to_starknet_core_message_to_l1).collect();
 
-        // TODO: use re_execute_transaction in order to rebuild the correct state and not have to skip the
-        // validation phase. It will return more accurate fees
-        let skip_validate = true;
         let parent_block_hash = self
             .substrate_block_hash_from_starknet_block(BlockId::Hash(
                 Felt252Wrapper::from(block_header.parent_block_hash).into(),
@@ -1494,8 +1492,9 @@ where
                 error!("Parent Block not found: {e}");
                 StarknetRpcApiError::BlockNotFound
             })?;
-        let simulation = self.simulate_tx(parent_block_hash, transaction.clone(), skip_validate, fee_disabled)?;
-        let execution_resources = actual_resources_to_execution_resources(simulation.actual_resources);
+        let execution_info =
+            self.get_transaction_execution_info(parent_block_hash, starknet_block.transactions(), transaction_hash)?;
+        let execution_resources = actual_resources_to_execution_resources(execution_info.actual_resources);
         let transaction_hash = Felt252Wrapper::from(transaction_hash).into();
 
         let receipt = match transaction {
@@ -1563,6 +1562,42 @@ where
         Ok(MaybePendingTransactionReceipt::Receipt(receipt))
     }
 
+    fn get_transaction_execution_info(
+        &self,
+        parent_substrate_block_hash: B::Hash,
+        block_transactions: &BlockTransactions,
+        transaction_hash: TransactionHash,
+    ) -> Result<TransactionExecutionInfo, StarknetRpcApiError>
+    where
+        B: BlockT,
+    {
+        let (transactions_before, transaction_to_trace) =
+            split_block_tx_for_reexecution(block_transactions, transaction_hash).map_err(|e| {
+                log::error!("Failed to split block transactions for re-execution: {e}");
+                StarknetRpcApiError::InternalServerError
+            })?;
+
+        if transaction_to_trace.is_empty() {
+            return Err(StarknetRpcApiError::TxnHashNotFound);
+        }
+
+        if transaction_to_trace.len() > 1 {
+            log::error!("More than one transaction with the same transaction hash {:#?}", transaction_to_trace);
+            return Err(StarknetRpcApiError::InternalServerError);
+        }
+
+        let mut trace = self
+            .re_execute_transactions(parent_substrate_block_hash, transactions_before, transaction_to_trace, false)
+            .map_err(|e| {
+                log::error!("Failed to re-execute transactions: {e}");
+                StarknetRpcApiError::InternalServerError
+            })?;
+
+        let execution_info = trace.remove(0);
+
+        Ok(execution_info.0)
+    }
+
     fn get_events_for_tx_by_hash(
         &self,
         substrate_block_hash: B::Hash,
@@ -1607,11 +1642,10 @@ where
         let messages_sent = Vec::new();
         let events = Vec::new();
 
-        // TODO -- should Tx be simulated with `skip_validate`?
-        let skip_validate = true;
-        let skip_fee_charge = self.is_transaction_fee_disabled(self.get_best_block_hash())?;
+        let parent_substrate_block_hash = self.get_best_block_hash();
+        let pending_txs = self.get_pending_txs(parent_substrate_block_hash)?;
         let simulation =
-            self.simulate_tx(self.get_best_block_hash(), pending_tx.clone(), skip_validate, skip_fee_charge)?;
+            self.get_transaction_execution_info(parent_substrate_block_hash, &pending_txs, transaction_hash)?;
         let actual_fee =
             FeePayment { amount: Felt252Wrapper::from(simulation.actual_fee.0).into(), unit: PriceUnit::Wei };
         let execution_result = revert_error_to_execution_result(simulation.revert_error);
@@ -1767,13 +1801,12 @@ fn actual_resources_to_execution_resources(resources: ResourcesMapping) -> Execu
 }
 
 fn split_block_tx_for_reexecution(
-    block: &mp_block::Block,
+    block_transactions: &BlockTransactions,
     transaction_hash: TransactionHash,
 ) -> RpcResult<(
     Vec<blockifier::transaction::transaction_execution::Transaction>,
     Vec<blockifier::transaction::transaction_execution::Transaction>,
 )> {
-    let block_transactions = block.transactions();
     let tx_to_trace_idx = block_transactions
         .iter()
         .rposition(|tx| get_transaction_hash(tx) == &transaction_hash)

crates/client/rpc/src/trace_api.rs

@@ -123,8 +123,14 @@ where
 
         let previous_block_substrate_hash = get_previous_block_substrate_hash(self, substrate_block_hash)?;
 
-        let execution_infos =
-            self.re_execute_transactions(previous_block_substrate_hash, vec![], block_transactions.clone())?;
+        let execution_infos = self
+            .re_execute_transactions(previous_block_substrate_hash, vec![], block_transactions.clone(), true)?
+            .into_iter()
+            .map(|(e, c)| match c {
+                Some(c) => Ok((e, c)),
+                None => Err(StarknetRpcApiError::InternalServerError),
+            })
+            .collect::<Result<Vec<(TransactionExecutionInfo, CommitmentStateDiff)>, StarknetRpcApiError>>()?;
 
         let traces = Self::execution_info_to_transaction_trace(execution_infos, block_transactions)?;
 
@@ -146,17 +152,23 @@ where
 
         let starknet_block = get_block_by_block_hash(self.client.as_ref(), substrate_block_hash)?;
 
-        let (txs_before, tx_to_trace) = super::split_block_tx_for_reexecution(&starknet_block, transaction_hash)?;
+        let (txs_before, tx_to_trace) =
+            super::split_block_tx_for_reexecution(starknet_block.transactions(), transaction_hash)?;
         let tx_type = TxType::from(&tx_to_trace[0]);
 
         let previous_block_substrate_hash = get_previous_block_substrate_hash(self, substrate_block_hash)?;
 
         let (execution_infos, commitment_state_diff) = self
-            .re_execute_transactions(previous_block_substrate_hash, txs_before, tx_to_trace)?
+            .re_execute_transactions(previous_block_substrate_hash, txs_before, tx_to_trace, true)?
             .into_iter()
             .next()
             .unwrap();
 
+        let commitment_state_diff = commitment_state_diff.ok_or_else(|| {
+            error!("Failed to get CommitmentStateDiff for transaction {transaction_hash}");
+            StarknetRpcApiError::InternalServerError
+        })?;
+
         let state_diff = blockifier_to_rpc_state_diff_types(commitment_state_diff.clone())
             .map_err(|_| StarknetRpcApiError::from(ConvertCallInfoToExecuteInvocationError::ConvertStateDiffFailed))?;
 
@@ -184,14 +196,16 @@ where
         previous_block_substrate_hash: B::Hash,
         transactions_before: Vec<Transaction>,
         transactions_to_trace: Vec<Transaction>,
-    ) -> RpcResult<Vec<(TransactionExecutionInfo, CommitmentStateDiff)>> {
+        with_state_diff: bool,
+    ) -> RpcResult<Vec<(TransactionExecutionInfo, Option<CommitmentStateDiff>)>> {
         Ok(self
             .client
             .runtime_api()
             .re_execute_transactions(
                 previous_block_substrate_hash,
                 transactions_before.clone(),
                 transactions_to_trace.clone(),
+                with_state_diff,
             )
             .map_err(|e| {
                 error!("Failed to execute runtime API call: {e}");

crates/pallets/starknet/runtime_api/src/lib.rs

@@ -30,6 +30,11 @@ pub enum StarknetTransactionExecutionError {
     ContractError,
 }
 
+type ReExecutionResult = Result<
+    Vec<(TransactionExecutionInfo, Option<CommitmentStateDiff>)>,
+    PlaceHolderErrorTypeForFailedStarknetExecution,
+>;
+
 sp_api::decl_runtime_apis! {
     pub trait StarknetRuntimeApi {
         /// Returns the nonce associated with the given address in the given block
@@ -78,7 +83,7 @@ sp_api::decl_runtime_apis! {
         ///
         /// Idealy, the execution traces of all of `transactions_to_trace`.
         /// If any of the transactions (from both arguments) fails, an error is returned.
-        fn re_execute_transactions(transactions_before: Vec<Transaction>, transactions_to_trace: Vec<Transaction>) -> Result<Result<Vec<(TransactionExecutionInfo, CommitmentStateDiff)>, PlaceHolderErrorTypeForFailedStarknetExecution>, DispatchError>;
+        fn re_execute_transactions(transactions_before: Vec<Transaction>, transactions_to_trace: Vec<Transaction>, with_state_diff: bool) -> Result<ReExecutionResult, DispatchError>;
 
         fn get_index_and_tx_for_tx_hash(xts: Vec<<Block as BlockT>::Extrinsic>, tx_hash: TransactionHash) -> Option<(u32, Transaction)>;
 

crates/pallets/starknet/src/simulations.rs

@@ -19,6 +19,11 @@ use starknet_api::transaction::TransactionVersion;
 use crate::blockifier_state_adapter::BlockifierStateAdapter;
 use crate::{Config, Error, Pallet};
 
+type ReExecutionResult = Result<
+    Vec<(TransactionExecutionInfo, Option<CommitmentStateDiff>)>,
+    PlaceHolderErrorTypeForFailedStarknetExecution,
+>;
+
 impl<T: Config> Pallet<T> {
     pub fn estimate_fee(
         transactions: Vec<AccountTransaction>,
@@ -178,12 +183,10 @@ impl<T: Config> Pallet<T> {
     pub fn re_execute_transactions(
         transactions_before: Vec<Transaction>,
         transactions_to_trace: Vec<Transaction>,
-    ) -> Result<
-        Result<Vec<(TransactionExecutionInfo, CommitmentStateDiff)>, PlaceHolderErrorTypeForFailedStarknetExecution>,
-        DispatchError,
-    > {
+        with_state_diff: bool,
+    ) -> Result<ReExecutionResult, DispatchError> {
         storage::transactional::with_transaction(|| {
-            let res = Self::re_execute_transactions_inner(transactions_before, transactions_to_trace);
+            let res = Self::re_execute_transactions_inner(transactions_before, transactions_to_trace, with_state_diff);
             storage::TransactionOutcome::Rollback(Result::<_, DispatchError>::Ok(Ok(res)))
         })
         .map_err(|_| Error::<T>::FailedToCreateATransactionalStorageExecution)?
@@ -192,8 +195,11 @@ impl<T: Config> Pallet<T> {
     fn re_execute_transactions_inner(
         transactions_before: Vec<Transaction>,
         transactions_to_trace: Vec<Transaction>,
-    ) -> Result<Vec<(TransactionExecutionInfo, CommitmentStateDiff)>, PlaceHolderErrorTypeForFailedStarknetExecution>
-    {
+        with_state_diff: bool,
+    ) -> Result<
+        Vec<(TransactionExecutionInfo, Option<CommitmentStateDiff>)>,
+        PlaceHolderErrorTypeForFailedStarknetExecution,
+    > {
         let block_context = Self::get_block_context();
         let mut state = BlockifierStateAdapter::<T>::default();
 
@@ -222,7 +228,8 @@ impl<T: Config> Pallet<T> {
                     PlaceHolderErrorTypeForFailedStarknetExecution
                 });
 
-                let res = res.map(|r| (r, transactional_state.to_state_diff()));
+                let res = res
+                    .map(|r| if with_state_diff { (r, Some(transactional_state.to_state_diff())) } else { (r, None) });
                 commit_transactional_state(transactional_state).map_err(|e| {
                     log::error!("Failed to commit state changes: {:?}", e);
                     PlaceHolderErrorTypeForFailedStarknetExecution

crates/pallets/starknet/src/tests/re_execute_transactions.rs

@@ -30,7 +30,7 @@ fn re_execute_tx_ok() {
                 .into();
 
         // Call the function we want to test
-        let res = Starknet::re_execute_transactions(txs_to_ignore, txs.clone()).unwrap().unwrap();
+        let res = Starknet::re_execute_transactions(txs_to_ignore, txs.clone(), false).unwrap().unwrap();
 
         // Storage changes have been reverted
         assert_eq!(Starknet::nonce(invoke_sender_address), Nonce(Felt252Wrapper::ZERO.into()));
@@ -59,7 +59,7 @@ fn re_execute_tx_with_a_transfer_ok() {
         )));
 
         // Call the function we want to test
-        let res = Starknet::re_execute_transactions(txs.clone(), vec![transfer_tx.clone()]).unwrap().unwrap();
+        let res = Starknet::re_execute_transactions(txs.clone(), vec![transfer_tx.clone()], false).unwrap().unwrap();
 
         // Storage changes have been reverted
         assert_eq!(Starknet::nonce(invoke_sender_address), Nonce(Felt252Wrapper::ZERO.into()));

crates/runtime/src/lib.rs

@@ -282,8 +282,8 @@ impl_runtime_apis! {
             Starknet::estimate_fee(transactions, &simulation_flags)
         }
 
-        fn re_execute_transactions(transactions_before: Vec<Transaction>, transactions_to_trace: Vec<Transaction>) -> Result<Result<Vec<(TransactionExecutionInfo, CommitmentStateDiff)>, PlaceHolderErrorTypeForFailedStarknetExecution>, DispatchError> {
-            Starknet::re_execute_transactions(transactions_before, transactions_to_trace)
+        fn re_execute_transactions(transactions_before: Vec<Transaction>, transactions_to_trace: Vec<Transaction>, with_state_diff: bool) -> Result<Result<Vec<(TransactionExecutionInfo, Option<CommitmentStateDiff>)>, PlaceHolderErrorTypeForFailedStarknetExecution>, DispatchError> {
+            Starknet::re_execute_transactions(transactions_before, transactions_to_trace, with_state_diff)
         }
 
         fn estimate_message_fee(message: L1HandlerTransaction) -> Result<(u128, u128, u128), DispatchError> {

starknet-rpc-test/get_transaction_receipt.rs

@@ -148,9 +148,7 @@ async fn work_with_pending_invoke_transaction(madara: &ThreadSafeMadaraClient) -
                 match final_receipt {
                     MaybePendingTransactionReceipt::Receipt(TransactionReceipt::Invoke(final_receipt)) => {
                         assert_eq!(receipt.transaction_hash, final_receipt.transaction_hash);
-                        // For pending receipt we are skiping the validation step, otherwise the simulation of tx may
-                        // fail, meaning the cost will always be lower than the actual ones
-                        assert!(receipt.actual_fee.amount < final_receipt.actual_fee.amount);
+                        assert_eq!(receipt.actual_fee.amount, final_receipt.actual_fee.amount);
                         // TODO: it's possible to add events and messages in the receipt right now but it makes more
                         // sense to have it once we've pending blocks in Substrate (which Massa labs is working on)
                         // assert_eq_msg_to_l1(receipt.messages_sent, final_receipt.messages_sent);