test(blockifier): insufficient amount new resource bounds

[aner-starkware]

This change is 

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.58%. Comparing base (b0cfe82) to head (11290bb).
Report is 220 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #811      +/-   ##
==========================================
- Coverage   74.18%   70.58%   -3.61%     
==========================================
  Files         359       88     -271     
  Lines       36240    11354   -24886     
  Branches    36240    11354   -24886     
==========================================
- Hits        26886     8014   -18872     
+ Misses       7220     2962    -4258     
+ Partials     2134      378    -1756     

Flag	Coverage Δ
	?

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.

[aner-starkware]

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

Previously, aner-starkware wrote…

Done. The reason there's no test for the l1_data_gas is because the test didn't use the use_kzg_da flag. I added this, and then it failed the "successful execution", so I removed the successful execution (because, actually, there's no reason to assume estimate_minimal_gas_vector is sufficient to execute the tx, we just got lucky). If you want to reinsert this check, it needs to check either the tx is successful, or that the error is not any of the prevalidation errors we expect to pass.

Reinserted the "successful execution" check, as I explained in the above comment.

[TzahiTaub]

Reviewed 2 of 2 files at r7, 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 1026 at r4 (raw file):

Previously, aner-starkware wrote…

Reinserted the "successful execution" check, as I explained in the above comment.

Great 👍
What made the valid_tx succeed in the end?

---

crates/blockifier/src/transaction/transactions_test.rs line 945 at r7 (raw file):

    #[values(CairoVersion::Cairo0, CairoVersion::Cairo1)] account_cairo_version: CairoVersion,
) {
    // TODO(Aner): test also with use_kzg_flag == true

remove the todo

---

crates/blockifier/src/transaction/transactions_test.rs line 1015 at r7 (raw file):

    // contains all insufficient resources.
    let resources_with_positive_amount = match use_kzg_da {
        true => [L2Gas, L1DataGas],

Shouldn't the L1 resource also appear in here?

Suggestion:

> [L1Gas, L2Gas, L1DataGas],

---

crates/blockifier/src/transaction/transactions_test.rs line 1047 at r7 (raw file):

            L1Gas => invalid_resources.l1_gas.max_price_per_unit -= 1,
            L2Gas => invalid_resources.l2_gas.max_price_per_unit -= 1,
            L1DataGas => invalid_resources.l1_data_gas.max_price_per_unit -= 1,

This fails a tx even if were use_kzg==False?

Code quote:

  L1DataGas => invalid_resources.l1_data_gas.max_price_per_unit -= 1,

[aner-starkware]

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

---

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

Previously, TzahiTaub (Tzahi) wrote…

Great 👍
What made the valid_tx succeed in the end?

What I wrote above - it can either succeed (in which case the nonce is increased), or fail, so long as the error is not one of the errors that we expect not to happen (in which case the nonce does not increase).

---

crates/blockifier/src/transaction/transactions_test.rs line 945 at r7 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

remove the todo

Done.

---

crates/blockifier/src/transaction/transactions_test.rs line 1015 at r7 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Shouldn't the L1 resource also appear in here?

Is it clearer this way? I can also put the bottom loop inside (shorter and less vars), though I think it's a bit less clear.

---

crates/blockifier/src/transaction/transactions_test.rs line 1047 at r7 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

This fails a tx even if were use_kzg==False?

Yes, that's the spec I understood (though I guess it's better to double-check here). @Yoni-Starkware? There's also a question about whether actual_gas_price can be 0 for any resource, as this test assumes this scenario does not happen.

[Yoni-Starkware]

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

---

crates/blockifier/src/transaction/transactions_test.rs line 1047 at r7 (raw file):

Previously, aner-starkware wrote…

Yes, that's the spec I understood (though I guess it's better to double-check here). @Yoni-Starkware? There's also a question about whether actual_gas_price can be 0 for any resource, as this test assumes this scenario does not happen.

Hmmm that's a good Q.
Since data gas amount might be 0, it's better to allow price 0 in this case.
I.e., skip the second check here:

sequencer/crates/blockifier/src/transaction/account_transaction.rs

Line 405 in faac38b

if max_gas_price < actual_gas_price {

[aner-starkware]

crates/blockifier/src/transaction/transactions_test.rs line 1047 at r7 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Hmmm that's a good Q.
Since data gas amount might be 0, it's better to allow price 0 in this case.
I.e., skip the second check here:

sequencer/crates/blockifier/src/transaction/account_transaction.rs

Line 405 in faac38b

if max_gas_price < actual_gas_price {

I can do as in the previous check, and only check those that are not 0.

[aner-starkware]

crates/blockifier/src/transaction/transactions_test.rs line 1047 at r7 (raw file):

Previously, aner-starkware wrote…

I can do as in the previous check, and only check those that are not 0.

But the question still remains whether to always check the prices, or if it should depend on the flag\computation_mode

[Yoni-Starkware]

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

---

crates/blockifier/src/transaction/transactions_test.rs line 1047 at r7 (raw file):

Previously, aner-starkware wrote…

But the question still remains whether to always check the prices, or if it should depend on the flag\computation_mode

It's enough to check against the min amount. If it's 0, you should not check the price, regardless of the reason (flag/lack of use)

[aner-starkware]

crates/blockifier/src/transaction/transactions_test.rs line 1047 at r7 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

It's enough to check against the min amount. If it's 0, you should not check the price, regardless of the reason (flag/lack of use)

That part is clear. But what if it's not 0, should I check the price regardless of the flags as well?

[Yoni-Starkware]

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

---

crates/blockifier/src/transaction/transactions_test.rs line 1047 at r7 (raw file):

Previously, aner-starkware wrote…

That part is clear. But what if it's not 0, should I check the price regardless of the flags as well?

Yes, you have a minimal_gas_vector calc - this logic should be (and is) encapsulated in it.
No reason to check the flag again

[TzahiTaub]

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

---

crates/blockifier/src/transaction/transactions_test.rs line 1015 at r7 (raw file):

Previously, aner-starkware wrote…

Is it clearer this way? I can also put the bottom loop inside (shorter and less vars), though I think it's a bit less clear.

It's clearer as in the previous the code assumes (what is correct but unknown to the reader and also very implicit) that L1Gas is 0 when kzg is True in the minimal_gas_vector. I think you should move the loop inside, but use a continue for if max_amount == 0: continue not avoid putting a lot of code under two indentations

---

crates/blockifier/src/transaction/transactions_test.rs line 1047 at r7 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Yes, you have a minimal_gas_vector calc - this logic should be (and is) encapsulated in it.
No reason to check the flag again

About the previous question - "There's also a question about whether actual_gas_price can be 0 for any resource...", the type of gas_prices are NoneZero so no.

[TzahiTaub]

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

---

crates/blockifier/src/transaction/transactions_test.rs line 1015 at r7 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

It's clearer as in the previous the code assumes (what is correct but unknown to the reader and also very implicit) that L1Gas is 0 when kzg is True in the minimal_gas_vector. I think you should move the loop inside, but use a continue for if max_amount == 0: continue not avoid putting a lot of code under two indentations

* to avoid putting a lot of code under two indentations

[TzahiTaub]

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

[TzahiTaub]

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

---

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

Previously, aner-starkware wrote…

What I wrote above - it can either succeed (in which case the nonce is increased), or fail, so long as the error is not one of the errors that we expect not to happen (in which case the nonce does not increase).

But catching the errors above is strange. We have an amount that is too low in the first place so it throws an error we ignore, and later on we deduct 1 from that amount and assert we get that same error. I also don't understand how the estimate function can raise the exceptions were catching - the prevalidation checks uses the same estimate function before they throw these errors, it's not a post validation error.

[aner-starkware]

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

Previously, TzahiTaub (Tzahi) wrote…

But catching the errors above is strange. We have an amount that is too low in the first place so it throws an error we ignore, and later on we deduct 1 from that amount and assert we get that same error. I also don't understand how the estimate function can raise the exceptions were catching - the prevalidation checks uses the same estimate function before they throw these errors, it's not a post validation error.

It's not the same error, that's the point! In the "successful" execution it passes the pre_validation checks here, but possibly fails later on (which we ignore). Later, when deducting 1, we verify it fails the expected pre_validation checks here, otherwise we panic.

[aner-starkware]

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

---

crates/blockifier/src/transaction/transactions_test.rs line 1015 at r7 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

* to avoid putting a lot of code under two indentations

Done.

---

crates/blockifier/src/transaction/transactions_test.rs line 1047 at r7 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

About the previous question - "There's also a question about whether actual_gas_price can be 0 for any resource...", the type of gas_prices are NoneZero so no.

Ahh, right, that part is relevant only for the amount, thanks!

[TzahiTaub]

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

---

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

Previously, aner-starkware wrote…

It's not the same error, that's the point! In the "successful" execution it passes the pre_validation checks here, but possibly fails later on (which we ignore). Later, when deducting 1, we verify it fails the expected pre_validation checks here, otherwise we panic.

I might be totally missing something here, but the errors you catch below (i.e., errors that the "valid_tx" might raise due to a "mistake") are both prevalidation errors, thrown when the resources the user signed on are insufficient according to the estimate function (look for the single nontest code that throws MaxGasAmountTooLow). So the valid tx does fail in the prevalidation stage, and we shouldn't catch this as this doesn't make sense.

[TzahiTaub]

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

---

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

Previously, TzahiTaub (Tzahi) wrote…

I might be totally missing something here, but the errors you catch below (i.e., errors that the "valid_tx" might raise due to a "mistake") are both prevalidation errors, thrown when the resources the user signed on are insufficient according to the estimate function (look for the single nontest code that throws MaxGasAmountTooLow). So the valid tx does fail in the prevalidation stage, and we shouldn't catch this as this doesn't make sense.

OK, seems I got confused :)

---

crates/blockifier/src/transaction/transactions_test.rs line 1016 at r9 (raw file):

                ),
            ) => panic!("Transaction failed with expected minimal price."),
            _ => 0,

Suggestion:

    _ => 0,  // Ignore failures other than those above (e.g., post-validation errors).

[TzahiTaub]

Reviewed 1 of 1 files at r10.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @dorimedini-starkware and @nimrod-starkware)

[TzahiTaub]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dorimedini-starkware and @nimrod-starkware)