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

[aner-starkware]

This change is 

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.49%. Comparing base (a7c0b87) to head (a0bf6e5).

Additional details and impacted files

@@                               Coverage Diff                                @@
##           aner/pre_validation_covers_sum_of_products_1     #764      +/-   ##
================================================================================
+ Coverage                                         70.33%   70.49%   +0.16%     
================================================================================
  Files                                                87       87              
  Lines                                             11180    11180              
  Branches                                          11180    11180              
================================================================================
+ Hits                                               7863     7881      +18     
+ Misses                                             2929     2911      -18     
  Partials                                            388      388              

Flag	Coverage Δ
	70.49% <100.00%> (+0.16%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[TzahiTaub]

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

---

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

    let invalid_max_fee = Fee(BALANCE + 1);
    // TODO(Ori, 1/2/2024): Write an indicative expect message explaining why the conversion works.
    let balance_over_l1_gas_price: u64 =

Clearer?

Suggestion:

maximal_l1_amount

---

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

    // TODO!(Aner): remove magic numbers, use function to calculate minimal amounts.
    let min_l1_amount = 1652;
    let min_l2_amount = 445500;

Do you know why these are needed here but weren't needed for the l1 cases above?

Code quote:

    let min_l1_amount = 1652;
    let min_l2_amount = 445500;

---

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

    // In the following cases, the balance is not enough to cover the gas price of the transaction.
    // Minimal amounts are used to avoid failing the test due to min gas usage not covered.
    for (l1_max_amount, l2_max_amount, l1_data_max_amount) in [

Why not modify the previous l1_gas txs tests for the all resources case as well? Loop between L1Resources once and AllResources and have less code (hopefully) and all scenarios for both types.

---

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

    // Minimal amounts are used to avoid failing the test due to min gas usage not covered.
    for (l1_max_amount, l2_max_amount, l1_data_max_amount) in [
        (min_l1_amount + balance_over_l1_gas_price + 1, min_l2_amount, 0),

Isn't this the tight bound we need?

Suggestion:

max(min_l1_amount, balance_over_l1_gas_price + 1)

[aner-starkware]

Reviewable status: 1 of 4 files reviewed, 3 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…

Clearer?

Done.

---

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

Previously, TzahiTaub (Tzahi) wrote…

Do you know why these are needed here but weren't needed for the l1 cases above?

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.

---

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

Previously, TzahiTaub (Tzahi) wrote…

Why not modify the previous l1_gas txs tests for the all resources case as well? Loop between L1Resources once and AllResources and have less code (hopefully) and all scenarios for both types.

I didn't understand your suggestion.

---

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

Previously, TzahiTaub (Tzahi) wrote…

Isn't this the tight bound we need?

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).