fix(rpc): call, simulate, estimate rpcs executed on top of the block,…

… not at the start of it

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
 - fix: trim hash of eth state was failing with 0x0
 - fix: devnet accounts getting deployed in sequencer mode
 - fix(rpc): fix BroadcastedDeclareTxn V3 in starknet-types-rpc

crates/client/block_production/src/lib.rs

@@ -148,8 +148,8 @@ impl<Mempool: MempoolProvider> BlockProductionTask<Mempool> {
             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,
@@ -411,7 +411,8 @@ impl<Mempool: MempoolProvider> BlockProductionTask<Mempool> {
 
         // Prepare 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();

crates/client/exec/src/block_context.rs

@@ -15,7 +15,8 @@ use std::sync::Arc;
 pub struct ExecutionContext {
     pub(crate) backend: Arc<MadaraBackend>,
     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<DbBlockId>,
 }
 
 impl ExecutionContext {
@@ -33,53 +34,91 @@ impl ExecutionContext {
     }
 
     pub fn init_cached_state(&self) -> CachedState<BlockifierStateAdapter> {
-        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<MadaraBackend>, block_info: &MadaraMaybePendingBlockInfo) -> Result<Self, Error> {
-        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<MadaraBackend>,
+        block_info: &MadaraMaybePendingBlockInfo,
+    ) -> Result<Self, Error> {
+        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<MadaraBackend>,
+        block_info: &MadaraMaybePendingBlockInfo,
+    ) -> Result<Self, Error> {
+        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<MadaraBackend>,
+        block_info: &MadaraMaybePendingBlockInfo,
+        latest_visible_block: Option<DbBlockId>,
+        block_number: u64,
+    ) -> Result<Self, Error> {
+        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,
         })
     }

crates/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)?;

crates/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 };

crates/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<DbBlockId>);
+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, "<none>"),
+        }
+    }
+}
+impl From<Option<DbBlockId>> for OnTopOf {
+    fn from(value: Option<DbBlockId>) -> 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,

crates/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) {

crates/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<Felt>, block_id: BlockId) -> StarknetRpcResult<Vec<Felt>> {
     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);
     }
 

crates/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<SimulationFlagForEstimateFee>,
     block_id: BlockId,
 ) -> StarknetRpcResult<Vec<FeeEstimate<Felt>>> {
+    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()

crates/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<FeeEstimate<Felt>> {
     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

crates/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);