fix(katana): per transaction validator (#2362)

Create a new validator instance for every transaction to be validated instead of creating a single instance to be used throughout the duration of a block

crates/dojo-test-utils/src/sequencer.rs

@@ -3,6 +3,7 @@ use std::sync::Arc;
 use jsonrpsee::core::Error;
 pub use katana_core::backend::config::{Environment, StarknetConfig};
 use katana_core::backend::Backend;
+use katana_core::constants::DEFAULT_SEQUENCER_ADDRESS;
 #[allow(deprecated)]
 pub use katana_core::sequencer::SequencerConfig;
 use katana_executor::implementation::blockifier::BlockifierFactory;
@@ -126,9 +127,12 @@ impl TestSequencer {
 }
 
 pub fn get_default_test_starknet_config() -> StarknetConfig {
-    StarknetConfig {
+    let mut cfg = StarknetConfig {
         disable_fee: true,
         env: Environment { chain_id: ChainId::SEPOLIA, ..Default::default() },
         ..Default::default()
-    }
+    };
+
+    cfg.genesis.sequencer_address = *DEFAULT_SEQUENCER_ADDRESS;
+    cfg
 }

crates/dojo-utils/src/tx/waiter.rs

@@ -30,10 +30,10 @@ pub enum TransactionWaitingError {
     Provider(ProviderError),
 }
 
-#[allow(clippy::too_long_first_doc_paragraph)]
-/// A type that waits for a transaction to achieve the desired status. The waiter will poll for the
-/// transaction receipt every `interval` miliseconds until it achieves the desired status or until
-/// `timeout` is reached.
+/// Utility for waiting on a transaction.
+///
+/// The waiter will poll for the transaction receipt every `interval` miliseconds until it achieves
+/// the desired status or until `timeout` is reached.
 ///
 /// The waiter can be configured to wait for a specific finality status (e.g, `ACCEPTED_ON_L2`), by
 /// default, it only waits until the transaction is included in the _pending_ block. It can also be

crates/dojo-world/src/config/profile_config.rs

@@ -11,8 +11,8 @@ use super::migration_config::MigrationConfig;
 use super::namespace_config::NamespaceConfig;
 use super::world_config::WorldConfig;
 
-#[allow(clippy::too_long_first_doc_paragraph)]
 /// Profile configuration that is used to configure the world and the environment.
+///
 /// This [`ProfileConfig`] is expected to be loaded from a TOML file that is located
 /// next to the `Scarb.toml` file, named with the profile name.
 #[derive(Debug, Clone, Default, Deserialize)]

crates/katana/core/src/service/block_producer.rs

@@ -220,7 +220,7 @@ impl<EF: ExecutorFactory> IntervalBlockProducer<EF> {
         let cfg = backend.executor_factory.cfg();
         let flags = backend.executor_factory.execution_flags();
         let validator =
-            TxValidator::new(state, flags.clone(), cfg.clone(), &block_env, permit.clone());
+            TxValidator::new(state, flags.clone(), cfg.clone(), block_env, permit.clone());
 
         Self {
             validator,
@@ -262,7 +262,7 @@ impl<EF: ExecutorFactory> IntervalBlockProducer<EF> {
                 let num = provider.latest_number().unwrap();
                 let block_env = provider.block_env_at(num.into()).unwrap().unwrap();
 
-                self.validator.update(state, &block_env);
+                self.validator.update(state, block_env);
 
                 // -------------------------------------------
 
@@ -448,7 +448,7 @@ impl<EF: ExecutorFactory> Stream for IntervalBlockProducer<EF> {
                                 let num = provider.latest_number()?;
                                 let block_env = provider.block_env_at(num.into()).unwrap().unwrap();
 
-                                pin.validator.update(state, &block_env);
+                                pin.validator.update(state, block_env);
 
                                 // -------------------------------------------
 
@@ -514,7 +514,7 @@ impl<EF: ExecutorFactory> InstantBlockProducer<EF> {
         let cfg = backend.executor_factory.cfg();
         let flags = backend.executor_factory.execution_flags();
         let validator =
-            TxValidator::new(state, flags.clone(), cfg.clone(), &block_env, permit.clone());
+            TxValidator::new(state, flags.clone(), cfg.clone(), block_env, permit.clone());
 
         Self {
             permit,
@@ -599,7 +599,7 @@ impl<EF: ExecutorFactory> InstantBlockProducer<EF> {
         let state = provider.latest()?;
         let latest_num = provider.latest_number()?;
         let block_env = provider.block_env_at(latest_num.into())?.expect("latest");
-        validator.update(state, &block_env);
+        validator.update(state, block_env);
 
         // -------------------------------------------
 

crates/katana/pool/src/validation/stateful.rs

@@ -1,3 +1,4 @@
+use std::collections::HashMap;
 use std::sync::Arc;
 
 use katana_executor::implementation::blockifier::blockifier::blockifier::stateful_validator::{
@@ -9,12 +10,14 @@ use katana_executor::implementation::blockifier::blockifier::transaction::errors
 };
 use katana_executor::implementation::blockifier::blockifier::transaction::transaction_execution::Transaction;
 use katana_executor::implementation::blockifier::utils::{
-    block_context_from_envs, to_address, to_blk_address, to_executor_tx,
+    block_context_from_envs, to_address, to_executor_tx,
 };
 use katana_executor::{SimulationFlag, StateProviderDb};
 use katana_primitives::contract::{ContractAddress, Nonce};
 use katana_primitives::env::{BlockEnv, CfgEnv};
 use katana_primitives::transaction::{ExecutableTx, ExecutableTxWithHash};
+use katana_primitives::FieldElement;
+use katana_provider::error::ProviderError;
 use katana_provider::traits::state::StateProvider;
 use parking_lot::Mutex;
 
@@ -24,144 +27,145 @@ use crate::tx::PoolTransaction;
 #[allow(missing_debug_implementations)]
 #[derive(Clone)]
 pub struct TxValidator {
-    inner: Arc<Inner>,
+    inner: Arc<Mutex<Inner>>,
+    permit: Arc<Mutex<()>>,
 }
 
 struct Inner {
+    // execution context
     cfg_env: CfgEnv,
+    block_env: BlockEnv,
     execution_flags: SimulationFlag,
-    validator: Mutex<StatefulValidatorAdapter>,
-    permit: Arc<Mutex<()>>,
+    state: Arc<Box<dyn StateProvider>>,
+
+    pool_nonces: HashMap<ContractAddress, Nonce>,
 }
 
 impl TxValidator {
     pub fn new(
         state: Box<dyn StateProvider>,
         execution_flags: SimulationFlag,
         cfg_env: CfgEnv,
-        block_env: &BlockEnv,
+        block_env: BlockEnv,
         permit: Arc<Mutex<()>>,
     ) -> Self {
-        let validator = StatefulValidatorAdapter::new(state, block_env, &cfg_env);
-        Self {
-            inner: Arc::new(Inner {
-                permit,
-                cfg_env,
-                execution_flags,
-                validator: Mutex::new(validator),
-            }),
-        }
+        let inner = Arc::new(Mutex::new(Inner {
+            cfg_env,
+            block_env,
+            execution_flags,
+            state: Arc::new(state),
+            pool_nonces: HashMap::new(),
+        }));
+        Self { permit, inner }
     }
 
     /// Reset the state of the validator with the given params. This method is used to update the
     /// validator's state with a new state and block env after a block is mined.
-    pub fn update(&self, new_state: Box<dyn StateProvider>, block_env: &BlockEnv) {
-        let mut validator = self.inner.validator.lock();
-
-        let mut state = validator.inner.tx_executor.block_state.take().unwrap();
-        state.state = StateProviderDb::new(new_state);
-
-        *validator = StatefulValidatorAdapter::new_inner(state, block_env, &self.inner.cfg_env);
+    pub fn update(&self, new_state: Box<dyn StateProvider>, block_env: BlockEnv) {
+        let mut this = self.inner.lock();
+        this.block_env = block_env;
+        this.state = Arc::new(new_state);
     }
 
     // NOTE:
     // If you check the get_nonce method of StatefulValidator in blockifier, under the hood it
     // unwraps the Option to get the state of the TransactionExecutor struct. StatefulValidator
     // guaranteees that the state will always be present so it is safe to uwnrap. However, this
     // safety is not guaranteed by TransactionExecutor itself.
-    pub fn get_nonce(&self, address: ContractAddress) -> Nonce {
-        let address = to_blk_address(address);
-        let nonce = self.inner.validator.lock().inner.get_nonce(address).expect("state err");
-        nonce.0
+    pub fn pool_nonce(&self, address: ContractAddress) -> Result<Option<Nonce>, ProviderError> {
+        let this = self.inner.lock();
+        match this.pool_nonces.get(&address) {
+            Some(nonce) => Ok(Some(*nonce)),
+            None => Ok(this.state.nonce(address)?),
+        }
     }
 }
 
-#[allow(missing_debug_implementations)]
-struct StatefulValidatorAdapter {
-    inner: StatefulValidator<StateProviderDb<'static>>,
-}
-
-impl StatefulValidatorAdapter {
-    fn new(state: Box<dyn StateProvider>, block_env: &BlockEnv, cfg_env: &CfgEnv) -> Self {
-        let state = CachedState::new(StateProviderDb::new(state));
-        Self::new_inner(state, block_env, cfg_env)
-    }
+impl Inner {
+    // Prepare the stateful validator with the current state and block env to be used
+    // for transaction validation.
+    fn prepare(&self) -> StatefulValidator<StateProviderDb<'static>> {
+        let state = Box::new(self.state.clone());
+        let cached_state = CachedState::new(StateProviderDb::new(state));
+        let context = block_context_from_envs(&self.block_env, &self.cfg_env);
 
-    fn new_inner(
-        state: CachedState<StateProviderDb<'static>>,
-        block_env: &BlockEnv,
-        cfg_env: &CfgEnv,
-    ) -> Self {
-        let block_context = block_context_from_envs(block_env, cfg_env);
-        let inner = StatefulValidator::create(state, block_context, Default::default());
-        Self { inner }
-    }
-
-    /// Used only in the [`Validator::validate`] trait
-    fn validate(
-        &mut self,
-        tx: ExecutableTxWithHash,
-        skip_validate: bool,
-        skip_fee_check: bool,
-    ) -> ValidationResult<ExecutableTxWithHash> {
-        match to_executor_tx(tx.clone()) {
-            Transaction::AccountTransaction(blockifier_tx) => {
-                // Check if the transaction nonce is higher than the current account nonce,
-                // if yes, dont't run its validation logic but tag it as dependent
-                let account = to_blk_address(tx.sender());
-                let account_nonce = self.inner.get_nonce(account).expect("state err");
-
-                if tx.nonce() > account_nonce.0 {
-                    return Ok(ValidationOutcome::Dependent {
-                        current_nonce: account_nonce.0,
-                        tx_nonce: tx.nonce(),
-                        tx,
-                    });
-                }
-
-                match self.inner.perform_validations(blockifier_tx, skip_validate, skip_fee_check) {
-                    Ok(()) => Ok(ValidationOutcome::Valid(tx)),
-                    Err(e) => match map_invalid_tx_err(e) {
-                        Ok(error) => Ok(ValidationOutcome::Invalid { tx, error }),
-                        Err(error) => Err(Error { hash: tx.hash, error }),
-                    },
-                }
-            }
-
-            // we skip validation for L1HandlerTransaction
-            Transaction::L1HandlerTransaction(_) => Ok(ValidationOutcome::Valid(tx)),
-        }
+        StatefulValidator::create(cached_state, context, Default::default())
     }
 }
 
 impl Validator for TxValidator {
     type Transaction = ExecutableTxWithHash;
 
     fn validate(&self, tx: Self::Transaction) -> ValidationResult<Self::Transaction> {
-        let _permit = self.inner.permit.lock();
-        let this = &mut *self.inner.validator.lock();
+        let _permit = self.permit.lock();
+        let mut this = self.inner.lock();
+
+        let tx_nonce = tx.nonce();
+        let address = tx.sender();
+
+        // Get the current nonce of the account from the pool or the state
+        let current_nonce = if let Some(nonce) = this.pool_nonces.get(&address) {
+            *nonce
+        } else {
+            this.state.nonce(address).unwrap().unwrap_or_default()
+        };
+
+        // Check if the transaction nonce is higher than the current account nonce,
+        // if yes, dont't run its validation logic and tag it as a dependent tx.
+        if tx_nonce > current_nonce {
+            return Ok(ValidationOutcome::Dependent { current_nonce, tx_nonce, tx });
+        }
 
         // Check if validation of an invoke transaction should be skipped due to deploy_account not
         // being proccessed yet. This feature is used to improve UX for users sending
         // deploy_account + invoke at once.
         let skip_validate = match tx.transaction {
             // we skip validation for invoke tx with nonce 1 and nonce 0 in the state, this
             ExecutableTx::DeployAccount(_) | ExecutableTx::Declare(_) => false,
-
             // we skip validation for invoke tx with nonce 1 and nonce 0 in the state, this
-            _ => {
-                let address = to_blk_address(tx.sender());
-                let account_nonce = this.inner.get_nonce(address).expect("state err");
-                tx.nonce() == Nonce::ONE && account_nonce.0 == Nonce::ZERO
-            }
+            _ => tx.nonce() == Nonce::ONE && current_nonce == Nonce::ZERO,
         };
 
-        StatefulValidatorAdapter::validate(
-            this,
+        // prepare a stateful validator and validate the transaction
+        let result = validate(
+            this.prepare(),
             tx,
-            self.inner.execution_flags.skip_validate || skip_validate,
-            self.inner.execution_flags.skip_fee_transfer,
-        )
+            this.execution_flags.skip_validate || skip_validate,
+            this.execution_flags.skip_fee_transfer,
+        );
+
+        match result {
+            res @ Ok(ValidationOutcome::Valid { .. }) => {
+                // update the nonce of the account in the pool only for valid tx
+                let updated_nonce = current_nonce + FieldElement::ONE;
+                this.pool_nonces.insert(address, updated_nonce);
+                res
+            }
+            _ => result,
+        }
+    }
+}
+
+// perform validation on the pool transaction using the provided stateful validator
+fn validate(
+    mut validator: StatefulValidator<StateProviderDb<'static>>,
+    pool_tx: ExecutableTxWithHash,
+    skip_validate: bool,
+    skip_fee_check: bool,
+) -> ValidationResult<ExecutableTxWithHash> {
+    match to_executor_tx(pool_tx.clone()) {
+        Transaction::AccountTransaction(tx) => {
+            match validator.perform_validations(tx, skip_validate, skip_fee_check) {
+                Ok(()) => Ok(ValidationOutcome::Valid(pool_tx)),
+                Err(e) => match map_invalid_tx_err(e) {
+                    Ok(error) => Ok(ValidationOutcome::Invalid { tx: pool_tx, error }),
+                    Err(error) => Err(Error { hash: pool_tx.hash, error }),
+                },
+            }
+        }
+
+        // we skip validation for L1HandlerTransaction
+        Transaction::L1HandlerTransaction(_) => Ok(ValidationOutcome::Valid(pool_tx)),
     }
 }
 

crates/katana/rpc/rpc/src/starknet/mod.rs

@@ -298,13 +298,14 @@ impl<EF: ExecutorFactory> StarknetApi<EF> {
             //
             // TODO: this is a temporary solution, we should have a better way to handle this.
             // perhaps a pending/pool state provider that implements all the state provider traits.
-            if let BlockIdOrTag::Tag(BlockTag::Pending) = block_id {
-                let pool_nonce = this.inner.validator.get_nonce(contract_address);
-                return Ok(pool_nonce);
-            }
+            let result = if let BlockIdOrTag::Tag(BlockTag::Pending) = block_id {
+                this.inner.validator.pool_nonce(contract_address)?
+            } else {
+                let state = this.state(&block_id)?;
+                state.nonce(contract_address)?
+            };
 
-            let state = this.state(&block_id)?;
-            let nonce = state.nonce(contract_address)?.ok_or(StarknetApiError::ContractNotFound)?;
+            let nonce = result.ok_or(StarknetApiError::ContractNotFound)?;
             Ok(nonce)
         })
         .await

crates/katana/rpc/rpc/tests/common/mod.rs

@@ -62,3 +62,11 @@ pub fn build_deploy_cairo1_contract_call(class_hash: Felt, salt: Felt) -> Call {
         selector: get_selector_from_name("deployContract").unwrap(),
     }
 }
+
+/// Splits a Felt into two Felts, representing its lower and upper 128 bits.
+#[allow(unused)]
+pub fn split_felt(felt: Felt) -> (Felt, Felt) {
+    let low: Felt = (felt.to_biguint() & Felt::from(u128::MAX).to_biguint()).into();
+    let high = felt.to_biguint() >> 128;
+    (low, Felt::from(high))
+}