apollo_starknet_os_program: address review comments

Author: Yoni-Starkware

crates/apollo_batcher/src/commitment_manager/commitment_manager_impl.rs

@@ -399,18 +399,21 @@ impl<S: StateCommitterTrait> CommitmentManager<S> {
             }
             true => {
                 info!("Finalizing commitment for block {height} with calculating block hash.");
-                let (parent_hash, partial_block_hash_components) =
+                let (previous_block_hash, partial_block_hash_components) =
                     storage_reader.get_parent_hash_and_partial_block_hash_components(height)?;
-                let parent_hash = parent_hash.ok_or_else(|| {
+                let previous_block_hash = previous_block_hash.ok_or_else(|| {
                     CommitmentManagerError::MissingBlockHash(height.prev().expect(
                         "For the genesis block, the block hash is constant and should not be \
                          fetched from storage.",
                     ))
                 })?;
                 let partial_block_hash_components = partial_block_hash_components
                     .ok_or(CommitmentManagerError::MissingPartialBlockHashComponents(height))?;
-                let block_hash =
-                    calculate_block_hash(&partial_block_hash_components, global_root, parent_hash)?;
+                let block_hash = calculate_block_hash(
+                    &partial_block_hash_components,
+                    global_root,
+                    previous_block_hash,
+                )?;
                 Ok(FinalBlockCommitment { height, block_hash: Some(block_hash), global_root })
             }
         }

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/block_context.cairo

@@ -66,7 +66,6 @@ func get_block_context{range_check_ptr}(os_global_context: OsGlobalContext*) ->
         block_info.block_timestamp, VALIDATE_TIMESTAMP_ROUNDING
     );
     tempvar block_timestamp_for_validate = divided_block_timestamp * VALIDATE_TIMESTAMP_ROUNDING;
-    let compiled_class_facts_bundle = os_global_context.compiled_class_facts_bundle;
     local block_context: BlockContext = BlockContext(
         os_global_context=[os_global_context],
         block_info_for_execute=block_info,

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/block_hash.cairo

@@ -10,7 +10,9 @@ struct BlockHeaderCommitments {
     event_commitment: felt,
     receipt_commitment: felt,
     state_diff_commitment: felt,
-    concatenated_counts: felt,
+    // Packed encoding of transaction count, event count, state diff length, and L1 data
+    // availability mode.
+    packed_lengths: felt,
 }
 
 // Calculates the block hash given the top level components.
@@ -19,25 +21,28 @@ func calculate_block_hash{poseidon_ptr: PoseidonBuiltin*}(
     header_commitments: BlockHeaderCommitments*,
     gas_prices_hash: felt,
     state_root: felt,
-    parent_hash: felt,
+    previous_block_hash: felt,
     starknet_version: felt,
 ) -> felt {
+    static_assert BlockInfo.SIZE == 3;
+    static_assert BlockHeaderCommitments.SIZE == 5;
+
     let hash_state = hash_init();
     with hash_state {
         hash_update_single(BLOCK_HASH_VERSION);
         hash_update_single(block_info.block_number);
         hash_update_single(state_root);
         hash_update_single(block_info.sequencer_address);
         hash_update_single(block_info.block_timestamp);
-        hash_update_single(header_commitments.concatenated_counts);
+        hash_update_single(header_commitments.packed_lengths);
         hash_update_single(header_commitments.state_diff_commitment);
         hash_update_single(header_commitments.transaction_commitment);
         hash_update_single(header_commitments.event_commitment);
         hash_update_single(header_commitments.receipt_commitment);
         hash_update_single(gas_prices_hash);
         hash_update_single(starknet_version);
         hash_update_single(0);
-        hash_update_single(parent_hash);
+        hash_update_single(previous_block_hash);
     }
 
     let block_hash = hash_finalize(hash_state=hash_state);
@@ -51,11 +56,13 @@ func get_block_hashes{poseidon_ptr: PoseidonBuiltin*}(block_info: BlockInfo*, st
     previous_block_hash: felt, new_block_hash: felt
 ) {
     alloc_locals;
-    local parent_hash;
+    local previous_block_hash;
     // Currently, the header commitments and gas prices are not computed by the OS.
     // TODO(Yoni, 1/1/2027): compute the header commitments and gas prices.
     local header_commitments: BlockHeaderCommitments*;
     local gas_prices_hash;
+    // TODO(Yoni): move to global context, and consider enforcing a specific version for the
+    // non-virtual OS.
     local starknet_version;
 
     %{ GetBlockHashes %}
@@ -65,11 +72,11 @@ func get_block_hashes{poseidon_ptr: PoseidonBuiltin*}(block_info: BlockInfo*, st
         header_commitments=header_commitments,
         gas_prices_hash=gas_prices_hash,
         state_root=state_root,
-        parent_hash=parent_hash,
+        previous_block_hash=previous_block_hash,
         starknet_version=starknet_version,
     );
 
     %{ CheckBlockHashConsistency %}
 
-    return (previous_block_hash=parent_hash, new_block_hash=block_hash);
+    return (previous_block_hash=previous_block_hash, new_block_hash=block_hash);
 }

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/constants.cairo

@@ -66,7 +66,7 @@ const STORED_BLOCK_HASH_BUFFER = 10;
 
 // Allowed virtual OS program hashes for client-side proving.
 const ALLOWED_VIRTUAL_OS_PROGRAM_HASHES_0 = (
-    0x06972cfa5c07f702981678547574e239a24ad8bb53cc081ad738ccb10839fd1a
+    0x0391095dffec88985e40bfa640aa05fd05ed050fcee5b79c27f492de3a68b77f
 );
 const ALLOWED_VIRTUAL_OS_PROGRAM_HASHES_LEN = 1;
 

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/execution/execution_constraints.cairo

@@ -2,7 +2,7 @@
 
 from starkware.cairo.common.bool import FALSE, TRUE
 from starkware.cairo.common.dict_access import DictAccess
-from starkware.cairo.common.math import assert_le, assert_not_zero
+from starkware.cairo.common.math import assert_le, assert_nn_le, assert_not_zero
 from starkware.starknet.core.os.constants import (
     ALLOWED_VIRTUAL_OS_PROGRAM_HASHES_0,
     ALLOWED_VIRTUAL_OS_PROGRAM_HASHES_LEN,
@@ -53,10 +53,12 @@ func check_proof_facts{range_check_ptr, contract_state_changes: DictAccess*}(
         assert os_output_header.output_version = VIRTUAL_OS_OUTPUT_VERSION;
     }
 
-    // validate that the proof facts block number is not too recent
-    // (the first check is to avoid underflow in the second one).
-    assert_le(STORED_BLOCK_HASH_BUFFER, current_block_number);
-    assert_le(os_output_header.base_block_number, current_block_number - STORED_BLOCK_HASH_BUFFER);
+    // Validate that the proof facts block number is not too recent.
+    // (This is a sanity check - the following non-zero check ensures that the block hash is
+    // not trivial).
+    assert_nn_le(
+        os_output_header.base_block_number, current_block_number - STORED_BLOCK_HASH_BUFFER
+    );
     // Not all block hashes are stored in the contract; Make sure the requested one is not trivial.
     assert_not_zero(os_output_header.base_block_hash);
 

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/execution/syscall_impls.cairo

@@ -16,13 +16,7 @@ from starkware.cairo.common.cairo_secp.signature import (
 )
 from starkware.cairo.common.dict import dict_read, dict_update
 from starkware.cairo.common.dict_access import DictAccess
-from starkware.cairo.common.math import (
-    assert_le,
-    assert_lt,
-    assert_nn,
-    assert_nn_le,
-    unsigned_div_rem,
-)
+from starkware.cairo.common.math import assert_lt, assert_nn, assert_nn_le, unsigned_div_rem
 from starkware.cairo.common.memcpy import memcpy
 from starkware.cairo.common.secp256r1.constants import SECP_PRIME_HIGH as SECP256R1_PRIME_HIGH
 from starkware.cairo.common.secp256r1.constants import SECP_PRIME_LOW as SECP256R1_PRIME_LOW
@@ -691,6 +685,8 @@ func execute_storage_write{
     return ();
 }
 
+// Verifies that the block hash for the given block number equals the expected block hash,
+// by adding a read entry to the contract_state_changes dict with it.
 func read_block_hash_from_storage{contract_state_changes: DictAccess*}(
     block_number: felt, expected_block_hash: felt
 ) {
@@ -754,7 +750,7 @@ func execute_get_block_hash{
         return ();
     }
 
-    assert_le(request_block_number + STORED_BLOCK_HASH_BUFFER, current_block_number);
+    assert_nn_le(request_block_number, current_block_number - STORED_BLOCK_HASH_BUFFER);
 
     // Gas reduction has succeeded and the request is valid; write the response header.
     let response_header = cast(syscall_ptr, ResponseHeader*);

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/execution/transaction_impls.cairo

@@ -170,13 +170,14 @@ func charge_fee{
 func get_account_tx_common_fields(
     block_context: BlockContext*, tx_hash_prefix: felt, sender_address: felt
 ) -> CommonTxFields* {
-    tempvar resource_bounds: ResourceBounds*;
-    tempvar tip;
-    tempvar paymaster_data_length;
-    tempvar paymaster_data: felt*;
-    tempvar nonce_data_availability_mode;
-    tempvar fee_data_availability_mode;
-    tempvar nonce;
+    alloc_locals;
+    local resource_bounds: ResourceBounds*;
+    local tip;
+    local paymaster_data_length;
+    local paymaster_data: felt*;
+    local nonce_data_availability_mode;
+    local fee_data_availability_mode;
+    local nonce;
     %{ LoadCommonTxFields %}
     %{ LoadTxNonceAccount %}
     tempvar common_tx_fields = new CommonTxFields(

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/os.cairo

@@ -125,7 +125,6 @@ func main{
         os_outputs=os_outputs,
         n_public_keys=n_public_keys,
         public_keys=public_keys,
-        os_global_context=os_global_context,
     );
 
     // The following code deals with the problem that untrusted code (contract code) could

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/os_utils.cairo

@@ -127,6 +127,8 @@ func get_block_os_output_header{poseidon_ptr: PoseidonBuiltin*}(
     // NOTE: both the previous block hash and previous state root are guessed, and the OS
     // does not verify their consistency (unlike the new hash and root).
     // The consumer of the OS output should verify both.
+    // TODO(Yoni): verify the consistency of the previous block hash and state root, and remove the
+    // state roots from the OS output header.
     let (prev_block_hash, new_block_hash) = get_block_hashes{poseidon_ptr=poseidon_ptr}(
         block_info=block_context.block_info_for_execute, state_root=state_update_output.final_root
     );
@@ -150,13 +152,7 @@ func get_block_os_output_header{poseidon_ptr: PoseidonBuiltin*}(
 // Processes OS outputs by combining blocks and serializing the result.
 func process_os_output{
     output_ptr: felt*, range_check_ptr, ec_op_ptr: EcOpBuiltin*, poseidon_ptr: PoseidonBuiltin*
-}(
-    n_blocks: felt,
-    os_outputs: OsOutput*,
-    n_public_keys: felt,
-    public_keys: felt*,
-    os_global_context: OsGlobalContext*,
-) {
+}(n_blocks: felt, os_outputs: OsOutput*, n_public_keys: felt, public_keys: felt*) {
     alloc_locals;
     // Guess whether to use KZG commitment scheme and whether to output the full state.
     // TODO(meshi): Once use_kzg_da field is used in the OS for the computation of fees and block

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/os_utils__virtual.cairo

@@ -27,19 +27,17 @@ func hash_messages_to_l1_recursive{output_ptr: felt*, poseidon_ptr: PoseidonBuil
 
     // Hash the message (header + payload).
     // The message consists of: from_address, to_address, payload_size, ...payload.
-    let message_size = MessageToL1Header.SIZE + payload_size;
+    local message_size = MessageToL1Header.SIZE + payload_size;
     let (message_hash) = poseidon_hash_many(
         n=message_size, elements=cast(messages_ptr_start, felt*)
     );
 
     // Store the hash and advance output_ptr.
-    assert [output_ptr] = message_hash;
+    assert output_ptr[0] = message_hash;
     let output_ptr = &output_ptr[1];
 
     // Move to the next message.
-    let next_message_ptr = cast(
-        messages_ptr_start + MessageToL1Header.SIZE + payload_size, MessageToL1Header*
-    );
+    let next_message_ptr = cast(messages_ptr_start + message_size, MessageToL1Header*);
 
     // Recursively process the remaining messages.
     return hash_messages_to_l1_recursive(
@@ -65,16 +63,16 @@ func get_block_os_output_header{poseidon_ptr: PoseidonBuiltin*}(
     state_update_output: CommitmentUpdate*,
     os_global_context: OsGlobalContext*,
 ) -> OsOutputHeader* {
-    // Calculate the block hash based on the block info and the **initial** state root.
-    let (_prev_block_hash, block_hash) = get_block_hashes{poseidon_ptr=poseidon_ptr}(
+    // Calculate the previous block hash based on the block info and the **initial** state root.
+    let (_prev_prev_block_hash, prev_block_hash) = get_block_hashes{poseidon_ptr=poseidon_ptr}(
         block_info=block_context.block_info_for_execute, state_root=state_update_output.initial_root
     );
 
     tempvar os_output_header = new OsOutputHeader(
         state_update_output=state_update_output,
         prev_block_number=block_context.block_info_for_execute.block_number,
         new_block_number=0,
-        prev_block_hash=block_hash,
+        prev_block_hash=prev_block_hash,
         new_block_hash=0,
         os_program_hash=0,
         starknet_os_config_hash=os_global_context.starknet_os_config_hash,
@@ -88,13 +86,7 @@ func get_block_os_output_header{poseidon_ptr: PoseidonBuiltin*}(
 // Outputs the virtual OS header and the messages to L1.
 func process_os_output{
     output_ptr: felt*, range_check_ptr, ec_op_ptr: EcOpBuiltin*, poseidon_ptr: PoseidonBuiltin*
-}(
-    n_blocks: felt,
-    os_outputs: OsOutput*,
-    n_public_keys: felt,
-    public_keys: felt*,
-    os_global_context: OsGlobalContext*,
-) {
+}(n_blocks: felt, os_outputs: OsOutput*, n_public_keys: felt, public_keys: felt*) {
     alloc_locals;
     assert n_public_keys = 0;
 
@@ -123,7 +115,7 @@ func process_os_output{
         output_version=VIRTUAL_OS_OUTPUT_VERSION,
         base_block_number=header.prev_block_number,
         base_block_hash=header.prev_block_hash,
-        starknet_os_config_hash=os_global_context.starknet_os_config_hash,
+        starknet_os_config_hash=header.starknet_os_config_hash,
         n_messages_to_l1=n_messages_to_l1,
     );