Merge pull request #2098 from starkware-libs/barak/merge-main-v0.13.2…

…-into-main

Merge main-v0.13.2 into main (#2098)

Co-Authored-By: AvivYossef-starkware <[email protected]>
Co-Authored-By: Yoni <[email protected]>
Co-Authored-By: avi-starkware <[email protected]>
Co-Authored-By: ilyalesokhin-starkware <[email protected]>
Co-Authored-By: Meshi Peled <[email protected]>

crates/blockifier/feature_contracts/cairo1/test_contract.cairo

@@ -553,4 +553,20 @@ mod TestContract {
 
         assert!(outputs.get_output(mul) == u384 { limb0: 6, limb1: 0, limb2: 0, limb3: 0 });
     }
+
+
+    // Add drop for AddInputResult as it only has PanicDestruct.
+    impl AddInputResultDrop<C> of Drop<core::circuit::AddInputResult<C>>;
+
+    #[external(v0)]
+    fn test_rc96_holes(ref self: ContractState) {
+        test_rc96_holes_helper();
+        test_rc96_holes_helper();
+    }
+
+    #[inline(never)]
+    fn test_rc96_holes_helper() {
+        let in1 = CircuitElement::<CircuitInput<0>> {};
+        (in1,).new_inputs().next([3, 0, 0, 0]);
+    }
 }

crates/blockifier/resources/versioned_constants.json

@@ -25,6 +25,7 @@
     },
     "disable_cairo0_redeclaration": true,
     "max_recursion_depth": 50,
+    "segment_arena_cells": false,
     "os_constants": {
         "block_hash_contract_address": 1,
         "call_contract_gas_cost": {

crates/blockifier/resources/versioned_constants_13_0.json

@@ -5,6 +5,7 @@
     },
     "invoke_tx_max_n_steps": 3000000,
     "max_recursion_depth": 50,
+    "segment_arena_cells": true,
     "os_constants": {
         "nop_entry_point_offset": -1,
         "entry_point_type_external": 0,

crates/blockifier/resources/versioned_constants_13_1.json

@@ -24,6 +24,7 @@
         ]
     },
     "max_recursion_depth": 50,
+    "segment_arena_cells": true,
     "os_constants": {
         "nop_entry_point_offset": -1,
         "entry_point_type_external": 0,

crates/blockifier/resources/versioned_constants_13_1_1.json

@@ -24,6 +24,7 @@
         ]
     },
     "max_recursion_depth": 50,
+    "segment_arena_cells": true,
     "os_constants": {
         "nop_entry_point_offset": -1,
         "entry_point_type_external": 0,

crates/blockifier/src/blockifier/transaction_executor.rs

@@ -1,6 +1,8 @@
 #[cfg(feature = "concurrency")]
 use std::collections::{HashMap, HashSet};
 #[cfg(feature = "concurrency")]
+use std::panic::{self, catch_unwind, AssertUnwindSafe};
+#[cfg(feature = "concurrency")]
 use std::sync::Arc;
 #[cfg(feature = "concurrency")]
 use std::sync::Mutex;
@@ -218,6 +220,8 @@ impl<S: StateReader + Send + Sync> TransactionExecutor<S> {
         &mut self,
         chunk: &[Transaction],
     ) -> Vec<TransactionExecutorResult<TransactionExecutionInfo>> {
+        use crate::concurrency::utils::AbortIfPanic;
+
         let block_state = self.block_state.take().expect("The block state should be `Some`.");
 
         let worker_executor = Arc::new(WorkerExecutor::initialize(
@@ -236,7 +240,24 @@ impl<S: StateReader + Send + Sync> TransactionExecutor<S> {
             for _ in 0..self.config.concurrency_config.n_workers {
                 let worker_executor = Arc::clone(&worker_executor);
                 s.spawn(move || {
-                    worker_executor.run();
+                    // Making sure that the program will abort if a panic accured while halting the
+                    // scheduler.
+                    let abort_guard = AbortIfPanic;
+                    // If a panic is not handled or the handling logic itself panics, then we abort
+                    // the program.
+                    if let Err(err) = catch_unwind(AssertUnwindSafe(|| {
+                        worker_executor.run();
+                    })) {
+                        // If the program panics here, the abort guard will exit the program.
+                        // In this case, no panic message will be logged. Add the cargo flag
+                        // --nocapture to log the panic message.
+
+                        worker_executor.scheduler.halt();
+                        abort_guard.release();
+                        panic::resume_unwind(err);
+                    }
+
+                    abort_guard.release();
                 });
             }
         });

crates/blockifier/src/concurrency/scheduler.rs

@@ -52,7 +52,7 @@ impl<'a> TransactionCommitter<'a> {
         assert!(*self.commit_index_guard > 0, "Commit index underflow.");
         *self.commit_index_guard -= 1;
 
-        self.scheduler.done_marker.store(true, Ordering::Release);
+        self.scheduler.halt();
     }
 }
 
@@ -161,6 +161,10 @@ impl Scheduler {
         *self.commit_index.lock().unwrap()
     }
 
+    pub fn halt(&self) {
+        self.done_marker.store(true, Ordering::Release);
+    }
+
     fn lock_tx_status(&self, tx_index: TxIndex) -> MutexGuard<'_, TransactionStatus> {
         lock_mutex_in_array(&self.tx_statuses, tx_index)
     }

crates/blockifier/src/concurrency/utils.rs

@@ -3,6 +3,23 @@ use std::sync::{Mutex, MutexGuard};
 
 use crate::concurrency::TxIndex;
 
+// This struct is used to abort the program if a panic occurred in a place where it could not be
+// handled.
+pub struct AbortIfPanic;
+
+impl Drop for AbortIfPanic {
+    fn drop(&mut self) {
+        eprintln!("detected unexpected panic; aborting");
+        std::process::abort();
+    }
+}
+
+impl AbortIfPanic {
+    pub fn release(self) {
+        std::mem::forget(self);
+    }
+}
+
 pub fn lock_mutex_in_array<T: Debug>(array: &[Mutex<T>], tx_index: TxIndex) -> MutexGuard<'_, T> {
     array[tx_index].lock().unwrap_or_else(|error| {
         panic!("Cell of transaction index {} is poisoned. Data: {:?}.", tx_index, *error.get_ref())

crates/blockifier/src/execution/deprecated_entry_point_execution.rs

@@ -1,3 +1,4 @@
+use cairo_vm::types::builtin_name::BuiltinName;
 use cairo_vm::types::layout_name::LayoutName;
 use cairo_vm::types::relocatable::{MaybeRelocatable, Relocatable};
 use cairo_vm::vm::errors::vm_errors::VirtualMachineError;
@@ -6,6 +7,7 @@ use starknet_api::core::EntryPointSelector;
 use starknet_api::deprecated_contract_class::EntryPointType;
 use starknet_api::hash::StarkHash;
 
+use super::execution_utils::SEGMENT_ARENA_BUILTIN_SIZE;
 use crate::abi::abi_utils::selector_from_name;
 use crate::abi::constants::{CONSTRUCTOR_ENTRY_POINT_NAME, DEFAULT_ENTRY_POINT_SELECTOR};
 use crate::execution::call_info::{CallExecution, CallInfo};
@@ -227,12 +229,18 @@ pub fn finalize_execution(
 
     // Take into account the VM execution resources of the current call, without inner calls.
     // Has to happen after marking holes in segments as accessed.
-    let vm_resources_without_inner_calls = runner
+    let mut vm_resources_without_inner_calls = runner
         .get_execution_resources()
         .map_err(VirtualMachineError::RunnerError)?
         .filter_unused_builtins();
-    *syscall_handler.resources += &vm_resources_without_inner_calls;
     let versioned_constants = syscall_handler.context.versioned_constants();
+    if versioned_constants.segment_arena_cells {
+        vm_resources_without_inner_calls
+            .builtin_instance_counter
+            .get_mut(&BuiltinName::segment_arena)
+            .map_or_else(|| {}, |val| *val *= SEGMENT_ARENA_BUILTIN_SIZE);
+    }
+    *syscall_handler.resources += &vm_resources_without_inner_calls;
     // Take into account the syscall resources of the current call.
     *syscall_handler.resources += &versioned_constants
         .get_additional_os_syscall_resources(&syscall_handler.syscall_counter)?;

crates/blockifier/src/execution/entry_point_execution.rs

@@ -3,9 +3,13 @@ use std::collections::HashSet;
 use cairo_vm::types::builtin_name::BuiltinName;
 use cairo_vm::types::layout_name::LayoutName;
 use cairo_vm::types::relocatable::{MaybeRelocatable, Relocatable};
+use cairo_vm::vm::errors::cairo_run_errors::CairoRunError;
+use cairo_vm::vm::errors::memory_errors::MemoryError;
 use cairo_vm::vm::errors::vm_errors::VirtualMachineError;
+use cairo_vm::vm::runners::builtin_runner::BuiltinRunner;
 use cairo_vm::vm::runners::cairo_runner::{CairoArg, CairoRunner, ExecutionResources};
-use num_traits::ToPrimitive;
+use cairo_vm::vm::security::verify_secure_runner;
+use num_traits::{ToPrimitive, Zero};
 use starknet_api::felt;
 use starknet_types_core::felt::Felt;
 
@@ -17,6 +21,7 @@ use crate::execution::entry_point::{
 use crate::execution::errors::{EntryPointExecutionError, PostExecutionError, PreExecutionError};
 use crate::execution::execution_utils::{
     read_execution_retdata, write_felt, write_maybe_relocatable, Args, ReadOnlySegments,
+    SEGMENT_ARENA_BUILTIN_SIZE,
 };
 use crate::execution::syscalls::hint_processor::SyscallHintProcessor;
 use crate::state::state_api::State;
@@ -157,7 +162,6 @@ pub fn initialize_execution_context<'a>(
         BuiltinName::bitwise,
         BuiltinName::ec_op,
         BuiltinName::ecdsa,
-        BuiltinName::output,
         BuiltinName::pedersen,
         BuiltinName::poseidon,
         BuiltinName::range_check,
@@ -280,17 +284,86 @@ pub fn run_entry_point(
     args: Args,
     program_segment_size: usize,
 ) -> EntryPointExecutionResult<()> {
-    let verify_secure = true;
+    // Note that we run `verify_secure_runner` manually after filling the holes in the rc96 segment.
+    let verify_secure = false;
     let args: Vec<&CairoArg> = args.iter().collect();
-    let result = runner.run_from_entrypoint(
+    runner.run_from_entrypoint(
         entry_point.pc(),
         &args,
         verify_secure,
         Some(program_segment_size),
         hint_processor,
-    );
+    )?;
+
+    maybe_fill_holes(entry_point, runner)?;
 
-    Ok(result?)
+    verify_secure_runner(runner, false, Some(program_segment_size))
+        .map_err(CairoRunError::VirtualMachine)?;
+
+    Ok(())
+}
+
+/// Fills the holes after running the entry point.
+/// Currently only fills the holes in the rc96 segment.
+fn maybe_fill_holes(
+    entry_point: EntryPointV1,
+    runner: &mut CairoRunner,
+) -> Result<(), EntryPointExecutionError> {
+    let Some(rc96_offset) = entry_point
+        .builtins
+        .iter()
+        .rev()
+        .position(|name| name.as_str() == BuiltinName::range_check96.to_str_with_suffix())
+    else {
+        return Ok(());
+    };
+    let rc96_builtin_runner = runner
+        .vm
+        .get_builtin_runners()
+        .iter()
+        .find_map(|builtin| {
+            if let BuiltinRunner::RangeCheck96(rc96_builtin_runner) = builtin {
+                Some(rc96_builtin_runner)
+            } else {
+                None
+            }
+        })
+        .expect("RangeCheck96 builtin runner not found.");
+
+    // 'EntryPointReturnValues' is returned after the implicits and its size is 5,
+    // So the last implicit is at offset 5 + 1.
+    const IMPLICITS_OFFSET: usize = 6;
+    let rc_96_stop_ptr = (runner.vm.get_ap() - (IMPLICITS_OFFSET + rc96_offset))
+        .map_err(|err| CairoRunError::VirtualMachine(VirtualMachineError::Math(err)))?;
+
+    let rc96_base = rc96_builtin_runner.base();
+    let rc96_segment: isize =
+        rc96_base.try_into().expect("Builtin segment index must fit in isize.");
+
+    let Relocatable { segment_index: rc96_stop_segment, offset: stop_offset } =
+        runner.vm.get_relocatable(rc_96_stop_ptr).map_err(CairoRunError::MemoryError)?;
+    assert_eq!(rc96_stop_segment, rc96_segment);
+
+    // Update `segment_used_sizes` to include the holes.
+    runner
+        .vm
+        .segments
+        .segment_used_sizes
+        .as_mut()
+        .expect("Segments used sizes should be calculated at this point")[rc96_base] = stop_offset;
+
+    for offset in 0..stop_offset {
+        match runner
+            .vm
+            .insert_value(Relocatable { segment_index: rc96_segment, offset }, Felt::zero())
+        {
+            // If the value is already set, ignore the error.
+            Ok(()) | Err(MemoryError::InconsistentMemory(_)) => {}
+            Err(err) => panic!("Unexpected error when filling holes: {err}."),
+        }
+    }
+
+    Ok(())
 }
 
 pub fn finalize_execution(
@@ -319,12 +392,18 @@ pub fn finalize_execution(
 
     // Take into account the VM execution resources of the current call, without inner calls.
     // Has to happen after marking holes in segments as accessed.
-    let vm_resources_without_inner_calls = runner
+    let mut vm_resources_without_inner_calls = runner
         .get_execution_resources()
         .map_err(VirtualMachineError::RunnerError)?
         .filter_unused_builtins();
-    *syscall_handler.resources += &vm_resources_without_inner_calls;
     let versioned_constants = syscall_handler.context.versioned_constants();
+    if versioned_constants.segment_arena_cells {
+        vm_resources_without_inner_calls
+            .builtin_instance_counter
+            .get_mut(&BuiltinName::segment_arena)
+            .map_or_else(|| {}, |val| *val *= SEGMENT_ARENA_BUILTIN_SIZE);
+    }
+    *syscall_handler.resources += &vm_resources_without_inner_calls;
     // Take into account the syscall resources of the current call.
     *syscall_handler.resources += &versioned_constants
         .get_additional_os_syscall_resources(&syscall_handler.syscall_counter)?;

crates/blockifier/src/execution/entry_point_test.rs

@@ -529,12 +529,11 @@ fn test_cairo1_entry_point_segment_arena() {
         ..trivial_external_entry_point_new(test_contract)
     };
 
-    assert!(
-        entry_point_call
-            .execute_directly(&mut state)
-            .unwrap()
-            .resources
-            .builtin_instance_counter
-            .contains_key(&BuiltinName::segment_arena)
+    assert_eq!(
+        entry_point_call.execute_directly(&mut state).unwrap().resources.builtin_instance_counter
+            [&BuiltinName::segment_arena],
+        // Note: the number of segment_arena instances should not depend on the compiler or VM
+        // version. Do not manually fix this then when upgrading them - it might be a bug.
+        2
     );
 }

crates/blockifier/src/execution/execution_utils.rs

@@ -34,6 +34,8 @@ use crate::transaction::objects::TransactionInfo;
 
 pub type Args = Vec<CairoArg>;
 
+pub const SEGMENT_ARENA_BUILTIN_SIZE: usize = 3;
+
 /// Executes a specific call to a contract entry point and returns its output.
 pub fn execute_entry_point_call(
     call: CallEntryPoint,

crates/blockifier/src/execution/stack_trace_test.rs

@@ -239,7 +239,7 @@ Unknown location (pc=0:{expected_pc1})
 Error at pc=0:767:
 1: Error in the called contract (contract address: {contract_address_felt:#064x}, class hash: \
                  {test_contract_hash:#064x}, selector: {invoke_call_chain_selector_felt:#064x}):
-Error at pc=0:9508:
+Error at pc=0:9631:
 Cairo traceback (most recent call last):
 Unknown location (pc=0:{pc_location})
 
@@ -259,10 +259,10 @@ Execution failed. Failure reason: {expected_error}.
 #[case(CairoVersion::Cairo0, "invoke_call_chain", "Couldn't compute operand op0. Unknown value for memory cell 1:23", 1_u8, 1_u8, (49_u16, 1111_u16, 1081_u16, 1166_u16))]
 #[case(CairoVersion::Cairo0, "fail", "An ASSERT_EQ instruction failed: 1 != 0.", 0_u8, 0_u8, (37_u16, 1093_u16, 1184_u16, 1188_u16))]
 #[case(CairoVersion::Cairo0, "fail", "An ASSERT_EQ instruction failed: 1 != 0.", 0_u8, 1_u8, (49_u16, 1111_u16, 1184_u16, 1188_u16))]
-#[case(CairoVersion::Cairo1, "invoke_call_chain", "0x4469766973696f6e2062792030 ('Division by 0')", 1_u8, 0_u8, (9508_u16, 9508_u16, 0_u16, 0_u16))]
-#[case(CairoVersion::Cairo1, "invoke_call_chain", "0x4469766973696f6e2062792030 ('Division by 0')", 1_u8, 1_u8, (9508_u16, 9577_u16, 0_u16, 0_u16))]
-#[case(CairoVersion::Cairo1, "fail", "0x6661696c ('fail')", 0_u8, 0_u8, (9508_u16, 9508_u16, 0_u16, 0_u16))]
-#[case(CairoVersion::Cairo1, "fail", "0x6661696c ('fail')", 0_u8, 1_u8, (9508_u16, 9577_u16, 0_u16, 0_u16))]
+#[case(CairoVersion::Cairo1, "invoke_call_chain", "0x4469766973696f6e2062792030 ('Division by 0')", 1_u8, 0_u8, (9631_u16, 9631_u16, 0_u16, 0_u16))]
+#[case(CairoVersion::Cairo1, "invoke_call_chain", "0x4469766973696f6e2062792030 ('Division by 0')", 1_u8, 1_u8, (9631_u16, 9700_u16, 0_u16, 0_u16))]
+#[case(CairoVersion::Cairo1, "fail", "0x6661696c ('fail')", 0_u8, 0_u8, (9631_u16, 9631_u16, 0_u16, 0_u16))]
+#[case(CairoVersion::Cairo1, "fail", "0x6661696c ('fail')", 0_u8, 1_u8, (9631_u16, 9700_u16, 0_u16, 0_u16))]
 fn test_trace_call_chain_with_syscalls(
     block_context: BlockContext,
     #[case] cairo_version: CairoVersion,

crates/blockifier/src/test_utils.rs

@@ -47,6 +47,8 @@ pub const TEST_ERC20_CONTRACT_CLASS_HASH: &str = "0x1010";
 pub const ERC20_CONTRACT_PATH: &str = "./ERC20/ERC20_Cairo0/ERC20_without_some_syscalls/ERC20/\
                                        erc20_contract_without_some_syscalls_compiled.json";
 
+// TODO(Aviv, 14/7/2024): Move from test utils module, and use it in ContractClassVersionMismatch
+// error.
 #[derive(Clone, Hash, PartialEq, Eq, Copy, Debug)]
 pub enum CairoVersion {
     Cairo0,

crates/blockifier/src/transaction.rs

@@ -1,5 +1,7 @@
 pub mod account_transaction;
 pub mod constants;
+#[cfg(test)]
+pub mod error_format_test;
 pub mod errors;
 pub mod objects;
 #[cfg(any(feature = "testing", test))]