test(blockifier): update test_insufficient_max_fee_reverts to include new resource bounds

[dorimedini-starkware]

This change is 

[dorimedini-starkware]

• test(blockifier): migrate remaining tests to all resource bounds (except execution flavors test) #1342
• test(blockifier): parametrize more tests in transactions_test by resource bounds types #1337
• test(blockifier): parametrize test_actual_fee_gt_resource_bounds by resource bounds types #1330
• test(blockifier): parametrize test_max_fee_exceeds_balance by resource bounds types #1322
• test(blockifier): parametrize test_invoke_tx by resource bounds types #1321
• test(blockifier): update test_revert_on_resource_overuse to include new resource bounds #1314
• test(blockifier): update test_insufficient_max_fee_reverts to include new resource bounds #1313 👈
• test(blockifier): l1 bounds -> all bounds (or parametrize) in account transaction test #1312
• test(blockifier): test enforce_fee flag with new resource bounds #1311
• test(blockifier): l1 bounds -> all bounds in stack trace test #1310
• test(blockifier): l1 bounds -> all bounds in worker logic test #1309
• test(blockifier): l1 bounds -> all bounds in versioned state test #1308
• test(blockifier): add all resource bounds to stateful validator test #1307
• refactor(blockifier): refactor default testing resource bounds #1306
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @dorimedini-starkware and the rest of your teammates on Graphite

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.46%. Comparing base (e3165c4) to head (f0f7db2).
Report is 81 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1313       +/-   ##
===========================================
+ Coverage   40.10%   77.46%   +37.36%     
===========================================
  Files          26      100       +74     
  Lines        1895    13447    +11552     
  Branches     1895    13447    +11552     
===========================================
+ Hits          760    10417     +9657     
- Misses       1100     2574     +1474     
- Partials       35      456      +421     

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

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.122 ms 34.180 ms 34.250 ms]
change: [-3.9562% -2.4333% -1.0950%] (p = 0.00 < 0.05)
Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
3 (3.00%) high mild
3 (3.00%) high severe

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [33.871 ms 33.905 ms 33.945 ms]
change: [-4.2749% -2.6448% -1.1876%] (p = 0.00 < 0.05)
Performance has improved.
Found 9 outliers among 100 measurements (9.00%)
4 (4.00%) high mild
5 (5.00%) high severe

[yoavGrs]

Reviewable status: 0 of 2 files reviewed, 6 unresolved discussions (waiting on @dorimedini-starkware)

---

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

) {
    let gas_mode = resource_bounds.get_gas_vector_computation_mode();
    let overdraft_resource = match gas_mode {

Move it next to its use.
It's the overdraft resource only for the second tx.

Code quote:

let overdraft_resource

---

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

                }
                GasVectorComputationMode::All => {
                    // We do not expect the exact same fee in the All case.

Despite the expectation, in practice, we do get equality. Why?

Code quote:

 We do not expect the exact same fee in the All case.

---

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

    // This call should fail due to insufficient max fee (steps bound based on max_fee is not so
    // tight as to stop execution between iterations 1 and 2).
    let resource_bounds_depth1 = match gas_mode {

It's not the bounds in tx_execution_info1

Suggestion:

resource_used_depth1

---

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

    )
    .unwrap();
    // In the deprecated bounds case, due to resource limit being estimated by steps, the execution

Is that the case?

Suggestion:

In the l1 gas bounds case

---

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

            resource_bounds: resource_bounds_depth1,
            nonce: nonce_manager.next(account_address),
            calldata: recursive_function_calldata(&contract_address, 824, false),

Is it tight?
Is it going to change frequently?

Code quote:

824

---

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

    .unwrap();
    // In the deprecated bounds case, the entire step limit is derived from 100% of all the
    // bounds (only L1 gas), so expected revert fee is identical to the original fee.

This comment should appear above the NoL2Gas case in the macro.

Code quote:

    // In the deprecated bounds case, the entire step limit is derived from 100% of all the
    // bounds (only L1 gas), so expected revert fee is identical to the original fee.

[dorimedini-starkware]

Reviewable status: 0 of 2 files reviewed, 6 unresolved discussions (waiting on @yoavGrs)

---

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

Previously, yoavGrs wrote…

Move it next to its use.
It's the overdraft resource only for the second tx.

Done.

---

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

Previously, yoavGrs wrote…

Despite the expectation, in practice, we do get equality. Why?

WDYM in practice we get the same? equality isn't asserted here

---

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

Previously, yoavGrs wrote…

It's not the bounds in tx_execution_info1

Done.

---

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

Previously, yoavGrs wrote…

Is that the case?

Done.

---

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

Previously, yoavGrs wrote…

Is it tight?
Is it going to change frequently?

this test is old, and it didn't change now, so I guess it's not very sensitive

---

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

Previously, yoavGrs wrote…

This comment should appear above the NoL2Gas case in the macro.

Done.

[github-actions]

Artifacts upload triggered. View details here

[yoavGrs]

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

---

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

Previously, dorimedini-starkware wrote…

WDYM in practice we get the same? equality isn't asserted here

In the code, you don't assert inequality.
But the explanations point out that execution_info.receipt.gas.l2_gas < tx_execution_info1.receipt.gas.l2_gas, but it's not the case: when I ran the test I got equality.

[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

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [34.757 ms 35.215 ms 35.762 ms]
change: [+1.3113% +2.6924% +4.2146%] (p = 0.00 < 0.05)
Performance has regressed.
Found 10 outliers among 100 measurements (10.00%)
2 (2.00%) high mild
8 (8.00%) high severe

[dorimedini-starkware]

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @yoavGrs)

---

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

Previously, yoavGrs wrote…

In the code, you don't assert inequality.
But the explanations point out that execution_info.receipt.gas.l2_gas < tx_execution_info1.receipt.gas.l2_gas, but it's not the case: when I ran the test I got equality.

Done. You're right, the final fee should be the same (snap to bounds). The final gas vector however need not be the same on revert, so I simplified this a bit (no need for the macro anymore)

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [35.271 ms 35.716 ms 36.255 ms]
change: [+2.4527% +3.6453% +5.2050%] (p = 0.00 < 0.05)
Performance has regressed.
Found 16 outliers among 100 measurements (16.00%)
7 (7.00%) low mild
1 (1.00%) high mild
8 (8.00%) high severe

[yoavGrs]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dorimedini-starkware)

[github-actions]

Artifacts upload triggered. View details here

[dorimedini-starkware]

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