Fix transactions bypassing the max_fee by introducing new revert logic (#901)

* Make tx fail when actual_fee exceeds max_fee

* Changed test

* Formatting

* Fix logic

* Leave fail only without charging

* Change test

* Fix test broken by better fee calc

* Fixed test fee

* Update fee on test_deploy_account

* Remove comment

* Added fee transfer

* Test with invoke

* Added revert logic for invoke

* Modify tests, add fixes

* Add revert error

* Fix test_invoke_tx_account

* Fixed test_invoke_tx_exceeded_max_fee

* Fix test_get_nonce_at

* Rely on another contract

* Introduced transactional state (#917)

* Introduced transactional state

* WIP

* Fixed the rest of tests

* Replaced old revert logic from entrypoint exec

* depl acc revert test

* Remove update writes fix

* WIP Fixed many tests

* fix test

* fix more tests

* more fixes

* fix another test

* fix latest test

* name

* remove comment

* merge

* unignore

* format

* vis

* need to be pub for  tests

* fix test

* format

* use the count_actual_storage_changes impl from cached state

* fix bug

* fix tests

---------

Co-authored-by: Juan Bono <juanbono94@gmail.com>
Co-authored-by: Edgar Luque <git@edgarluque.com>

Author: 3 people

Cargo.lock

@@ -61,6 +61,7 @@ serde_json_pythonic = { git = "https://github.com/xJonathanLEI/serde_json_python
 [dev-dependencies]
 assert_matches = "1.5.0"
 coverage-helper = "0.1.0"
+pretty_assertions_sorted = "1.2.3"
 
 [[bench]]
 path = "bench/internals.rs"

Cargo.toml

@@ -27,7 +27,7 @@ use starknet_in_rust::{
         compiled_class::CompiledClass, deprecated_contract_class::ContractClass,
     },
     state::{cached_state::CachedState, state_api::State},
-    state::{in_memory_state_reader::InMemoryStateReader, ExecutionResourcesManager},
+    state::{in_memory_state_reader::InMemoryStateReader, ExecutionResourcesManager, StateDiff},
     transaction::{error::TransactionError, InvokeFunction},
     utils::{felt_to_hash, string_to_hash, Address},
 };
@@ -200,7 +200,9 @@ fn invoke_parser(
         Some(Felt252::zero()),
         transaction_hash.unwrap(),
     )?;
-    let _tx_info = internal_invoke.apply(cached_state, &BlockContext::default(), 0)?;
+    let mut transactional_state = cached_state.create_transactional();
+    let _tx_info = internal_invoke.apply(&mut transactional_state, &BlockContext::default(), 0)?;
+    cached_state.apply_state_update(&StateDiff::from_cached_state(transactional_state)?)?;
 
     let tx_hash = calculate_transaction_hash_common(
         TransactionHashPrefix::Invoke,

cli/src/main.rs

@@ -331,3 +331,29 @@ fn blockifier_test_case_tx(hash: &str, block_number: u64, chain: RpcChain) {
             .len()
     );
 }
+
+#[test_case(
+    "0x00b6d59c19d5178886b4c939656167db0660fe325345138025a3cc4175b21897",
+    200303, // real block     200304
+    RpcChain::MainNet
+)]
+#[test_case(
+    "0x02b28b4846a756e0cec6385d6d13f811e745a88c7e75a3ebc5fead5b4af152a3",
+    200302, // real block     200304
+    RpcChain::MainNet
+    => ignore["broken on both due to a cairo-vm error"]
+)]
+fn blockifier_test_case_reverted_tx(hash: &str, block_number: u64, chain: RpcChain) {
+    let (tx_info, trace, receipt) = execute_tx(hash, chain, BlockNumber(block_number));
+
+    assert_eq!(tx_info.revert_error.is_some(), trace.revert_error.is_some());
+
+    let diff = 100 * receipt.actual_fee.abs_diff(tx_info.actual_fee.0) / receipt.actual_fee;
+
+    if diff >= 5 {
+        assert_eq!(
+            tx_info.actual_fee.0, receipt.actual_fee,
+            "actual_fee mismatch differs from the baseline by more than 5% ({diff}%)",
+        );
+    }
+}

rpc_state_reader/tests/blockifier_tests.rs

@@ -293,3 +293,29 @@ fn starknet_in_rust_test_case_tx(hash: &str, block_number: u64, chain: RpcChain)
         }
     }
 }
+
+#[test_case(
+    "0x00b6d59c19d5178886b4c939656167db0660fe325345138025a3cc4175b21897",
+    200303, // real block     200304
+    RpcChain::MainNet
+)]
+#[test_case(
+    "0x02b28b4846a756e0cec6385d6d13f811e745a88c7e75a3ebc5fead5b4af152a3",
+    200302, // real block     200304
+    RpcChain::MainNet
+    => ignore["broken on both due to a cairo-vm error"]
+)]
+fn starknet_in_rust_test_case_reverted_tx(hash: &str, block_number: u64, chain: RpcChain) {
+    let (tx_info, trace, receipt) = execute_tx(hash, chain, BlockNumber(block_number));
+
+    assert_eq!(tx_info.revert_error.is_some(), trace.revert_error.is_some());
+
+    let diff = 100 * receipt.actual_fee.abs_diff(tx_info.actual_fee) / receipt.actual_fee;
+
+    if diff >= 5 {
+        assert_eq!(
+            tx_info.actual_fee, receipt.actual_fee,
+            "actual_fee mismatch differs from the baseline by more than 5% ({diff}%)",
+        );
+    }
+}

rpc_state_reader/tests/sir_tests.rs

@@ -4,7 +4,6 @@ use crate::services::api::contract_classes::deprecated_contract_class::{
     ContractEntryPoint, EntryPointType,
 };
 use crate::state::cached_state::CachedState;
-use crate::state::StateDiff;
 use crate::{
     definitions::{block_context::BlockContext, constants::DEFAULT_ENTRY_POINT_SELECTOR},
     runner::StarknetRunner,
@@ -128,28 +127,20 @@ impl ExecutionEntryPoint {
                 })
             }
             CompiledClass::Casm(contract_class) => {
-                let mut tmp_state =
-                    CachedState::new(state.state_reader.clone(), state.contract_classes.clone());
-                tmp_state.cache = state.cache.clone();
-
                 match self._execute(
-                    &mut tmp_state,
+                    state,
                     resources_manager,
                     block_context,
                     tx_execution_context,
                     contract_class,
                     class_hash,
                     support_reverted,
                 ) {
-                    Ok(call_info) => {
-                        let state_diff = StateDiff::from_cached_state(tmp_state)?;
-                        state.apply_state_update(&state_diff)?;
-                        Ok(ExecutionResult {
-                            call_info: Some(call_info),
-                            revert_error: None,
-                            n_reverted_steps: 0,
-                        })
-                    }
+                    Ok(call_info) => Ok(ExecutionResult {
+                        call_info: Some(call_info),
+                        revert_error: None,
+                        n_reverted_steps: 0,
+                    }),
                     Err(e) => {
                         if !support_reverted {
                             return Err(e);

src/execution/execution_entry_point.rs

@@ -609,6 +609,16 @@ impl TransactionExecutionInfo {
 
         Ok(sorted_messages)
     }
+
+    pub fn to_revert_error(self, revert_error: &str) -> Self {
+        TransactionExecutionInfo {
+            validate_info: None,
+            call_info: None,
+            revert_error: Some(revert_error.to_string()),
+            fee_transfer_info: None,
+            ..self
+        }
+    }
 }
 
 // --------------------

src/execution/mod.rs

@@ -249,6 +249,7 @@ mod test {
 
     use crate::services::api::contract_classes::compiled_class::CompiledClass;
     use lazy_static::lazy_static;
+    use pretty_assertions_sorted::assert_eq;
 
     lazy_static! {
         // include_str! doesn't seem to work in CI

src/lib.rs

@@ -99,6 +99,19 @@ impl<T: StateReader> CachedState<T> {
         self.contract_classes = contract_classes;
         Ok(())
     }
+
+    /// Creates a copy of this state with an empty cache for saving changes and applying them
+    /// later.
+    pub fn create_transactional(&self) -> TransactionalCachedState<T> {
+        let state_reader = Arc::new(TransactionalCachedStateReader::new(self));
+        CachedState {
+            state_reader,
+            cache: self.cache.clone(),
+            contract_classes: self.contract_classes.clone(),
+            cache_hits: 0,
+            cache_misses: 0,
+        }
+    }
 }
 
 impl<T: StateReader> StateReader for CachedState<T> {
@@ -134,19 +147,13 @@ impl<T: StateReader> StateReader for CachedState<T> {
     // TODO: check if that the proper way to store it (converting hash to address)
     /// Returned the compiled class hash for a given class hash.
     fn get_compiled_class_hash(&self, class_hash: &ClassHash) -> Result<ClassHash, StateError> {
-        if self
-            .cache
-            .class_hash_to_compiled_class_hash
-            .get(class_hash)
-            .is_none()
+        if let Some(compiled_class_hash) =
+            self.cache.class_hash_to_compiled_class_hash.get(class_hash)
         {
-            return self.state_reader.get_compiled_class_hash(class_hash);
+            Ok(*compiled_class_hash)
+        } else {
+            self.state_reader.get_compiled_class_hash(class_hash)
         }
-        self.cache
-            .class_hash_to_compiled_class_hash
-            .get(class_hash)
-            .ok_or_else(|| StateError::NoneCompiledClass(*class_hash))
-            .cloned()
     }
 
     /// Returns the contract class for a given class hash.
@@ -438,6 +445,114 @@ impl<T: StateReader> State for CachedState<T> {
     }
 }
 
+/// A CachedState which has access to another, "parent" state, used for executing transactions
+/// without commiting changes to the parent.
+pub type TransactionalCachedState<'a, T> = CachedState<TransactionalCachedStateReader<'a, T>>;
+
+/// State reader used for transactional states which allows to check the parent state's cache and
+/// state reader if a transactional cache miss happens.
+///
+/// In practice this will act as a way to access the parent state's cache and other fields,
+/// without referencing the whole parent state, so there's no need to adapt state-modifying
+/// functions in the case that a transactional state is needed.
+#[derive(Debug, MutGetters, Getters, PartialEq, Clone)]
+pub struct TransactionalCachedStateReader<'a, T: StateReader> {
+    /// The parent state's state_reader
+    #[get(get = "pub")]
+    pub(crate) state_reader: Arc<T>,
+    /// The parent state's cache
+    #[get(get = "pub")]
+    pub(crate) cache: &'a StateCache,
+    /// The parent state's contract_classes
+    #[get(get = "pub")]
+    pub(crate) contract_classes: ContractClassCache,
+}
+
+impl<'a, T: StateReader> TransactionalCachedStateReader<'a, T> {
+    fn new(state: &'a CachedState<T>) -> Self {
+        Self {
+            state_reader: state.state_reader.clone(),
+            cache: &state.cache,
+            contract_classes: state.contract_classes.clone(),
+        }
+    }
+}
+
+impl<'a, T: StateReader> StateReader for TransactionalCachedStateReader<'a, T> {
+    /// Returns the class hash for a given contract address.
+    /// Returns zero as default value if missing
+    fn get_class_hash_at(&self, contract_address: &Address) -> Result<ClassHash, StateError> {
+        self.cache
+            .get_class_hash(contract_address)
+            .map(|a| Ok(*a))
+            .unwrap_or_else(|| self.state_reader.get_class_hash_at(contract_address))
+    }
+
+    /// Returns the nonce for a given contract address.
+    fn get_nonce_at(&self, contract_address: &Address) -> Result<Felt252, StateError> {
+        if self.cache.get_nonce(contract_address).is_none() {
+            return self.state_reader.get_nonce_at(contract_address);
+        }
+        self.cache
+            .get_nonce(contract_address)
+            .ok_or_else(|| StateError::NoneNonce(contract_address.clone()))
+            .cloned()
+    }
+
+    /// Returns storage data for a given storage entry.
+    /// Returns zero as default value if missing
+    fn get_storage_at(&self, storage_entry: &StorageEntry) -> Result<Felt252, StateError> {
+        self.cache
+            .get_storage(storage_entry)
+            .map(|v| Ok(v.clone()))
+            .unwrap_or_else(|| self.state_reader.get_storage_at(storage_entry))
+    }
+
+    // TODO: check if that the proper way to store it (converting hash to address)
+    /// Returned the compiled class hash for a given class hash.
+    fn get_compiled_class_hash(&self, class_hash: &ClassHash) -> Result<ClassHash, StateError> {
+        if self
+            .cache
+            .class_hash_to_compiled_class_hash
+            .get(class_hash)
+            .is_none()
+        {
+            return self.state_reader.get_compiled_class_hash(class_hash);
+        }
+        self.cache
+            .class_hash_to_compiled_class_hash
+            .get(class_hash)
+            .ok_or_else(|| StateError::NoneCompiledClass(*class_hash))
+            .cloned()
+    }
+
+    /// Returns the contract class for a given class hash.
+    fn get_contract_class(&self, class_hash: &ClassHash) -> Result<CompiledClass, StateError> {
+        // This method can receive both compiled_class_hash & class_hash and return both casm and deprecated contract classes
+        //, which can be on the cache or on the state_reader, different cases will be described below:
+        if class_hash == UNINITIALIZED_CLASS_HASH {
+            return Err(StateError::UninitiaizedClassHash);
+        }
+
+        // I: FETCHING FROM CACHE
+        if let Some(compiled_class) = self.contract_classes.get(class_hash) {
+            return Ok(compiled_class.clone());
+        }
+
+        // I: CASM CONTRACT CLASS : CLASS_HASH
+        if let Some(compiled_class_hash) =
+            self.cache.class_hash_to_compiled_class_hash.get(class_hash)
+        {
+            if let Some(casm_class) = self.contract_classes.get(compiled_class_hash) {
+                return Ok(casm_class.clone());
+            }
+        }
+
+        // II: FETCHING FROM STATE_READER
+        self.state_reader.get_contract_class(class_hash)
+    }
+}
+
 impl<T: StateReader> CachedState<T> {
     // Updates the cache's storage_initial_values according to those in storage_writes
     // If a key is present in the storage_writes but not in storage_initial_values,

src/state/cached_state.rs

@@ -25,7 +25,7 @@ pub struct InMemoryStateReader {
     #[getset(get_mut = "pub")]
     pub class_hash_to_compiled_class: HashMap<ClassHash, CompiledClass>,
     #[getset(get_mut = "pub")]
-    pub(crate) class_hash_to_compiled_class_hash: HashMap<ClassHash, CompiledClassHash>,
+    pub class_hash_to_compiled_class_hash: HashMap<ClassHash, CompiledClassHash>,
 }
 
 impl InMemoryStateReader {