build(fee): sn_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 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @nimrod-starkware and @TzahiTaub)

---

crates/blockifier/src/fee/actual_cost_test.rs line 0 at r1 (raw file):
I would prefer the include_l2_gas flag be a "parametrized fixture"
TAL at rstest_reuse, so you can define something like:

#[template]
#[rstest]
fn include_l2_gas_param(#[values(false)] include_l2_gas: bool) {}

and use it in tests like this:

#[apply(include_l2_gas_param)]
fn test_calculate_tx_gas_usage_basic<'a>(#[values(false, true)] use_kzg_da: bool, include_l2_gas: bool) {
   ...

not sure if it works, but if it does, you'll be able to edit include_l2_gas_param by adding , true to #[values] and get test coverage for free

---

crates/blockifier/src/fee/gas_usage_test.rs line 0 at r1 (raw file):
ditto on the parametrization comment above

---

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

            + self.get_messages_cost()
            + self.get_events_cost(versioned_constants, include_l2_gas)
    }

wouldn't this be cleaner?
then you wouldn't need to propagate this flag everywhere

Suggestion:

        versioned_constants: &VersionedConstants,
        use_kzg_da: bool,
        include_l2_gas: bool,
    ) -> GasVector {
        if include_l2_gas {
            todo!();
        } else {
            self.get_calldata_and_signature_cost(versioned_constants, include_l2_gas)
                + self.get_code_cost(versioned_constants, include_l2_gas)
                + self.get_state_changes_cost(use_kzg_da)
                + self.get_messages_cost()
                + self.get_events_cost(versioned_constants, include_l2_gas)
        }
    }

[nimrod-starkware]

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

---

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

Previously, dorimedini-starkware wrote…

I would prefer the include_l2_gas flag be a "parametrized fixture"
TAL at rstest_reuse, so you can define something like:

#[template]
#[rstest]
fn include_l2_gas_param(#[values(false)] include_l2_gas: bool) {}

and use it in tests like this:

#[apply(include_l2_gas_param)]
fn test_calculate_tx_gas_usage_basic<'a>(#[values(false, true)] use_kzg_da: bool, include_l2_gas: bool) {
   ...

not sure if it works, but if it does, you'll be able to edit include_l2_gas_param by adding , true to #[values] and get test coverage for free

Nice!
I'll add it to tests that I think are independent of l1/l2 gas calculation (that don't compare hard coded gas vectors for example)

---

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

Previously, dorimedini-starkware wrote…

wouldn't this be cleaner?
then you wouldn't need to propagate this flag everywhere

it's cleaner but get_calldata_and_signature_cost needs to know whether to calculate as l1 or l2... no?

[nimrod-starkware]

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

---

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

Previously, nimrod-starkware wrote…

Nice!
I'll add it to tests that I think are independent of l1/l2 gas calculation (that don't compare hard coded gas vectors for example)

added it to test_calculate_tx_gas_usage

---

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

Previously, dorimedini-starkware wrote…

ditto on the parametrization comment above

Done

[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/Cargo.toml line 68 at r2 (raw file):

regex.workspace = true
rstest.workspace = true
test-case.workspace = true

to pass machete I think you'll need to do this (rstest_reuse isn't used in non-testing code)

Suggestion:

rand = { workspace = true, optional = true }
rstest = { workspace = true, optional = true }
rstest_reuse = { workspace = true, optional = true }
serde = { workspace = true, features = ["derive"] }
serde_json = { workspace = true, features = ["arbitrary_precision"] }
sha2.workspace = true
sha3.workspace = true
starknet-types-core.workspace = true
starknet_api = { workspace = true, features = ["testing"] }
strum.workspace = true
strum_macros.workspace = true
tempfile.workspace = true
thiserror.workspace = true
tikv-jemallocator = { workspace = true, optional = true }
toml.workspace = true

[dev-dependencies]
assert_matches.workspace = true
criterion = { workspace = true, features = ["html_reports"] }
glob.workspace = true
pretty_assertions.workspace = true
rand.workspace = true
regex.workspace = true
rstest.workspace = true
rstest_reuse.workspace = true
test-case.workspace = true

---

crates/blockifier/src/fee/actual_cost_test.rs line 36 at r2 (raw file):

#[rstest]
// TODO(Nimrod): Add true as a case.
fn include_l2_gas(#[values(false)] has_l2_gas: bool) {}

since this is used on other tests, please move it to a test utils module

Code quote:

#[rstest_reuse::template]
#[rstest]
// TODO(Nimrod): Add true as a case.
fn include_l2_gas(#[values(false)] has_l2_gas: bool) {}

---

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

Previously, nimrod-starkware wrote…

it's cleaner but get_calldata_and_signature_cost needs to know whether to calculate as l1 or l2... no?

won't there be a completely different function to compute L2 gas cost? I guess not... unblocking

[codecov]

Codecov Report

Attention: Patch coverage is 90.90909% with 4 lines in your changes missing coverage. Please review.

Project coverage is 75.82%. Comparing base (f357376) to head (35201a3).

Files with missing lines	Patch %	Lines
crates/blockifier/src/transaction/objects.rs	90.90%	4 Missing ⚠️

Additional details and impacted files

@@                          Coverage Diff                           @@
##           nimrod/resources_enum/impl_methods     #454      +/-   ##
======================================================================
+ Coverage                               75.72%   75.82%   +0.09%     
======================================================================
  Files                                      85      352     +267     
  Lines                                   10984    37507   +26523     
  Branches                                10984    37507   +26523     
======================================================================
+ Hits                                     8318    28440   +20122     
- Misses                                   2242     6746    +4504     
- Partials                                  424     2321    +1897     

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

[nimrod-starkware]

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

---

crates/blockifier/Cargo.toml line 68 at r2 (raw file):

Previously, dorimedini-starkware wrote…

to pass machete I think you'll need to do this (rstest_reuse isn't used in non-testing code)

machete passed, but added it anyway

---

crates/blockifier/src/fee/actual_cost_test.rs line 36 at r2 (raw file):

Previously, dorimedini-starkware wrote…

since this is used on other tests, please move it to a test utils module

Done

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [65.358 ms 65.410 ms 65.469 ms]
change: [-10.741% -6.6969% -3.0873%] (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

[dorimedini-starkware]

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

---

crates/blockifier/src/test_utils.rs line 446 at r3 (raw file):

}

#[cfg(test)]

why is this needed...? in case feature = "testing" causes this module to compile, in non-test mode?

Code quote:

#[cfg(test)]

[nimrod-starkware]

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

---

crates/blockifier/src/test_utils.rs line 446 at r3 (raw file):

Previously, dorimedini-starkware wrote…

why is this needed...? in case feature = "testing" causes this module to compile, in non-test mode?

without this, i get a clippy warning for unused macros.
I think it's because it's only being used in tests (under cfg(test)) but not declared in a test module.
For some weird reason annotating #[allow(unused_macros)] didn't solve it...

[dorimedini-starkware]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [65.117 ms 65.215 ms 65.318 ms]
change: [-8.1215% -4.8397% -1.9984%] (p = 0.00 < 0.05)
Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high mild

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [65.521 ms 65.589 ms 65.659 ms]
change: [-9.5738% -6.3590% -3.5581%] (p = 0.00 < 0.05)
Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
1 (1.00%) low mild
3 (3.00%) high mild
1 (1.00%) high severe

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [66.019 ms 66.089 ms 66.167 ms]
change: [-9.1579% -5.8972% -3.1875%] (p = 0.00 < 0.05)
Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high severe

full_committer_flow performance improved 😺
full_committer_flow time: [47.410 ms 47.519 ms 47.653 ms]
change: [-3.4517% -3.1901% -2.8712%] (p = 0.00 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
1 (1.00%) high mild
2 (2.00%) high severe

[dorimedini-starkware]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [65.648 ms 65.747 ms 65.854 ms]
change: [-3.6650% -2.3668% -1.3176%] (p = 0.00 < 0.05)
Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
1 (1.00%) high mild
1 (1.00%) high severe

[dorimedini-starkware]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [65.295 ms 65.362 ms 65.437 ms]
change: [-9.6371% -5.5857% -2.3856%] (p = 0.00 < 0.05)
Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
4 (4.00%) high mild
1 (1.00%) high severe

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [65.833 ms 66.347 ms 67.102 ms]
change: [-9.1572% -5.4957% -2.0675%] (p = 0.00 < 0.05)
Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
4 (4.00%) high mild
4 (4.00%) high severe

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [65.415 ms 65.482 ms 65.554 ms]
change: [-9.6852% -6.2758% -3.2844%] (p = 0.00 < 0.05)
Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
5 (5.00%) high mild