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 70.21%. Comparing base (77410e1) to head (4ada9a8).

Additional details and impacted files

@@                           Coverage Diff                            @@
##           tzahi/blockifier/build_recurse_calldata    #1453   +/-   ##
========================================================================
  Coverage                                    70.21%   70.21%           
========================================================================
  Files                                           96       96           
  Lines                                        13068    13068           
  Branches                                     13068    13068           
========================================================================
  Hits                                          9176     9176           
  Misses                                        3495     3495           
  Partials                                       397      397           

☔ 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()