feat(execution): round block_number and timestamp for the execution info syscall in validate_mode

[TzahiTaub]

This change is 

[ArniStarkware]

crates/blockifier/src/execution/syscalls/hint_processor.rs line 146 at r1 (raw file):

pub const VALIDATE_BLOCK_NUMBER_FLOORING: u64 = 100;
// "Ceiling factor for timestamp in validate mode";
pub const VALIDATE_TIMESTAMP_ROUNDING_CEILING: u64 = 3600;

These constants are not related to Error codes.

Write a regular doc string, and add a TODO to move to the correct constants file.

Code quote:

// "Flooring factor for block number in validate mode";
pub const VALIDATE_BLOCK_NUMBER_FLOORING: u64 = 100;
// "Ceiling factor for timestamp in validate mode";
pub const VALIDATE_TIMESTAMP_ROUNDING_CEILING: u64 = 3600;

[TzahiTaub]

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware)

---

crates/blockifier/src/execution/syscalls/hint_processor.rs line 146 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

These constants are not related to Error codes.

Write a regular doc string, and add a TODO to move to the correct constants file.

Done.

[ArniStarkware]

Reviewed 3 of 3 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @TzahiTaub)

[ArniStarkware]

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @TzahiTaub)

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8fe05a4) 69.04% compared to head (634bdd5) 70.41%.
Report is 9 commits behind head on main-v0.13.1.

Additional details and impacted files

@@               Coverage Diff                @@
##           main-v0.13.1    #1467      +/-   ##
================================================
+ Coverage         69.04%   70.41%   +1.37%     
================================================
  Files                58       59       +1     
  Lines              7949     8873     +924     
  Branches           7949     8873     +924     
================================================
+ Hits               5488     6248     +760     
- Misses             2005     2212     +207     
+ Partials            456      413      -43     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ArniStarkware]

Reviewed 2 of 2 files at r4.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @TzahiTaub)

[ArniStarkware]

crates/blockifier/src/test_utils/struct_impls.rs line 125 at r5 (raw file):

            use_kzg_da: false,
        }
    }

To improve maintainability (Not change all other irrelevant instances of create_for_testing, I would create a function that accepts the validate mode flag.

Suggestion:

    pub fn create_for_testing() -> Self {
        Self::create_for_validation_mode_testing(false)
    }
    
    pub fn create_for_validation_mode_testing(is_validate: bool) -> Self {
        let block_number = if is_validate {
            (CURRENT_BLOCK_NUMBER / VALIDATE_BLOCK_NUMBER_ROUNDING) * VALIDATE_BLOCK_NUMBER_ROUNDING
        } else {
            CURRENT_BLOCK_NUMBER
        };
        let timestamp = if is_validate {
            (CURRENT_BLOCK_TIMESTAMP / VALIDATE_TIMESTAMP_ROUNDING) * VALIDATE_TIMESTAMP_ROUNDING
        } else {
            CURRENT_BLOCK_TIMESTAMP
        };
        Self {
            block_number: BlockNumber(block_number),
            block_timestamp: BlockTimestamp(timestamp),
            sequencer_address: contract_address!(TEST_SEQUENCER_ADDRESS),
            gas_prices: GasPrices {
                eth_l1_gas_price: DEFAULT_ETH_L1_GAS_PRICE.try_into().unwrap(),
                strk_l1_gas_price: DEFAULT_STRK_L1_GAS_PRICE.try_into().unwrap(),
                eth_l1_data_gas_price: DEFAULT_ETH_L1_DATA_GAS_PRICE.try_into().unwrap(),
                strk_l1_data_gas_price: DEFAULT_STRK_L1_DATA_GAS_PRICE.try_into().unwrap(),
            },
            use_kzg_da: false,
        }
    }

[elintul]

Reviewable status: 3 of 8 files reviewed, 2 unresolved discussions (waiting on @TzahiTaub)

a discussion (no related file):
Python PR?

---

[ArniStarkware]

Reviewable status: 3 of 8 files reviewed, 3 unresolved discussions (waiting on @TzahiTaub)

---

crates/blockifier/src/execution/deprecated_syscalls/mod.rs line 409 at r5 (raw file):

        ),
        false => block_number,
    };

When a new execution mode is added, we want the compiler to scream.

Suggestion:

    let block_number = match syscall_handler.execution_mode() {
        ExecutionMode::Validate => BlockNumber(
            (block_number.0 / VALIDATE_BLOCK_NUMBER_ROUNDING) * VALIDATE_BLOCK_NUMBER_ROUNDING,
        ),
        ExecutionMode::Execute => block_number,
    };

[ArniStarkware]

Reviewable status: 3 of 8 files reviewed, 4 unresolved discussions (waiting on @TzahiTaub)

---

crates/blockifier/src/execution/deprecated_syscalls/mod.rs line 435 at r5 (raw file):

) -> DeprecatedSyscallResult<GetBlockTimestampResponse> {
    let block_timestamp = syscall_handler.get_block_info().block_timestamp;
    let block_timestamp = match syscall_handler.is_validate_mode() {

same as above.

Suggestion:

    let block_timestamp = match syscall_handler.execution_mode() {

[ArniStarkware]

crates/blockifier/src/test_utils/struct_impls.rs line 102 at r5 (raw file):

impl BlockInfo {
    pub fn create_for_testing(is_validate: bool) -> Self {

Another, non-conflicting comment about create_for_testing:
use the execution mode as the parameter that affects the sequencer address.

Suggestion:

pub fn create_for_testing(execution_mode: ExecutionMode) -> Self {

[ArniStarkware]

Reviewable status: 3 of 8 files reviewed, 6 unresolved discussions (waiting on @TzahiTaub)

---

crates/blockifier/src/test_utils/struct_impls.rs line 116 at r5 (raw file):

            block_number: BlockNumber(block_number),
            block_timestamp: BlockTimestamp(timestamp),
            sequencer_address: contract_address!(TEST_SEQUENCER_ADDRESS),

The sequencer address should be 0 on execution mode Validate.

Code quote:

contract_address!(TEST_SEQUENCER_ADDRESS),

[TzahiTaub]

Reviewable status: 3 of 8 files reviewed, 6 unresolved discussions (waiting on @ArniStarkware, @elintul, and @TzahiTaub)

a discussion (no related file):

Previously, elintul (Elin) wrote…

Python PR?

https://reviewable.io/reviews/starkware-industries/starkware/33887#-
(and also this https://reviewable.io/reviews/starkware-industries/starkware/33896#- though it's for deprecated code)

---

---

crates/blockifier/src/execution/deprecated_syscalls/mod.rs line 409 at r5 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

When a new execution mode is added, we want the compiler to scream.

Done.

---

crates/blockifier/src/execution/deprecated_syscalls/mod.rs line 435 at r5 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

same as above.

Done.

---

crates/blockifier/src/test_utils/struct_impls.rs line 102 at r5 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Another, non-conflicting comment about create_for_testing:
use the execution mode as the parameter that affects the sequencer address.

Done.

---

crates/blockifier/src/test_utils/struct_impls.rs line 116 at r5 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

The sequencer address should be 0 on execution mode Validate.

Um, OK. Done.

---

crates/blockifier/src/test_utils/struct_impls.rs line 125 at r5 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

To improve maintainability (Not change all other irrelevant instances of create_for_testing, I would create a function that accepts the validate mode flag.

I think this is OK.

[giladchase]

Reviewable status: 3 of 8 files reviewed, 3 unresolved discussions (waiting on @elintul and @TzahiTaub)

---

crates/blockifier/src/execution/syscalls/hint_processor.rs line 24 at r6 (raw file):

use starknet_api::state::StorageKey;
use starknet_api::transaction::{Calldata, Resource};
use starknet_api::{stark_felt, StarknetApiError};

This macro is for tests only, it uses unwrap injudiciously (see its definition).
It's unfortunantely available in non-test modules due to implementation constraints.

Use regular constructors for StarkFelt plz

Code quote:

:{stark_felt, 

---

crates/blockifier/src/execution/syscalls/hint_processor.rs line 494 at r6 (raw file):

            let block_number_u64: u64 = block_number.try_into()?;
            let rounded_block_number = (block_number_u64 / VALIDATE_BLOCK_NUMBER_ROUNDING)
                * VALIDATE_BLOCK_NUMBER_ROUNDING;

Ditto for the other one.

Suggestion:

            // Round down to the nearest multiple of VALIDATE_BLOCK_NUMBER_ROUNDING.
            let rounded_block_number = (block_number_u64 / VALIDATE_BLOCK_NUMBER_ROUNDING)
                * VALIDATE_BLOCK_NUMBER_ROUNDING;

[ArniStarkware]

crates/blockifier/src/test_utils/struct_impls.rs line 78 at r6 (raw file):

            ),
            tx_info,
        };

Do we want to change the block context, given that we are in validate mode?

Code quote:

        let tx_context = TransactionContext {
            block_context: BlockContext::create_for_validation_mode_testing(
                ExecutionMode::Validate,
            ),
            tx_info,
        };

[liorgold2]

Reviewable status: 3 of 8 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware and @elintul)

---

crates/blockifier/src/test_utils/struct_impls.rs line 78 at r6 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Do we want to change the block context, given that we are in validate mode?

IIUC, tx_context shouldn't depend on validate mode. It's for the entire tx.

[liorgold2]

Reviewed 2 of 4 files at r6.
Reviewable status: 7 of 8 files reviewed, 11 unresolved discussions (waiting on @elintul, @giladchase, @TzahiTaub, and @Yoni-Starkware)

[liorgold2]

Reviewable status: 7 of 8 files reviewed, 11 unresolved discussions (waiting on @ArniStarkware, @elintul, @giladchase, @TzahiTaub, and @Yoni-Starkware)

---

crates/blockifier/src/test_utils/struct_impls.rs line 78 at r6 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

The confusing thing is this: there are two (sets of) related numbers.

The block number (and block timestamp) and the returned block number (timestamp) by the syscall in validate mode.
The block number should not be changed. We are only affecting the returned value.

(The block number itself is independent of validate mode. Only the block number that's returned by the syscall (which is not the real block number) depends on validate mode.)

[TzahiTaub]

Reviewable status: 7 of 8 files reviewed, 8 unresolved discussions (waiting on @ArniStarkware, @elintul, @giladchase, and @Yoni-Starkware)

---

crates/blockifier/src/execution/syscalls/hint_processor.rs line 492 at r7 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Why not use block_info.block_number.0 and wrapping with StarkFelt only in the else clause?

Done

[noaov1]

Reviewed 2 of 4 files at r7, 2 of 2 files at r8, all commit messages.
Reviewable status: 7 of 8 files reviewed, 8 unresolved discussions (waiting on @ArniStarkware, @elintul, @giladchase, @TzahiTaub, and @Yoni-Starkware)

[Yoni-Starkware]

Reviewed 1 of 2 files at r8.
Reviewable status: 7 of 8 files reviewed, 6 unresolved discussions (waiting on @ArniStarkware, @elintul, @giladchase, and @TzahiTaub)

[giladchase]

Reviewable status: 7 of 8 files reviewed, 4 unresolved discussions (waiting on @ArniStarkware, @elintul, @liorgold2, and @TzahiTaub)

---

crates/blockifier/src/test_utils/struct_impls.rs line 111 at r6 (raw file):

Previously, liorgold2 wrote…

I don't think this should be there. @giladchase ?

Right.
I'm with tzahi now talking about this.

[TzahiTaub]

Reviewable status: 7 of 8 files reviewed, 4 unresolved discussions (waiting on @ArniStarkware, @elintul, and @liorgold2)

---

crates/blockifier/src/test_utils/struct_impls.rs line 78 at r6 (raw file):

Previously, liorgold2 wrote…

(The block number itself is independent of validate mode. Only the block number that's returned by the syscall (which is not the real block number) depends on validate mode.)

Done.

---

crates/blockifier/src/test_utils/struct_impls.rs line 111 at r6 (raw file):

Previously, giladchase wrote…

Right.
I'm with tzahi now talking about this.

Done.

[TzahiTaub]

Dismissed @liorgold2 from a discussion.
Reviewable status: 7 of 8 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware and @elintul)

[TzahiTaub]

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArniStarkware and @elintul)

[TzahiTaub]

Dismissed @ArniStarkware from a discussion.
Reviewable status: 7 of 8 files reviewed, 2 unresolved discussions (waiting on @elintul, @liorgold2, and @noaov1)

[TzahiTaub]

Reviewed 1 of 1 files at r10.
Reviewable status: all files reviewed (commit messages unreviewed), 2 unresolved discussions (waiting on @elintul)

[TzahiTaub]

Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @elintul)

[elintul]

Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @TzahiTaub)

[TzahiTaub]

Reviewed 2 of 4 files at r6.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @TzahiTaub)

[noaov1]

Reviewed 1 of 1 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @TzahiTaub)

---

crates/blockifier/src/execution/syscalls/syscalls_test.rs line 294 at r10 (raw file):

            // Rounded timestamp.
            stark_felt!(CURRENT_BLOCK_TIMESTAMP_FOR_VALIDATE),
            StarkFelt::ZERO,

You can use the sequencer address :) It is set to 0 (or even delete this argument and use the constants directly ) (non- blocking)

Code quote:

            StarkFelt::ZERO,

[ArniStarkware]

Reviewed 3 of 4 files at r6, 1 of 4 files at r7, 2 of 2 files at r8, 1 of 1 files at r9.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @TzahiTaub)

[TzahiTaub]

Dismissed @liorgold2 from a discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @TzahiTaub)

[TzahiTaub]

Dismissed @Yoni-Starkware from a discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @TzahiTaub)

Reviewable bug

[ArniStarkware]

Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @TzahiTaub)