Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build(fee): tx_resources depend on resource bounds signature #455

Conversation

nimrod-starkware
Copy link
Contributor

@nimrod-starkware nimrod-starkware commented Aug 14, 2024

This change is Reviewable

@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/txs_resources branch from 338583b to 8e23dff Compare August 14, 2024 11:45
@nimrod-starkware nimrod-starkware changed the title message build(fee): tx_resources depend on resource bounds signature Aug 14, 2024
@nimrod-starkware nimrod-starkware self-assigned this Aug 14, 2024
@nimrod-starkware nimrod-starkware marked this pull request as ready for review August 14, 2024 12:35
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@nimrod-starkware nimrod-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/starknet_resources branch from 855c361 to 991209e Compare August 15, 2024 10:18
@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/txs_resources branch 2 times, most recently from 8f1db14 to 18c1d34 Compare August 15, 2024 10:21
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 nimrod-starkware force-pushed the nimrod/l2_gas/txs_resources branch from 18c1d34 to 510e9e3 Compare August 15, 2024 11:43
Copy link
Contributor Author

@nimrod-starkware nimrod-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/starknet_resources branch from 991209e to 85b439a Compare August 28, 2024 09:09
@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/txs_resources branch from 510e9e3 to bf7d8be Compare August 28, 2024 09:09
@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/starknet_resources branch from 85b439a to 45f3fbe Compare August 28, 2024 13:21
@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/txs_resources branch from bf7d8be to ec3790b Compare August 28, 2024 13:21
@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/starknet_resources branch from 45f3fbe to e0533bb Compare September 1, 2024 05:31
@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/txs_resources branch from ec3790b to e823cfa Compare September 1, 2024 05:32
Copy link

codecov bot commented Sep 1, 2024

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.

@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/starknet_resources branch from 597f7c1 to 5aafa22 Compare September 3, 2024 06:44
@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/txs_resources branch from 9ddc2b9 to db47673 Compare September 3, 2024 06:44
@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/starknet_resources branch from 5aafa22 to 338fe8c Compare September 3, 2024 13:42
@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/txs_resources branch from db47673 to 6324c9a Compare September 3, 2024 13:42
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/starknet_resources branch from 338fe8c to 60e4881 Compare September 3, 2024 13:45
@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/txs_resources branch from 6324c9a to 8b3d045 Compare September 3, 2024 13:46
@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/starknet_resources branch from 60e4881 to 324e884 Compare September 3, 2024 13:52
@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/txs_resources branch from 8b3d045 to d5f495b Compare September 3, 2024 13:52
@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/starknet_resources branch from 324e884 to 10a6f72 Compare September 3, 2024 15:18
@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/txs_resources branch from d5f495b to afeda2e Compare September 3, 2024 15:18
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/starknet_resources branch from 10a6f72 to e396522 Compare September 4, 2024 06:39
@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/txs_resources branch from afeda2e to 48335f4 Compare September 4, 2024 06:39
@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/starknet_resources branch from e396522 to b3581ba Compare September 4, 2024 07:05
@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/txs_resources branch from 48335f4 to 24f1748 Compare September 4, 2024 07:05
@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/starknet_resources branch from b3581ba to 0e755c4 Compare September 4, 2024 11:07
@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/txs_resources branch from 24f1748 to 49c16a8 Compare September 4, 2024 11:07
@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/starknet_resources branch from 0e755c4 to 427a81a Compare September 4, 2024 11:10
@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/txs_resources branch from 49c16a8 to caf494f Compare September 4, 2024 11:10
@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/starknet_resources branch from 427a81a to ae18c8f Compare September 4, 2024 13:36
@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/txs_resources branch from caf494f to 7eaba87 Compare September 4, 2024 13:37
Copy link

github-actions bot commented Sep 4, 2024

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

@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/starknet_resources branch from ae18c8f to b1a68c1 Compare September 5, 2024 07:18
@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/txs_resources branch from 7eaba87 to 1d1c197 Compare September 5, 2024 07:18
@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/starknet_resources branch from b1a68c1 to 35201a3 Compare September 5, 2024 11:08
@nimrod-starkware nimrod-starkware force-pushed the nimrod/l2_gas/txs_resources branch from 1d1c197 to a8898b8 Compare September 5, 2024 11:09
@github-actions github-actions bot locked and limited conversation to collaborators Sep 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants