test(blockifier): global validate and execute sierra_gas

[aner-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[github-actions]

Artifacts upload workflows:

• Started at 2024-12-09T15:32:42Z
• Started at 2024-12-09T15:35:08Z
• Started at 2024-12-09T15:35:42Z
• Started at 2024-12-09T15:36:03Z
• Started at 2024-12-10T11:36:10Z
• Started at 2024-12-10T13:45:36Z
• Started at 2024-12-10T14:02:58Z
• Started at 2024-12-10T14:04:09Z
• Started at 2024-12-10T14:08:29Z
• Started at 2024-12-10T14:15:03Z
• Started at 2024-12-10T14:57:47Z
• Started at 2024-12-10T15:11:34Z
• Started at 2024-12-15T05:15:09Z
• Started at 2024-12-15T05:17:06Z
• Started at 2024-12-15T05:31:04Z
• Started at 2024-12-15T05:45:09Z
• Started at 2024-12-15T19:54:03Z
• Started at 2024-12-16T09:23:17Z
• Started at 2024-12-16T13:06:39Z
• Started at 2024-12-16T15:31:48Z
• Started at 2024-12-16T15:37:30Z
• Started at 2024-12-17T09:11:37Z

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.28%. Comparing base (bda947b) to head (603bea0).

Additional details and impacted files

@@                       Coverage Diff                        @@
##           aner/global_max_sierra_gas_2    #2596      +/-   ##
================================================================
- Coverage                         75.85%   68.28%   -7.57%     
================================================================
  Files                               109      105       -4     
  Lines                             14440    14356      -84     
  Branches                          14440    14356      -84     
================================================================
- Hits                              10953     9803    -1150     
- Misses                             3004     4124    +1120     
+ Partials                            483      429      -54     

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

[dorimedini-starkware]

Reviewed 4 of 5 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @aner-starkware)

---

crates/blockifier/src/transaction/test_utils.rs line 98 at r2 (raw file):

    l2_gas_amount: GasAmount,
    l1_data_gas_amount: GasAmount,
) -> ValidResourceBounds {

consider making this input a GasVector

Suggestion:

pub fn create_gas_amount_bounds_with_default_price(
    GasVector { l1_gas, l2_gas, l1_data_gas }
) -> ValidResourceBounds {

---

crates/blockifier/src/transaction/test_utils.rs line 107 at r2 (raw file):

        DEFAULT_STRK_L1_DATA_GAS_PRICE.into(),
    )
}

cool!
I see three places where this function can be dropped in; can you search for usages of create_all_resource_bounds and use this util where applicable?

Code quote:

pub fn create_gas_amount_bounds_with_default_price(
    l1_gas_amount: GasAmount,
    l2_gas_amount: GasAmount,
    l1_data_gas_amount: GasAmount,
) -> ValidResourceBounds {
    create_all_resource_bounds(
        l1_gas_amount,
        DEFAULT_STRK_L1_GAS_PRICE.into(),
        l2_gas_amount,
        DEFAULT_STRK_L2_GAS_PRICE.into(),
        l1_data_gas_amount,
        DEFAULT_STRK_L1_DATA_GAS_PRICE.into(),
    )
}

---

crates/blockifier/src/transaction/test_utils.rs line 117 at r2 (raw file):

pub fn versioned_constants(block_context: BlockContext) -> VersionedConstants {
    block_context.versioned_constants().clone()
}

can you use references...? cloning the VC can be slow if it really copies data,
VC instances should be 'static refs so no cloning should be necessary unless the test edits the VC
non-blocking

Code quote:

pub fn versioned_constants(block_context: BlockContext) -> VersionedConstants {
    block_context.versioned_constants().clone()
}

---

crates/blockifier/feature_contracts/cairo1/test_contract.cairo line 654 at r2 (raw file):

    fn return_result(ref self: ContractState, num: felt252) -> felt252 {
        let result = num;
        result

this doesn't compile? non-blocking

Suggestion:

        num

---

crates/blockifier/src/transaction/transactions_test.rs line 2620 at r2 (raw file):

#[test]
fn test_balance_print() {
    let int = balance_to_big_uint(&Felt::from(16_u64), &Felt::from(1_u64));

in both tests: deprecated tx testing is also required, not just different user bounds in V3 txs.
I suggest changing the type of #[case] param you have, from ResourceBounds to InvokeTxArgs / DeployAccountTxArgs. also pass a version field (v1 or v3) and a max_fee field if the version is v1. this will require very minimal changes to inputs, though you will need to update the checks as well.

---

crates/blockifier/src/transaction/transactions_test.rs line 2630 at r2 (raw file):

    GasAmount(0)
))]
#[case::user_bounds_between_validate_and_execute(create_gas_amount_bounds_with_default_price(GasAmount(1652), versioned_constants.validate_max_sierra_gas + GasAmount(1234567), GasAmount(0)))]

break lines

Code quote:

#[case::user_bounds_between_validate_and_execute(create_gas_amount_bounds_with_default_price(GasAmount(1652), versioned_constants.validate_max_sierra_gas + GasAmount(1234567), GasAmount(0)))]

---

crates/blockifier/src/transaction/transactions_test.rs line 2707 at r2 (raw file):

        // DOES THIS MAKE SENSE?!
        actual_execute_initial_gas
            - actual_execution_info.execute_call_info.as_ref().unwrap().execution.gas_consumed

hmmm... it looks like the initial gas of the inner call actually takes into account the gas consumption of the entire tree, right?
that doesn't sound right
@Yoni-Starkware ?

Code quote:

        // DOES THIS MAKE SENSE?!
        actual_execute_initial_gas
            - actual_execution_info.execute_call_info.as_ref().unwrap().execution.gas_consumed

---

crates/blockifier/src/transaction/transactions_test.rs line 2720 at r2 (raw file):

    GasAmount(0)
))]
#[case::user_bounds_between_validate_and_execute(create_gas_amount_bounds_with_default_price(GasAmount(2203), versioned_constants.validate_max_sierra_gas + GasAmount(1234567), GasAmount(0)))]

break lines

Code quote:

#[case::user_bounds_between_validate_and_execute(create_gas_amount_bounds_with_default_price(GasAmount(2203), versioned_constants.validate_max_sierra_gas + GasAmount(1234567), GasAmount(0)))]

---

crates/blockifier/src/transaction/transactions_test.rs line 2759 at r2 (raw file):

            )
            .unwrap();
    }

we have a fund_account test util for this

Code quote:

    for fee_type in FeeType::iter() {
        state
            .set_storage_at(
                chain_info.fee_token_address(&fee_type),
                deployed_account_balance_key,
                felt!(BALANCE.0),
            )
            .unwrap();
    }

---

crates/blockifier/src/transaction/transactions_test.rs line 2781 at r2 (raw file):

                .validate_max_sierra_gas
                .min(GasAmount(
                    actual_execute_initial_gas

isn't this the expected amount to subtract from...?

Suggestion:

user_initial_gas

[aner-starkware]

Reviewable status: 3 of 7 files reviewed, 7 unresolved discussions (waiting on @dorimedini-starkware)

---

crates/blockifier/feature_contracts/cairo1/test_contract.cairo line 654 at r2 (raw file):

Previously, dorimedini-starkware wrote…

this doesn't compile? non-blocking

Compiles. I just tried to make it as similar to the Cairo0 function as possible (though it's subjective).

---

crates/blockifier/src/transaction/test_utils.rs line 98 at r2 (raw file):

Previously, dorimedini-starkware wrote…

consider making this input a GasVector

Good idea!

---

crates/blockifier/src/transaction/test_utils.rs line 107 at r2 (raw file):

Previously, dorimedini-starkware wrote…

cool!
I see three places where this function can be dropped in; can you search for usages of create_all_resource_bounds and use this util where applicable?

Done.

---

crates/blockifier/src/transaction/test_utils.rs line 117 at r2 (raw file):

Previously, dorimedini-starkware wrote…

can you use references...? cloning the VC can be slow if it really copies data,
VC instances should be 'static refs so no cloning should be necessary unless the test edits the VC
non-blocking

Not sure. I can try, but there seem to be issues with borrowing & lifetimes.

---

crates/blockifier/src/transaction/transactions_test.rs line 2630 at r2 (raw file):

Previously, dorimedini-starkware wrote…

break lines

Done.

---

crates/blockifier/src/transaction/transactions_test.rs line 2720 at r2 (raw file):

Previously, dorimedini-starkware wrote…

break lines

Done.

---

crates/blockifier/src/transaction/transactions_test.rs line 2781 at r2 (raw file):

Previously, dorimedini-starkware wrote…

isn't this the expected amount to subtract from...?

Done.

[aner-starkware]

crates/blockifier/src/transaction/transactions_test.rs line 2759 at r2 (raw file):

Previously, dorimedini-starkware wrote…

we have a fund_account test util for this

Done.

[dorimedini-starkware]

Reviewed 4 of 4 files at r6, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @aner-starkware)

---

crates/blockifier/src/transaction/post_execution_test.rs line 265 at r6 (raw file):

    use starknet_api::execution_resources::GasVector;

    use crate::transaction::test_utils::create_gas_amount_bounds_with_default_price;

move to top

Code quote:

    use starknet_api::execution_resources::GasVector;

    use crate::transaction::test_utils::create_gas_amount_bounds_with_default_price;

---

crates/blockifier/src/transaction/account_transactions_test.rs line 197 at r6 (raw file):

    #[values(true, false)] zero_bounds: bool,
) {
    use crate::transaction::test_utils::create_gas_amount_bounds_with_default_price;

move to top

Code quote:

use crate::transaction::test_utils::create_gas_amount_bounds_with_default_price;

---

crates/blockifier/src/transaction/transactions_test.rs line 2614 at r6 (raw file):

#[rstest]
#[case::small_user_bounds(create_gas_amount_bounds_with_default_price(
    GasVector{ l1_gas: GasAmount(1652), l2_gas: GasAmount(654321), l1_data_gas: GasAmount(0) }

is this over 100 chars? need to line break if so

Code quote:

GasVector{ l1_gas: GasAmount(1652), l2_gas: GasAmount(654321), l1_data_gas: GasAmount(0) }

---

crates/blockifier/src/transaction/transactions_test.rs line 2708 at r6 (raw file):

#[rstest]
#[case::small_user_bounds(create_gas_amount_bounds_with_default_price(
    GasVector{ l1_gas: GasAmount(2203), l1_data_gas: GasAmount(0), l2_gas: GasAmount(654321) }

line break?

Code quote:

GasVector{ l1_gas: GasAmount(2203), l1_data_gas: GasAmount(0), l2_gas: GasAmount(654321) }

[dorimedini-starkware]

Reviewed 3 of 7 files at r7, 4 of 4 files at r8, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @aner-starkware)

[Yoni-Starkware]

Reviewable status: 3 of 7 files reviewed, 7 unresolved discussions (waiting on @aner-starkware and @dorimedini-starkware)

---

crates/blockifier/src/transaction/transactions_test.rs line 2707 at r2 (raw file):

Previously, dorimedini-starkware wrote…

hmmm... it looks like the initial gas of the inner call actually takes into account the gas consumption of the entire tree, right?
that doesn't sound right
@Yoni-Starkware ?

It is only possible if the inner call has gas_consumed 0 and the caller exists immediately after the call; please check that.
It can happen if the function is super cheap - below 10K gas (remember we put the initial 10K on the caller)

If it's indeed the case, please make the inner call more expensive.

---

crates/blockifier/src/transaction/transactions_test.rs line 2656 at r10 (raw file):

    let contract_tracked_resource = test_contract
        .get_runnable_class()
        .tracked_resource(&versioned_constants.min_compiler_version_for_sierra_gas, None);

Right?
If this account is the caller of the test contract.

I don't know what's the intention of this test though

Suggestion:

    let contract_tracked_resource = test_contract
        .get_runnable_class()
        .tracked_resource(&versioned_constants.min_compiler_version_for_sierra_gas, account_tracked_resource);

[aner-starkware]

Reviewable status: 3 of 7 files reviewed, 7 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)

---

crates/blockifier/src/transaction/account_transactions_test.rs line 197 at r6 (raw file):

Previously, dorimedini-starkware wrote…

move to top

Done.

---

crates/blockifier/src/transaction/post_execution_test.rs line 265 at r6 (raw file):

Previously, dorimedini-starkware wrote…

move to top

Done.

---

crates/blockifier/src/transaction/transactions_test.rs line 2620 at r2 (raw file):

Previously, dorimedini-starkware wrote…

in both tests: deprecated tx testing is also required, not just different user bounds in V3 txs.
I suggest changing the type of #[case] param you have, from ResourceBounds to InvokeTxArgs / DeployAccountTxArgs. also pass a version field (v1 or v3) and a max_fee field if the version is v1. this will require very minimal changes to inputs, though you will need to update the checks as well.

Sounds like it's not such a small change; added a TODO and will do it in the next PR. Though I'm not actually sure I understand what it is that I'm supposed to check and what is the expected result.

---

crates/blockifier/src/transaction/transactions_test.rs line 2707 at r2 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

It is only possible if the inner call has gas_consumed 0 and the caller exists immediately after the call; please check that.
It can happen if the function is super cheap - below 10K gas (remember we put the initial 10K on the caller)

If it's indeed the case, please make the inner call more expensive.

Done. Though I'm not sure I completely understand the logic here.

---

crates/blockifier/src/transaction/transactions_test.rs line 2614 at r6 (raw file):

Previously, dorimedini-starkware wrote…

is this over 100 chars? need to line break if so

I don't think it is. There are longer lines above, for example in test_balance_print.

---

crates/blockifier/src/transaction/transactions_test.rs line 2708 at r6 (raw file):

Previously, dorimedini-starkware wrote…

line break?

I don't think it's needed here.

---

crates/blockifier/src/transaction/transactions_test.rs line 2656 at r10 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Right?
If this account is the caller of the test contract.

I don't know what's the intention of this test though

Done. It's for the inner call initial_gas check.

[dorimedini-starkware]

Reviewed 4 of 4 files at r14, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yoni-Starkware)

---

crates/blockifier/src/transaction/transactions_test.rs line 2620 at r2 (raw file):

Previously, aner-starkware wrote…

Sounds like it's not such a small change; added a TODO and will do it in the next PR. Though I'm not actually sure I understand what it is that I'm supposed to check and what is the expected result.

we want to test that sierra gas tracking works, regardless of the tx version / resource bounds.
you are currently only testing v3.
unblocking; maybe it's not such a small change, although I think if you parametrize like this

#[case(InvokeTxArgs { resource_bounds: default_l1_bounds(), ..Default::default() })]
#[case(InvokeTxArgs { resource_bounds: default_all_bounds(), ..Default::default() })]
#[case(InvokeTxArgs { max_fee: MAX_FEE, ..Default::default() })]
fn test_...(tx_args: InvokeTxArgs, ...) {
...
    let invoke_tx = account_invoke_tx(InvokeTxArgs {
        <your params>,
        ...tx_args
    }

... it should work ok

[aner-starkware]

crates/blockifier/src/transaction/transactions_test.rs line 2620 at r2 (raw file):

Previously, dorimedini-starkware wrote…

we want to test that sierra gas tracking works, regardless of the tx version / resource bounds.
you are currently only testing v3.
unblocking; maybe it's not such a small change, although I think if you parametrize like this

#[case(InvokeTxArgs { resource_bounds: default_l1_bounds(), ..Default::default() })]
#[case(InvokeTxArgs { resource_bounds: default_all_bounds(), ..Default::default() })]
#[case(InvokeTxArgs { max_fee: MAX_FEE, ..Default::default() })]
fn test_...(tx_args: InvokeTxArgs, ...) {
...
    let invoke_tx = account_invoke_tx(InvokeTxArgs {
        <your params>,
        ...tx_args
    }

... it should work ok

Done. (Merged commit above)

[aner-starkware]

Dismissed @Yoni-Starkware from a discussion.
Reviewable status: 3 of 7 files reviewed, all discussions resolved (waiting on @dorimedini-starkware)

[dorimedini-starkware]

Reviewed 3 of 4 files at r16, 1 of 1 files at r17, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @aner-starkware)

On vacation

[dorimedini-starkware]

Reviewed 3 of 4 files at r19, 1 of 1 files at r20, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @aner-starkware)