From 6291a933e32478a36a16836859d6f7610a63c49b Mon Sep 17 00:00:00 2001 From: cchudant Date: Mon, 23 Dec 2024 11:36:44 +0000 Subject: [PATCH] fix(rpc): call, simulate, estimate rpcs executed on top of the block, not at the start of it --- CHANGELOG.md | 1 + .../madara/client/block_production/src/lib.rs | 7 +- .../madara/client/exec/src/block_context.rs | 111 ++++++++++++------ crates/madara/client/exec/src/call.rs | 3 +- crates/madara/client/exec/src/execution.rs | 16 ++- crates/madara/client/exec/src/lib.rs | 28 ++++- crates/madara/client/mempool/src/lib.rs | 2 +- .../versions/user/v0_7_1/methods/read/call.rs | 6 +- .../user/v0_7_1/methods/read/estimate_fee.rs | 7 +- .../methods/read/estimate_message_fee.rs | 6 +- .../methods/trace/simulate_transactions.rs | 6 +- .../methods/trace/trace_block_transactions.rs | 6 +- .../v0_7_1/methods/trace/trace_transaction.rs | 8 +- 13 files changed, 138 insertions(+), 69 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ac02bfcc..ec07c4300 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## Next release +- fix(rpc): call, simulate, estimate rpcs executed on top of the block, not at the start of it - chore: Move crates under a madara subdir - chore(nix): resolve flake and direnv compatibility issues - fix: Gateway path fix diff --git a/crates/madara/client/block_production/src/lib.rs b/crates/madara/client/block_production/src/lib.rs index 81e18645c..314f598b6 100644 --- a/crates/madara/client/block_production/src/lib.rs +++ b/crates/madara/client/block_production/src/lib.rs @@ -171,8 +171,8 @@ impl BlockProductionTask { l1_data_provider.as_ref(), )); - let executor = - ExecutionContext::new_in_block(Arc::clone(&backend), &pending_block.info.clone().into())?.tx_executor(); + let executor = ExecutionContext::new_at_block_start(Arc::clone(&backend), &pending_block.info.clone().into())? + .tx_executor(); Ok(Self { importer, @@ -339,7 +339,8 @@ impl BlockProductionTask { // Prepare executor for next block self.executor = - ExecutionContext::new_in_block(Arc::clone(&self.backend), &self.block.info.clone().into())?.tx_executor(); + ExecutionContext::new_at_block_start(Arc::clone(&self.backend), &self.block.info.clone().into())? + .tx_executor(); self.current_pending_tick = 0; let end_time = start_time.elapsed(); diff --git a/crates/madara/client/exec/src/block_context.rs b/crates/madara/client/exec/src/block_context.rs index 8bcb4acfd..be45564a6 100644 --- a/crates/madara/client/exec/src/block_context.rs +++ b/crates/madara/client/exec/src/block_context.rs @@ -15,7 +15,8 @@ use std::sync::Arc; pub struct ExecutionContext { pub(crate) backend: Arc, pub(crate) block_context: BlockContext, - pub(crate) db_id: DbBlockId, + /// None means we are executing the genesis block. (no latest block) + pub(crate) latest_visible_block: Option, } impl ExecutionContext { @@ -33,53 +34,91 @@ impl ExecutionContext { } pub fn init_cached_state(&self) -> CachedState { - let on_top_of = match self.db_id { - DbBlockId::Pending => Some(DbBlockId::Pending), - DbBlockId::Number(block_n) => { - // We exec on top of the previous block. None means we are executing genesis. - block_n.checked_sub(1).map(DbBlockId::Number) - } - }; - tracing::debug!( "Init cached state on top of {:?}, block number {:?}", - on_top_of, + self.latest_visible_block, self.block_context.block_info().block_number.0 ); CachedState::new(BlockifierStateAdapter::new( Arc::clone(&self.backend), self.block_context.block_info().block_number.0, - on_top_of, + self.latest_visible_block, )) } - /// Create an execution context for executing transactions **within** that block. + /// Init execution at the beginning of a block. The header of the block will be used, but all of the + /// transactions' state modifications will not be visible. + /// + /// This function is usually what you would want for the `trace` rpc enpoints, for example. #[tracing::instrument(skip(backend, block_info), fields(module = "ExecutionContext"))] - pub fn new_in_block(backend: Arc, block_info: &MadaraMaybePendingBlockInfo) -> Result { - let (db_id, protocol_version, block_number, block_timestamp, sequencer_address, l1_gas_price, l1_da_mode) = - match block_info { - MadaraMaybePendingBlockInfo::Pending(block) => ( - DbBlockId::Pending, - block.header.protocol_version, - // when the block is pending, we use the latest block n + 1 + pub fn new_at_block_start( + backend: Arc, + block_info: &MadaraMaybePendingBlockInfo, + ) -> Result { + let (latest_visible_block, header_block_id) = match block_info { + MadaraMaybePendingBlockInfo::Pending(_block) => { + let latest_block_n = backend.get_latest_block_n()?; + ( + latest_block_n.map(DbBlockId::Number), + // when the block is pending, we use the latest block n + 1 to make the block header // if there is no latest block, the pending block is actually the genesis block - backend.get_latest_block_n()?.map(|el| el + 1).unwrap_or(0), - block.header.block_timestamp, - block.header.sequencer_address, - block.header.l1_gas_price.clone(), - block.header.l1_da_mode, - ), - MadaraMaybePendingBlockInfo::NotPending(block) => ( - DbBlockId::Number(block.header.block_number), - block.header.protocol_version, - block.header.block_number, - block.header.block_timestamp, - block.header.sequencer_address, - block.header.l1_gas_price.clone(), - block.header.l1_da_mode, - ), - }; + latest_block_n.map(|el| el + 1).unwrap_or(0), + ) + } + MadaraMaybePendingBlockInfo::NotPending(block) => { + // If the block is genesis, latest visible block is None. + (block.header.block_number.checked_sub(1).map(DbBlockId::Number), block.header.block_number) + } + }; + Self::new(backend, block_info, latest_visible_block, header_block_id) + } + + /// Init execution on top of a block. All of the transactions' state modifications are visible + /// but the execution still happens within that block. + /// This is essentially as if we're executing on top of the block after all of the transactions + /// are executed, but before we switched to making a new block. + /// + /// This function is usually what you would want for the `estimateFee`, `simulateTransaction`, `call` rpc endpoints, for example. + #[tracing::instrument(skip(backend, block_info), fields(module = "ExecutionContext"))] + pub fn new_at_block_end( + backend: Arc, + block_info: &MadaraMaybePendingBlockInfo, + ) -> Result { + let (latest_visible_block, header_block_id) = match block_info { + MadaraMaybePendingBlockInfo::Pending(_block) => { + let latest_block_n = backend.get_latest_block_n()?; + (Some(DbBlockId::Pending), latest_block_n.map(|el| el + 1).unwrap_or(0)) + } + MadaraMaybePendingBlockInfo::NotPending(block) => { + (Some(DbBlockId::Number(block.header.block_number)), block.header.block_number) + } + }; + Self::new(backend, block_info, latest_visible_block, header_block_id) + } + + fn new( + backend: Arc, + block_info: &MadaraMaybePendingBlockInfo, + latest_visible_block: Option, + block_number: u64, + ) -> Result { + let (protocol_version, block_timestamp, sequencer_address, l1_gas_price, l1_da_mode) = match block_info { + MadaraMaybePendingBlockInfo::Pending(block) => ( + block.header.protocol_version, + block.header.block_timestamp, + block.header.sequencer_address, + block.header.l1_gas_price.clone(), + block.header.l1_da_mode, + ), + MadaraMaybePendingBlockInfo::NotPending(block) => ( + block.header.protocol_version, + block.header.block_timestamp, + block.header.sequencer_address, + block.header.l1_gas_price.clone(), + block.header.l1_da_mode, + ), + }; let versioned_constants = backend.chain_config().exec_constants_by_protocol_version(protocol_version)?; let chain_info = ChainInfo { @@ -106,7 +145,7 @@ impl ExecutionContext { versioned_constants, backend.chain_config().bouncer_config.clone(), ), - db_id, + latest_visible_block, backend, }) } diff --git a/crates/madara/client/exec/src/call.rs b/crates/madara/client/exec/src/call.rs index 49e820c97..97ebce8b1 100644 --- a/crates/madara/client/exec/src/call.rs +++ b/crates/madara/client/exec/src/call.rs @@ -24,7 +24,8 @@ impl ExecutionContext { // We don't need a tx_executor here - let make_err = |err| CallContractError { block_n: self.db_id, contract: *contract_address, err }; + let make_err = + |err| CallContractError { block_n: self.latest_visible_block.into(), contract: *contract_address, err }; let storage_address = (*contract_address).try_into().map_err(TransactionExecutionError::StarknetApiError).map_err(make_err)?; diff --git a/crates/madara/client/exec/src/execution.rs b/crates/madara/client/exec/src/execution.rs index 81ed634f5..60788890c 100644 --- a/crates/madara/client/exec/src/execution.rs +++ b/crates/madara/client/exec/src/execution.rs @@ -28,7 +28,7 @@ impl ExecutionContext { let hash = tx.tx_hash(); tracing::debug!("executing {hash:#}"); tx.execute(&mut cached_state, &self.block_context, charge_fee, validate).map_err(|err| TxExecError { - block_n: self.db_id, + block_n: self.latest_visible_block.into(), hash, index, err, @@ -50,13 +50,21 @@ impl ExecutionContext { Transaction::AccountTransaction(tx) => Some( estimate_minimal_gas_vector(&self.block_context, tx) .map_err(TransactionExecutionError::TransactionPreValidationError) - .map_err(|err| TxFeeEstimationError { block_n: self.db_id, index, err })?, + .map_err(|err| TxFeeEstimationError { + block_n: self.latest_visible_block.into(), + index, + err, + })?, ), Transaction::L1HandlerTransaction(_) => None, // There is no minimal_l1_gas field for L1 handler transactions. }; - let make_reexec_error = - |err| TxExecError { block_n: self.db_id, hash, index: executed_prev + index, err }; + let make_reexec_error = |err| TxExecError { + block_n: self.latest_visible_block.into(), + hash, + index: executed_prev + index, + err, + }; let mut transactional_state = TransactionalState::create_transactional(&mut cached_state); let execution_flags = ExecutionFlags { charge_fee, validate, concurrency_mode: false }; diff --git a/crates/madara/client/exec/src/lib.rs b/crates/madara/client/exec/src/lib.rs index 5506b7eca..558cf5f3d 100644 --- a/crates/madara/client/exec/src/lib.rs +++ b/crates/madara/client/exec/src/lib.rs @@ -1,3 +1,5 @@ +use core::fmt; + use blockifier::{ state::cached_state::CommitmentStateDiff, transaction::{ @@ -22,6 +24,22 @@ pub use block_context::ExecutionContext; pub use blockifier_state_adapter::BlockifierStateAdapter; pub use trace::execution_result_to_tx_trace; +#[derive(Debug)] +struct OnTopOf(Option); +impl fmt::Display for OnTopOf { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self.0 { + Some(id) => write!(f, "{:#}", id), + None => write!(f, ""), + } + } +} +impl From> for OnTopOf { + fn from(value: Option) -> Self { + Self(value) + } +} + #[derive(thiserror::Error, Debug)] pub enum Error { #[error(transparent)] @@ -43,7 +61,7 @@ pub enum Error { #[derive(thiserror::Error, Debug)] #[error("Executing tx {hash:#} (index {index}) on top of {block_n}: {err:#}")] pub struct TxExecError { - block_n: DbBlockId, + block_n: OnTopOf, hash: TransactionHash, index: usize, #[source] @@ -53,7 +71,7 @@ pub struct TxExecError { #[derive(thiserror::Error, Debug)] #[error("Estimating fee for tx index {index} on top of {block_n}: {err:#}")] pub struct TxFeeEstimationError { - block_n: DbBlockId, + block_n: OnTopOf, index: usize, #[source] err: TransactionExecutionError, @@ -62,15 +80,15 @@ pub struct TxFeeEstimationError { #[derive(thiserror::Error, Debug)] #[error("Estimating message fee on top of {block_n}: {err:#}")] pub struct MessageFeeEstimationError { - block_n: DbBlockId, + block_n: OnTopOf, #[source] err: TransactionExecutionError, } #[derive(thiserror::Error, Debug)] -#[error("Calling contract {contract:#x} on top of {block_n:#}: {err:#}")] +#[error("Calling contract {contract:#x} on top of {block_n}: {err:#}")] pub struct CallContractError { - block_n: DbBlockId, + block_n: OnTopOf, contract: Felt, #[source] err: TransactionExecutionError, diff --git a/crates/madara/client/mempool/src/lib.rs b/crates/madara/client/mempool/src/lib.rs index 6103be0cd..9337cf148 100644 --- a/crates/madara/client/mempool/src/lib.rs +++ b/crates/madara/client/mempool/src/lib.rs @@ -166,7 +166,7 @@ impl Mempool { tracing::debug!("Mempool verify tx_hash={:#x}", tx_hash); // Perform validations - let exec_context = ExecutionContext::new_in_block(Arc::clone(&self.backend), &pending_block_info)?; + let exec_context = ExecutionContext::new_at_block_end(Arc::clone(&self.backend), &pending_block_info)?; let mut validator = exec_context.tx_validator(); if let Transaction::AccountTransaction(account_tx) = clone_transaction(&tx) { diff --git a/crates/madara/client/rpc/src/versions/user/v0_7_1/methods/read/call.rs b/crates/madara/client/rpc/src/versions/user/v0_7_1/methods/read/call.rs index 265bfaf99..15bc89796 100644 --- a/crates/madara/client/rpc/src/versions/user/v0_7_1/methods/read/call.rs +++ b/crates/madara/client/rpc/src/versions/user/v0_7_1/methods/read/call.rs @@ -7,7 +7,7 @@ use starknet_types_rpc::FunctionCall; use crate::errors::StarknetRpcApiError; use crate::errors::StarknetRpcResult; -use crate::versions::user::v0_7_1::methods::trace::trace_transaction::FALLBACK_TO_SEQUENCER_WHEN_VERSION_BELOW; +use crate::versions::user::v0_7_1::methods::trace::trace_transaction::EXECUTION_UNSUPPORTED_BELOW_VERSION; use crate::Starknet; /// Call a Function in a Contract Without Creating a Transaction @@ -33,9 +33,9 @@ use crate::Starknet; pub fn call(starknet: &Starknet, request: FunctionCall, block_id: BlockId) -> StarknetRpcResult> { let block_info = starknet.get_block_info(&block_id)?; - let exec_context = ExecutionContext::new_in_block(Arc::clone(&starknet.backend), &block_info)?; + let exec_context = ExecutionContext::new_at_block_end(Arc::clone(&starknet.backend), &block_info)?; - if block_info.protocol_version() < &FALLBACK_TO_SEQUENCER_WHEN_VERSION_BELOW { + if block_info.protocol_version() < &EXECUTION_UNSUPPORTED_BELOW_VERSION { return Err(StarknetRpcApiError::UnsupportedTxnVersion); } diff --git a/crates/madara/client/rpc/src/versions/user/v0_7_1/methods/read/estimate_fee.rs b/crates/madara/client/rpc/src/versions/user/v0_7_1/methods/read/estimate_fee.rs index e399ee8ad..b5373cdaf 100644 --- a/crates/madara/client/rpc/src/versions/user/v0_7_1/methods/read/estimate_fee.rs +++ b/crates/madara/client/rpc/src/versions/user/v0_7_1/methods/read/estimate_fee.rs @@ -1,7 +1,7 @@ use crate::errors::StarknetRpcApiError; use crate::errors::StarknetRpcResult; use crate::utils::ResultExt; -use crate::versions::user::v0_7_1::methods::trace::trace_transaction::FALLBACK_TO_SEQUENCER_WHEN_VERSION_BELOW; +use crate::versions::user::v0_7_1::methods::trace::trace_transaction::EXECUTION_UNSUPPORTED_BELOW_VERSION; use crate::Starknet; use mc_exec::ExecutionContext; use mp_block::BlockId; @@ -26,14 +26,15 @@ pub async fn estimate_fee( simulation_flags: Vec, block_id: BlockId, ) -> StarknetRpcResult>> { + tracing::debug!("estimate fee on block_id {block_id:?}"); let block_info = starknet.get_block_info(&block_id)?; let starknet_version = *block_info.protocol_version(); - if starknet_version < FALLBACK_TO_SEQUENCER_WHEN_VERSION_BELOW { + if starknet_version < EXECUTION_UNSUPPORTED_BELOW_VERSION { return Err(StarknetRpcApiError::UnsupportedTxnVersion); } - let exec_context = ExecutionContext::new_in_block(Arc::clone(&starknet.backend), &block_info)?; + let exec_context = ExecutionContext::new_at_block_end(Arc::clone(&starknet.backend), &block_info)?; let transactions = request .into_iter() diff --git a/crates/madara/client/rpc/src/versions/user/v0_7_1/methods/read/estimate_message_fee.rs b/crates/madara/client/rpc/src/versions/user/v0_7_1/methods/read/estimate_message_fee.rs index 7011bc842..8f90b1114 100644 --- a/crates/madara/client/rpc/src/versions/user/v0_7_1/methods/read/estimate_message_fee.rs +++ b/crates/madara/client/rpc/src/versions/user/v0_7_1/methods/read/estimate_message_fee.rs @@ -10,7 +10,7 @@ use starknet_types_rpc::{FeeEstimate, MsgFromL1}; use crate::errors::StarknetRpcApiError; use crate::errors::StarknetRpcResult; use crate::utils::OptionExt; -use crate::versions::user::v0_7_1::methods::trace::trace_transaction::FALLBACK_TO_SEQUENCER_WHEN_VERSION_BELOW; +use crate::versions::user::v0_7_1::methods::trace::trace_transaction::EXECUTION_UNSUPPORTED_BELOW_VERSION; use crate::Starknet; /// Estimate the L2 fee of a message sent on L1 @@ -36,11 +36,11 @@ pub async fn estimate_message_fee( ) -> StarknetRpcResult> { let block_info = starknet.get_block_info(&block_id)?; - if block_info.protocol_version() < &FALLBACK_TO_SEQUENCER_WHEN_VERSION_BELOW { + if block_info.protocol_version() < &EXECUTION_UNSUPPORTED_BELOW_VERSION { return Err(StarknetRpcApiError::UnsupportedTxnVersion); } - let exec_context = ExecutionContext::new_in_block(Arc::clone(&starknet.backend), &block_info)?; + let exec_context = ExecutionContext::new_at_block_end(Arc::clone(&starknet.backend), &block_info)?; let transaction = convert_message_into_transaction(message, starknet.chain_id()); let execution_result = exec_context diff --git a/crates/madara/client/rpc/src/versions/user/v0_7_1/methods/trace/simulate_transactions.rs b/crates/madara/client/rpc/src/versions/user/v0_7_1/methods/trace/simulate_transactions.rs index 3bd55fa66..70b49b0a0 100644 --- a/crates/madara/client/rpc/src/versions/user/v0_7_1/methods/trace/simulate_transactions.rs +++ b/crates/madara/client/rpc/src/versions/user/v0_7_1/methods/trace/simulate_transactions.rs @@ -1,4 +1,4 @@ -use super::trace_transaction::FALLBACK_TO_SEQUENCER_WHEN_VERSION_BELOW; +use super::trace_transaction::EXECUTION_UNSUPPORTED_BELOW_VERSION; use crate::errors::{StarknetRpcApiError, StarknetRpcResult}; use crate::utils::ResultExt; use crate::Starknet; @@ -18,10 +18,10 @@ pub async fn simulate_transactions( let block_info = starknet.get_block_info(&block_id)?; let starknet_version = *block_info.protocol_version(); - if starknet_version < FALLBACK_TO_SEQUENCER_WHEN_VERSION_BELOW { + if starknet_version < EXECUTION_UNSUPPORTED_BELOW_VERSION { return Err(StarknetRpcApiError::UnsupportedTxnVersion); } - let exec_context = ExecutionContext::new_in_block(Arc::clone(&starknet.backend), &block_info)?; + let exec_context = ExecutionContext::new_at_block_end(Arc::clone(&starknet.backend), &block_info)?; let charge_fee = !simulation_flags.contains(&SimulationFlag::SkipFeeCharge); let validate = !simulation_flags.contains(&SimulationFlag::SkipValidate); diff --git a/crates/madara/client/rpc/src/versions/user/v0_7_1/methods/trace/trace_block_transactions.rs b/crates/madara/client/rpc/src/versions/user/v0_7_1/methods/trace/trace_block_transactions.rs index 0bc43413f..890cf4da6 100644 --- a/crates/madara/client/rpc/src/versions/user/v0_7_1/methods/trace/trace_block_transactions.rs +++ b/crates/madara/client/rpc/src/versions/user/v0_7_1/methods/trace/trace_block_transactions.rs @@ -7,7 +7,7 @@ use starknet_types_core::felt::Felt; use starknet_types_rpc::TraceBlockTransactionsResult; use std::sync::Arc; -use super::trace_transaction::FALLBACK_TO_SEQUENCER_WHEN_VERSION_BELOW; +use super::trace_transaction::EXECUTION_UNSUPPORTED_BELOW_VERSION; use crate::errors::{StarknetRpcApiError, StarknetRpcResult}; use crate::utils::ResultExt; use crate::Starknet; @@ -18,11 +18,11 @@ pub async fn trace_block_transactions( ) -> StarknetRpcResult>> { let block = starknet.get_block(&block_id)?; - if block.info.protocol_version() < &FALLBACK_TO_SEQUENCER_WHEN_VERSION_BELOW { + if block.info.protocol_version() < &EXECUTION_UNSUPPORTED_BELOW_VERSION { return Err(StarknetRpcApiError::UnsupportedTxnVersion); } - let exec_context = ExecutionContext::new_in_block(Arc::clone(&starknet.backend), &block.info)?; + let exec_context = ExecutionContext::new_at_block_start(Arc::clone(&starknet.backend), &block.info)?; let transactions: Vec<_> = block .inner diff --git a/crates/madara/client/rpc/src/versions/user/v0_7_1/methods/trace/trace_transaction.rs b/crates/madara/client/rpc/src/versions/user/v0_7_1/methods/trace/trace_transaction.rs index 3040d34a8..207b5aac3 100644 --- a/crates/madara/client/rpc/src/versions/user/v0_7_1/methods/trace/trace_transaction.rs +++ b/crates/madara/client/rpc/src/versions/user/v0_7_1/methods/trace/trace_transaction.rs @@ -11,8 +11,8 @@ use starknet_types_core::felt::Felt; use starknet_types_rpc::TraceBlockTransactionsResult; use std::sync::Arc; -// For now, we fallback to the sequencer - that is what pathfinder and juno do too, but this is temporary -pub const FALLBACK_TO_SEQUENCER_WHEN_VERSION_BELOW: StarknetVersion = StarknetVersion::V0_13_0; +/// Blockifier does not support execution for versions earlier than that. +pub const EXECUTION_UNSUPPORTED_BELOW_VERSION: StarknetVersion = StarknetVersion::V0_13_0; pub async fn trace_transaction( starknet: &Starknet, @@ -24,11 +24,11 @@ pub async fn trace_transaction( .or_internal_server_error("Error while getting block from tx hash")? .ok_or(StarknetRpcApiError::TxnHashNotFound)?; - if block.info.protocol_version() < &FALLBACK_TO_SEQUENCER_WHEN_VERSION_BELOW { + if block.info.protocol_version() < &EXECUTION_UNSUPPORTED_BELOW_VERSION { return Err(StarknetRpcApiError::UnsupportedTxnVersion); } - let exec_context = ExecutionContext::new_in_block(Arc::clone(&starknet.backend), &block.info)?; + let exec_context = ExecutionContext::new_at_block_start(Arc::clone(&starknet.backend), &block.info)?; let mut block_txs = Iterator::zip(block.inner.transactions.into_iter(), block.info.tx_hashes()).map(|(tx, hash)| {