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

feat: add l1_gas_consumed for block hash #268

Merged
merged 2 commits into from
Jun 4, 2024

Conversation

yoavGrs
Copy link
Contributor

@yoavGrs yoavGrs commented Jun 3, 2024

This change is Reviewable

@yoavGrs yoavGrs self-assigned this Jun 3, 2024
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 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ShahakShama and @yoavGrs)


src/transaction.rs line 969 at r1 (raw file):

    pub da_l1_gas_consumed: u64,
    pub da_l1_data_gas_consumed: u64,
    pub l1_gas_consumed: u64,

what about data gas?

Code quote:

pub l1_gas_consumed: u64,

src/block_hash/receipt_commitment.rs line 95 at r1 (raw file):

        .chain(&Felt::ZERO) // L2 gas consumed
        .chain(&execution_resources.l1_gas_consumed.into())
        .chain(&execution_resources.da_l1_data_gas_consumed.into())

not data-availability related gas; just total L1 blob gas

Suggestion:

l1_data_gas_consumed

Copy link
Contributor Author

@yoavGrs yoavGrs 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: 3 of 6 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware and @ShahakShama)


src/transaction.rs line 969 at r1 (raw file):

Previously, dorimedini-starkware wrote…

what about data gas?

Done.


src/block_hash/receipt_commitment.rs line 95 at r1 (raw file):

Previously, dorimedini-starkware wrote…

not data-availability related gas; just total L1 blob gas

Done.

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 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ShahakShama and @yoavGrs)


src/transaction.rs line 965 at r2 (raw file):

    pub l1_gas: u64,
    pub l1_data_gas: u64,
}

why u64s? in the blockifier they're u128

Code quote:

pub struct GasVector {
    pub l1_gas: u64,
    pub l1_data_gas: u64,
}

src/block_hash/test_utils.rs line 24 at r2 (raw file):

        memory_holes: 76,
        da_gas_consumed: GasVector { l1_gas: 54, l1_data_gas: 10 },
        gas_consumed: GasVector { l1_gas: 16580, l1_data_gas: 32 },

where did the 10 come from?

Code quote:

        da_gas_consumed: GasVector { l1_gas: 54, l1_data_gas: 10 },
        gas_consumed: GasVector { l1_gas: 16580, l1_data_gas: 32 },

Copy link
Contributor Author

@yoavGrs yoavGrs 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, 2 unresolved discussions (waiting on @dorimedini-starkware and @ShahakShama)


src/transaction.rs line 965 at r2 (raw file):

Previously, dorimedini-starkware wrote…

why u64s? in the blockifier they're u128

There was a u64 here before my change.
Would you like me to change it?


src/block_hash/test_utils.rs line 24 at r2 (raw file):

Previously, dorimedini-starkware wrote…

where did the 10 come from?

It's a new number.
The 32 was taken to preserve the hash, and I want a different number here.

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.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ShahakShama and @Yoni-Starkware)


src/transaction.rs line 965 at r2 (raw file):

Previously, yoavGrs wrote…

There was a u64 here before my change.
Would you like me to change it?

in the ExecutionResources struct?
I would run this by @Yoni-Starkware

Copy link

@Yoni-Starkware Yoni-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, 1 unresolved discussion (waiting on @dorimedini-starkware and @ShahakShama)


src/transaction.rs line 965 at r2 (raw file):

Previously, dorimedini-starkware wrote…

in the ExecutionResources struct?
I would run this by @Yoni-Starkware

u64 is enough, but gas_price must be u128 and @dorimedini-starkware set gas to u128 because we need to multiple them.

I see that other fields are not consistent with the blockifier as well.
Whatever you choose

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.

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

Copy link
Contributor Author

@yoavGrs yoavGrs 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ShahakShama)


src/transaction.rs line 965 at r2 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

u64 is enough, but gas_price must be u128 and @dorimedini-starkware set gas to u128 because we need to multiple them.

I see that other fields are not consistent with the blockifier as well.
Whatever you choose

GasPrice is u128

@yoavGrs yoavGrs merged commit 059376a into main Jun 4, 2024
6 checks passed
@yoavGrs yoavGrs deleted the yoav/block_hash/add_l1_gas_consumed branch June 4, 2024 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants