fix: Identifier retdata is not relocatable (keep-starknet-strange#393)

* wip: adds integration tests for invoke with delpoy_contract syscall * test: add tests for contract with no calldata * Fix inconsistent hash (keep-starknet-strange#390) * point to snos_requirements branch * fix inconsistent hash * add blocks to CI * Using reference PIE to validate our PIEs are correct (keep-starknet-strange#389) The reference PIE was created with `full_output` enabled, so `output.rs` was updated to support that. The code was refactored to better mirror the Cairo serialization code. Co-authored-by: Herman Obst Demaestri <[email protected]> * fix: `split_commitment` function in KZG data availability implementation (keep-starknet-strange#391) * debug : prints added for blob * debug : prints added for blob * debug : prints added for blob * debug : prints added for blob * fix: fixed split commitment function * fix: fixed split commitment function * fix: fixed split commitment function * feat : removed logs * fix : lint and clippy * test : added a test for split commitment function * feat : requested changes done * chore : added constants for hardcoded values --------- Co-authored-by: Herman Obst Demaestri <[email protected]> * QOL++ * Bugfix: work around retdata being Int(0) * Add test case for retdata issue * Update hashes for modified contract code * First pass at retdata hack * fmt * clippy * Fix compile errors in tests * fmt * Wrap os_input in Arc * s/Arc/Rc/ * fmt * clippy * Avoid hideous clone * Don't unwrap * Leave link to relevant code * Refine comment about no-oping * Add link to retdata cast() * clippy * Pass compiled OS to run_os --------- Co-authored-by: ftheirs <[email protected]> Co-authored-by: Shams Asari <[email protected]> Co-authored-by: Herman Obst Demaestri <[email protected]> Co-authored-by: Arun Jangra <[email protected]> Co-authored-by: Herman Obst Demaestri <[email protected]> Co-authored-by: Stephen Shelton <[email protected]>
cartridge-gg · Oct 18, 2024 · 662d170 · 662d170
1 parent 8776b73
commit 662d170
Show file tree

Hide file tree

Showing 21 changed files with 6,879 additions and 6,462 deletions.
diff --git a/crates/bin/prove_block/src/lib.rs b/crates/bin/prove_block/src/lib.rs
@@ -1,4 +1,5 @@
 use std::collections::HashMap;
+use std::rc::Rc;
 
 use blockifier::state::cached_state::CachedState;
 use cairo_vm::types::layout_name::LayoutName;
@@ -202,6 +203,7 @@ pub async fn prove_block(
 
     let mut contract_states = HashMap::new();
     let mut contract_storages = ContractStorageMap::new();
+    let mut contract_address_to_class_hash = HashMap::new();
 
     // TODO: remove this clone()
     for (contract_address, storage_proof) in storage_proofs.clone() {
@@ -247,6 +249,10 @@ pub async fn prove_block(
                 Err(ProviderError::StarknetError(StarknetError::ContractNotFound)) => Ok(Felt252::ZERO),
                 Err(e) => Err(e),
             }?;
+
+            let class_hash = rpc_client.starknet_rpc().get_class_hash_at(block_id, contract_address).await?;
+            contract_address_to_class_hash.insert(contract_address, class_hash);
+
             (previous_class_hash, previous_nonce)
         };
 
@@ -313,25 +319,27 @@ pub async fn prove_block(
 
     let contract_class_commitment_info = compute_class_commitment(&previous_class_proofs, &class_proofs);
 
-    let os_input = StarknetOsInput {
+    let os_input = Rc::new(StarknetOsInput {
         contract_state_commitment_info,
         contract_class_commitment_info,
         deprecated_compiled_classes,
         compiled_classes,
         compiled_class_visited_pcs: visited_pcs,
         contracts: contract_states,
+        contract_address_to_class_hash,
         class_hash_to_compiled_class_hash,
         general_config,
         transactions,
         declared_class_hash_to_component_hashes: declared_class_hash_component_hashes,
         new_block_hash: block_with_txs.block_hash,
         prev_block_hash: previous_block.block_hash,
         full_output,
-    };
+    });
     let execution_helper = ExecutionHelperWrapper::<ProverPerContractStorage>::new(
         contract_storages,
         tx_execution_infos,
         &block_context,
+        Some(os_input.clone()),
         (old_block_number, old_block_hash),
     );
 

diff --git a/crates/bin/prove_block/tests/prove_block.rs b/crates/bin/prove_block/tests/prove_block.rs
@@ -62,6 +62,7 @@ pub const DEFAULT_COMPILED_OS: &[u8] = include_bytes!("../../../../build/os_late
 #[case::timestamp_rounding_1(162389)]
 #[case::timestamp_rounding_2(167815)]
 #[case::missing_constant_max_high(164684)]
+#[case::retdata_not_a_relocatable(160033)]
 #[ignore = "Requires a running Pathfinder node"]
 #[tokio::test(flavor = "multi_thread")]
 async fn test_prove_selected_blocks(#[case] block_number: u64) {

diff --git a/crates/starknet-os-types/src/class_hash_utils.rs b/crates/starknet-os-types/src/class_hash_utils.rs
@@ -105,11 +105,11 @@ mod tests {
         TEST_CONTRACT_SIERRA_CLASS,
         ContractClassComponentHashes {
             contract_class_version: Felt::from_hex_unchecked("0x434f4e54524143545f434c4153535f56302e312e30"),
-            external_functions_hash: Felt::from_hex_unchecked("0x36e860e1b55e22bf8ef7c84ad021ea718035ecddb0f690640abfdfa8afbe9f7"),
-            l1_handlers_hash: Felt::from_hex_unchecked("0x67f476df563e9995e5ec07abe031ee6e91209b6753de0f55cf1f734e4e97ab7"),
-            constructors_hash: Felt::from_hex_unchecked("0x2d110662880724966ec540bfc070c2652d11f518ddb4b4008bc6697b3796bc4"),
-            abi_hash: Felt::from_hex_unchecked("0x24e66ecb31bb31e172571b7651bc93ede3d2ffabcd7f44fd06daf5e8169fd99"),
-            sierra_program_hash: Felt::from_hex_unchecked("0x4d83801d2a12eb9704585ce206b65737229ff66caa483a4f986935549d0af88"),
+            external_functions_hash: Felt::from_hex_unchecked("0x22ea1c879b5fdb2f25faa8c85f749e4d9e5216827e4920456523679da537054"),
+            l1_handlers_hash: Felt::from_hex_unchecked("0x5a1be23d9907cf863d6547a10e0da2e5b20f2e2a3079102e98c46b4eab8d9a3"),
+            constructors_hash: Felt::from_hex_unchecked("0x32efa33857ba35a456c993af69ba93f2f936a4208070afb7b28fa10bcae83f0"),
+            abi_hash: Felt::from_hex_unchecked("0x6e6ccf79d27ce45711cda5964b29bf1558f01938ca005bee0ae17de6915199"),
+            sierra_program_hash: Felt::from_hex_unchecked("0x50d763c8720c24bb5c895be3dbfd68d296499a381d1e00ff626be93fbbec762"),
         }
     )]
     #[case::empty_contract(

diff --git a/crates/starknet-os/src/execution/deprecated_syscall_handler.rs b/crates/starknet-os/src/execution/deprecated_syscall_handler.rs
@@ -370,6 +370,7 @@ mod test {
             ContractStorageMap::default(),
             execution_infos,
             &block_context,
+            None,
             old_block_number_and_hash,
         );
 

diff --git a/crates/starknet-os/src/execution/helper.rs b/crates/starknet-os/src/execution/helper.rs
@@ -15,6 +15,7 @@ use tokio::sync::RwLock;
 
 use super::secp_handler::SecpSyscallProcessor;
 use crate::config::STORED_BLOCK_HASH_BUFFER;
+use crate::io::input::StarknetOsInput;
 use crate::starknet::core::os::kzg_manager::KzgManager;
 use crate::starknet::starknet_storage::{CommitmentInfo, CommitmentInfoError, PerContractStorage};
 use crate::storage::storage::StorageError;
@@ -28,6 +29,7 @@ where
     PCS: PerContractStorage,
 {
     pub _prev_block_context: Option<BlockContext>,
+    pub os_input: Option<Rc<StarknetOsInput>>,
     pub kzg_manager: KzgManager,
     // Pointer tx execution info
     pub tx_execution_info_iter: IntoIter<TransactionExecutionInfo>,
@@ -112,6 +114,7 @@ where
         contract_storage_map: ContractStorageMap<PCS>,
         tx_execution_infos: Vec<TransactionExecutionInfo>,
         block_context: &BlockContext,
+        os_input: Option<Rc<StarknetOsInput>>,
         old_block_number_and_hash: (Felt252, Felt252),
     ) -> Self {
         // Block number and block hash (current_block_number - buffer) block buffer=STORED_BLOCK_HASH_BUFFER
@@ -126,6 +129,7 @@ where
         Self {
             execution_helper: Rc::new(RwLock::new(ExecutionHelper {
                 _prev_block_context: prev_block_context,
+                os_input,
                 kzg_manager: Default::default(),
                 tx_execution_info_iter: tx_execution_infos.into_iter(),
                 tx_execution_info: None,

diff --git a/crates/starknet-os/src/execution/syscall_handler.rs b/crates/starknet-os/src/execution/syscall_handler.rs
@@ -247,6 +247,7 @@ pub struct DeployHandler;
 pub struct DeployResponse {
     pub contract_address: Felt252,
     pub constructor_retdata: ReadOnlySegment,
+    pub need_retdata_hack: bool,
 }
 
 impl<PCS> SyscallHandler<PCS> for DeployHandler
@@ -291,12 +292,59 @@ where
             "No more deployed contracts available to replay".to_string().into_boxed_str(),
         ))?;
 
-        Ok(DeployResponse { contract_address, constructor_retdata })
+        let need_retdata_hack = if let Some(os_input) = execution_helper.os_input.as_ref() {
+            let class_hash = os_input
+                .contract_address_to_class_hash
+                .get(&contract_address)
+                .expect("No class_hash for contract_address");
+            let num_constructors =
+                if let Some(compiled_class_hash) = os_input.class_hash_to_compiled_class_hash.get(class_hash) {
+                    let casm = os_input.compiled_classes.get(compiled_class_hash).expect("No CASM");
+                    let num_constructors = casm
+                        .get_cairo_lang_contract_class()
+                        .expect("couldn't get cairo lang class")
+                        .entry_points_by_type
+                        .constructor
+                        .len();
+                    num_constructors
+                } else {
+                    let deprecated_cc = os_input.deprecated_compiled_classes.get(class_hash).expect("no deprecated CC");
+                    let num_constructors = deprecated_cc
+                        .get_starknet_api_contract_class()
+                        .expect("couldn't get starknet api class")
+                        .entry_points_by_type
+                        .get(&starknet_api::deprecated_contract_class::EntryPointType::Constructor)
+                        .expect("should have constructor list")
+                        .len();
+                    num_constructors
+                };
+
+            // we need the hack if there are no constructor entry points
+            num_constructors == 0
+        } else {
+            false
+        };
+
+        Ok(DeployResponse { contract_address, constructor_retdata, need_retdata_hack })
     }
 
     fn write_response(response: Self::Response, vm: &mut VirtualMachine, ptr: &mut Relocatable) -> WriteResponseResult {
         write_felt(vm, ptr, response.contract_address)?;
-        write_segment(vm, ptr, response.constructor_retdata)?;
+
+        // Bugfix:
+        // When retdata size is 0, the OS will return retdata as a Int(0) instead of a relocatable
+        // as a result of `cast(0, felt*)`. To work around this, we can write the same here so that
+        // later the OS will assert this value is the same as retdata; that is, both need to be the
+        // same value which is Int(0).
+        //
+        // retdata is cast here: https://github.com/starkware-libs/cairo-lang/blob/a86e92bfde9c171c0856d7b46580c66e004922f3/src/starkware/starknet/core/os/execution/execute_entry_point.cairo#L170
+        if response.need_retdata_hack {
+            log::warn!("Writing Felt::ZERO instead of pointer since retdata size is 0");
+            write_felt(vm, ptr, Felt252::ZERO)?; // casted pointer
+            write_felt(vm, ptr, Felt252::ZERO)?; // size
+        } else {
+            write_segment(vm, ptr, response.constructor_retdata)?;
+        }
         Ok(())
     }
 }

diff --git a/crates/starknet-os/src/hints/block_context.rs b/crates/starknet-os/src/hints/block_context.rs
@@ -2,6 +2,7 @@ use core::panic;
 use std::any::Any;
 use std::collections::hash_map::IntoIter;
 use std::collections::HashMap;
+use std::rc::Rc;
 
 use blockifier::context::BlockContext;
 use cairo_vm::hint_processor::builtin_hint_processor::dict_manager::Dictionary;
@@ -41,7 +42,7 @@ pub fn load_class_facts(
     ap_tracking: &ApTracking,
     _constants: &HashMap<String, Felt252>,
 ) -> Result<(), HintError> {
-    let os_input = exec_scopes.get::<StarknetOsInput>(vars::scopes::OS_INPUT)?;
+    let os_input: Rc<StarknetOsInput> = exec_scopes.get::<Rc<StarknetOsInput>>(vars::scopes::OS_INPUT)?.clone();
     let compiled_class_facts_ptr = vm.add_memory_segment();
     insert_value_from_var_name(vars::ids::COMPILED_CLASS_FACTS, compiled_class_facts_ptr, vm, ids_data, ap_tracking)?;
 
@@ -53,8 +54,8 @@ pub fn load_class_facts(
         ap_tracking,
     )?;
 
-    let compiled_class_facts: Box<dyn Any> = Box::new(os_input.compiled_classes.into_iter());
-    let compiled_class_visited_pcs: Box<dyn Any> = Box::new(os_input.compiled_class_visited_pcs);
+    let compiled_class_facts: Box<dyn Any> = Box::new(os_input.compiled_classes.clone().into_iter());
+    let compiled_class_visited_pcs: Box<dyn Any> = Box::new(os_input.compiled_class_visited_pcs.clone());
     exec_scopes.enter_scope(HashMap::from([
         (String::from(vars::scopes::COMPILED_CLASS_FACTS), compiled_class_facts),
         (String::from(vars::scopes::COMPILED_CLASS_VISITED_PCS), compiled_class_visited_pcs),
@@ -218,7 +219,7 @@ pub fn chain_id(
     _ap_tracking: &ApTracking,
     _constants: &HashMap<String, Felt252>,
 ) -> Result<(), HintError> {
-    let os_input = exec_scopes.get::<StarknetOsInput>(vars::scopes::OS_INPUT)?;
+    let os_input = exec_scopes.get::<Rc<StarknetOsInput>>(vars::scopes::OS_INPUT)?;
     let chain_id = chain_id_to_felt(&os_input.general_config.starknet_os_config.chain_id);
     insert_value_into_ap(vm, chain_id)
 }
@@ -231,7 +232,7 @@ pub fn fee_token_address(
     _ap_tracking: &ApTracking,
     _constants: &HashMap<String, Felt252>,
 ) -> Result<(), HintError> {
-    let os_input = exec_scopes.get::<StarknetOsInput>(vars::scopes::OS_INPUT)?;
+    let os_input = exec_scopes.get::<Rc<StarknetOsInput>>(vars::scopes::OS_INPUT)?;
     let fee_token_address = *os_input.general_config.starknet_os_config.fee_token_address.0.key();
     log::debug!("fee_token_address: {}", fee_token_address);
     insert_value_into_ap(vm, fee_token_address)
@@ -246,7 +247,7 @@ pub fn deprecated_fee_token_address(
     _ap_tracking: &ApTracking,
     _constants: &HashMap<String, Felt252>,
 ) -> Result<(), HintError> {
-    let os_input = exec_scopes.get::<StarknetOsInput>(vars::scopes::OS_INPUT)?;
+    let os_input = exec_scopes.get::<Rc<StarknetOsInput>>(vars::scopes::OS_INPUT)?;
     let deprecated_fee_token_address = *os_input.general_config.starknet_os_config.deprecated_fee_token_address.0.key();
     log::debug!("deprecated_fee_token_address: {}", deprecated_fee_token_address);
     insert_value_into_ap(vm, deprecated_fee_token_address)
@@ -357,7 +358,7 @@ pub fn write_use_kzg_da_to_mem(
     let block_context = exec_scopes.get_ref::<BlockContext>(vars::scopes::BLOCK_CONTEXT)?;
     let use_kzg_da = block_context.block_info().use_kzg_da;
 
-    let os_input: &StarknetOsInput = exec_scopes.get_ref(vars::scopes::OS_INPUT)?;
+    let os_input = exec_scopes.get::<Rc<StarknetOsInput>>(vars::scopes::OS_INPUT)?;
     let full_output = os_input.full_output;
 
     let use_kzg_da_felt = if use_kzg_da && !full_output { Felt252::ONE } else { Felt252::ZERO };

diff --git a/crates/starknet-os/src/hints/deprecated_compiled_class.rs b/crates/starknet-os/src/hints/deprecated_compiled_class.rs
@@ -1,6 +1,7 @@
 use std::any::Any;
 use std::collections::hash_map::IntoIter;
 use std::collections::{HashMap, HashSet};
+use std::rc::Rc;
 
 use cairo_vm::hint_processor::builtin_hint_processor::hint_utils::{get_ptr_from_var_name, insert_value_from_var_name};
 use cairo_vm::hint_processor::hint_processor_definition::{HintExtension, HintProcessor, HintReference};
@@ -35,7 +36,7 @@ pub fn load_deprecated_class_facts(
     ap_tracking: &ApTracking,
     _constants: &HashMap<String, Felt252>,
 ) -> Result<(), HintError> {
-    let os_input = exec_scopes.get::<StarknetOsInput>(vars::scopes::OS_INPUT)?;
+    let os_input = exec_scopes.get::<Rc<StarknetOsInput>>(vars::scopes::OS_INPUT)?;
     let deprecated_class_hashes: HashSet<Felt252> =
         HashSet::from_iter(os_input.deprecated_compiled_classes.keys().cloned());
     exec_scopes.insert_value(vars::scopes::DEPRECATED_CLASS_HASHES, deprecated_class_hashes);
@@ -48,7 +49,7 @@ pub fn load_deprecated_class_facts(
         ids_data,
         ap_tracking,
     )?;
-    let scoped_classes: Box<dyn Any> = Box::new(os_input.deprecated_compiled_classes.into_iter());
+    let scoped_classes: Box<dyn Any> = Box::new(os_input.deprecated_compiled_classes.clone().into_iter());
     exec_scopes.enter_scope(HashMap::from([(String::from(vars::scopes::COMPILED_CLASS_FACTS), scoped_classes)]));
 
     Ok(())

diff --git a/crates/starknet-os/src/hints/execution.rs b/crates/starknet-os/src/hints/execution.rs
@@ -1,11 +1,13 @@
 use std::any::Any;
 use std::borrow::Cow;
 use std::collections::{HashMap, HashSet};
+use std::rc::Rc;
 use std::vec::IntoIter;
 
 use cairo_vm::hint_processor::builtin_hint_processor::dict_manager::Dictionary;
 use cairo_vm::hint_processor::builtin_hint_processor::hint_utils::{
-    get_integer_from_var_name, get_ptr_from_var_name, insert_value_from_var_name, insert_value_into_ap,
+    get_integer_from_var_name, get_maybe_relocatable_from_var_name, get_ptr_from_var_name, insert_value_from_var_name,
+    insert_value_into_ap,
 };
 use cairo_vm::hint_processor::hint_processor_definition::HintReference;
 use cairo_vm::hint_processor::hint_processor_utils::felt_to_usize;
@@ -434,11 +436,11 @@ pub fn enter_syscall_scopes<PCS>(
 where
     PCS: PerContractStorage + 'static,
 {
-    let os_input = exec_scopes.get::<StarknetOsInput>(vars::scopes::OS_INPUT)?;
+    let os_input = exec_scopes.get::<Rc<StarknetOsInput>>(vars::scopes::OS_INPUT)?;
     let deprecated_class_hashes: Box<dyn Any> =
         Box::new(exec_scopes.get::<HashSet<Felt252>>(vars::scopes::DEPRECATED_CLASS_HASHES)?);
-    let transactions: Box<dyn Any> = Box::new(os_input.transactions.into_iter());
-    let component_hashes: Box<dyn Any> = Box::new(os_input.declared_class_hash_to_component_hashes);
+    let transactions: Box<dyn Any> = Box::new(os_input.transactions.clone().into_iter());
+    let component_hashes: Box<dyn Any> = Box::new(os_input.declared_class_hash_to_component_hashes.clone());
     let execution_helper: Box<dyn Any> =
         Box::new(exec_scopes.get::<ExecutionHelperWrapper<PCS>>(vars::scopes::EXECUTION_HELPER)?);
     let deprecated_syscall_handler: Box<dyn Any> =
@@ -1119,6 +1121,27 @@ pub fn check_new_deploy_response(
     _constants: &HashMap<String, Felt252>,
 ) -> Result<(), HintError> {
     let response_ptr = get_ptr_from_var_name(vars::ids::RESPONSE, vm, ids_data, ap_tracking)?;
+
+    // Bugfix:
+    // If constructor_retdata_start is a Int(0) instead of a Relocatable, we need to do nothing. This
+    // can happen sometimes when a constructor is called but not defined in the class'es entry
+    // points. Immediately following this, `add_relocation_rule()` will be called with src=0, dest=0
+    // and it will also no-op.
+    //
+    // If "retdata" is an Int instead of a Relocatable, then the vm will error when we try to
+    // request it as such. In this case, there is no retdata anyway, so there is no need to assert
+    // that the contents are equal.
+    let retdata_start_key: Relocatable =
+        (response_ptr + new_syscalls::DeployResponse::constructor_retdata_start_offset())?;
+    let maybe_retdata_start = vm
+        .get_maybe(&retdata_start_key)
+        .ok_or(HintError::VariableNotInScopeError("retdata".to_string().into_boxed_str()))?;
+    let zero = MaybeRelocatable::Int(Felt252::ZERO);
+    if maybe_retdata_start == zero {
+        log::warn!("retdata_start is 0, not a relocatable, ignoring add_relocation_rule()");
+        return Ok(());
+    }
+
     let constructor_retdata_start =
         vm.get_relocatable((response_ptr + new_syscalls::DeployResponse::constructor_retdata_start_offset())?)?;
     let constructor_retdata_end =
@@ -1266,6 +1289,19 @@ pub fn add_relocation_rule(
     ap_tracking: &ApTracking,
     _constants: &HashMap<String, Felt252>,
 ) -> Result<(), HintError> {
+    // Bugfix:
+    // add_relocation_rule() can be called sometimes with Int(0) instead of Relocatables. This is
+    // a result of `cast(0, felt*)` being treated as Int(0). We can work around this here by
+    // ensuring that both src and dest end up being Int(0), and in that case we no-op here.
+    let src_ptr_maybe = get_maybe_relocatable_from_var_name(vars::ids::SRC_PTR, vm, ids_data, ap_tracking)?;
+    let dest_ptr_maybe = get_maybe_relocatable_from_var_name(vars::ids::DEST_PTR, vm, ids_data, ap_tracking)?;
+
+    let zero = MaybeRelocatable::Int(Felt252::ZERO);
+    if src_ptr_maybe == zero && dest_ptr_maybe == zero {
+        log::warn!("add_relocation_rule with Int(0) => Int(0), doing nothing");
+        return Ok(());
+    }
+
     let src_ptr = get_ptr_from_var_name(vars::ids::SRC_PTR, vm, ids_data, ap_tracking)?;
 
     assert_eq!(src_ptr.offset, 0, "src_ptr offset should be 0");
@@ -1913,7 +1949,7 @@ mod tests {
 
     #[fixture]
     fn execution_helper(block_context: BlockContext, old_block_number_and_hash: (Felt252, Felt252)) -> EHW {
-        EHW::new(ContractStorageMap::default(), vec![], &block_context, old_block_number_and_hash)
+        EHW::new(ContractStorageMap::default(), vec![], &block_context, None, old_block_number_and_hash)
     }
 
     #[fixture]