use trace instead of simulate to get txn receipt (keep-starknet-stran…

…ge#1559)

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 ## Next release
 
+- fix: transaction receipt fails for txs in the middle of a block
 - chore: add makefile for developer experience improvements and cleanup
 - fix: fix cargo-lint issues
 - feat: added chain-id to the GenesisConfig in pallet-starknet

crates/client/rpc/src/lib.rs

@@ -15,7 +15,7 @@ use std::collections::HashMap;
 use std::marker::PhantomData;
 use std::sync::Arc;
 
-use blockifier::transaction::objects::ResourcesMapping;
+use blockifier::transaction::objects::{ResourcesMapping, TransactionExecutionInfo};
 use errors::StarknetRpcApiError;
 use jsonrpsee::core::{async_trait, RpcResult};
 use jsonrpsee::types::error::CallError;
@@ -27,6 +27,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_transactions::compute_hash::ComputeTransactionHash;
@@ -64,6 +65,7 @@ use starknet_core::types::{
 use starknet_core::utils::get_selector_from_name;
 
 use crate::constants::{MAX_EVENTS_CHUNK_SIZE, MAX_EVENTS_KEYS};
+use crate::trace_api::map_transaction_to_user_transaction;
 use crate::types::RpcEventFilter;
 
 /// A Starknet RPC server for Madara
@@ -1599,16 +1601,20 @@ where
 
         // TODO
         // Is any better way to get execution resources of processed tx?
-        let skip_validate = true;
-        // let parent_block_hash = Felt252Wrapper::from().into();
-        let parent_block_hash = self
+        let parent_substrate_block_hash = self
             .substrate_block_hash_from_starknet_block(BlockId::Hash(block_header.parent_block_hash.into()))
             .map_err(|e| {
                 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_substrate_block_hash,
+            starknet_block.transactions(),
+            chain_id,
+            transaction_hash,
+        )?;
+
+        let execution_resources = actual_resources_to_execution_resources(execution_info.actual_resources);
 
         let receipt = match transaction {
             mp_transactions::Transaction::Declare(_, _) => TransactionReceipt::Declare(DeclareTransactionReceipt {
@@ -1687,14 +1693,10 @@ where
         &self,
         chain_id: Felt252Wrapper,
         tx_hash: FieldElement,
+        pending_txs: &[mp_transactions::Transaction],
     ) -> Result<Option<mp_transactions::Transaction>, StarknetRpcApiError> {
-        let latest_block = self.get_best_block_hash();
-
-        let pending_tx = self
-            .get_pending_txs(latest_block)?
-            .iter()
-            .find(|&tx| tx.compute_hash::<H>(chain_id.0.into(), false).0 == tx_hash)
-            .cloned();
+        let pending_tx =
+            pending_txs.iter().find(|&tx| tx.compute_hash::<H>(chain_id.0.into(), false).0 == tx_hash).cloned();
 
         Ok(pending_tx)
     }
@@ -1704,22 +1706,26 @@ where
         chain_id: Felt252Wrapper,
         transaction_hash: FieldElement,
     ) -> Result<MaybePendingTransactionReceipt, StarknetRpcApiError> {
-        let pending_tx =
-            self.find_pending_tx(chain_id, transaction_hash)?.ok_or(StarknetRpcApiError::TxnHashNotFound)?;
+        let parent_substrate_block_hash = self.get_best_block_hash();
+        let pending_txs = self.get_pending_txs(parent_substrate_block_hash)?;
+        let pending_tx = self
+            .find_pending_tx(chain_id, transaction_hash, &pending_txs)?
+            .ok_or(StarknetRpcApiError::TxnHashNotFound)?;
 
-        // TODO -- no way of getting messages sent to L1 for the tx
+        // TODO: Massa labs is working on pending blocks within Substrate. That will allow fetching
+        // events and messages directly from the runtime the same way we do for finalized blocks.
+        // So for now we return empty events and messages. Another option is to expose the event and message
+        // ordering functions from the runtime, order events inside execution info and use it. But the
+        // effort will not be worth it after pending blocks, so we've skipped implementing this for
+        // now.
         let messages_sent = Vec::new();
-        // TODO -- no  way of getting events for the tx
         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 simulation =
-            self.simulate_tx(self.get_best_block_hash(), pending_tx.clone(), skip_validate, skip_fee_charge)?;
-        let actual_fee = simulation.actual_fee.0.into();
-        let execution_result = revert_error_to_execution_result(simulation.revert_error);
-        let execution_resources = actual_resources_to_execution_resources(simulation.actual_resources);
+        let execution_info =
+            self.get_transaction_execution_info(parent_substrate_block_hash, &pending_txs, chain_id, transaction_hash)?;
+        let actual_fee = execution_info.actual_fee.0.into();
+        let execution_result = revert_error_to_execution_result(execution_info.revert_error);
+        let execution_resources = actual_resources_to_execution_resources(execution_info.actual_resources);
 
         let receipt = match pending_tx {
             mp_transactions::Transaction::Declare(_tx, _contract_class) => {
@@ -1774,6 +1780,43 @@ where
         Ok(MaybePendingTransactionReceipt::PendingReceipt(receipt))
     }
 
+    fn get_transaction_execution_info(
+        &self,
+        parent_substrate_block_hash: B::Hash,
+        previous_transactions: &BlockTransactions,
+        chain_id: Felt252Wrapper,
+        transaction_hash: FieldElement,
+    ) -> Result<TransactionExecutionInfo, StarknetRpcApiError>
+    where
+        B: BlockT,
+    {
+        let (transactions_before, transaction_to_trace) =
+            map_transaction_to_user_transaction(self, previous_transactions, chain_id, Some(transaction_hash.into()))?;
+
+        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 trace = self
+            .re_execute_transactions(parent_substrate_block_hash, transactions_before, transaction_to_trace)
+            .map_err(|e| {
+                log::error!("Failed to re-execute transactions: {e}");
+                StarknetRpcApiError::InternalServerError
+            })?;
+
+        let execution_info = trace.get(0).ok_or_else(|| {
+            log::error!("Failed to get execution info");
+            StarknetRpcApiError::InternalServerError
+        })?;
+
+        Ok(execution_info.0.clone())
+    }
+
     fn convert_error<T>(
         &self,
         best_block_hash: <B as BlockT>::Hash,

crates/client/rpc/src/trace_api.rs

@@ -8,7 +8,7 @@ use log::error;
 use mc_genesis_data_provider::GenesisProvider;
 use mc_rpc_core::utils::{blockifier_to_rpc_state_diff_types, get_block_by_block_hash};
 use mc_rpc_core::{StarknetReadRpcApiServer, StarknetTraceRpcApiServer};
-use mp_block::Block;
+use mp_block::BlockTransactions;
 use mp_felt::Felt252Wrapper;
 use mp_hashers::HasherT;
 use mp_simulations::{SimulationFlags, TransactionSimulationResult};
@@ -107,7 +107,7 @@ where
         let chain_id = Felt252Wrapper(self.chain_id()?.0);
 
         let (block_transactions, _) =
-            map_transaction_to_user_transaction(self, starknet_block, substrate_block_hash, chain_id, None)?;
+            map_transaction_to_user_transaction(self, starknet_block.transactions(), chain_id, None)?;
 
         let previous_block_substrate_hash = get_previous_block_substrate_hash(self, substrate_block_hash)?;
 
@@ -138,8 +138,7 @@ where
 
         let (txs_to_execute_before, tx_to_trace) = map_transaction_to_user_transaction(
             self,
-            starknet_block,
-            substrate_block_hash,
+            starknet_block.transactions(),
             chain_id,
             Some(transaction_hash_to_trace),
         )?;
@@ -178,7 +177,7 @@ where
     P: TransactionPool<Block = B> + 'static,
     H: HasherT + Send + Sync + 'static,
 {
-    fn re_execute_transactions(
+    pub fn re_execute_transactions(
         &self,
         previous_block_substrate_hash: B::Hash,
         transactions_before: Vec<UserOrL1HandlerTransaction>,
@@ -434,10 +433,9 @@ fn tx_execution_infos_to_simulated_transactions(
     Ok(results)
 }
 
-fn map_transaction_to_user_transaction<A, B, BE, G, C, P, H>(
+pub fn map_transaction_to_user_transaction<A, B, BE, G, C, P, H>(
     starknet: &Starknet<A, B, BE, G, C, P, H>,
-    starknet_block: Block,
-    substrate_block_hash: B::Hash,
+    transactions: &BlockTransactions,
     chain_id: Felt252Wrapper,
     target_transaction_hash: Option<Felt252Wrapper>,
 ) -> Result<(Vec<UserOrL1HandlerTransaction>, Vec<UserOrL1HandlerTransaction>), StarknetRpcApiError>
@@ -448,29 +446,28 @@ where
     H: HasherT + Send + Sync + 'static,
     BE: Backend<B> + 'static,
 {
-    let mut transactions = Vec::new();
+    let mut user_transactions = Vec::new();
     let mut transaction_to_trace = Vec::new();
 
-    for tx in starknet_block.transactions() {
+    for tx in transactions {
         let current_tx_hash = tx.compute_hash::<H>(chain_id, false);
 
         if Some(current_tx_hash) == target_transaction_hash {
-            let converted_tx = convert_transaction(tx, starknet, substrate_block_hash, chain_id)?;
+            let converted_tx = convert_transaction(tx, starknet, chain_id)?;
             transaction_to_trace.push(converted_tx);
             break;
         } else {
-            let converted_tx = convert_transaction(tx, starknet, substrate_block_hash, chain_id)?;
-            transactions.push(converted_tx);
+            let converted_tx = convert_transaction(tx, starknet, chain_id)?;
+            user_transactions.push(converted_tx);
         }
     }
 
-    Ok((transactions, transaction_to_trace))
+    Ok((user_transactions, transaction_to_trace))
 }
 
 fn convert_transaction<A, B, BE, G, C, P, H>(
     tx: &Transaction,
     starknet: &Starknet<A, B, BE, G, C, P, H>,
-    substrate_block_hash: B::Hash,
     chain_id: Felt252Wrapper,
 ) -> Result<UserOrL1HandlerTransaction, StarknetRpcApiError>
 where
@@ -487,22 +484,12 @@ where
         Transaction::DeployAccount(deploy_account_tx) => {
             Ok(UserOrL1HandlerTransaction::User(UserTransaction::DeployAccount(deploy_account_tx.clone())))
         }
-        Transaction::Declare(declare_tx, _) => {
+        Transaction::Declare(declare_tx, contract_class) => {
             let class_hash = ClassHash::from(*declare_tx.class_hash());
-
             match declare_tx {
-                DeclareTransaction::V0(_) | DeclareTransaction::V1(_) => {
-                    let contract_class = starknet
-                        .overrides
-                        .for_block_hash(starknet.client.as_ref(), substrate_block_hash)
-                        .contract_class_by_class_hash(substrate_block_hash, class_hash)
-                        .ok_or_else(|| {
-                            error!("Failed to retrieve contract class from hash '{class_hash}'");
-                            StarknetRpcApiError::InternalServerError
-                        })?;
-
-                    Ok(UserOrL1HandlerTransaction::User(UserTransaction::Declare(declare_tx.clone(), contract_class)))
-                }
+                DeclareTransaction::V0(_) | DeclareTransaction::V1(_) => Ok(UserOrL1HandlerTransaction::User(
+                    UserTransaction::Declare(declare_tx.clone(), contract_class.clone()),
+                )),
                 DeclareTransaction::V2(_tx) => {
                     let contract_class = starknet
                         .backend

crates/pallets/starknet/src/simulations.rs

@@ -291,7 +291,10 @@ impl<T: Config> Pallet<T> {
         for (exec_result, state_diff) in exec_transactions {
             match exec_result {
                 Ok(info) => execution_infos.push((info, state_diff)),
-                Err(_err) => return Err(PlaceHolderErrorTypeForFailedStarknetExecution),
+                Err(err) => {
+                    log::error!("Transaction execution failed: {err}");
+                    return Err(PlaceHolderErrorTypeForFailedStarknetExecution);
+                }
             }
         }
 

starknet-rpc-test/get_transaction_receipt.rs

@@ -2,6 +2,7 @@ use std::vec;
 
 use assert_matches::assert_matches;
 use rstest::rstest;
+use starknet_accounts::ConnectedAccount;
 use starknet_core::types::{
     Event, ExecutionResult, MaybePendingTransactionReceipt, MsgToL1, PendingTransactionReceipt,
     TransactionFinalityStatus, TransactionReceipt,
@@ -107,36 +108,57 @@ async fn work_with_pending_invoke_transaction(madara: &ThreadSafeMadaraClient) -
     let recipient = FieldElement::from_hex_be("0x12345").unwrap();
     let transfer_amount = FieldElement::ONE;
 
-    let (rpc_response, invoke_tx_pending_receipt) = {
-        let mut madara_write_lock = madara.write().await;
-        let account = build_single_owner_account(&rpc, SIGNER_PRIVATE, ARGENT_CONTRACT_ADDRESS, true);
-        let mut txs = madara_write_lock
-            .submit_txs(vec![Transaction::Execution(account.transfer_tokens(recipient, transfer_amount, None))])
-            .await;
-
-        assert_eq!(txs.len(), 1);
-        let rpc_response = match txs.remove(0).unwrap() {
-            TransactionResult::Execution(rpc_response) => rpc_response,
-            _ => panic!("expected execution result"),
-        };
-        let pending_receipt = get_transaction_receipt(&rpc, rpc_response.transaction_hash).await?;
-
-        // Create block with pending txs to clear state
-        madara_write_lock.create_block_with_pending_txs().await?;
+    let mut madara_write_lock = madara.write().await;
+    let account = build_single_owner_account(&rpc, SIGNER_PRIVATE, ARGENT_CONTRACT_ADDRESS, true);
+    let nonce = account.get_nonce().await?.try_into()?;
+    let mut txs = madara_write_lock
+        .submit_txs(vec![
+            Transaction::Execution(account.transfer_tokens(recipient, transfer_amount, Some(nonce))),
+            Transaction::Execution(account.transfer_tokens(recipient, transfer_amount, Some(nonce + 1))),
+        ])
+        .await;
 
-        (rpc_response, pending_receipt)
+    assert_eq!(txs.len(), 2);
+    let rpc_response_one = match txs.remove(0).unwrap() {
+        TransactionResult::Execution(rpc_response) => rpc_response,
+        _ => panic!("expected execution result"),
     };
-
-    match invoke_tx_pending_receipt {
-        MaybePendingTransactionReceipt::PendingReceipt(PendingTransactionReceipt::Invoke(receipt)) => {
-            assert_eq!(receipt.transaction_hash, rpc_response.transaction_hash);
-            assert!(receipt.actual_fee > FieldElement::ZERO);
-            assert_eq_msg_to_l1(receipt.messages_sent, vec![]);
-            assert_eq!(receipt.events, vec![]);
-            assert_matches!(receipt.execution_result, ExecutionResult::Succeeded);
+    let rpc_response_two = match txs.remove(0).unwrap() {
+        TransactionResult::Execution(rpc_response) => rpc_response,
+        _ => panic!("expected execution result"),
+    };
+    let pending_receipt_one = get_transaction_receipt(&rpc, rpc_response_one.transaction_hash).await?;
+    let pending_receipt_two = get_transaction_receipt(&rpc, rpc_response_two.transaction_hash).await?;
+
+    // Create block with pending txs to clear state
+    madara_write_lock.create_block_with_pending_txs().await?;
+
+    let final_receipt_one = get_transaction_receipt(&rpc, rpc_response_one.transaction_hash).await?;
+    let final_receipt_two = get_transaction_receipt(&rpc, rpc_response_two.transaction_hash).await?;
+
+    let assert_receipt_match = |pending_receipt: MaybePendingTransactionReceipt,
+                                final_receipt: MaybePendingTransactionReceipt| {
+        match pending_receipt {
+            MaybePendingTransactionReceipt::PendingReceipt(PendingTransactionReceipt::Invoke(receipt)) => {
+                match final_receipt {
+                    MaybePendingTransactionReceipt::Receipt(TransactionReceipt::Invoke(final_receipt)) => {
+                        assert_eq!(receipt.transaction_hash, final_receipt.transaction_hash);
+                        assert_eq!(receipt.actual_fee, final_receipt.actual_fee);
+                        // 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);
+                        // assert_eq!(receipt.events, final_receipt.events);
+                        assert_matches!(receipt.execution_result, ExecutionResult::Succeeded);
+                        assert_eq!(receipt.execution_resources, final_receipt.execution_resources);
+                    }
+                    _ => panic!("expected final invoke transaction receipt"),
+                }
+            }
+            _ => panic!("expected pending invoke transaction receipt"),
         }
-        _ => panic!("expected invoke transaction receipt"),
     };
+    assert_receipt_match(pending_receipt_one, final_receipt_one);
+    assert_receipt_match(pending_receipt_two, final_receipt_two);
 
     Ok(())
 }