Fix syscall ptr bug

CHANGELOG.md

@@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 #### Changed
 
 - Spying events interface is updated to enable the use of events defined inside contracts in assertions
+- Fixed inconsistent pointers bug https://github.com/foundry-rs/starknet-foundry/issues/659
 
 ## [0.7.1] - 2023-09-27
 

crates/cheatnet/src/execution/syscalls.rs

@@ -174,7 +174,6 @@ impl HintProcessorLogic for CheatableSyscallHandler<'_> {
             PrintingResult::Passed => (),
             PrintingResult::Err(err) => return Err(err),
         }
-
         self.syscall_handler
             .execute_hint(vm, exec_scopes, hint_data, constants)
     }
@@ -228,38 +227,36 @@ fn felt_from_ptr_immutable(
     Ok(felt)
 }
 
+fn get_syscall_operand(hint: &StarknetHint) -> Result<&ResOperand, HintError> {
+    let StarknetHint::SystemCall { system: syscall } = hint else {
+        return Err(HintError::CustomHint(
+            "Test functions are unsupported on starknet.".into(),
+        ));
+    };
+    Ok(syscall)
+}
+
 impl CheatableSyscallHandler<'_> {
     fn execute_next_syscall_cheated(
         &mut self,
         vm: &mut VirtualMachine,
         hint: &StarknetHint,
     ) -> HintExecutionResult {
         // We peak into the selector without incrementing the pointer as it is done later
-        let syscall_selector_pointer = self.syscall_handler.syscall_ptr;
-        let selector = SyscallSelector::try_from(stark_felt_from_ptr_immutable(
-            vm,
-            &syscall_selector_pointer,
-        )?)?;
+        let syscall = get_syscall_operand(hint)?;
+        let initial_syscall_ptr = get_ptr_from_res_operand_unchecked(vm, syscall);
+        let selector =
+            SyscallSelector::try_from(stark_felt_from_ptr_immutable(vm, &initial_syscall_ptr)?)?;
+        self.verify_syscall_ptr(initial_syscall_ptr)?;
         let contract_address = self.syscall_handler.storage_address();
 
         if SyscallSelector::GetExecutionInfo == selector
             && self.cheatnet_state.address_is_cheated(&contract_address)
         {
-            let StarknetHint::SystemCall { system: syscall } = hint else {
-                return Err(HintError::CustomHint(
-                    "Test functions are unsupported on starknet.".into(),
-                ));
-            };
-            let initial_syscall_ptr = get_ptr_from_res_operand_unchecked(vm, syscall);
-            self.verify_syscall_ptr(initial_syscall_ptr)?;
-
             // Increment, since the selector was peeked into before
             self.syscall_handler.syscall_ptr += 1;
-
-            if selector != SyscallSelector::Keccak {
-                self.syscall_handler
-                    .increment_syscall_count_by(&selector, 1);
-            }
+            self.syscall_handler
+                .increment_syscall_count_by(&selector, 1);
 
             let SyscallRequestWrapper {
                 gas_counter,
@@ -293,6 +290,9 @@ impl CheatableSyscallHandler<'_> {
         } else if SyscallSelector::CallContract == selector {
             // Increment, since the selector was peeked into before
             self.syscall_handler.syscall_ptr += 1;
+            self.syscall_handler
+                .increment_syscall_count_by(&selector, 1);
+
             return self.execute_syscall(
                 vm,
                 call_contract_syscall,
@@ -301,6 +301,9 @@ impl CheatableSyscallHandler<'_> {
         } else if SyscallSelector::LibraryCall == selector {
             // Increment, since the selector was peeked into before
             self.syscall_handler.syscall_ptr += 1;
+            self.syscall_handler
+                .increment_syscall_count_by(&selector, 1);
+
             return self.execute_syscall(
                 vm,
                 library_call_syscall,
@@ -309,6 +312,9 @@ impl CheatableSyscallHandler<'_> {
         } else if SyscallSelector::Deploy == selector {
             // Increment, since the selector was peeked into before
             self.syscall_handler.syscall_ptr += 1;
+            self.syscall_handler
+                .increment_syscall_count_by(&selector, 1);
+
             return self.execute_syscall(vm, deploy_syscall, constants::DEPLOY_GAS_COST);
         } else if SyscallSelector::EmitEvent == selector {
             // No incrementation, since execute_next_syscall reads selector and increments syscall_ptr

crates/forge/src/test_execution_syscall_handler.rs

@@ -4,20 +4,14 @@ use std::path::PathBuf;
 
 use crate::scarb::StarknetContractArtifacts;
 use anyhow::{anyhow, Context, Result};
-use blockifier::abi::constants::GET_BLOCK_HASH_GAS_COST;
 use blockifier::execution::deprecated_syscalls::DeprecatedSyscallSelector;
-use blockifier::execution::execution_utils::{
-    felt_to_stark_felt, stark_felt_from_ptr, stark_felt_to_felt,
-};
-use blockifier::execution::syscalls::{
-    GetBlockHashRequest, GetBlockHashResponse, SyscallRequest, SyscallRequestWrapper,
-    SyscallResponse, SyscallResponseWrapper,
-};
+use blockifier::execution::execution_utils::{felt_to_stark_felt, stark_felt_to_felt};
 use cairo_felt::Felt252;
 use cairo_vm::hint_processor::hint_processor_definition::HintProcessorLogic;
 use cairo_vm::hint_processor::hint_processor_definition::HintReference;
 use cairo_vm::serde::deserialize_program::ApTracking;
 use cairo_vm::types::exec_scope::ExecutionScopes;
+use cairo_vm::types::relocatable::Relocatable;
 use cairo_vm::vm::errors::hint_errors::HintError;
 use cairo_vm::vm::errors::vm_errors::VirtualMachineError;
 use cairo_vm::vm::vm_core::VirtualMachine;
@@ -38,16 +32,14 @@ use cairo_lang_runner::{
     casm_run::{cell_ref_to_relocatable, extract_buffer, get_ptr},
     insert_value_to_cellref,
 };
+use starknet_api::core::ContractAddress;
 
 use crate::test_execution_syscall_handler::file_operations::string_into_felt;
 use cairo_lang_starknet::contract::starknet_keccak;
 use cairo_lang_utils::bigint::BigIntAsHex;
-use cairo_vm::types::relocatable::Relocatable;
 use cairo_vm::vm::errors::hint_errors::HintError::CustomHint;
 use cairo_vm::vm::runners::cairo_runner::{ResourceTracker, RunResources};
 use cheatnet::cheatcodes::spy_events::SpyTarget;
-use starknet_api::block::BlockHash;
-use starknet_api::hash::StarkFelt;
 
 mod file_operations;
 
@@ -165,12 +157,8 @@ impl HintProcessorLogic for TestExecutionSyscallHandler<'_> {
                 &mut self.cheatable_syscall_handler,
             );
         }
-        self.cheatable_syscall_handler.syscall_handler.execute_hint(
-            vm,
-            exec_scopes,
-            hint_data,
-            constants,
-        )
+        self.cheatable_syscall_handler
+            .execute_hint(vm, exec_scopes, hint_data, constants)
     }
 
     /// Trait function to store hint in the hint processor by string.
@@ -657,15 +645,25 @@ fn execute_syscall(
     let selector = DeprecatedSyscallSelector::try_from(felt_to_stark_felt(
         &vm.get_integer(system_ptr).unwrap(),
     ))?;
-    let mut blockifier_state =
-        BlockifierState::from(cheatable_syscall_handler.syscall_handler.state);
 
     match selector {
         DeprecatedSyscallSelector::CallContract => {
-            execute_call_contract(
-                MemBuffer::new(vm, system_ptr),
-                cheatable_syscall_handler.cheatnet_state,
+            let (system_ptr, call_args) =
+                get_call_contract_args(cheatable_syscall_handler, system_ptr, vm);
+
+            let mut blockifier_state =
+                BlockifierState::from(cheatable_syscall_handler.syscall_handler.state);
+            let call_result = execute_call_contract(
                 &mut blockifier_state,
+                cheatable_syscall_handler.cheatnet_state,
+                &call_args,
+            );
+            write_call_contract_response(
+                cheatable_syscall_handler,
+                system_ptr,
+                vm,
+                &call_args,
+                call_result,
             )?;
             Ok(())
         }
@@ -675,76 +673,86 @@ fn execute_syscall(
         DeprecatedSyscallSelector::ReplaceClass => Err(HintError::CustomHint(Box::from(
             "Replace class can't be used in tests".to_string(),
         ))),
-        DeprecatedSyscallSelector::GetBlockHash => {
-            execute_get_block_hash(
-                vm,
-                &mut system_ptr.clone(),
-                cheatable_syscall_handler.cheatnet_state,
-            )?;
-            Ok(())
-        }
         _ => cheatable_syscall_handler.execute_hint(vm, exec_scopes, hint_data, constants),
     }
 }
 
-fn execute_get_block_hash(
-    vm: &mut VirtualMachine,
-    system_ptr: &mut Relocatable,
-    _cheatnet_state: &CheatnetState,
-) -> Result<(), HintError> {
-    let _selector = stark_felt_from_ptr(vm, system_ptr)?;
-    let SyscallRequestWrapper {
-        gas_counter,
-        request: _,
-    } = SyscallRequestWrapper::<GetBlockHashRequest>::read(vm, system_ptr)?;
-
-    let sc_response = SyscallResponseWrapper::Success {
-        gas_counter: gas_counter - GET_BLOCK_HASH_GAS_COST,
-        response: GetBlockHashResponse {
-            block_hash: BlockHash(StarkFelt::from(0_u32)),
-        },
-    };
-    sc_response.write(vm, system_ptr)?;
-    Ok(())
+struct CallContractArgs {
+    _selector: Felt252,
+    gas_counter: u64,
+    contract_address: ContractAddress,
+    entry_point_selector: Felt252,
+    calldata: Vec<Felt252>,
 }
 
-fn execute_call_contract(
-    mut buffer: MemBuffer,
-    cheatnet_state: &mut CheatnetState,
-    blockifier_state: &mut BlockifierState,
-) -> Result<(), HintError> {
-    let _selector = buffer.next_felt252().unwrap();
-    let gas_counter = buffer.next_usize().unwrap();
+fn get_call_contract_args(
+    cheatable_syscall_handler: &mut CheatableSyscallHandler,
+    system_ptr: Relocatable,
+    vm: &mut VirtualMachine,
+) -> (Relocatable, CallContractArgs) {
+    let mut buffer = MemBuffer::new(vm, system_ptr);
+
+    let selector = buffer.next_felt252().unwrap().into_owned();
+    let gas_counter = buffer.next_felt252().unwrap().to_u64().unwrap();
 
     let contract_address = buffer.next_felt252().unwrap().into_owned();
     let contract_address = contract_address.to_contract_address();
 
     let entry_point_selector = buffer.next_felt252().unwrap().into_owned();
 
     let calldata = buffer.next_arr().unwrap();
-
-    let call_result = call_contract(
-        blockifier_state,
-        cheatnet_state,
-        &contract_address,
-        &entry_point_selector,
-        &calldata,
+    cheatable_syscall_handler.syscall_handler.syscall_ptr += (buffer.ptr - system_ptr).unwrap();
+
+    (
+        buffer.ptr,
+        CallContractArgs {
+            _selector: selector,
+            gas_counter,
+            contract_address,
+            entry_point_selector,
+            calldata,
+        },
     )
-    .unwrap_or_else(|err| panic!("Transaction execution error: {err}"));
+}
+
+fn write_call_contract_response(
+    cheatable_syscall_handler: &mut CheatableSyscallHandler,
+    system_ptr: Relocatable,
+    vm: &mut VirtualMachine,
+    call_args: &CallContractArgs,
+    call_result: CallContractOutput,
+) -> Result<(), HintError> {
+    let mut buffer = MemBuffer::new(vm, system_ptr);
 
     let (result, exit_code) = match call_result {
         CallContractOutput::Success { ret_data, .. } => (ret_data, 0),
         CallContractOutput::Panic { panic_data, .. } => (panic_data, 1),
         CallContractOutput::Error { msg, .. } => return Err(HintError::CustomHint(Box::from(msg))),
     };
 
-    buffer.write(gas_counter).unwrap();
+    buffer.write(Felt252::from(call_args.gas_counter)).unwrap();
     buffer.write(Felt252::from(exit_code)).unwrap();
 
     buffer.write_arr(result.iter()).unwrap();
+    cheatable_syscall_handler.syscall_handler.syscall_ptr += (buffer.ptr - system_ptr).unwrap();
     Ok(())
 }
 
+fn execute_call_contract(
+    blockifier_state: &mut BlockifierState,
+    cheatnet_state: &mut CheatnetState,
+    call_args: &CallContractArgs,
+) -> CallContractOutput {
+    call_contract(
+        blockifier_state,
+        cheatnet_state,
+        &call_args.contract_address,
+        &call_args.entry_point_selector,
+        &call_args.calldata,
+    )
+    .unwrap_or_else(|err| panic!("Transaction execution error: {err}"))
+}
+
 fn print(inputs: Vec<Felt252>) {
     for value in inputs {
         if let Some(short_string) = as_cairo_short_string(&value) {

crates/forge/tests/integration/test_state.rs

@@ -562,3 +562,32 @@ fn test_spy_events_simple() {
 
     assert_passed!(result);
 }
+
+#[test]
+fn test_inconsistent_syscall_pointers() {
+    let test = test_case!(indoc!(
+        r#"
+        use starknet::ContractAddress;
+        use starknet::info::get_block_number;
+        use snforge_std::start_mock_call;
+
+        #[starknet::interface]
+        trait IContract<TContractState> {
+            fn get_value(self: @TContractState, arg: ContractAddress) -> u128;
+        }
+
+        #[test]
+        fn test_deploy_error_handling() {
+            // verifies if SyscallHandler.syscal_ptr is incremented correctly when calling a contract
+            let address = 'address'.try_into().unwrap();
+            start_mock_call(address, 'get_value', 55);
+            let contract = IContractDispatcher { contract_address: address };
+            let value = contract.get_value(address);
+            let block_number = get_block_number();
+        }
+    "#
+    ),);
+    let result = run_test_case(&test);
+
+    assert_passed!(result);
+}