diff --git a/CHANGELOG.md b/CHANGELOG.md index 94a47ccd48..90f9cae7cd 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/crates/client/rpc/src/lib.rs b/crates/client/rpc/src/lib.rs index 9d61aa3db1..db582f9d25 100644 --- a/crates/client/rpc/src/lib.rs +++ b/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, StarknetRpcApiError> { - let latest_block = self.get_best_block_hash(); - - let pending_tx = self - .get_pending_txs(latest_block)? - .iter() - .find(|&tx| tx.compute_hash::(chain_id.0.into(), false).0 == tx_hash) - .cloned(); + let pending_tx = + pending_txs.iter().find(|&tx| tx.compute_hash::(chain_id.0.into(), false).0 == tx_hash).cloned(); Ok(pending_tx) } @@ -1704,22 +1706,26 @@ where chain_id: Felt252Wrapper, transaction_hash: FieldElement, ) -> Result { - 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 + 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( &self, best_block_hash: ::Hash, diff --git a/crates/client/rpc/src/trace_api.rs b/crates/client/rpc/src/trace_api.rs index 8cc6e75752..db44064795 100644 --- a/crates/client/rpc/src/trace_api.rs +++ b/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 + '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, @@ -434,10 +433,9 @@ fn tx_execution_infos_to_simulated_transactions( Ok(results) } -fn map_transaction_to_user_transaction( +pub fn map_transaction_to_user_transaction( starknet: &Starknet, - starknet_block: Block, - substrate_block_hash: B::Hash, + transactions: &BlockTransactions, chain_id: Felt252Wrapper, target_transaction_hash: Option, ) -> Result<(Vec, Vec), StarknetRpcApiError> @@ -448,29 +446,28 @@ where H: HasherT + Send + Sync + 'static, BE: Backend + '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::(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( tx: &Transaction, starknet: &Starknet, - substrate_block_hash: B::Hash, chain_id: Felt252Wrapper, ) -> Result 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 diff --git a/crates/pallets/starknet/src/simulations.rs b/crates/pallets/starknet/src/simulations.rs index 8cb5b876b9..8129d02ff9 100644 --- a/crates/pallets/starknet/src/simulations.rs +++ b/crates/pallets/starknet/src/simulations.rs @@ -291,7 +291,10 @@ impl Pallet { 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); + } } } diff --git a/starknet-rpc-test/get_transaction_receipt.rs b/starknet-rpc-test/get_transaction_receipt.rs index c601538f39..d7bbfbd2a0 100644 --- a/starknet-rpc-test/get_transaction_receipt.rs +++ b/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(()) }