blockifier: improve revert handling (#11426)

wip

Author: Yoni-Starkware

crates/blockifier/src/bouncer_test.rs

@@ -775,11 +775,11 @@ fn class_hash_migration_data_from_state(
 
     if should_migrate {
         expect![[r#"
-            100709658
+            102995620
         "#]]
         .assert_debug_eq(&migration_sierra_gas.0);
         expect![[r#"
-            243210896
+            248629270
         "#]]
         .assert_debug_eq(&migration_proving_gas.0);
     } else {

crates/blockifier/src/execution/syscalls/syscall_base.rs

@@ -191,7 +191,25 @@ impl<'state> SyscallHandlerBase<'state> {
 
         match self.original_values.entry(key) {
             hash_map::Entry::Vacant(entry) => {
-                entry.insert(self.state.get_storage_at(contract_address, key)?);
+                // Check if any inner call (entries created after this handler's entry) already
+                // captured an original value for this key. If so, use that value instead of the
+                // current state, because the inner call captured the true original before any
+                // writes in this execution scope.
+                let original_value = self
+                    .context
+                    .revert_infos
+                    .0
+                    .iter()
+                    .skip(self.revert_info_idx + 1)
+                    .find_map(|info| {
+                        if info.contract_address == contract_address {
+                            info.original_values.get(&key).copied()
+                        } else {
+                            None
+                        }
+                    })
+                    .map_or_else(|| self.state.get_storage_at(contract_address, key), Ok)?;
+                entry.insert(original_value);
             }
             hash_map::Entry::Occupied(_) => {}
         }

crates/blockifier/src/execution/syscalls/syscall_tests/call_contract.rs

@@ -10,6 +10,7 @@ use pretty_assertions::assert_eq;
 use rstest::rstest;
 use starknet_api::abi::abi_utils::selector_from_name;
 use starknet_api::execution_utils::format_panic_data;
+use starknet_api::state::StorageKey;
 use starknet_api::transaction::fields::Calldata;
 use starknet_api::{calldata as calldata_macro, felt};
 use test_case::test_case;
@@ -18,6 +19,7 @@ use crate::context::{BlockContext, ChainInfo};
 use crate::execution::contract_class::TrackedResource;
 use crate::execution::entry_point::CallEntryPoint;
 use crate::retdata;
+use crate::state::state_api::StateReader;
 use crate::test_utils::initial_test_state::test_state;
 use crate::test_utils::syscall::build_recurse_calldata;
 use crate::test_utils::{trivial_external_entry_point_new, CompilerBasedVersion, BALANCE};
@@ -501,3 +503,55 @@ fn test_empty_function_flow(#[case] runnable: RunnableCairo1) {
     // Contract should not fail.
     assert!(!call_info.execution.failed);
 }
+
+/// Test that storage is correctly reverted when nested calls write to the same key.
+///
+/// Scenario:
+/// - catch_write_revert_panic calls call_write_rewrite_panic and catches/ignores the revert
+/// - call_write_rewrite_panic calls write_1, then writes storage = 2, then panics
+/// - write_1 writes storage = 1
+///
+/// After call_write_rewrite_panic's revert, storage should be 0 (original), not 1 (write_1's
+/// write). This tests the fix for a bug where call_write_rewrite_panic would capture write_1's
+/// written value (1) as the "original" instead of the true original (0), causing incorrect state
+/// after revert.
+#[cfg_attr(feature = "cairo_native", test_case(RunnableCairo1::Native; "Native"))]
+#[test_case(RunnableCairo1::Casm; "VM")]
+fn test_nested_call_storage_revert(runnable_version: RunnableCairo1) {
+    let test_contract = FeatureContract::TestContract(CairoVersion::Cairo1(runnable_version));
+    let chain_info = &ChainInfo::create_for_testing();
+    let mut state = test_state(chain_info, BALANCE, &[(test_contract, 1)]);
+
+    let contract_address = test_contract.get_instance_address(0);
+    let storage_key = StorageKey::try_from(felt!(123_u64)).unwrap();
+
+    // Call catch_write_revert_panic which:
+    // 1. Calls call_write_rewrite_panic
+    // 2. call_write_rewrite_panic calls write_1
+    // 3. write_1 writes storage = 1
+    // 4. call_write_rewrite_panic writes storage = 2
+    // 5. call_write_rewrite_panic panics (gets reverted)
+    // 6. catch_write_revert_panic catches the revert and reads storage (should be 0)
+    let entry_point_call = CallEntryPoint {
+        entry_point_selector: selector_from_name("catch_write_revert_panic"),
+        calldata: calldata_macro![*contract_address.0.key(), *storage_key.0.key()],
+        ..trivial_external_entry_point_new(test_contract)
+    };
+
+    let call_info = entry_point_call.execute_directly(&mut state).unwrap();
+
+    // catch_write_revert_panic should succeed (it catches call_write_rewrite_panic's revert).
+    assert!(!call_info.execution.failed, "catch_write_revert_panic should not fail");
+
+    // catch_write_revert_panic returns the storage value it read after call_write_rewrite_panic's
+    // revert. This should be 0 (the original value), not 1 (write_1's write).
+    assert_eq!(
+        call_info.execution.retdata.0,
+        vec![felt!(0_u8)],
+        "Storage should be reverted to 0, not write_1's value (1)"
+    );
+
+    // Double-check by reading storage directly.
+    let final_value = state.get_storage_at(contract_address, storage_key).unwrap();
+    assert_eq!(final_value, felt!(0_u8), "Storage should be 0 after revert");
+}