test(blockifier): test initial_gas for consecutive call_contract calls

[TzahiTaub]

No description provided.

[lotem-starkware]

This change is 

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.12%. Comparing base (e3165c4) to head (615e29c).
Report is 76 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1453       +/-   ##
===========================================
+ Coverage   40.10%   69.12%   +29.01%     
===========================================
  Files          26      100       +74     
  Lines        1895    13447    +11552     
  Branches     1895    13447    +11552     
===========================================
+ Hits          760     9295     +8535     
- Misses       1100     3751     +2651     
- Partials       35      401      +366     

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

[dorimedini-starkware]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @TzahiTaub and @yoavGrs)

---

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

    CompilerBasedVersion::CairoVersion(CairoVersion::Cairo0),
    CompilerBasedVersion::CairoVersion(CairoVersion::Cairo1)
])]

which other scenarios are you planning?
strange to have a single #[case]

Code quote:

#[case(&[
    CompilerBasedVersion::CairoVersion(CairoVersion::Cairo1),
    CompilerBasedVersion::CairoVersion(CairoVersion::Cairo1),
    CompilerBasedVersion::CairoVersion(CairoVersion::Cairo0),
    CompilerBasedVersion::CairoVersion(CairoVersion::Cairo1)
])]

---

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

])]
fn test_initial_gas(#[case] versions: &[CompilerBasedVersion]) {
    use crate::execution::call_info::CallInfo;

move to top

Code quote:

use crate::execution::call_info::CallInfo;

---

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

            max_price_per_unit: DEFAULT_STRK_L1_DATA_GAS_PRICE.into(),
        },
    });

there's a default_all_resource_bounds fixture; use it, it provides exactly the same numbers IIRC

Suggestion:

fn test_initial_gas(
    default_all_resource_bounds: ValidResourceBounds,
    #[case] versions: &[CompilerBasedVersion]
) {
    use crate::execution::call_info::CallInfo;

    let block_context = BlockContext::create_for_account_testing();
    let account = FeatureContract::AccountWithoutValidations(CairoVersion::Cairo1);
    let account_address = account.get_instance_address(0_u16);
    let used_test_contracts: HashSet<FeatureContract> =
        HashSet::from_iter(versions.iter().map(|x| x.get_test_contract()));
    let mut contracts: Vec<FeatureContract> = used_test_contracts.into_iter().collect();
    contracts.push(account);
    let sender_balance = BALANCE;
    let chain_info = &block_context.chain_info;
    let state = &mut test_state(
        chain_info,
        sender_balance,
        &contracts.into_iter().map(|contract| (contract, 1u16)).collect::<Vec<_>>(),
    );

---

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

    resource_bounds:  unlimited_resource_bounds,
        version: TransactionVersion::THREE
    });

fix formatting (not automatic in macros)

Code quote:

    let account_tx = account_invoke_tx(invoke_tx_args! {
    sender_address: account_address,
    calldata: build_recurse_calldata(versions),
    resource_bounds:  unlimited_resource_bounds,
        version: TransactionVersion::THREE
    });

---

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

    });

    let mut transactional_state = TransactionalState::create_transactional(state);

you need this because you're calling execute_raw instead of execute? why not call execute?

Code quote:

let mut transactional_state = TransactionalState::create_transactional(state);

---

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

    let val_call_info = &transaction_ex_info.validate_call_info.unwrap();
    let val_initial_gas = val_call_info.call.initial_gas;

(a) consistency (you have validate_gas_consumed), (b) val can mean value, better write it out

Suggestion:

    let validate_call_info = &transaction_ex_info.validate_call_info.unwrap();
    let validate_initial_gas = val_call_info.call.initial_gas;

---

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

    let mut started_vm_mode = false;
    // The __validate__ call of a the account contract.
    let mut prev_version = &CompilerBasedVersion::CairoVersion(CairoVersion::Cairo1);

in case we add a test case where the account is cairo0 - better to name this (you use this value twice)

Suggestion:

    let account_version = CairoVersion::Cairo1;
    let account = FeatureContract::AccountWithoutValidations(account_version);
    let account_address = account.get_instance_address(0_u16);
    let used_test_contracts: HashSet<FeatureContract> =
        HashSet::from_iter(versions.iter().map(|x| x.get_test_contract()));
    let mut contracts: Vec<FeatureContract> = used_test_contracts.into_iter().collect();
    contracts.push(account);
    let sender_balance = BALANCE;
    let chain_info = &block_context.chain_info;
    let state = &mut test_state(
        chain_info,
        sender_balance,
        &contracts.into_iter().map(|contract| (contract, 1u16)).collect::<Vec<_>>(),
    );
    let unlimited_resource_bounds = ValidResourceBounds::AllResources(AllResourceBounds {
        l1_gas: ResourceBounds {
            max_amount: DEFAULT_L1_GAS_AMOUNT,
            max_price_per_unit: DEFAULT_STRK_L1_GAS_PRICE.into(),
        },
        l2_gas: ResourceBounds {
            max_amount: DEFAULT_L2_GAS_MAX_AMOUNT.into(),
            max_price_per_unit: DEFAULT_STRK_L2_GAS_PRICE.into(),
        },
        l1_data_gas: ResourceBounds {
            max_amount: DEFAULT_L1_DATA_GAS_MAX_AMOUNT,
            max_price_per_unit: DEFAULT_STRK_L1_DATA_GAS_PRICE.into(),
        },
    });
    let account_tx = account_invoke_tx(invoke_tx_args! {
    sender_address: account_address,
    calldata: build_recurse_calldata(versions),
    resource_bounds:  unlimited_resource_bounds,
        version: TransactionVersion::THREE
    });

    let mut transactional_state = TransactionalState::create_transactional(state);
    let execution_flags =
        ExecutionFlags { charge_fee: true, validate: true, concurrency_mode: false };
    let transaction_ex_info =
        account_tx.execute_raw(&mut transactional_state, &block_context, execution_flags).unwrap();

    let val_call_info = &transaction_ex_info.validate_call_info.unwrap();
    let val_initial_gas = val_call_info.call.initial_gas;
    assert_eq!(val_initial_gas, DEFAULT_L2_GAS_MAX_AMOUNT.0);
    let validate_gas_consumed = val_call_info.execution.gas_consumed;
    assert!(validate_gas_consumed > 0, "New Cairo1 contract should consume gas.");

    let default_call_info = CallInfo::default();
    let mut prev_initial_gas = val_initial_gas;
    let mut ex_call_info = &transaction_ex_info.execute_call_info.unwrap();
    let mut curr_initial_gas;
    let mut started_vm_mode = false;
    // The __validate__ call of a the account contract.
    let mut prev_version = &CompilerBasedVersion::CairoVersion(account_version);

---

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

    let mut prev_version = &CompilerBasedVersion::CairoVersion(CairoVersion::Cairo1);
    // Insert the __execute__ call in the beginning of versions.
    for version in [CompilerBasedVersion::CairoVersion(CairoVersion::Cairo1)].iter().chain(versions)

Suggestion:

(account_version)

---

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

        } else {
            assert!(ex_call_info.execution.gas_consumed == 0);
        }

will this logic look better if you match (prev_version, version, started_vm_mode)? I have a feeling it will

Code quote:

        if version == &CompilerBasedVersion::CairoVersion(CairoVersion::Cairo0) && !started_vm_mode
        {
            // First time we are in VM mode.
            assert_eq!(
                curr_initial_gas,
                block_context.versioned_constants.default_initial_gas_cost()
            );
            started_vm_mode = true;
        } else {
            if prev_version != &CompilerBasedVersion::CairoVersion(CairoVersion::Cairo0) {
                // Non Cairo0 contract consumes gas from the initial gas.
                assert!(curr_initial_gas < prev_initial_gas);
            } else {
                assert_eq!(curr_initial_gas, prev_initial_gas);
            }
        }

        if version == &CompilerBasedVersion::CairoVersion(CairoVersion::Cairo1) {
            assert!(ex_call_info.execution.gas_consumed > 0);
        } else {
            assert!(ex_call_info.execution.gas_consumed == 0);
        }

---

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

        // If inner_calls is empty, this SHOULD be the last call and thus last loop iteration.
        // Assigning the default call info, will cause an error if the loop continues.
        ex_call_info = &ex_call_info.inner_calls.first().unwrap_or(&default_call_info);

you expect there to be exactly zero or one inner call infos, right? please assert this

Code quote:

.first()

[github-actions]

Artifacts upload triggered. View details here

[TzahiTaub]

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @dorimedini-starkware and @yoavGrs)

---

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

Previously, dorimedini-starkware wrote…

which other scenarios are you planning?
strange to have a single #[case]

Would like to use OldCairo1 instead of Cairo0 later on. Two variants in total.

---

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

Previously, dorimedini-starkware wrote…

move to top

Done.

---

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

Previously, dorimedini-starkware wrote…

there's a default_all_resource_bounds fixture; use it, it provides exactly the same numbers IIRC

Done.

---

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

Previously, dorimedini-starkware wrote…

fix formatting (not automatic in macros)

Done.

---

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

Previously, dorimedini-starkware wrote…

you need this because you're calling execute_raw instead of execute? why not call execute?

Copied from other tests on the file.

---

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

Previously, dorimedini-starkware wrote…

(a) consistency (you have validate_gas_consumed), (b) val can mean value, better write it out

Done.

---

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

Previously, dorimedini-starkware wrote…

in case we add a test case where the account is cairo0 - better to name this (you use this value twice)

Done.

---

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

    let mut prev_version = &CompilerBasedVersion::CairoVersion(CairoVersion::Cairo1);
    // Insert the __execute__ call in the beginning of versions.
    for version in [CompilerBasedVersion::CairoVersion(CairoVersion::Cairo1)].iter().chain(versions)

Done.

---

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

Previously, dorimedini-starkware wrote…

will this logic look better if you match (prev_version, version, started_vm_mode)? I have a feeling it will

Not sure if it's clearer, but here it is

---

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

Previously, dorimedini-starkware wrote…

you expect there to be exactly zero or one inner call infos, right? please assert this

Done.

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[dorimedini-starkware]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @TzahiTaub and @yoavGrs)

---

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

Previously, TzahiTaub (Tzahi) wrote…

Would like to use OldCairo1 instead of Cairo0 later on. Two variants in total.

can you add a TODO with the cases you still want to check?

---

crates/blockifier/src/transaction/account_transactions_test.rs line 1596 at r3 (raw file):

                started_vm_mode = true;
            }
            _ => {

IIUC - there is currently no scenario where the first contract is old cairo1; however this is what you must assume in this match arm, right?
you can add a final _ arm that panics with test case not implemented or something

Suggestion:

(CompilerBasedVersion::CairoVersion(CairoVersion::Cairo1), _, _)

[TzahiTaub]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware and @yoavGrs)

---

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

Previously, dorimedini-starkware wrote…

can you add a TODO with the cases you still want to check?

Done.

---

crates/blockifier/src/transaction/account_transactions_test.rs line 1596 at r3 (raw file):

Previously, dorimedini-starkware wrote…

IIUC - there is currently no scenario where the first contract is old cairo1; however this is what you must assume in this match arm, right?
you can add a final _ arm that panics with test case not implemented or something

I don't understand - what do I assume in this match arm? We match this arm if prev is Cairo1 or OldCairo1 (and not the first time in VM mode).

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[dorimedini-starkware]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @TzahiTaub and @yoavGrs)

---

crates/blockifier/src/transaction/account_transactions_test.rs line 1596 at r3 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

I don't understand - what do I assume in this match arm? We match this arm if prev is Cairo1 or OldCairo1 (and not the first time in VM mode).

I'm confused, why is this true?

// prev_version is a non Cairo0 contract, thus it consumes gas from the initial
// gas.

if prev version is OldCairo1, why does it consume gas from the initial gas?

[TzahiTaub]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @yoavGrs)

---

crates/blockifier/src/transaction/account_transactions_test.rs line 1596 at r3 (raw file):

Previously, dorimedini-starkware wrote…

I'm confused, why is this true?

// prev_version is a non Cairo0 contract, thus it consumes gas from the initial
// gas.

if prev version is OldCairo1, why does it consume gas from the initial gas?

Because this update is done in every cairo1 contract, we will ignore it when we collect the fee.

[dorimedini-starkware]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yoavGrs)

[TzahiTaub]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yoavGrs)

[github-actions]

Artifacts upload triggered. View details here