blockifier: verify that the proof facts block number matches the block hash #11345

meship-starkware · 2026-01-01T10:03:36Z

No description provided.

reviewable-StarkWare · 2026-01-01T10:03:44Z

This change is

github-actions · 2026-01-01T10:03:50Z

Artifacts upload workflows:

meship-starkware · 2026-01-01T10:03:52Z

blockifier: verify that the proof facts block number matches the block hash #11345 : 3 dependent PRs (#11476 , #11485 , #11704 ) 👈 (View in Graphite)
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

noaov1

@noaov1 reviewed 1 file and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @meship-starkware).

crates/blockifier/src/transaction/account_transaction.rs line 258 at r1 (raw file):

        if let Transaction::Invoke(tx) = &self.tx {
            if tx.tx.version() == TransactionVersion::THREE {
                let proof_facts_variant = ProofFactsVariant::try_from(&tx.tx.proof_facts())

Suggestion:

        if let Transaction::Invoke(tx) = &self.tx {
            if tx.version() == TransactionVersion::THREE {
                let proof_facts_variant = ProofFactsVariant::try_from(&tx.proof_facts())

crates/blockifier/src/transaction/account_transaction.rs line 296 at r1 (raw file):

                    }
                }
            }

Personal preference (early returns).

Suggestion:

    fn validate_proof_facts(
        &self,
        tx_context: &TransactionContext,
        state: &mut dyn State,
    ) -> TransactionPreValidationResult<()> {
        // Only Invoke V3 transactions can carry proof facts that we validate here.
        let Transaction::Invoke(invoke_tx) = &self.tx else {
            return Ok(());
        };
        if invoke_tx.version() != TransactionVersion::THREE {
            return Ok(());
        }
    
        let proof_facts = invoke_tx.proof_facts();
        let proof_facts_variant = ProofFactsVariant::try_from(&proof_facts)
            .map_err(|e| TransactionPreValidationError::InvalidProofFacts(e.to_string()))?;
    
        let ProofFactsVariant::Snos(snos_proof_facts) = proof_facts_variant else {
            // Empty proof facts are valid.
            return Ok(());
        };
    
        // TODO(Meshi/AvivG): Add more proof facts validations.
        let proof_block_number = snos_proof_facts.block_number;
        let proof_block_hash = snos_proof_facts.block_hash.0;
    
        // Validate the proof block number is not too recent.
        // Block-number to block-hash mapping is only guaranteed up to
        // current_block_number - STORED_BLOCK_HASH_BUFFER.
        let current_block_number = tx_context.block_context.block_info.block_number;
        if current_block_number.0 - proof_block_number.0 < STORED_BLOCK_HASH_BUFFER {
            return Err(TransactionPreValidationError::InvalidProofFacts(format!(
                "Proof block number {proof_block_number} is too recent. \
                 Block hashes are only available for blocks ≤ \
                current_block_number - STORED_BLOCK_HASH_BUFFER ({current_block_number} - {STORED_BLOCK_HASH_BUFFER})."
            )));
        }
    
        // Validate that the proof block hash matches the stored hash.
        let block_hash_contract_address = &tx_context
            .block_context
            .versioned_constants
            .os_constants
            .os_contract_addresses
            .block_hash_contract_address();
    
        let stored_block_hash = state.get_storage_at(
            block_hash_contract_address,
            StorageKey::from(proof_block_number.0),
        )?;
    
        if stored_block_hash != proof_block_hash {
            return Err(TransactionPreValidationError::InvalidProofFacts(format!(
                "Block hash mismatch for block {proof_block_number}. Proof block hash: \
                {proof_block_hash}, stored block hash: {stored_block_hash}."
            )));
        }
    
        Ok(())
    }

noaov1

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @meship-starkware).

noaov1

@noaov1 made 1 comment and resolved 1 discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @meship-starkware).

crates/blockifier/src/transaction/account_transaction.rs line 287 at r1 (raw file):

                            os_addresses.block_hash_contract_address(),
                            StorageKey::from(proof_block_number.0),
                        )?;

Please add an assert that it's a non-zero value.

Code quote:

                        let stored_block_hash = state.get_storage_at(
                            os_addresses.block_hash_contract_address(),
                            StorageKey::from(proof_block_number.0),
                        )?;

meship-starkware

@meship-starkware made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @noaov1).

crates/blockifier/src/transaction/account_transaction.rs line 287 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Please add an assert that it's a non-zero value.

This will fail the gateway tests, in which the stored hash is zero; the possible solutions I see are

Check this assertion only if the test feature is on
Write a non-zero value to the memory of the block number in the hash contract when creating the

I prefer the second solution mor,e but do we want to enforce passing the VC to the function that creates the proof facts or can we just create a dummy VC to get the contract adress for the tests?

meship-starkware

@meship-starkware made 1 comment and resolved 1 discussion.
Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @noaov1).

crates/blockifier/src/transaction/account_transactions_test.rs line 2210 at r2 (raw file):

    ]
    .into()
}

I am considering moving it to a better location, maybe test utils

Code quote:

/// Converts SnosProofFacts to ProofFacts for testing.
fn snos_to_proof_facts(snos: SnosProofFacts) -> ProofFacts {
    vec![
        felt!(VIRTUAL_SNOS),
        snos.program_hash,
        felt!(snos.block_number.0),
        snos.block_hash.0,
        snos.config_hash,
    ]
    .into()
}

meship-starkware

@meship-starkware made 1 comment.
Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @noaov1).

crates/blockifier/src/transaction/account_transaction.rs line 285 at r2 (raw file):

                 {proof_block_number}."
            )));
        }

I am not sure it is our scope to check this, but this should always be true, as you cannot prove anything on a further block.

Code quote:

        if current_block_number.0 < proof_block_number.0 {
            return Err(TransactionPreValidationError::InvalidProofFacts(format!(
                "Current block number {current_block_number} is less than proof block number \
                 {proof_block_number}."
            )));
        }

meship-starkware

@meship-starkware made 1 comment.
Reviewable status: 0 of 7 files reviewed, 3 unresolved discussions (waiting on @noaov1).

crates/blockifier/src/transaction/account_transaction.rs line 287 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

This will fail the gateway tests, in which the stored hash is zero; the possible solutions I see are

Check this assertion only if the test feature is on

Write a non-zero value to the memory of the block number in the hash contract when creating the

I prefer the second solution mor,e but do we want to enforce passing the VC to the function that creates the proof facts or can we just create a dummy VC to get the contract adress for the tests?

Done.

meship-starkware

@meship-starkware resolved 1 discussion.
Reviewable status: 0 of 7 files reviewed, 2 unresolved discussions (waiting on @avivg-starkware and @noaov1).

meship-starkware

This PR is a bit longer than expected becuse I had to add patches so that the check that the stored block hash is non-zero will pass both os tests. and the gateway. LMK if you think I should separate this check into a different PR.

@meship-starkware made 1 comment.
Reviewable status: 0 of 7 files reviewed, 2 unresolved discussions (waiting on @avivg-starkware and @noaov1).

avivg-starkware

@avivg-starkware reviewed 7 files and all commit messages, and made 8 comments.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @meship-starkware and @noaov1).

crates/starknet_os_flow_tests/src/tests.rs line 1190 at r4 (raw file):

    // Add proof facts block hash storage to the decompressed state diff.
    // The Blockifier writes this for proof facts verification, but the OS doesn't include it.
    // TODO(Meshi): write the mapping for the os validation as well.

I think this info is crucial in some way or another, especially around the temporary fixes that you plan to remove. It doesn't need to be altogether here, just wanted to make sure:
1- I understand properly
2- Others could understand once the fix is merged
3- We don't end up leaving those behind by accident in further refactorings.

Suggestion:

    // Reconcile proof facts block-hash storage between Blockifier and OS outputs.
    //
    // Context: To allow testing transactions with proof_facts validations, the Blockifier writes a storage
    // entry: (block_hash_contract, proof_block_number) -> proof_block_hash. 
    //
    // However, the OS doesn't emit this storage write in its output (the proof facts verification was 
    // not yet added to the OS, and thus having non non-empty (block_number -> block_hash) mapping is not yet
    // required for the tests to pass).
    //
    // This causes a mismatch: Blockifier's state diff includes the write, but OS output doesn't.
    // We manually add it here so the test comparison passes.
    //
    // TODO(Meshi): Move block hash storage setup to not affect the writes, then remove this fixup.

crates/starknet_os_flow_tests/src/tests.rs line 1191 at r4 (raw file):

    // The Blockifier writes this for proof facts verification, but the OS doesn't include it.
    // TODO(Meshi): write the mapping for the os validation as well.
    if let Ok(ProofFactsVariant::Snos(snos_proof_facts)) = ProofFactsVariant::try_from(&proof_facts)

Suggestion:

proof_facts.try_into()

crates/blockifier/src/transaction/account_transaction.rs line 312 at r4 (raw file):

        Ok(())
    }

few small clean up suggestions + consider exporting the block hash validation to another func if we will add other validations

Suggestion:

    fn validate_proof_facts(
        &self,
        os_constants: &OsConstants,
        current_block_number: BlockNumber,
        state: &mut dyn State,
    ) -> TransactionPreValidationResult<()> {
        // Only Invoke V3 transactions can carry proof facts.
        let Transaction::Invoke(invoke_tx) = &self.tx else {
            return Ok(());
        };
        if invoke_tx.version() != TransactionVersion::THREE {
            return Ok(());
        }

        // Parse proof facts.
        let proof_facts = invoke_tx.proof_facts();
        let snos_proof_facts = match ProofFactsVariant::try_from(&proof_facts)
            .map_err(|e| TransactionPreValidationError::InvalidProofFacts(e.to_string()))?
        {
            ProofFactsVariant::Empty => return Ok(()),
            ProofFactsVariant::Snos(snos_proof_facts) => snos_proof_facts,
        };

        // Proof block must be old enough to have a stored block hash.
        // Stored block hashes are guaranteed only up to: current - STORED_BLOCK_HASH_BUFFER.
        let max_allowed = current_block_number.0.saturating_sub(STORED_BLOCK_HASH_BUFFER);

        let proof_block_number = snos_proof_facts.block_number.0;
        if proof_block_number > max_allowed {
            return Err(TransactionPreValidationError::InvalidProofFacts(format!(
                "Invalid proof block number {proof_block_number}. Block hashes are only available \
                 for blocks ≤ current_block_number - STORED_BLOCK_HASH_BUFFER \
                 ({current_block_number} - {STORED_BLOCK_HASH_BUFFER} = {max_allowed})."
            )));
        }

        // Compare the proof's block hash with the stored block hash.
        let contract = os_constants.os_contract_addresses.block_hash_contract_address();

        let stored_block_hash =
            state.get_storage_at(contract, StorageKey::from(proof_block_number))?;
        assert_ne!(stored_block_hash, Felt::ZERO, "Stored block hash is zero");

        let proof_block_hash = snos_proof_facts.block_hash.0;
        if stored_block_hash != proof_block_hash {
            return Err(TransactionPreValidationError::InvalidProofFacts(format!(
                "Block hash mismatch for block {proof_block_number}. Proof block hash: \
                 {proof_block_hash}, stored block hash: {stored_block_hash}."
            )));
        }

        Ok(())
    }

crates/blockifier/src/test_utils/initial_test_state.rs line 59 at r4 (raw file):

        );
    }
}

small suggestion.
consider exporting to a func pub fn block_hash_contract_address() -> ContractAddress that can be used also below in def of BLOCK_HASH_CONTRACT_ADDRESS

Suggestion:

/// Returns the contract address for the block hash contract used in tests.
pub fn block_hash_contract_address() -> ContractAddress {
    VersionedConstants::create_for_testing()
        .os_constants
        .os_contract_addresses
        .block_hash_contract_address()
}

/// Sets up block hash in the block hash contract from SNOS proof facts.
///
/// This is a test helper used to prepare state for proof-facts validation.
pub fn setup_block_hash_from_proof_facts(
    proof_facts: &ProofFacts,
    state_reader: &mut DictStateReader,
) {
    if let Ok(ProofFactsVariant::Snos(snos_proof_facts)) = proof_facts.try_into() {
        let contract = block_hash_contract_address();
        let storage_key = StorageKey::from(snos_proof_facts.block_number.0);
        state_reader.storage_view.insert((contract, storage_key), snos_proof_facts.block_hash.0);
    }
}

crates/apollo_gateway/src/gateway_test.rs line 293 at r4 (raw file):

            &mut mock_dependencies.state_reader_factory.state_reader.blockifier_state_reader,
        );
    }

Suggestion:

    // Setup state: fund account and store proof block hash if needed.
    let state_reader =
        &mut mock_dependencies.state_reader_factory.state_reader.blockifier_state_reader;
    let address = expected_internal_tx.contract_address();
    fund_account(
        &mock_dependencies.config.chain_info,
        address,
        VALID_ACCOUNT_BALANCE,
        state_reader,
    );

    if let RpcTransaction::Invoke(RpcInvokeTransaction::V3(invoke_tx)) = &input_tx {
        store_block_hash_from_proof_facts(&invoke_tx.proof_facts, state_reader);
    }

crates/starknet_os_flow_tests/src/test_manager.rs line 131 at r4 (raw file):

/// Stores block hash for invoke transactions with SNOS proof facts.
/// This is needed so the OS can verify block hash proofs during execution.
fn store_block_hash_for_proof_facts<S: UpdatableState>(tx: &BlockifierTransaction, state: &mut S) {

consider.
Also, please add a TODO regarding the discussed unwanted writes. (or perhaps add the todo above where the function is called or above calling add_proof_facts_block_hash_storage / it's defenitaion

Suggestion:

fn store_block_hash_from_transaction<S: UpdatableState>(tx: &BlockifierTransaction, state: &mut S)

crates/starknet_os_flow_tests/src/test_manager.rs line 294 at r4 (raw file):

    /// Adds the proof facts block hash storage mapping to the decompressed state diff.
    /// This is needed because the Blockifier writes block hash storage for proof facts
    /// verification, but the OS doesn't include this in its output.

Perhaps mention it's meant to be temporary until the tests set up support correct block number -> block hash mapping (for testing proof facts validations) ?

Code quote:

    /// Adds the proof facts block hash storage mapping to the decompressed state diff.
    /// This is needed because the Blockifier writes block hash storage for proof facts
    /// verification, but the OS doesn't include this in its output.

crates/starknet_os_flow_tests/src/test_manager.rs line 780 at r4 (raw file):

                .into_iter()
                .map(|flow_test_tx| {
                    store_block_hash_for_proof_facts(&flow_test_tx.tx, &mut state);

Add a documentation explaining that we affect the writes here, and that this shuold be fixed by setup that inclused block number-->block hash .

Code quote:

store_block_hash_for_proof_facts(&flow_test_tx.tx, &mut state);

meship-starkware

@meship-starkware made 4 comments and resolved 5 discussions.
Reviewable status: 5 of 7 files reviewed, 5 unresolved discussions (waiting on @avivg-starkware and @noaov1).

crates/blockifier/src/transaction/account_transaction.rs line 312 at r4 (raw file):

Previously, avivg-starkware wrote…

few small clean up suggestions + consider exporting the block hash validation to another func if we will add other validations

I will split it into different functions in a separate PR so we can move with the tag
Done.

crates/starknet_os_flow_tests/src/tests.rs line 1190 at r4 (raw file):

Previously, avivg-starkware wrote…

I think this info is crucial in some way or another, especially around the temporary fixes that you plan to remove. It doesn't need to be altogether here, just wanted to make sure:
1- I understand properly
2- Others could understand once the fix is merged
3- We don't end up leaving those behind by accident in further refactorings.

Done.

crates/apollo_gateway/src/gateway_test.rs line 293 at r4 (raw file):

            &mut mock_dependencies.state_reader_factory.state_reader.blockifier_state_reader,
        );
    }

Nice, I liked that

crates/starknet_os_flow_tests/src/tests.rs line 1191 at r4 (raw file):

    // The Blockifier writes this for proof facts verification, but the OS doesn't include it.
    // TODO(Meshi): write the mapping for the os validation as well.
    if let Ok(ProofFactsVariant::Snos(snos_proof_facts)) = ProofFactsVariant::try_from(&proof_facts)

This does not compile as it is going to be removed in the next PR I don't see the importance

Yoni-Starkware

@Yoni-Starkware made 3 comments.
Reviewable status: 2 of 8 files reviewed, 8 unresolved discussions (waiting on @avivg-starkware, @meship-starkware, and @noaov1).

crates/starknet_os_flow_tests/src/test_manager.rs line 122 at r5 (raw file):

/// The block hash contract address, cached to avoid repeated VersionedConstants creation.
static BLOCK_HASH_CONTRACT_ADDRESS: LazyLock<ContractAddress> =

No need, VersionedConstants::latest_constants() is already cached

Code quote:

/// The block hash contract address, cached to avoid repeated VersionedConstants creation.
static BLOCK_HASH_CONTRACT_ADDRESS: LazyLock<ContractAddress> =

crates/starknet_os_flow_tests/src/test_manager.rs line 149 at r5 (raw file):

        }
    }
}

Instead, try the opposite: add this phase to the creation of the initial state.
E.g., if the block number of the initial state is 2000, please populate the block hash contract with deterministic values (deterministic in the block number) for blocks 1950-1990

Use these determisitic block hashes in your proof facts tx

Code quote:

/// Stores block hash for invoke transactions with SNOS proof facts.
/// This is needed so the OS can verify block hash proofs during execution.
// TODO(Meshi): change it so the block number to block hash mapping will be a part of the state and
// not written during the transaction execution.
fn store_block_hash_from_transaction<S: UpdatableState>(tx: &BlockifierTransaction, state: &mut S) {
    if let BlockifierTransaction::Account(account_tx) = tx {
        if let AccountTransaction::Invoke(invoke_tx) = &account_tx.tx {
            if let Ok(ProofFactsVariant::Snos(snos_proof_facts)) =
                ProofFactsVariant::try_from(&invoke_tx.proof_facts())
            {
                let storage_writes = StateMaps {
                    storage: HashMap::from([(
                        (
                            *BLOCK_HASH_CONTRACT_ADDRESS,
                            StorageKey::from(snos_proof_facts.block_number.0),
                        ),
                        snos_proof_facts.block_hash.0,
                    )]),
                    ..Default::default()
                };
                state.apply_writes(&storage_writes, &HashMap::new());
            }
        }
    }
}

crates/starknet_os_flow_tests/src/test_manager.rs line 307 at r5 (raw file):

            .or_default()
            .insert(storage_key, storage_value);
    }

This should not be needed at all. The proof facts should refer to blocks from the past (initial state)

Yoni-Starkware

@Yoni-Starkware made 1 comment.
Reviewable status: 2 of 8 files reviewed, 9 unresolved discussions (waiting on @avivg-starkware, @meship-starkware, and @noaov1).

crates/starknet_os_flow_tests/src/tests.rs line 1191 at r5 (raw file):

            snos_proof_facts.block_hash,
        );
    }

No need as well

Code quote:

    // Reconcile proof facts block-hash storage between Blockifier and OS outputs.
    //
    // Context: To allow testing transactions with proof_facts validations, the Blockifier writes a
    // storage entry: (block_hash_contract, proof_block_number) -> proof_block_hash.
    //
    // However, the OS doesn't emit this storage write in its output (the proof facts verification
    // was not yet added to the OS, and thus having non non-empty (block_number -> block_hash)
    // mapping is not yet required for the tests to pass).
    //
    // This causes a mismatch: Blockifier's state diff includes the write, but OS output doesn't.
    // We manually add it here so the test comparison passes.
    //
    // TODO(Meshi): Move block hash storage setup to not affect the writes, then remove this fixup.

    if let Ok(ProofFactsVariant::Snos(snos_proof_facts)) = ProofFactsVariant::try_from(&proof_facts)
    {
        test_output.add_proof_facts_block_hash_storage(
            snos_proof_facts.block_number,
            snos_proof_facts.block_hash,
        );
    }

Yoni-Starkware

@Yoni-Starkware reviewed 6 files and all commit messages, made 3 comments, and resolved 5 discussions.
Reviewable status: 9 of 15 files reviewed, 4 unresolved discussions (waiting on @avivg-starkware, @meship-starkware, and @noaov1).

crates/blockifier/src/transaction/account_transaction.rs line 252 at r11 (raw file):

    }

    fn validate_proof_facts(

Please add negative unit tests to all checks;
Create a dummy tx and call this function directly and expect to get the errors

crates/starknet_os_flow_tests/src/initial_state.rs line 191 at r11 (raw file):

    for (key, value) in &block_hash_storage_updates {
        block_hash_state_maps.storage.insert((block_hash_contract_address(), *key), *value);
    }

Maybe return a StateMaps object from generate_block_hash_storage_updates?
Then, extend the state dif with it and apply once

Code quote:

    // Generates block hash storage updates for historical blocks.
    let block_hash_storage_updates: HashMap<_, _> = generate_block_hash_storage_updates().collect();

    // Add historical block hashes.
    let mut block_hash_state_maps = StateMaps::default();
    for (key, value) in &block_hash_storage_updates {
        block_hash_state_maps.storage.insert((block_hash_contract_address(), *key), *value);
    }

crates/starknet_api/src/test_utils.rs line 62 at r11 (raw file):

// E.g., for block 2001: blocks 1950 to 1990.
pub const BLOCK_HASH_HISTORY_START: u64 = 1950;
pub const BLOCK_HASH_HISTORY_END: u64 = 1990;

Instead, move them inside the func and rely on the const CURRENT_BLOCK_NUMBER

meship-starkware

@meship-starkware made 3 comments.
Reviewable status: 8 of 15 files reviewed, 4 unresolved discussions (waiting on @avivg-starkware, @noaov1, and @Yoni-Starkware).

crates/blockifier/src/transaction/account_transaction.rs line 252 at r11 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Please add negative unit tests to all checks;
Create a dummy tx and call this function directly and expect to get the errors

I have done it in this PR
I will have to update it according to the changes we made here.

crates/starknet_api/src/test_utils.rs line 62 at r11 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Instead, move them inside the func and rely on the const CURRENT_BLOCK_NUMBER

Done.

crates/starknet_os_flow_tests/src/initial_state.rs line 191 at r11 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Maybe return a StateMaps object from generate_block_hash_storage_updates?
Then, extend the state dif with it and apply once

Done.

noaov1

@noaov1 reviewed 15 files and all commit messages, and made 4 comments.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware).

crates/blockifier/src/transaction/account_transaction.rs line 287 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Done.

Oh, avoiding panicking is indeed a better idea (for some reason I assumed that the virtual os will force that the block is not too old, but it does not make much sense).
Added to Monday.

a discussion (no related file):
Not relevant now but for future reference-
I think you should have seperated it into smaller PRs:

tests setup: Modify the json files, add the relevant test utils.
Add the block hash verification logic (you can add relevant tests)
This PR indeed contains too many changes :)

crates/starknet_os_flow_tests/src/tests.rs line 1066 at r12 (raw file):

    let test_contract = FeatureContract::TestContract(CairoVersion::Cairo1(RunnableCairo1::Casm));
    let test_class_hash = get_class_hash_of_feature_contract(test_contract);
    // Create proof facts before creating the test builder, so it can set up the initial state.

Where can I see that if affect the initial state?

Code quote:

    // Create proof facts before creating the test builder, so it can set up the initial state.

crates/starknet_os_flow_tests/src/initial_state.rs line 182 at r12 (raw file):

    final_state
        .state
        .apply_writes(&block_hash_state_maps, &final_state.class_hash_to_class.borrow());

Suggestion:

    
    // Update the state reader with the state diff.
    let state_diff = final_state.to_state_diff().unwrap().state_maps;
    // Sanity check to verify the STRK_FEE_TOKEN_ADDRESS constant.
    assert_eq!(
        state_diff.class_hashes[&STRK_FEE_TOKEN_ADDRESS],
        FeatureContract::ERC20(CairoVersion::Cairo1(RunnableCairo1::Casm))
            .get_sierra()
            .calculate_class_hash()
    );
    // Add historical block hashes.
    let block_hash_state_diff = generate_block_hash_storage_updates();
    state_diff.extend(block_hash_state_diff);
    
    final_state.state.apply_writes(&state_diff, &final_state.class_hash_to_class.borrow());

noaov1

@noaov1 made 1 comment.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware).

crates/blockifier/src/transaction/account_transaction.rs line 300 at r12 (raw file):

            .get_storage_at(block_hash_contract_address, StorageKey::from(proof_block_number))?;

        if stored_block_hash == Felt::ZERO {

Better to check that the given block hash is not zero, no?

Code quote:

        if stored_block_hash == Felt::ZERO {

Yoni-Starkware

@Yoni-Starkware reviewed 1 file and resolved 2 discussions.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @meship-starkware).

meship-starkware

@meship-starkware made 3 comments and resolved 1 discussion.
Reviewable status: 10 of 15 files reviewed, 5 unresolved discussions (waiting on @noaov1 and @Yoni-Starkware).

crates/blockifier/src/transaction/account_transaction.rs line 287 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Oh, avoiding panicking is indeed a better idea (for some reason I assumed that the virtual os will force that the block is not too old, but it does not make much sense).
Added to Monday.

Done.

crates/blockifier/src/transaction/account_transaction.rs line 300 at r12 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Better to check that the given block hash is not zero, no?

Done.

crates/starknet_os_flow_tests/src/tests.rs line 1066 at r12 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Where can I see that if affect the initial state?

This was relevant for the earlier solution, nice catch!

meship-starkware

@meship-starkware made 1 comment.
Reviewable status: 10 of 15 files reviewed, 5 unresolved discussions (waiting on @noaov1 and @Yoni-Starkware).

a discussion (no related file):

Previously, noaov1 (Noa Oved) wrote…

Not relevant now but for future reference-
I think you should have seperated it into smaller PRs:

tests setup: Modify the json files, add the relevant test utils.

Add the block hash verification logic (you can add relevant tests)
This PR indeed contains too many changes :)

Done.

noaov1

@noaov1 reviewed 5 files and all commit messages, made 3 comments, and resolved 4 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware).

crates/blockifier/src/transaction/account_transaction.rs line 300 at r12 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Done.

(separate PR)- Reconsider the order of the checks

crates/starknet_os_flow_tests/src/initial_state.rs line 186 at r13 (raw file):

    final_state
        .state
        .apply_writes(&block_hash_state_maps, &final_state.class_hash_to_class.borrow());

crates/starknet_api/src/test_utils.rs line 58 at r13 (raw file):

// Range of historical blocks to populate in the block hash contract.
pub const BLOCK_HASH_HISTORY_RANGE: u64 = 51;

Why?

Code quote:

51;

noaov1

@noaov1 made 1 comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware).

crates/blockifier/src/transaction/account_transaction.rs line 300 at r12 (raw file):

Previously, noaov1 (Noa Oved) wrote…

(separate PR)- Reconsider the order of the checks

.

meship-starkware

@meship-starkware made 3 comments.
Reviewable status: 14 of 15 files reviewed, 4 unresolved discussions (waiting on @noaov1 and @Yoni-Starkware).

crates/blockifier/src/transaction/account_transaction.rs line 300 at r12 (raw file):

Previously, noaov1 (Noa Oved) wrote…

.

I have a PR that separates the checks into two different functions, one for the block number and one for the block hash

crates/starknet_api/src/test_utils.rs line 58 at r13 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Why?

becuse we have block 2001, and the range is 1950-1990, so the range is 50 blocks, but the difference is 51.

crates/starknet_os_flow_tests/src/initial_state.rs line 186 at r13 (raw file):

    final_state
        .state
        .apply_writes(&block_hash_state_maps, &final_state.class_hash_to_class.borrow());

next pr

Yoni-Starkware

@Yoni-Starkware reviewed 1 file and all commit messages, and resolved 1 discussion.
Reviewable status: 14 of 15 files reviewed, 3 unresolved discussions (waiting on @noaov1).

noaov1

@noaov1 reviewed 3 files and all commit messages, and resolved 2 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @meship-starkware).

Yoni-Starkware

@Yoni-Starkware reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @meship-starkware).

…k hash

avivg-starkware

@avivg-starkware reviewed 10 files and all commit messages, and made 6 comments.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @meship-starkware and @noaov1).

crates/blockifier/src/transaction/account_transaction.rs line 280 at r5 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Yes

So we don't?
only block muber < current - STORED_BLOCK_HASH_BUFFER is stored?

crates/starknet_api/src/test_utils.rs line 58 at r13 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

becuse we have block 2001, and the range is 1950-1990, so the range is 50 blocks, but the difference is 51.

Perhaps add a doc where this number is from? where do we see we have block 2001, and the range is 1950-1990 ?

crates/starknet_os_flow_tests/src/tests.rs line 1073 at r17 (raw file):

    .await;

    let chain_id = &test_builder.chain_id();

intentional?

Suggestion:

    .await;
    let chain_id = &test_builder.chain_id();

crates/starknet_api/src/test_utils.rs line 391 at r17 (raw file):

                && block_number >= felt!(block_hash_history_start),
            "Block number is out of range"
        );

Consider clarifying "out of range"

Code quote:

        assert!(
            block_number < felt!(CURRENT_BLOCK_NUMBER)
                && block_number >= felt!(block_hash_history_start),
            "Block number is out of range"
        );

crates/blockifier/src/transaction/account_transaction.rs line 281 at r17 (raw file):

                TransactionPreValidationError::InvalidProofFacts(format!(
                    "The current block number {current_block_number} is too recent to have a \
                     stored block hash."

Was that your intention?

Suggestion:

                    "The current block number {current_block_number} is below the required \
                    block-hash retention buffer."

crates/blockifier/src/test_utils.rs line 436 at r17 (raw file):

pub fn generate_block_hash_storage_updates() -> StateMaps {
    let block_hash_history_start = CURRENT_BLOCK_NUMBER - BLOCK_HASH_HISTORY_RANGE;
    let block_hash_history_end = CURRENT_BLOCK_NUMBER - constants::STORED_BLOCK_HASH_BUFFER - 1;

Consider giving context that this reflects the logic on chain

Suggestion:

// On-chain, block hashes are guaranteed only for blocks up to: current - STORED_BLOCK_HASH_BUFFER
let block_hash_history_end = CURRENT_BLOCK_NUMBER - constants::STORED_BLOCK_HASH_BUFFER - 1;

meship-starkware marked this pull request as ready for review January 1, 2026 10:03

meship-starkware requested a review from noaov1 January 1, 2026 10:04

noaov1 approved these changes Jan 2, 2026

View reviewed changes

noaov1 requested changes Jan 2, 2026

View reviewed changes

meship-starkware commented Jan 4, 2026

View reviewed changes

meship-starkware force-pushed the meship/verify_block_number_and_hash branch from f427327 to 6664f81 Compare January 4, 2026 14:45

meship-starkware commented Jan 4, 2026

View reviewed changes

meship-starkware force-pushed the meship/verify_block_number_and_hash branch 3 times, most recently from 6f607ba to 5a200cb Compare January 7, 2026 07:14

meship-starkware commented Jan 7, 2026

View reviewed changes

meship-starkware force-pushed the meship/verify_block_number_and_hash branch from 5a200cb to 24a8629 Compare January 7, 2026 07:22

meship-starkware mentioned this pull request Jan 7, 2026

blockifier: test proof facts verifications #11473

Open

meship-starkware requested a review from avivg-starkware January 7, 2026 07:23

meship-starkware commented Jan 7, 2026

View reviewed changes

This was referenced Jan 7, 2026

blockifier: add a client side proving test state #11476

Open

starknet_os_flow_tests: add the tests proof facts block hash to number to the state initial reads #11485

Closed

avivg-starkware requested changes Jan 7, 2026

View reviewed changes

meship-starkware mentioned this pull request Jan 7, 2026

starknet_os: add proof fact block number to block hash validations #11506

Open

meship-starkware force-pushed the meship/verify_block_number_and_hash branch from 24a8629 to e5a95cf Compare January 8, 2026 09:18

meship-starkware commented Jan 8, 2026

View reviewed changes

meship-starkware force-pushed the meship/verify_block_number_and_hash branch from e5a95cf to fbca6e8 Compare January 8, 2026 10:15

Yoni-Starkware requested changes Jan 8, 2026

View reviewed changes

Yoni-Starkware requested changes Jan 12, 2026

View reviewed changes

meship-starkware force-pushed the meship/verify_block_number_and_hash branch from 4574c95 to df250a4 Compare January 12, 2026 13:32

meship-starkware commented Jan 12, 2026

View reviewed changes

noaov1 requested a review from Yoni-Starkware January 13, 2026 11:06

noaov1 requested changes Jan 13, 2026

View reviewed changes

Yoni-Starkware reviewed Jan 13, 2026

View reviewed changes

meship-starkware force-pushed the meship/verify_block_number_and_hash branch from df250a4 to cd98152 Compare January 13, 2026 15:05

meship-starkware commented Jan 13, 2026

View reviewed changes

noaov1 requested a review from Yoni-Starkware January 13, 2026 16:09

noaov1 approved these changes Jan 13, 2026

View reviewed changes

noaov1 requested changes Jan 13, 2026

View reviewed changes

meship-starkware force-pushed the meship/verify_block_number_and_hash branch from cd98152 to c5a5f14 Compare January 14, 2026 07:48

meship-starkware mentioned this pull request Jan 14, 2026

starknet_os: add proof virtual os output header #11704

Open

meship-starkware commented Jan 14, 2026

View reviewed changes

meship-starkware force-pushed the meship/verify_block_number_and_hash branch 2 times, most recently from e685247 to 36d3421 Compare January 14, 2026 09:39

Yoni-Starkware approved these changes Jan 14, 2026

View reviewed changes

meship-starkware force-pushed the meship/verify_block_number_and_hash branch from 36d3421 to d73288a Compare January 14, 2026 13:11

noaov1 approved these changes Jan 14, 2026

View reviewed changes

Yoni-Starkware reviewed Jan 15, 2026

View reviewed changes

meship-starkware force-pushed the meship/verify_block_number_and_hash branch 5 times, most recently from 966f4d2 to 07a97a2 Compare January 15, 2026 12:43

blockifier: verify that the proof facts block number matches the bloc…

abacec2

…k hash

meship-starkware force-pushed the meship/verify_block_number_and_hash branch from 07a97a2 to abacec2 Compare January 15, 2026 14:21

avivg-starkware reviewed Jan 15, 2026

View reviewed changes

blockifier: verify that the proof facts block number matches the block hash #11345

Are you sure you want to change the base?

blockifier: verify that the proof facts block number matches the block hash #11345

Uh oh!

Conversation

meship-starkware commented Jan 1, 2026

Uh oh!

reviewable-StarkWare commented Jan 1, 2026

Uh oh!

github-actions bot commented Jan 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meship-starkware commented Jan 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

noaov1 left a comment

Choose a reason for hiding this comment

Uh oh!

noaov1 left a comment

Choose a reason for hiding this comment

Uh oh!

noaov1 left a comment

Choose a reason for hiding this comment

Uh oh!

meship-starkware left a comment

Choose a reason for hiding this comment

Uh oh!

meship-starkware left a comment

Choose a reason for hiding this comment

Uh oh!

meship-starkware left a comment

Choose a reason for hiding this comment

Uh oh!

meship-starkware left a comment

Choose a reason for hiding this comment

Uh oh!

meship-starkware left a comment

Choose a reason for hiding this comment

Uh oh!

meship-starkware left a comment

Choose a reason for hiding this comment

Uh oh!

avivg-starkware left a comment

Choose a reason for hiding this comment

Uh oh!

meship-starkware left a comment

Choose a reason for hiding this comment

Uh oh!

Yoni-Starkware left a comment

Choose a reason for hiding this comment

Uh oh!

Yoni-Starkware left a comment

Choose a reason for hiding this comment

Uh oh!

Yoni-Starkware left a comment

Choose a reason for hiding this comment

Uh oh!

meship-starkware left a comment

Choose a reason for hiding this comment

Uh oh!

noaov1 left a comment

Choose a reason for hiding this comment

Uh oh!

noaov1 left a comment

Choose a reason for hiding this comment

Uh oh!

Yoni-Starkware left a comment

Choose a reason for hiding this comment

Uh oh!

meship-starkware left a comment

Choose a reason for hiding this comment

Uh oh!

meship-starkware left a comment

Choose a reason for hiding this comment

Uh oh!

noaov1 left a comment

Choose a reason for hiding this comment

Uh oh!

noaov1 left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 1, 2026 •

edited

Loading

meship-starkware commented Jan 1, 2026 •

edited

Loading