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

[aner-starkware]

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 39.06250% with 39 lines in your changes missing coverage. Please review.

Project coverage is 75.23%. Comparing base (b0cfe82) to head (de0aad3).
Report is 52 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #710      +/-   ##
==========================================
+ Coverage   74.18%   75.23%   +1.04%     
==========================================
  Files         359       87     -272     
  Lines       36240    11157   -25083     
  Branches    36240    11157   -25083     
==========================================
- Hits        26886     8394   -18492     
+ Misses       7220     2352    -4868     
+ Partials     2134      411    -1723     

Flag	Coverage Δ
	75.23% <39.06%> (+1.04%)	⬆️

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

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [66.877 ms 66.995 ms 67.133 ms]
change: [-9.1173% -5.7525% -2.7908%] (p = 0.00 < 0.05)
Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
4 (4.00%) high mild
3 (3.00%) high severe

[TzahiTaub]

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

---

crates/blockifier/src/transaction/account_transaction.rs line 239 at r17 (raw file):

            &tx_context.get_gas_vector_computation_mode(),
        )?;

Remove the blank line IMO, as the first let just creates a temp var for the second let.

---

crates/blockifier/src/transaction/account_transaction.rs line 247 at r17 (raw file):

        let fee_type = &tx_info.fee_type();
        match tx_info {
            TransactionInfo::Current(context) => {

Remove this and its counterpart and you'll earn one less indentation.

Suggestion:

TransactionInfo::Current(context) => 

---

crates/blockifier/src/transaction/account_transaction.rs line 249 at r17 (raw file):

            TransactionInfo::Current(context) => {
                match &context.resource_bounds {
                    starknet_api::transaction::ValidResourceBounds::L1Gas(ResourceBounds {

Remove and add ValidResourceBounds to the imports.

---

crates/blockifier/src/transaction/account_transaction.rs line 277 at r17 (raw file):

                        }
                    }
                    starknet_api::transaction::ValidResourceBounds::AllResources(

As above

[aner-starkware]

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

---

crates/blockifier/src/transaction/account_transaction.rs line 239 at r17 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Remove the blank line IMO, as the first let just creates a temp var for the second let.

OK, but this part will change anyway in a future PR.

---

crates/blockifier/src/transaction/account_transaction.rs line 247 at r17 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Remove this and its counterpart and you'll earn one less indentation.

Done.

---

crates/blockifier/src/transaction/account_transaction.rs line 249 at r17 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Remove and add ValidResourceBounds to the imports.

Done.

---

crates/blockifier/src/transaction/account_transaction.rs line 277 at r17 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

As above

Done.

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [66.957 ms 67.072 ms 67.213 ms]
change: [-9.3194% -5.9521% -2.9060%] (p = 0.00 < 0.05)
Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
2 (2.00%) high mild
5 (5.00%) high severe

[TzahiTaub]

Reviewed 1 of 5 files at r2, 1 of 4 files at r3, 56 of 60 files at r16, 1 of 1 files at r18, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @aner-starkware and @nimrod-starkware)

---

crates/blockifier/src/transaction/account_transaction.rs line 293 at r18 (raw file):

                                .expect("Failed to convert u128 to u64.")),
                        })?;
                    }

IMO - add some inner function (defined in this function) for this and call it here and above to avoid code duplication.

Code quote:

                    let max_l1_gas_amount_as_u128: u128 = l1_gas.max_amount.into();
                    if max_l1_gas_amount_as_u128 < minimal_l1_gas_amount {
                        return Err(TransactionFeeError::MaxGasAmountTooLow {
                            resource: L1Gas,
                            max_gas_amount: l1_gas.max_amount,
                            // TODO(Ori, 1/2/2024): Write an indicative expect message
                            // explaining why the conversion
                            // works.
                            minimal_gas_amount: (minimal_l1_gas_amount
                                .try_into()
                                .expect("Failed to convert u128 to u64.")),
                        })?;
                    }

[aner-starkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nimrod-starkware and @TzahiTaub)

---

crates/blockifier/src/transaction/account_transaction.rs line 293 at r18 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

IMO - add some inner function (defined in this function) for this and call it here and above to avoid code duplication.

I think this part changes, and also this refactoring appears in several places. I'll add a TODO for refactoring in the following PR.

Away

[TzahiTaub]

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

[aner-starkware]

Dismissed @nimrod-starkware from a discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nimrod-starkware)