Merge pull request #10 from dojoengine/cairo-2.7-newer

refactor: add Dojo patch for `StatefulValidator`

crates/blockifier/src/blockifier/stateful_validator.rs

@@ -1,7 +1,9 @@
+#![allow(unused)]
+
 use std::sync::Arc;
 
 use cairo_vm::vm::runners::cairo_runner::ExecutionResources;
-use starknet_api::core::Nonce;
+use starknet_api::core::{ContractAddress, Nonce};
 use starknet_api::transaction::TransactionHash;
 use starknet_types_core::felt::Felt;
 use thiserror::Error;
@@ -16,10 +18,10 @@ use crate::fee::actual_cost::TransactionReceipt;
 use crate::fee::fee_checks::PostValidationReport;
 use crate::state::cached_state::CachedState;
 use crate::state::errors::StateError;
-use crate::state::state_api::StateReader;
+use crate::state::state_api::{State, StateReader};
 use crate::transaction::account_transaction::AccountTransaction;
 use crate::transaction::errors::{TransactionExecutionError, TransactionPreValidationError};
-use crate::transaction::objects::TransactionInfo;
+use crate::transaction::objects::{TransactionExecutionResult, TransactionInfo};
 use crate::transaction::transaction_execution::Transaction;
 use crate::transaction::transactions::ValidatableTransaction;
 
@@ -43,7 +45,7 @@ pub type StatefulValidatorResult<T> = Result<T, StatefulValidatorError>;
 
 /// Manages state related transaction validations for pre-execution flows.
 pub struct StatefulValidator<S: StateReader> {
-    tx_executor: TransactionExecutor<S>,
+    pub tx_executor: TransactionExecutor<S>,
     max_nonce_for_validation_skip: Nonce,
 }
 
@@ -58,10 +60,26 @@ impl<S: StateReader> StatefulValidator<S> {
         Self { tx_executor, max_nonce_for_validation_skip }
     }
 
+    /// Perform validations on an account transaction.
+    ///
+    /// # Arguments
+    ///
+    /// * `tx` - The account transaction to validate.
+    /// * `skip_validate` - If true, skip the account validation.
+    /// * `skip_fee_check` - If true, ignore any fee related checks on the transaction and account
+    ///   balance.
+    ///
+    /// NOTE:
+    ///
+    /// We add a flag specifically for avoiding fee checks to allow the pool validator
+    /// in Katana to run in 'fee disabled' mode. Basically, to adapt StatefulValidator to Katana's
+    /// execution flag abstraction (Katana's config that allows running in fee-disabled or
+    /// no-validation mode).
     pub fn perform_validations(
         &mut self,
         tx: AccountTransaction,
-        deploy_account_tx_hash: Option<TransactionHash>,
+        skip_validate: bool,
+        skip_fee_check: bool,
     ) -> StatefulValidatorResult<()> {
         // Deploy account transactions should be fully executed, since the constructor must run
         // before `__validate_deploy__`. The execution already includes all necessary validations,
@@ -75,23 +93,34 @@ impl<S: StateReader> StatefulValidator<S> {
         // processed. It is done before the pre-validations checks because, in these checks, we
         // change the state (more precisely, we increment the nonce).
         let tx_context = self.tx_executor.block_context.to_tx_context(&tx);
-        let skip_validate = self.skip_validate_due_to_unprocessed_deploy_account(
-            &tx_context.tx_info,
-            deploy_account_tx_hash,
-        )?;
-        self.perform_pre_validation_stage(&tx, &tx_context)?;
-
-        if skip_validate {
-            return Ok(());
+        // let skip_validate = self.skip_validate_due_to_unprocessed_deploy_account(
+        //     &tx_context.tx_info,
+        //     deploy_account_tx_hash,
+        // )?;
+        self.perform_pre_validation_stage(&tx, &tx_context, !skip_fee_check)?;
+
+        if !skip_validate {
+            // `__validate__` call.
+            let versioned_constants = &tx_context.block_context.versioned_constants();
+            let (_optional_call_info, actual_cost) =
+                self.validate(&tx, versioned_constants.tx_initial_gas())?;
+
+            // Post validations.
+            PostValidationReport::verify(&tx_context, &actual_cost)?;
         }
 
-        // `__validate__` call.
-        let versioned_constants = &tx_context.block_context.versioned_constants();
-        let (_optional_call_info, actual_cost) =
-            self.validate(&tx, versioned_constants.tx_initial_gas())?;
-
-        // Post validations.
-        PostValidationReport::verify(&tx_context, &actual_cost)?;
+        // See similar comment in `run_revertible` for context.
+        //
+        // From what I've seen there is not suitable method that is used by both the validator and
+        // the normal transaction flow where the nonce increment logic can be placed. So
+        // this is manually placed here.
+        //
+        // TODO: find a better place to put this without needing this duplication.
+        self.tx_executor
+            .block_state
+            .as_mut()
+            .expect(BLOCK_STATE_ACCESS_ERR)
+            .increment_nonce(tx_context.tx_info.sender_address())?;
 
         Ok(())
     }
@@ -105,14 +134,14 @@ impl<S: StateReader> StatefulValidator<S> {
         &mut self,
         tx: &AccountTransaction,
         tx_context: &TransactionContext,
+        fee_check: bool,
     ) -> StatefulValidatorResult<()> {
         let strict_nonce_check = false;
         // Run pre-validation in charge fee mode to perform fee and balance related checks.
-        let charge_fee = true;
         tx.perform_pre_validation_stage(
             self.tx_executor.block_state.as_mut().expect(BLOCK_STATE_ACCESS_ERR),
             tx_context,
-            charge_fee,
+            fee_check,
             strict_nonce_check,
         )?;
 
@@ -181,4 +210,16 @@ impl<S: StateReader> StatefulValidator<S> {
 
         Ok((validate_call_info, tx_receipt))
     }
+
+    pub fn get_nonce(
+        &mut self,
+        account_address: ContractAddress,
+    ) -> StatefulValidatorResult<Nonce> {
+        Ok(self
+            .tx_executor
+            .block_state
+            .as_ref()
+            .expect(BLOCK_STATE_ACCESS_ERR)
+            .get_nonce_at(account_address)?)
+    }
 }

crates/blockifier/src/blockifier/stateful_validator_test.rs

@@ -65,6 +65,6 @@ fn test_transaction_validator(
     // Test the stateful validator.
     let mut stateful_validator = StatefulValidator::create(state, block_context, nonce!(0_u32));
 
-    let reuslt = stateful_validator.perform_validations(tx, None);
+    let reuslt = stateful_validator.perform_validations(tx, false, false);
     assert!(reuslt.is_ok(), "Validation failed: {:?}", reuslt.unwrap_err());
 }

crates/blockifier/src/blockifier/transaction_executor_test.rs

@@ -56,7 +56,10 @@ fn tx_executor_test_body<S: StateReader>(
     TransactionVersion::ZERO,
     CairoVersion::Cairo0,
     BouncerWeights {
-        state_diff_size: 0,
+	    // The state diff size is 2 because to support declaring Cairo0 contracts, we
+		// need to include the compiled class hash and declared contracts of the Cairo
+		// 0 contract in the state diff.
+        state_diff_size: 2,
         message_segment_length: 0,
         n_events: 0,
         ..Default::default()
@@ -66,7 +69,7 @@ fn tx_executor_test_body<S: StateReader>(
     TransactionVersion::ONE,
     CairoVersion::Cairo0,
     BouncerWeights {
-        state_diff_size: 2,
+        state_diff_size: 4,
         message_segment_length: 0,
         n_events: 0,
         ..Default::default()

crates/blockifier/src/concurrency/test_utils.rs

@@ -76,7 +76,8 @@ pub fn create_fee_transfer_call_info<S: StateReader>(
 ) -> CallInfo {
     let block_context = BlockContext::create_for_account_testing();
     let mut transactional_state = TransactionalState::create_transactional(state);
-    let execution_flags = ExecutionFlags { charge_fee: true, validate: true, concurrency_mode };
+    let execution_flags =
+        ExecutionFlags { charge_fee: true, validate: true, concurrency_mode, nonce_check: true };
     let execution_info =
         account_tx.execute_raw(&mut transactional_state, &block_context, execution_flags).unwrap();
 

crates/blockifier/src/concurrency/versioned_state_test.rs

@@ -251,11 +251,11 @@ fn test_run_parallel_txs() {
     // Execute transactions
     thread::scope(|s| {
         s.spawn(move || {
-            let result = account_tx_1.execute(&mut state_1, &block_context_1, true, true);
+            let result = account_tx_1.execute(&mut state_1, &block_context_1, true, true, true);
             assert_eq!(result.is_err(), enforce_fee);
         });
         s.spawn(move || {
-            account_tx_2.execute(&mut state_2, &block_context_2, true, true).unwrap();
+            account_tx_2.execute(&mut state_2, &block_context_2, true, true, true).unwrap();
 
             // Check that the constructor wrote ctor_arg to the storage.
             let storage_key = get_storage_var_address("ctor_arg", &[]);

crates/blockifier/src/concurrency/worker_logic.rs

@@ -126,8 +126,12 @@ impl<'a, S: StateReader> WorkerExecutor<'a, S> {
         let tx = &self.chunk[tx_index];
         let mut transactional_state =
             TransactionalState::create_transactional(&mut tx_versioned_state);
-        let execution_flags =
-            ExecutionFlags { charge_fee: true, validate: true, concurrency_mode: true };
+        let execution_flags = ExecutionFlags {
+            charge_fee: true,
+            validate: true,
+            concurrency_mode: true,
+            nonce_check: true,
+        };
         let execution_result =
             tx.execute_raw(&mut transactional_state, self.block_context, execution_flags);
 

crates/blockifier/src/execution/entry_point.rs

@@ -3,7 +3,7 @@ use std::cmp::min;
 use std::sync::Arc;
 
 use cairo_vm::vm::runners::cairo_runner::{ExecutionResources, ResourceTracker, RunResources};
-use num_traits::{Inv, Zero};
+use num_traits::{Inv, ToPrimitive, Zero};
 use serde::Serialize;
 use starknet_api::core::{ClassHash, ContractAddress, EntryPointSelector};
 use starknet_api::deprecated_contract_class::EntryPointType;
@@ -173,10 +173,10 @@ impl EntryPointExecutionContext {
             // TODO(Ori, 1/2/2024): Write an indicative expect message explaining why the conversion
             // works.
             ExecutionMode::Validate => {
-                block_context.versioned_constants.validate_max_n_steps as usize
+                block_context.versioned_constants.validate_max_n_steps.to_usize().unwrap()
             }
             ExecutionMode::Execute => {
-                block_context.versioned_constants.invoke_tx_max_n_steps as usize
+                block_context.versioned_constants.invoke_tx_max_n_steps.to_usize().unwrap()
             }
         };
 

crates/blockifier/src/execution/execution_utils.rs

@@ -152,6 +152,7 @@ pub struct ReadOnlySegment {
 pub struct ReadOnlySegments(Vec<ReadOnlySegment>);
 
 impl ReadOnlySegments {
+    #[allow(clippy::ptr_arg)]
     pub fn allocate(
         &mut self,
         vm: &mut VirtualMachine,

crates/blockifier/src/lib.rs

@@ -2,11 +2,7 @@
 // length to pointer type ([not necessarily true](https://github.com/rust-lang/rust/issues/65473),
 // but it is a reasonable assumption for now), this attribute protects against potential overflow
 // when converting usize to u128.
-#![cfg(any(
-    target_pointer_width = "16",
-    target_pointer_width = "32",
-    target_pointer_width = "64",
-))]
+#![cfg(any(target_pointer_width = "16", target_pointer_width = "32", target_pointer_width = "64",))]
 
 #[cfg(feature = "jemalloc")]
 // Override default allocator.

crates/blockifier/src/transaction/account_transaction.rs

@@ -230,6 +230,12 @@ impl AccountTransaction {
         Ok(())
     }
 
+    /// This method no longer increments the nonce. Refer to individual functions to see
+    /// in which stage the nonce is incremented.
+    ///
+    /// Nonce incremental logics manually placed in
+    /// [`AccountTransaction::run_revertible`], [`AccountTransaction::run_non_revertible`],
+    /// [`StatefulValidator::perform_validations`](crate::blockifier::stateful_validator::StatefulValidator::perform_validations)
     fn handle_nonce(
         state: &mut dyn State,
         tx_info: &TransactionInfo,
@@ -248,7 +254,8 @@ impl AccountTransaction {
             account_nonce <= incoming_tx_nonce
         };
         if valid_nonce {
-            return Ok(state.increment_nonce(address)?);
+            // return Ok(state.increment_nonce(address)?);
+            return Ok(());
         }
         Err(TransactionPreValidationError::InvalidNonce {
             address,
@@ -422,7 +429,18 @@ impl AccountTransaction {
         let mut resources = ExecutionResources::default();
         let validate_call_info: Option<CallInfo>;
         let execute_call_info: Option<CallInfo>;
+
         if matches!(self, Self::DeployAccount(_)) {
+            // See similar comment in `run_revertible` for context. Not sure for this case if
+            // incrementing the nonce at this stage is correct.
+            //
+            // Only increment the nonce if it's not V0 transaction. Why? not exactly sure but
+            // that's what the tests implies.
+            if !tx_context.tx_info.is_v0() {
+                // See similar comment in `run_revertible` for context.
+                state.increment_nonce(tx_context.tx_info.sender_address())?;
+            }
+
             // Handle `DeployAccount` transactions separately, due to different order of things.
             // Also, the execution context required form the `DeployAccount` execute phase is
             // validation context.
@@ -449,6 +467,14 @@ impl AccountTransaction {
                 validate,
                 charge_fee,
             )?;
+
+            // Only increment the nonce if it's not V0 transaction. Why? not exactly sure but
+            // that's what the tests implies.
+            if !execution_context.tx_context.tx_info.is_v0() {
+                // See similar comment in `run_revertible` for context.
+                state.increment_nonce(tx_context.tx_info.sender_address())?;
+            }
+
             execute_call_info =
                 self.run_execute(state, &mut resources, &mut execution_context, remaining_gas)?;
         }
@@ -495,6 +521,23 @@ impl AccountTransaction {
             charge_fee,
         )?;
 
+        // Increment the sender nonce only after the tx has passed validation.
+        //
+        // NOTE:
+        //
+        // Before this, the nonce is incremented in `AccountTransaction::handle_nonce` which is
+        // called at the initial stage of transaction processing (ie in
+        // ExecutableTransaction::execute_raw of AccountTransaction), which is even before the
+        // account validation logic is being run. Which is weird because the nonce would get
+        // incremented even if the transaction failed validation. And this is not what I
+        // observed from mainnet/testnet. So, I moved the nonce incrementation here.
+        //
+        // Only increment the nonce if it's not V0 transaction. Why? not exactly sure but
+        // that's what the tests implies.
+        if !execution_context.tx_context.tx_info.is_v0() {
+            state.increment_nonce(tx_context.tx_info.sender_address())?;
+        }
+
         let n_allotted_execution_steps = execution_context.subtract_validation_and_overhead_steps(
             &validate_call_info,
             &self.tx_type(),

crates/blockifier/src/transaction/transactions_test.rs

@@ -1015,6 +1015,15 @@ fn test_invalid_nonce(
         .perform_pre_validation_stage(&mut transactional_state, &valid_tx_context, false, false)
         .unwrap();
 
+    // See comments on changing where nonce is incremented in `ExecutableTransaction::execute_raw`
+    // of AccountTransaction.
+    //
+    // Basically, before this, the nonce is incremented in `perform_pre_validation_stage` but that
+    // method is called even before the account validation stage is executed. So, now we have to
+    // increment the sender's account nonce manually here.
+    let account = valid_tx_context.tx_info.sender_address();
+    transactional_state.increment_nonce(account).expect("failed to increment nonce");
+
     // Negative flow: account nonce = 1, incoming tx nonce = 0.
     let invalid_nonce = nonce!(0_u8);
     let invalid_tx =
@@ -1063,12 +1072,18 @@ fn declare_expected_state_changes_count(version: TransactionVersion) -> StateCha
     if version == TransactionVersion::ZERO {
         StateChangesCount {
             n_storage_updates: 1, // Sender balance.
+            // Supposed to be ZERO, but because due to the changes made for supporting declaring
+            // Cairo 0 contracts, `n_compiled_class_hash_updates` is 1 for V0 declare transactions.
+            n_compiled_class_hash_updates: 1, // Also set compiled class hash.
             ..StateChangesCount::default()
         }
     } else if version == TransactionVersion::ONE {
         StateChangesCount {
             n_storage_updates: 1,    // Sender balance.
             n_modified_contracts: 1, // Nonce.
+            // Supposed to be ZERO, but because due to the changes made for supporting declaring
+            // Cairo 0 contracts, `n_compiled_class_hash_updates` is 1 for V0 declare transactions.
+            n_compiled_class_hash_updates: 1, // Also set compiled class hash.
             ..StateChangesCount::default()
         }
     } else if version == TransactionVersion::TWO || version == TransactionVersion::THREE {

scripts/clippy.sh

@@ -1,4 +1,4 @@
 #!/bin/bash
 
-cargo clippy "$@" --all-targets --all-features -- -D warnings -D future-incompatible \
+cargo +nightly-2024-01-12 clippy "$@" --all-targets --all-features -- -D warnings -D future-incompatible \
     -D nonstandard-style -D rust-2018-idioms -D unused