test(blockifier): max fee exceeds balance errors on new resource bounds

[aner-starkware]

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 77.40%. Comparing base (e3165c4) to head (52ec5c8).
Report is 92 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/starknet_api/src/execution_resources.rs	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #764       +/-   ##
===========================================
+ Coverage   40.10%   77.40%   +37.30%     
===========================================
  Files          26      100       +74     
  Lines        1895    13429    +11534     
  Branches     1895    13429    +11534     
===========================================
+ Hits          760    10395     +9635     
- Misses       1100     2578     +1478     
- Partials       35      456      +421     

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

[TzahiTaub]

Reviewed 2 of 3 files at r8.
Reviewable status: 1 of 4 files reviewed, 5 unresolved discussions (waiting on @aner-starkware, @dorimedini-starkware, and @nimrod-starkware)

---

crates/blockifier/src/transaction/transactions_test.rs line 912 at r4 (raw file):

Previously, aner-starkware wrote…

Done.

Nope, it should be max amount (balance >= price * amount)

---

crates/blockifier/src/transaction/transactions_test.rs line 961 at r4 (raw file):

Previously, aner-starkware wrote…

Yes, because here I want to test each individual resource, so the other resources, if they are 0 for example, might fail a previous check and we won't get the correct error.

Why can't we just use the maxes instead of the mins (and then avoid defining them)? I assume we fail on the per resource assertion before the total balance assertion.

---

crates/blockifier/src/transaction/transactions_test.rs line 969 at r4 (raw file):

Previously, aner-starkware wrote…

Yes, but in any case, it was a temporary patch because of the bug. Now, it can be done without the max (though max is more correct, but is significantly longer and seems to not be needed).

The bug? Do you mean that min_amount could have been larger than the max_amount and now it doesn't? I don't understand what happened

---

crates/blockifier/src/transaction/transactions_test.rs line 971 at r8 (raw file):

    let min_l2_amount = 445500;
    let min_l1_data_amount = 128;
    let max_l2_gas_price: u64 =

Suggestion:

max_l2_gas_amount

---

crates/blockifier/src/transaction/transactions_test.rs line 973 at r8 (raw file):

    let max_l2_gas_price: u64 =
        (BALANCE / MAX_L2_GAS_PRICE).try_into().expect("Failed to convert u128 to u64.");
    let max_l1_data_gas_price: u64 =

Suggestion:

max_l1_data_gas_amount

[aner-starkware]

Reviewable status: 1 of 4 files reviewed, 5 unresolved discussions (waiting on @dorimedini-starkware, @nimrod-starkware, and @TzahiTaub)

---

crates/blockifier/src/transaction/transactions_test.rs line 912 at r4 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Nope, it should be max amount (balance >= price * amount)

Done. Sorry, I must have been super tired 😬

---

crates/blockifier/src/transaction/transactions_test.rs line 961 at r4 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Why can't we just use the maxes instead of the mins (and then avoid defining them)? I assume we fail on the per resource assertion before the total balance assertion.

1. It wouldn't help if min_amount>`max_amount' (as was the case before the bug-fix)
2. It's better to make the test as tight as possible; I'm adding the min_amount only because it's necessary, putting max_amount slightly defeats the point of the test.

---

crates/blockifier/src/transaction/transactions_test.rs line 969 at r4 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

The bug? Do you mean that min_amount could have been larger than the max_amount and now it doesn't? I don't understand what happened

There was a bug that made the l2_gas_price super high, so the max_amount was very low (in particular, lower than min_amount); now that it's back to normal, there's no reason min_amount should be bigger than max_amount (though, technically, it can happen...). Your call if you want me to insert max(...).

---

crates/blockifier/src/transaction/transactions_test.rs line 971 at r8 (raw file):

    let min_l2_amount = 445500;
    let min_l1_data_amount = 128;
    let max_l2_gas_price: u64 =

Done.

---

crates/blockifier/src/transaction/transactions_test.rs line 973 at r8 (raw file):

    let max_l2_gas_price: u64 =
        (BALANCE / MAX_L2_GAS_PRICE).try_into().expect("Failed to convert u128 to u64.");
    let max_l1_data_gas_price: u64 =

Done.

[TzahiTaub]

Reviewed 1 of 2 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: 3 of 4 files reviewed, 3 unresolved discussions (waiting on @aner-starkware, @dorimedini-starkware, and @nimrod-starkware)

---

crates/blockifier/src/transaction/transactions_test.rs line 968 at r4 (raw file):

Previously, aner-starkware wrote…

I didn't understand your suggestion.

Never mind, too much of a change, I'll try to open a PR later on.

---

crates/blockifier/src/transaction/transactions_test.rs line 969 at r4 (raw file):

Previously, aner-starkware wrote…

There was a bug that made the l2_gas_price super high, so the max_amount was very low (in particular, lower than min_amount); now that it's back to normal, there's no reason min_amount should be bigger than max_amount (though, technically, it can happen...). Your call if you want me to insert max(...).

Replace with asserts that max_amount/3 >= min_amount

---

crates/blockifier/src/transaction/transactions_test.rs line 964 at r8 (raw file):

        class_info,
    );
    assert_failure_if_resource_bounds_exceed_balance(state, block_context, invalid_tx);

Suggestion:

    // V3 invoke.
    
    for (l1_max_amount, l2_max_amount, l1_data_max_amount) in [
        (max_l1_gas_price + 1, min_l2_amount, min_l1_data_amount),
        (min_l1_amount, max_l2_gas_price + 1, min_l1_data_amount),
        (min_l1_amount, min_l2_amount, max_l1_data_gas_price + 1),
        (max_l1_gas_price / 3 + 1, max_l2_gas_price / 3 + 1, max_l1_data_gas_price / 3 + 1),
    ] {
        let invalid_resource_bounds = ValidResourceBounds::AllResources(AllResourceBounds {
            l1_gas: ResourceBounds {
                max_amount: l1_max_amount,
                max_price_per_unit: MAX_L1_GAS_PRICE,
            },
            l2_gas: ResourceBounds {
                max_amount: l2_max_amount,
                max_price_per_unit: MAX_L2_GAS_PRICE,
            },
            l1_data_gas: ResourceBounds {
                max_amount: l1_data_max_amount,
                max_price_per_unit: MAX_L1_DATA_GAS_PRICE,
            },
        });

    let invalid_tx = account_invoke_tx(invoke_tx_args! {
        resource_bounds: invalid_l1_resource_bounds,
        ..default_args.clone()
    });
    assert_failure_if_resource_bounds_exceed_balance(state, block_context, invalid_tx);

    // Deploy.
    let invalid_tx = AccountTransaction::DeployAccount(deploy_account_tx(
        deploy_account_tx_args! {
            resource_bounds: max_l1_resource_bounds,
            class_hash: test_contract.get_class_hash()
        },
        &mut NonceManager::default(),
    ));
    assert_failure_if_resource_bounds_exceed_balance(state, block_context, invalid_tx);

    // Declare.
    let contract_to_declare = FeatureContract::Empty(CairoVersion::Cairo1);
    let class_info = calculate_class_info_for_testing(contract_to_declare.get_class());
    let invalid_tx = declare_tx(
        declare_tx_args! {
            class_hash: contract_to_declare.get_class_hash(),
            compiled_class_hash: contract_to_declare.get_compiled_class_hash(),
            sender_address: account_contract_address,
            resource_bounds: invalid_l1_resource_bounds,
        },
        class_info,
    );
    assert_failure_if_resource_bounds_exceed_balance(state, block_context, invalid_tx);

[TzahiTaub]

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

---

crates/blockifier/src/transaction/transactions_test.rs line 968 at r4 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Never mind, too much of a change, I'll try to open a PR later on.

FYI https://reviewable.io/reviews/starkware-libs/sequencer/1228

[aner-starkware]

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware, @nimrod-starkware, and @TzahiTaub)

---

crates/blockifier/src/transaction/transactions_test.rs line 969 at r4 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Replace with asserts that max_amount/3 >= min_amount

Done.

[TzahiTaub]

Reviewed 1 of 3 files at r12.
Reviewable status: 1 of 4 files reviewed, all discussions resolved (waiting on @dorimedini-starkware and @nimrod-starkware)

[github-actions]

Artifacts upload triggered. View details here

[TzahiTaub]

Reviewed 1 of 4 files at r13, 3 of 3 files at r15, all commit messages.
Reviewable status: 4 of 5 files reviewed, 3 unresolved discussions (waiting on @aner-starkware, @dorimedini-starkware, and @nimrod-starkware)

---

crates/blockifier/src/test_utils.rs line 105 at r15 (raw file):

pub const MAX_L1_GAS_PRICE: NonzeroGasPrice = DEFAULT_STRK_L1_GAS_PRICE;
pub const MAX_RESOURCE_COMMITMENT: Fee =
    DEFAULT_L1_GAS_AMOUNT.nonzero_saturating_mul(MAX_L1_GAS_PRICE);

Not used

Code quote:

pub const MAX_RESOURCE_COMMITMENT: Fee =
    DEFAULT_L1_GAS_AMOUNT.nonzero_saturating_mul(MAX_L1_GAS_PRICE);

---

crates/blockifier/src/transaction/transactions_test.rs line 937 at r15 (raw file):

    #[values(CairoVersion::Cairo0, CairoVersion::Cairo1)] account_cairo_version: CairoVersion,
) {
    use starknet_api::execution_resources::GasAmount;

move up

---

crates/starknet_api/src/execution_resources.rs line 28 at r15 (raw file):

    Hash,
)]
#[cfg_attr(any(test, feature = "testing"), derive(derive_more::Div))]

Suggestion:

#[cfg_attr(
    any(test, feature = "testing"),
    derive(derive_more::Add, derive_more::Div, derive_more::Sum, derive_more::AddAssign)
)]
#[derive(
    derive_more::Display,
    Clone,
    Copy,
    Debug,
    Default,
    Eq,
    PartialEq,
    PartialOrd,
    Ord,
    Serialize,
    Deserialize,
    Hash,
)]

[github-actions]

Artifacts upload triggered. View details here

[aner-starkware]

Reviewable status: 1 of 5 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware, @nimrod-starkware, and @TzahiTaub)

---

crates/blockifier/src/test_utils.rs line 105 at r15 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Not used

Done.

---

crates/blockifier/src/transaction/transactions_test.rs line 937 at r15 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

move up

Done.

---

crates/starknet_api/src/execution_resources.rs line 28 at r15 (raw file):

    Hash,
)]
#[cfg_attr(any(test, feature = "testing"), derive(derive_more::Div))]

Done.

[TzahiTaub]

Reviewed 3 of 3 files at r16, all commit messages.
Reviewable status: 4 of 5 files reviewed, all discussions resolved (waiting on @dorimedini-starkware and @nimrod-starkware)

[TzahiTaub]

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