build(fee): tx_resources depend on resource bounds signature

[nimrod-starkware]

This change is 

[nimrod-starkware]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• build(fee): add events to l2 cost #462
• build(fee): add code to l2 cost #461
• build(fee): add calldata and signature l2 cost #460
• build(fee): tx_resources depend on resource bounds signature #455 👈
• build(fee): sn_resources depend on resource bounds signature #454
• build(fee): implement methods on resource bounds #600
• refactor(fee): remove more occurnces of resource mapping #547 : 1 other dependent PR (#557 )
• build(fee): tx_context and create_tx_info cannot_fail #538
• refactor(fee): use valid resource bounds in blockifier #537
• refactor(fee): tx_args holds valid resource bounds #508 : 1 other dependent PR (#509 )
• refactor(fee): block context creator may fail #507
• refactor(fee): transaction info creator may fail #506
• build(fee): current tx_info holds validResourceBounds #505
• refactor(fee): remove resource mapping usage in rpc and adjust to use all resources #580
• main

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

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

[dorimedini-starkware]

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @nimrod-starkware and @TzahiTaub)

---

crates/blockifier/src/fee/actual_cost.rs line 92 at r1 (raw file):

            tx_context.block_context.block_info.use_kzg_da,
            tx_context.tx_info.has_l2_gas_bounds(),
        )?;

maybe to_gas_vector should simply borrow the tx_context? seems weird, passing three of it's fields explicitly

Code quote:

        let gas = tx_resources.to_gas_vector(
            &tx_context.block_context.versioned_constants,
            tx_context.block_context.block_info.use_kzg_da,
            tx_context.tx_info.has_l2_gas_bounds(),
        )?;

---

crates/blockifier/src/test_utils/struct_impls.rs line 123 at r1 (raw file):

            &block_context.versioned_constants,
            block_context.block_info.use_kzg_da,
            false,

this should be injected somehow; it's a test util, and we should be able to test both cases

Code quote:

false,

---

crates/blockifier/src/transaction/account_transactions_test.rs line 0 at r1 (raw file):
see here

---

crates/blockifier/src/transaction/execution_flavors_test.rs line 135 at r1 (raw file):

                &block_context.versioned_constants,
                block_context.block_info.use_kzg_da,
                false

see here

Code quote:

 false

---

crates/blockifier/src/transaction/objects.rs line 116 at r1 (raw file):

    pub fn has_l2_gas_bounds(&self) -> bool {
        match self {
            TransactionInfo::Current(context) => context.resource_bounds.0.len() == 3,

didn't we want to first look into changing this type to an enum?

Code quote:

context.resource_bounds

---

crates/blockifier/src/transaction/post_execution_test.rs line 262 at r1 (raw file):

            &block_context.versioned_constants,
            block_context.block_info.use_kzg_da,
            false,

see here

Code quote:

false,

---

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

[nimrod-starkware]

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

---

crates/blockifier/src/fee/actual_cost.rs line 92 at r1 (raw file):

Previously, dorimedini-starkware wrote…

maybe to_gas_vector should simply borrow the tx_context? seems weird, passing three of it's fields explicitly

I agree it looks weird but I think passing more information than needed might be more confusing, no?

---

crates/blockifier/src/test_utils/struct_impls.rs line 123 at r1 (raw file):

Previously, dorimedini-starkware wrote…

this should be injected somehow; it's a test util, and we should be able to test both cases

Done.

---

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

Previously, dorimedini-starkware wrote…

see here

Changed it to be dependent on the user's bounds signature.

---

crates/blockifier/src/transaction/execution_flavors_test.rs line 135 at r1 (raw file):

Previously, dorimedini-starkware wrote…

see here

Done.

---

crates/blockifier/src/transaction/objects.rs line 116 at r1 (raw file):

Previously, dorimedini-starkware wrote…

didn't we want to first look into changing this type to an enum?

I will do it separately. I haven't figured out yet how and where to do it.

---

crates/blockifier/src/transaction/post_execution_test.rs line 262 at r1 (raw file):

Previously, dorimedini-starkware wrote…

see here

Added TODO to derive it from user's bounds

---

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

    let total_gas = expected_tx_resources
        .to_gas_vector(versioned_constants, block_context.block_info.use_kzg_da, false)

here it's false since it tests l1 handler

Code quote:

 false)

---

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

Previously, dorimedini-starkware wrote…

see here

Changed it to be dependent on the user's bounds signature.

[dorimedini-starkware]

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nimrod-starkware and @TzahiTaub)

---

crates/blockifier/src/fee/actual_cost.rs line 92 at r1 (raw file):

Previously, nimrod-starkware wrote…

I agree it looks weird but I think passing more information than needed might be more confusing, no?

add a TODO to consider this... maybe adding a to_gas_vector_with_context wrapper to to_gas_vector

---

crates/blockifier/src/transaction/objects.rs line 116 at r1 (raw file):

Previously, nimrod-starkware wrote…

I will do it separately. I haven't figured out yet how and where to do it.

add a TODO then for now

[nimrod-starkware]

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

---

crates/blockifier/src/fee/actual_cost.rs line 92 at r1 (raw file):

Previously, dorimedini-starkware wrote…

add a TODO to consider this... maybe adding a to_gas_vector_with_context wrapper to to_gas_vector

added a wrapper

---

crates/blockifier/src/transaction/objects.rs line 116 at r1 (raw file):

Previously, dorimedini-starkware wrote…

add a TODO then for now

WDYT about how i did it now?
added a method for ResourceBoundsMapping

[dorimedini-starkware]

Reviewed 3 of 4 files at r3, all commit messages.
Reviewable status: 7 of 8 files reviewed, all discussions resolved (waiting on @TzahiTaub)

[codecov]

Codecov Report

Attention: Patch coverage is 92.00000% with 2 lines in your changes missing coverage. Please review.

Please upload report for BASE (nimrod/l2_gas/starknet_resources@b1a68c1). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
crates/blockifier/src/fee/actual_cost.rs	0.00%	0 Missing and 1 partial ⚠️
crates/starknet_api/src/transaction.rs	80.00%	1 Missing ⚠️

Additional details and impacted files

@@                         Coverage Diff                         @@
##             nimrod/l2_gas/starknet_resources     #455   +/-   ##
===================================================================
  Coverage                                    ?   75.76%           
===================================================================
  Files                                       ?       85           
  Lines                                       ?    11015           
  Branches                                    ?    11015           
===================================================================
  Hits                                        ?     8345           
  Misses                                      ?     2246           
  Partials                                    ?      424           

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

[dorimedini-starkware]

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

[dorimedini-starkware]

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

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [65.964 ms 66.464 ms 67.209 ms]
change: [-8.6503% -5.1749% -2.0039%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
1 (1.00%) high mild
3 (3.00%) high severe