chore(blockifier): check max price of l1_data_gas and l2_gas

[aner-starkware]

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 43.07692% with 37 lines in your changes missing coverage. Please review.

Project coverage is 75.24%. Comparing base (e6feb76) to head (1968b80).

Files with missing lines	Patch %	Lines
.../blockifier/src/transaction/account_transaction.rs	47.36%	30 Missing ⚠️
crates/blockifier/src/blockifier/block.rs	0.00%	7 Missing ⚠️

Additional details and impacted files

@@                         Coverage Diff                         @@
##           aner/pre_validation_enforce_fee     #710      +/-   ##
===================================================================
- Coverage                            75.49%   75.24%   -0.26%     
===================================================================
  Files                                   87       87              
  Lines                                11109    11146      +37     
  Branches                             11109    11146      +37     
===================================================================
  Hits                                  8387     8387              
- Misses                                2311     2348      +37     
  Partials                               411      411              

Flag	Coverage Δ
	75.24% <43.07%> (-0.26%)	⬇️

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.

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [65.574 ms 65.656 ms 65.749 ms]
change: [-7.0712% -3.8349% -1.1384%] (p = 0.01 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
3 (3.00%) high mild
1 (1.00%) high severe

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [66.684 ms 66.764 ms 66.854 ms]
change: [-8.8482% -5.3789% -2.3108%] (p = 0.00 < 0.05)
Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
5 (5.00%) high mild
3 (3.00%) high severe

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [66.152 ms 66.228 ms 66.313 ms]
change: [-7.5112% -4.3234% -1.7567%] (p = 0.00 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
2 (2.00%) high mild
1 (1.00%) high severe

[nimrod-starkware]

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

---

crates/blockifier/src/transaction/account_transaction.rs line 234 at r1 (raw file):

        tx_context: &TransactionContext,
    ) -> TransactionPreValidationResult<()> {
        // TODO(Aner): seprate to cases based on context.resource_bounds type

Looks like you can remove the todo, right?

Code quote:

// TODO(Aner): seprate to cases based on context.resource_bounds type

---

crates/blockifier/src/transaction/account_transaction.rs line 260 at r1 (raw file):

                                // TODO(Ori, 1/2/2024): Write an indicative expect message
                                // explaining why the convertion
                                // works.

why did this change?

Code quote:

                                // TODO(Ori, 1/2/2024): Write an indicative expect message
                                // explaining why the convertion
                                // works.

---

crates/blockifier/src/transaction/account_transaction.rs line 292 at r1 (raw file):

                            })?;
                        }
                        // TODO(Aner): add checks for minimal_l1_data_gas and minimal_l2_gas

Suggestion:

// TODO(Aner): Add checks for minimal l1_data_gas and minimal l2_gas.

---

crates/blockifier/src/transaction/account_transaction.rs line 296 at r1 (raw file):

                        let GasPricesForFeeType { l1_gas_price, l1_data_gas_price, l2_gas_price } =
                            block_info.gas_prices.get_gas_prices_by_fee_type(fee_type);
                        // TODO!(Aner): add tests for l1_data_gas_price and l2_gas_price

Suggestion:

// TODO(Aner): Add tests for l1_data_gas_price and l2_gas_price.

---

crates/blockifier/src/transaction/account_transaction.rs line 300 at r1 (raw file):

                            (L1Gas, l1_gas.max_price_per_unit, l1_gas_price.into()),
                            (L1DataGas, l1_data_gas.max_price_per_unit, l1_data_gas_price.into()),
                            (L2Gas, l2_gas.max_price_per_unit, l2_gas_price.into()),

The order of the iteration matters if there is more than one resource that is too expensive, @dorimedini-starkware WDYT?

Code quote:

                        for (gas_type, max_gas_price, actual_gas_price) in [
                            (L1Gas, l1_gas.max_price_per_unit, l1_gas_price.into()),
                            (L1DataGas, l1_data_gas.max_price_per_unit, l1_data_gas_price.into()),
                            (L2Gas, l2_gas.max_price_per_unit, l2_gas_price.into()),

---

crates/blockifier/src/transaction/transactions_test.rs line 986 at r1 (raw file):

    );

    // TODO(Aner): add test for low Max L1 data gas price and L2 gas price

Suggestion:

// TODO(Aner): Add test for low max L1 data gas price and L2 gas price.

[aner-starkware]

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

---

crates/blockifier/src/transaction/account_transaction.rs line 234 at r1 (raw file):

Previously, nimrod-starkware wrote…

Looks like you can remove the todo, right?

Done.

---

crates/blockifier/src/transaction/account_transaction.rs line 260 at r1 (raw file):

Previously, nimrod-starkware wrote…

why did this change?

The quoted comment didn't change (though it had a typo, so I fixed it now), but the error type changed to accommodate the different resources.

---

crates/blockifier/src/transaction/account_transaction.rs line 292 at r1 (raw file):

                            })?;
                        }
                        // TODO(Aner): add checks for minimal_l1_data_gas and minimal_l2_gas

Done.

---

crates/blockifier/src/transaction/account_transaction.rs line 296 at r1 (raw file):

                        let GasPricesForFeeType { l1_gas_price, l1_data_gas_price, l2_gas_price } =
                            block_info.gas_prices.get_gas_prices_by_fee_type(fee_type);
                        // TODO!(Aner): add tests for l1_data_gas_price and l2_gas_price

Done. The ! symbolizes that it needs to be done in the following PR.

---

crates/blockifier/src/transaction/transactions_test.rs line 986 at r1 (raw file):

    );

    // TODO(Aner): add test for low Max L1 data gas price and L2 gas price

Done.

[nimrod-starkware]

Reviewed 1 of 5 files at r2, 1 of 4 files at r3, all commit messages.
Reviewable status: 2 of 6 files reviewed, 1 unresolved discussion (waiting on @aner-starkware, @dorimedini-starkware, and @TzahiTaub)

[dorimedini-starkware]

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

---

crates/blockifier/src/transaction/account_transaction.rs line 300 at r1 (raw file):

Previously, nimrod-starkware wrote…

The order of the iteration matters if there is more than one resource that is too expensive, @dorimedini-starkware WDYT?

I think we should do better: the error we return should indicate exactly which gas price(s) are too low.
You should be collecting all problematic (gas_type, max_price, actual_price) tuples and outputting all of them in the error.
The MaxGasPriceTooLow error should be updated to include a collection of such triples.
That way, the order won't matter, and the users will get the most information

[aner-starkware]

crates/blockifier/src/transaction/account_transaction.rs line 300 at r1 (raw file):

Previously, dorimedini-starkware wrote…

I think we should do better: the error we return should indicate exactly which gas price(s) are too low.
You should be collecting all problematic (gas_type, max_price, actual_price) tuples and outputting all of them in the error.
The MaxGasPriceTooLow error should be updated to include a collection of such triples.
That way, the order won't matter, and the users will get the most information

@AvivYossef-starkware I'm guessing this will affect your PRs as well.

[AvivYossef-starkware]

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

---

crates/blockifier/src/transaction/account_transaction.rs line 300 at r1 (raw file):

Previously, aner-starkware wrote…

@AvivYossef-starkware I'm guessing this will affect your PRs as well.

I changed the error in my PR to include it

[aner-starkware]

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

---

crates/blockifier/src/transaction/account_transaction.rs line 300 at r1 (raw file):

Previously, AvivYossef-starkware wrote…

I changed the error in my PR to include it

Added a TODO