From ad94ba68430fa154aa73154d77d9cef6ead493e4 Mon Sep 17 00:00:00 2001 From: Ammar Arif Date: Thu, 23 May 2024 09:16:25 -0400 Subject: [PATCH] refactor(katana): replace cursor-based api with by block basis for simplicity (#1986) # Description currently, the `saya_getTransactionsExecutions` API is based on a cursor to determine how the traces are being fetched. https://github.com/dojoengine/dojo/blob/855da3112c87faea87646db5a406ac77b4daf149/crates/saya/provider/src/rpc/mod.rs#L160-L175 one issue it currently has it that the api params does not put any constraint on whether the returned traces are guarantee to only be from a specific block. however, the implementation seems to indicate otherwise. anyway, this pr basically want to remove the complexity of the pagination logic (which we dont even need!) and to replace it with a basic `get Xs from block Y` api instead. so we can simplify the endpoint itself to just return the traces per block instead of based on a cursor. the cursor is only adding more complexity to the api itself. ## Related issue ## Tests - [ ] Yes - [ ] No, because they aren't needed - [ ] No, because I need help ## Added to documentation? - [ ] README.md - [x] [Dojo Book](https://github.com/dojoengine/book) - [ ] No documentation needed ## Checklist - [x] I've formatted my code (`scripts/prettier.sh`, `scripts/rust_fmt.sh`, `scripts/cairo_fmt.sh`) - [x] I've linted my code (`scripts/clippy.sh`, `scripts/docs.sh`) - [x] I've commented my code - [ ] I've requested a review after addressing the comments --- Cargo.lock | 1 + crates/katana/rpc/rpc-api/src/saya.rs | 9 ++ crates/katana/rpc/rpc-types/src/trace.rs | 13 +- crates/katana/rpc/rpc/Cargo.toml | 1 + crates/katana/rpc/rpc/src/saya.rs | 79 +++++++++- crates/katana/rpc/rpc/tests/saya.rs | 147 +++++++++++++++++- crates/katana/storage/provider/src/lib.rs | 11 +- .../storage/provider/src/providers/db/mod.rs | 37 +++-- .../provider/src/providers/fork/mod.rs | 24 ++- .../provider/src/providers/in_memory/mod.rs | 24 ++- .../provider/src/traits/transaction.rs | 8 +- crates/katana/storage/provider/tests/block.rs | 4 +- 12 files changed, 315 insertions(+), 43 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 605c18402a..92080bfc69 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7002,6 +7002,7 @@ dependencies = [ "cairo-lang-starknet-classes", "dojo-metrics", "dojo-test-utils", + "dojo-world", "flate2", "futures", "hex", diff --git a/crates/katana/rpc/rpc-api/src/saya.rs b/crates/katana/rpc/rpc-api/src/saya.rs index fa9017250f..d972084a8d 100644 --- a/crates/katana/rpc/rpc-api/src/saya.rs +++ b/crates/katana/rpc/rpc-api/src/saya.rs @@ -1,5 +1,7 @@ use jsonrpsee::core::RpcResult; use jsonrpsee::proc_macros::rpc; +use katana_primitives::block::BlockIdOrTag; +use katana_rpc_types::trace::TxExecutionInfo; use katana_rpc_types::transaction::{TransactionsExecutionsPage, TransactionsPageCursor}; #[cfg_attr(not(feature = "client"), rpc(server, namespace = "saya"))] @@ -17,4 +19,11 @@ pub trait SayaApi { &self, cursor: TransactionsPageCursor, ) -> RpcResult; + + /// Retrieves a list of transaction execution informations of a given block. + #[method(name = "getTransactionExecutionsByBlock")] + async fn transaction_executions_by_block( + &self, + block_id: BlockIdOrTag, + ) -> RpcResult>; } diff --git a/crates/katana/rpc/rpc-types/src/trace.rs b/crates/katana/rpc/rpc-types/src/trace.rs index 3bea0d6edb..971d1b46a0 100644 --- a/crates/katana/rpc/rpc-types/src/trace.rs +++ b/crates/katana/rpc/rpc-types/src/trace.rs @@ -1,4 +1,6 @@ -use katana_primitives::trace::CallInfo; +use katana_primitives::trace::{CallInfo, TxExecInfo}; +use katana_primitives::transaction::TxHash; +use serde::{Deserialize, Serialize}; use starknet::core::types::{ CallType, EntryPointType, ExecutionResources, OrderedEvent, OrderedMessage, }; @@ -72,3 +74,12 @@ impl From for FunctionInvocation { }) } } + +/// The type returned by the `saya_getTransactionExecutionsByBlock` RPC method. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct TxExecutionInfo { + /// The transaction hash. + pub hash: TxHash, + /// The transaction execution trace. + pub trace: TxExecInfo, +} diff --git a/crates/katana/rpc/rpc/Cargo.toml b/crates/katana/rpc/rpc/Cargo.toml index ff5f5de395..b455d5c465 100644 --- a/crates/katana/rpc/rpc/Cargo.toml +++ b/crates/katana/rpc/rpc/Cargo.toml @@ -40,6 +40,7 @@ assert_matches.workspace = true cairo-lang-starknet-classes.workspace = true cairo-lang-starknet.workspace = true dojo-test-utils.workspace = true +dojo-world.workspace = true jsonrpsee = { workspace = true, features = [ "client" ] } katana-rpc-api = { workspace = true, features = [ "client" ] } url.workspace = true diff --git a/crates/katana/rpc/rpc/src/saya.rs b/crates/katana/rpc/rpc/src/saya.rs index 330fe3eac6..83656b8d7b 100644 --- a/crates/katana/rpc/rpc/src/saya.rs +++ b/crates/katana/rpc/rpc/src/saya.rs @@ -3,10 +3,13 @@ use std::sync::Arc; use jsonrpsee::core::{async_trait, RpcResult}; use katana_core::sequencer::KatanaSequencer; use katana_executor::ExecutorFactory; -use katana_primitives::block::BlockHashOrNumber; -use katana_provider::traits::transaction::TransactionTraceProvider; +use katana_primitives::block::{BlockHashOrNumber, BlockIdOrTag, BlockTag}; +use katana_provider::error::ProviderError; +use katana_provider::traits::block::{BlockIdReader, BlockProvider}; +use katana_provider::traits::transaction::{TransactionTraceProvider, TransactionsProviderExt}; use katana_rpc_api::saya::SayaApiServer; use katana_rpc_types::error::saya::SayaApiError; +use katana_rpc_types::trace::TxExecutionInfo; use katana_rpc_types::transaction::{TransactionsExecutionsPage, TransactionsPageCursor}; use katana_tasks::TokioTaskSpawner; @@ -46,7 +49,7 @@ impl SayaApiServer for SayaApi { let mut next_cursor = cursor; let transactions_executions = provider - .transactions_executions_by_block(BlockHashOrNumber::Num(cursor.block_number)) + .transaction_executions_by_block(BlockHashOrNumber::Num(cursor.block_number)) .map_err(SayaApiError::from)? .ok_or(SayaApiError::BlockNotFound)?; @@ -73,4 +76,74 @@ impl SayaApiServer for SayaApi { }) .await } + + async fn transaction_executions_by_block( + &self, + block_id: BlockIdOrTag, + ) -> RpcResult> { + self.on_io_blocking_task(move |this| { + let provider = this.sequencer.backend.blockchain.provider(); + + match block_id { + BlockIdOrTag::Tag(BlockTag::Pending) => { + // if there is no pending block (eg on instant mining), return an empty list + let Some(pending) = this.sequencer.pending_executor() else { + return Ok(Vec::new()); + }; + + // get the read lock on the pending block + let lock = pending.read(); + + // extract the traces from the pending block + let mut traces = Vec::new(); + for (tx, res) in lock.transactions() { + if let Some(trace) = res.trace().cloned() { + traces.push(TxExecutionInfo { hash: tx.hash, trace }); + } + } + + Ok(traces) + } + + id => { + let number = provider + .convert_block_id(id) + .map_err(SayaApiError::from)? + .ok_or(SayaApiError::BlockNotFound)?; + + // get the transaction traces and their corresponding hashes + + let traces = provider + .transaction_executions_by_block(number.into()) + .map_err(SayaApiError::from)? + .expect("qed; must be Some if block exists"); + + // get the block body indices for the requested block to determine its tx range + // in the db for the tx hashes + + let block_indices = provider + .block_body_indices(number.into()) + .map_err(SayaApiError::from)? + .ok_or(ProviderError::MissingBlockBodyIndices(number)) + .expect("qed; must be Some if block exists"); + + // TODO: maybe we should add a `_by_block` method for the tx hashes as well? + let hashes = provider + .transaction_hashes_in_range(block_indices.clone().into()) + .map_err(SayaApiError::from)?; + + // build the rpc response + + let traces = hashes + .into_iter() + .zip(traces) + .map(|(hash, trace)| TxExecutionInfo { hash, trace }) + .collect::>(); + + Ok(traces) + } + } + }) + .await + } } diff --git a/crates/katana/rpc/rpc/tests/saya.rs b/crates/katana/rpc/rpc/tests/saya.rs index e706177157..ca2b3249f4 100644 --- a/crates/katana/rpc/rpc/tests/saya.rs +++ b/crates/katana/rpc/rpc/tests/saya.rs @@ -3,19 +3,22 @@ use std::sync::Arc; use std::time::Duration; use dojo_test_utils::sequencer::{get_default_test_starknet_config, TestSequencer}; +use dojo_world::utils::TransactionWaiter; use jsonrpsee::http_client::HttpClientBuilder; use katana_core::sequencer::SequencerConfig; +use katana_primitives::block::{BlockIdOrTag, BlockTag}; use katana_rpc_api::dev::DevApiClient; use katana_rpc_api::saya::SayaApiClient; use katana_rpc_api::starknet::StarknetApiClient; use katana_rpc_types::transaction::{ TransactionsExecutionsPage, TransactionsPageCursor, CHUNK_SIZE_DEFAULT, }; -use starknet::accounts::Account; +use starknet::accounts::{Account, ConnectedAccount}; use starknet::core::types::{FieldElement, TransactionStatus}; +use starknet::macros::felt; use tokio::time::sleep; -pub const ENOUGH_GAS: &str = "0x100000000000000000"; +const ENOUGH_GAS: FieldElement = felt!("0x100000000000000000"); mod common; @@ -134,7 +137,6 @@ async fn executions_chunks_logic_ok() { let declare_res = account.declare(contract.clone(), compiled_class_hash).send().await.unwrap(); - let max_fee = FieldElement::from_hex_be(ENOUGH_GAS).unwrap(); let mut nonce = FieldElement::ONE; let mut last_tx_hash = FieldElement::ZERO; @@ -142,7 +144,7 @@ async fn executions_chunks_logic_ok() { for i in 0..29 { let deploy_call = common::build_deploy_cairo1_contract_call(declare_res.class_hash, (i + 2_u32).into()); - let deploy_txn = account.execute(vec![deploy_call]).nonce(nonce).max_fee(max_fee); + let deploy_txn = account.execute(vec![deploy_call]).nonce(nonce).max_fee(ENOUGH_GAS); let tx_hash = deploy_txn.send().await.unwrap().transaction_hash; nonce += FieldElement::ONE; @@ -202,3 +204,140 @@ async fn executions_chunks_logic_ok() { sequencer.stop().expect("failed to stop sequencer"); } + +#[tokio::test(flavor = "multi_thread")] +async fn fetch_traces_from_block() { + let sequencer = TestSequencer::start( + SequencerConfig { no_mining: true, ..Default::default() }, + get_default_test_starknet_config(), + ) + .await; + + let client = HttpClientBuilder::default().build(sequencer.url()).unwrap(); + + let account = sequencer.account(); + + let path: PathBuf = PathBuf::from("tests/test_data/cairo1_contract.json"); + let (contract, compiled_class_hash) = + common::prepare_contract_declaration_params(&path).unwrap(); + let contract = Arc::new(contract); + + let res = account.declare(contract.clone(), compiled_class_hash).send().await.unwrap(); + // wait for the tx to be mined + TransactionWaiter::new(res.transaction_hash, account.provider()) + .with_interval(200) + .await + .expect("tx failed"); + + // Store the tx hashes to check the retrieved traces later. + let mut tx_hashes = vec![res.transaction_hash]; + + for i in 0..29 { + let call = common::build_deploy_cairo1_contract_call(res.class_hash, (i + 2_u32).into()); + + let res = account + .execute(vec![call]) + .max_fee(ENOUGH_GAS) + .send() + .await + .expect("failed to send tx"); + + // wait for the tx to be mined + TransactionWaiter::new(res.transaction_hash, account.provider()) + .with_interval(200) + .await + .expect("tx failed"); + + tx_hashes.push(res.transaction_hash); + } + + // Generate a new block. + let _: () = client.generate_block().await.unwrap(); + + // Get the traces from the latest block. + let traces = client + .transaction_executions_by_block(BlockIdOrTag::Tag(BlockTag::Latest)) + .await + .expect("failed to get traces from latest block"); + + assert_eq!( + tx_hashes.len(), + traces.len(), + "traces count in the latest block must equal to the total txs" + ); + + for (expected, actual) in tx_hashes.iter().zip(traces) { + // Assert that the traces are from the txs in the requested block. + assert_eq!(expected, &actual.hash); + } + + sequencer.stop().expect("failed to stop sequencer"); +} + +#[tokio::test(flavor = "multi_thread")] +async fn fetch_traces_from_pending_block() { + let sequencer = TestSequencer::start( + SequencerConfig { no_mining: true, ..Default::default() }, + get_default_test_starknet_config(), + ) + .await; + + let client = HttpClientBuilder::default().build(sequencer.url()).unwrap(); + + let account = sequencer.account(); + + let path: PathBuf = PathBuf::from("tests/test_data/cairo1_contract.json"); + let (contract, compiled_class_hash) = + common::prepare_contract_declaration_params(&path).unwrap(); + let contract = Arc::new(contract); + + let res = account.declare(contract.clone(), compiled_class_hash).send().await.unwrap(); + // wait for the tx to be mined + TransactionWaiter::new(res.transaction_hash, account.provider()) + .with_interval(200) + .await + .expect("tx failed"); + + // Store the tx hashes to check the retrieved traces later. + let mut tx_hashes = vec![res.transaction_hash]; + + for i in 0..29 { + let call = common::build_deploy_cairo1_contract_call(res.class_hash, (i + 2_u32).into()); + + // we set the nonce manually so that we can send the tx rapidly without waiting for the + // previous tx to be mined first. + let res = account + .execute(vec![call]) + .max_fee(ENOUGH_GAS) + .send() + .await + .expect("failed to send tx"); + + // wait for the tx to be mined + TransactionWaiter::new(res.transaction_hash, account.provider()) + .with_interval(200) + .await + .expect("tx failed"); + + tx_hashes.push(res.transaction_hash); + } + + // Get the traces from the pending block. + let traces = client + .transaction_executions_by_block(BlockIdOrTag::Tag(BlockTag::Pending)) + .await + .expect("failed to get traces from pending block"); + + assert_eq!( + tx_hashes.len(), + traces.len(), + "traces count in the pending block must equal to the total txs" + ); + + for (expected, actual) in tx_hashes.iter().zip(traces) { + // Assert that the traces are from the txs in the requested block. + assert_eq!(expected, &actual.hash); + } + + sequencer.stop().expect("failed to stop sequencer"); +} diff --git a/crates/katana/storage/provider/src/lib.rs b/crates/katana/storage/provider/src/lib.rs index 5b247c70da..3f5581d94c 100644 --- a/crates/katana/storage/provider/src/lib.rs +++ b/crates/katana/storage/provider/src/lib.rs @@ -195,11 +195,18 @@ where TransactionTraceProvider::transaction_execution(&self.provider, hash) } - fn transactions_executions_by_block( + fn transaction_executions_by_block( &self, block_id: BlockHashOrNumber, ) -> ProviderResult>> { - TransactionTraceProvider::transactions_executions_by_block(&self.provider, block_id) + TransactionTraceProvider::transaction_executions_by_block(&self.provider, block_id) + } + + fn transaction_executions_in_range( + &self, + range: Range, + ) -> ProviderResult> { + TransactionTraceProvider::transaction_executions_in_range(&self.provider, range) } } diff --git a/crates/katana/storage/provider/src/providers/db/mod.rs b/crates/katana/storage/provider/src/providers/db/mod.rs index c3b302aedc..ec07fa213b 100644 --- a/crates/katana/storage/provider/src/providers/db/mod.rs +++ b/crates/katana/storage/provider/src/providers/db/mod.rs @@ -506,27 +506,36 @@ impl TransactionTraceProvider for DbProvider { } } - fn transactions_executions_by_block( + fn transaction_executions_by_block( &self, block_id: BlockHashOrNumber, ) -> ProviderResult>> { - if let Some(indices) = self.block_body_indices(block_id)? { - let db_tx = self.0.tx()?; - let mut executions = Vec::with_capacity(indices.tx_count as usize); - - let range = Range::from(indices); - for i in range { - if let Some(execution) = db_tx.get::(i)? { - executions.push(execution); - } - } - - db_tx.commit()?; - Ok(Some(executions)) + if let Some(index) = self.block_body_indices(block_id)? { + let traces = self.transaction_executions_in_range(index.into())?; + Ok(Some(traces)) } else { Ok(None) } } + + fn transaction_executions_in_range( + &self, + range: Range, + ) -> ProviderResult> { + let db_tx = self.0.tx()?; + + let total = range.end - range.start; + let mut traces = Vec::with_capacity(total as usize); + + for i in range { + if let Some(trace) = db_tx.get::(i)? { + traces.push(trace); + } + } + + db_tx.commit()?; + Ok(traces) + } } impl ReceiptProvider for DbProvider { diff --git a/crates/katana/storage/provider/src/providers/fork/mod.rs b/crates/katana/storage/provider/src/providers/fork/mod.rs index 3fc82df95b..0e55e1b99d 100644 --- a/crates/katana/storage/provider/src/providers/fork/mod.rs +++ b/crates/katana/storage/provider/src/providers/fork/mod.rs @@ -332,7 +332,7 @@ impl TransactionTraceProvider for ForkedProvider { Ok(exec) } - fn transactions_executions_by_block( + fn transaction_executions_by_block( &self, block_id: BlockHashOrNumber, ) -> ProviderResult>> { @@ -341,26 +341,34 @@ impl TransactionTraceProvider for ForkedProvider { BlockHashOrNumber::Hash(hash) => self.storage.read().block_numbers.get(&hash).cloned(), }; - let Some(StoredBlockBodyIndices { tx_offset, tx_count }) = + let Some(index) = block_num.and_then(|num| self.storage.read().block_body_indices.get(&num).cloned()) else { return Ok(None); }; - let offset = tx_offset as usize; - let count = tx_count as usize; + let traces = self.transaction_executions_in_range(index.into())?; + Ok(Some(traces)) + } + + fn transaction_executions_in_range( + &self, + range: Range, + ) -> ProviderResult> { + let start = range.start as usize; + let total = range.end as usize - start; - let execs = self + let traces = self .storage .read() .transactions_executions .iter() - .skip(offset) - .take(count) + .skip(start) + .take(total) .cloned() .collect(); - Ok(Some(execs)) + Ok(traces) } } diff --git a/crates/katana/storage/provider/src/providers/in_memory/mod.rs b/crates/katana/storage/provider/src/providers/in_memory/mod.rs index 82b704cb29..f59d0ec94a 100644 --- a/crates/katana/storage/provider/src/providers/in_memory/mod.rs +++ b/crates/katana/storage/provider/src/providers/in_memory/mod.rs @@ -326,7 +326,7 @@ impl TransactionTraceProvider for InMemoryProvider { Ok(exec) } - fn transactions_executions_by_block( + fn transaction_executions_by_block( &self, block_id: BlockHashOrNumber, ) -> ProviderResult>> { @@ -335,26 +335,34 @@ impl TransactionTraceProvider for InMemoryProvider { BlockHashOrNumber::Hash(hash) => self.storage.read().block_numbers.get(&hash).cloned(), }; - let Some(StoredBlockBodyIndices { tx_offset, tx_count }) = + let Some(index) = block_num.and_then(|num| self.storage.read().block_body_indices.get(&num).cloned()) else { return Ok(None); }; - let offset = tx_offset as usize; - let count = tx_count as usize; + let traces = self.transaction_executions_in_range(index.into())?; + Ok(Some(traces)) + } + + fn transaction_executions_in_range( + &self, + range: Range, + ) -> ProviderResult> { + let start = range.start as usize; + let total = range.end as usize - start; - let execs = self + let traces = self .storage .read() .transactions_executions .iter() - .skip(offset) - .take(count) + .skip(start) + .take(total) .cloned() .collect(); - Ok(Some(execs)) + Ok(traces) } } diff --git a/crates/katana/storage/provider/src/traits/transaction.rs b/crates/katana/storage/provider/src/traits/transaction.rs index 6054f0c3c0..ddc9b9bdc6 100644 --- a/crates/katana/storage/provider/src/traits/transaction.rs +++ b/crates/katana/storage/provider/src/traits/transaction.rs @@ -59,10 +59,16 @@ pub trait TransactionTraceProvider: Send + Sync { fn transaction_execution(&self, hash: TxHash) -> ProviderResult>; /// Returns all the transactions executions for a given block. - fn transactions_executions_by_block( + fn transaction_executions_by_block( &self, block_id: BlockHashOrNumber, ) -> ProviderResult>>; + + /// Retrieves the execution traces for the given range of tx numbers. + fn transaction_executions_in_range( + &self, + range: Range, + ) -> ProviderResult>; } #[auto_impl::auto_impl(&, Box, Arc)] diff --git a/crates/katana/storage/provider/tests/block.rs b/crates/katana/storage/provider/tests/block.rs index c2ff3b3a8b..2378ee3968 100644 --- a/crates/katana/storage/provider/tests/block.rs +++ b/crates/katana/storage/provider/tests/block.rs @@ -139,7 +139,7 @@ where let actual_block_tx_count = provider.transaction_count_by_block(block_id)?; let actual_receipts = provider.receipts_by_block(block_id)?; - let actual_executions = provider.transactions_executions_by_block(block_id)?; + let actual_executions = provider.transaction_executions_by_block(block_id)?; let expected_block_with_tx_hashes = BlockWithTxHashes { header: expected_block.header.clone(), @@ -244,7 +244,7 @@ where let actual_block_tx_count = provider.transaction_count_by_block(block_id)?; let actual_receipts = provider.receipts_by_block(block_id)?; - let actual_executions = provider.transactions_executions_by_block(block_id)?; + let actual_executions = provider.transaction_executions_by_block(block_id)?; let expected_block_with_tx_hashes = BlockWithTxHashes { header: expected_block.header.clone(), body: vec![] };