feat(starknet_api, protobuf): add l2_gas_price to block header

[TzahiTaub]

This change is 

[TzahiTaub]

• feat(mempool): add l2_gas_price support in BlockHeader #652
• feat(execution): add l2_gas_price to FeeEstimation #644
• chore(execution): add l2_gas_price to PendingData #643
• feat(starknet_api, protobuf): add l2_gas_price to block header #641 👈
• main

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

Join @TzahiTaub and the rest of your teammates on Graphite

[codecov]

Codecov Report

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

Project coverage is 59.52%. Comparing base (8aa893b) to head (18c835a).
Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/papyrus_protobuf/src/converters/header.rs	88.88%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #641       +/-   ##
===========================================
- Coverage   74.44%   59.52%   -14.93%     
===========================================
  Files         364       77      -287     
  Lines       38395     9969    -28426     
  Branches    38395     9969    -28426     
===========================================
- Hits        28583     5934    -22649     
+ Misses       7493     3137     -4356     
+ Partials     2319      898     -1421     

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

[TzahiTaub]

Reviewable status: 0 of 16 files reviewed, 1 unresolved discussion

---

crates/papyrus_storage/src/header.rs line 74 at r1 (raw file):

    pub l1_gas_price: GasPricePerToken,
    pub l1_data_gas_price: GasPricePerToken,
    pub l2_gas_price: GasPricePerToken,

@DvirYo-starkware note the change of storage struct - please point me to the version change needed as discussed.

[TzahiTaub]

Reviewable status: 0 of 16 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware, @DvirYo-starkware, and @ShahakShama)

---

crates/starknet_client/src/reader/objects/block.rs line 62 at r1 (raw file):

    pub l1_data_gas_price: GasPricePerToken,
    // New field in V0.13.3.
    #[serde(default)]

ATM, default value of 0. Might need adjustmens as a result of the discussion in here.

Code quote:

    #[serde(default)]

[dorimedini-starkware]

Reviewed 2 of 16 files at r1, all commit messages.
Reviewable status: 2 of 16 files reviewed, 2 unresolved discussions (waiting on @DvirYo-starkware and @ShahakShama)

[DvirYo-starkware]

Reviewed 12 of 16 files at r1.
Reviewable status: 14 of 16 files reviewed, 9 unresolved discussions (waiting on @ShahakShama and @TzahiTaub)

a discussion (no related file):
About the changes in the files in papyrus_common:

1. The blocks there are real blocks from mainet. we can't just add a field
2. If the new field will be included in the block hash calculation, the calculation should be changed, and the test that calculates the block hash should also be changed

---

---

crates/papyrus_common/resources/block_hash.json line 14 at r1 (raw file):

      "price_in_wei": "0x1"
    },
    "l2_gas_price": {

X

---

crates/papyrus_common/resources/deprecated_block_hash_v0.json line 14 at r1 (raw file):

      "price_in_wei": "0x1"
    },
    "l2_gas_price": {

X

---

crates/papyrus_common/resources/deprecated_block_hash_v1.json line 14 at r1 (raw file):

      "price_in_wei": "0x1"
    },
    "l2_gas_price": {

X

---

crates/papyrus_common/resources/deprecated_block_hash_v1_no_events.json line 14 at r1 (raw file):

      "price_in_wei": "0x1"
    },
    "l2_gas_price": {

X

---

crates/papyrus_common/resources/deprecated_block_hash_v2.json line 14 at r1 (raw file):

      "price_in_wei": "0x1"
    },
    "l2_gas_price": {

X

---

crates/papyrus_storage/src/header.rs line 74 at r1 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

@DvirYo-starkware note the change of storage struct - please point me to the version change needed as discussed.

I will do that in a different PR. If you change something in objects written to the Papyrus database, notify someone from the Papyrus team.
What should be changed is the major versions here:
https://github.com/starkware-libs/sequencer/blob/ac2838a6ffc8b8d0a5f43eeda5872278fc4a1931/crates/papyrus_storage/src/lib.rs#L156C1-L160C76
This will prevent the old database from opening with new code.

---

crates/starknet_client/src/reader/objects/block_test.rs line 27 at r1 (raw file):

#[test]
fn load_block_succeeds() {
    // TODO(Tzahi): Replace block_post_0_13_3 (copied from 0_13_2) with live data once available.

If you know that the schema of new blocks will change, add those changes manually, i.e., add the l2_get_price field to the block.

[TzahiTaub]

Reviewable status: 14 of 16 files reviewed, 9 unresolved discussions (waiting on @DvirYo-starkware and @ShahakShama)

a discussion (no related file):

Previously, DvirYo-starkware wrote…

About the changes in the files in papyrus_common:

1. The blocks there are real blocks from mainet. we can't just add a field
2. If the new field will be included in the block hash calculation, the calculation should be changed, and the test that calculates the block hash should also be changed

Why not change them manually and fix the hash when needed? We won't have real blocks with l2_gas_price until the code will be in... And unless there's a really good reason, I don't understand why we need to keep the "real blocks that were created on chain" thing 100% true.

---

[DvirYo-starkware]

Reviewable status: 14 of 16 files reviewed, 9 unresolved discussions (waiting on @ShahakShama and @TzahiTaub)

a discussion (no related file):

Previously, TzahiTaub (Tzahi) wrote…

Why not change them manually and fix the hash when needed? We won't have real blocks with l2_gas_price until the code will be in... And unless there's a really good reason, I don't understand why we need to keep the "real blocks that were created on chain" thing 100% true.

1. I agree that the rule of real blocks is not 100% mandatory, but it is what has been decided. The advantage of that is that we know for sure that the block's hash calculation is what it should be, and we can always verify this information in the future again against the FGW.
2. Because the L2_gas_price field should not affect the hash of old blocks, if you know for sure if and how this field will be present in old blocks, you can add it. But, I found it very hard to believe that old blocks will retroactively get a 0x2 value in this field.

---

[DvirYo-starkware]

Reviewable status: 14 of 16 files reviewed, 9 unresolved discussions (waiting on @ShahakShama and @TzahiTaub)

a discussion (no related file):

Previously, DvirYo-starkware wrote…

1. I agree that the rule of real blocks is not 100% mandatory, but it is what has been decided. The advantage of that is that we know for sure that the block's hash calculation is what it should be, and we can always verify this information in the future again against the FGW.
2. Because the L2_gas_price field should not affect the hash of old blocks, if you know for sure if and how this field will be present in old blocks, you can add it. But, I found it very hard to believe that old blocks will retroactively get a 0x2 value in this field.

In addition, the 0x2 value does not fit with what you implement in the reader client

---

[dorimedini-starkware]

Reviewable status: 14 of 16 files reviewed, 10 unresolved discussions (waiting on @ShahakShama and @TzahiTaub)

---

crates/papyrus_protobuf/src/proto/p2p/proto/header.proto line 25 at r1 (raw file):

    Uint128 data_gas_price_wei = 15;
    optional Uint128 l2_gas_price_fri = 18;  // Added on v0.13.3.
    optional Uint128 l2_gas_price_wei = 19;  // Added on v0.13.3.

does this need to be moved down? @ShahakShama

Code quote:

    optional Uint128 l2_gas_price_fri = 18;  // Added on v0.13.3.
    optional Uint128 l2_gas_price_wei = 19;  // Added on v0.13.3.

[dorimedini-starkware]

Reviewed 1 of 16 files at r1.
Reviewable status: 15 of 16 files reviewed, 10 unresolved discussions (waiting on @ShahakShama and @TzahiTaub)

[TzahiTaub]

Reviewable status: 9 of 16 files reviewed, 8 unresolved discussions (waiting on @DvirYo-starkware and @ShahakShama)

a discussion (no related file):

Previously, DvirYo-starkware wrote…

In addition, the 0x2 value does not fit with what you implement in the reader client

I've changed the value to 0 like it should be old blocks. I'm not sure what should be maintained here, according to @ShahakShama, the hash computation in papyrus_common is deprecated and can probably be removed. And so are this test files, only used for validating the deprecated hash computation. Hashes are now computed via the starknet_api crate (and the hash computation in papyrus_common is not called), so I don't think we need to worry about the hash mismatch in the new edited jsons, and they can probably be removed entirely.

---

---

crates/papyrus_common/resources/block_hash.json line 14 at r1 (raw file):

Previously, DvirYo-starkware wrote…

X

Done.

---

crates/papyrus_common/resources/deprecated_block_hash_v0.json line 14 at r1 (raw file):

Previously, DvirYo-starkware wrote…

X

Done.

---

crates/papyrus_common/resources/deprecated_block_hash_v1.json line 14 at r1 (raw file):

Previously, DvirYo-starkware wrote…

X

Done.

---

crates/papyrus_common/resources/deprecated_block_hash_v1_no_events.json line 14 at r1 (raw file):

Previously, DvirYo-starkware wrote…

X

Done.

---

crates/papyrus_common/resources/deprecated_block_hash_v2.json line 14 at r1 (raw file):

Previously, DvirYo-starkware wrote…

X

Done.

---

crates/papyrus_protobuf/src/proto/p2p/proto/header.proto line 25 at r1 (raw file):

Previously, dorimedini-starkware wrote…

does this need to be moved down? @ShahakShama

I preferred the prices to be grouped by context over keeping the indices increasing visually.

---

crates/starknet_client/src/reader/objects/block_test.rs line 27 at r1 (raw file):

Previously, DvirYo-starkware wrote…

If you know that the schema of new blocks will change, add those changes manually, i.e., add the l2_get_price field to the block.

I manually changed it. When real data is live, I'd like to replace the manually edited file with such data.

[dorimedini-starkware]

Reviewable status: 9 of 16 files reviewed, 8 unresolved discussions (waiting on @DvirYo-starkware and @ShahakShama)

---

crates/papyrus_protobuf/src/proto/p2p/proto/header.proto line 25 at r1 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

I preferred the prices to be grouped by context over keeping the indices increasing visually.

the order may have meaning here, I want shahak's input

[dorimedini-starkware]

Reviewed 1 of 16 files at r1, 5 of 6 files at r2, all commit messages.
Reviewable status: 15 of 16 files reviewed, 9 unresolved discussions (waiting on @DvirYo-starkware, @ShahakShama, and @TzahiTaub)

---

crates/papyrus_protobuf/src/converters/header.rs line 0 at r2 (raw file):
is the block hash computation of old block backward-compatible? we shouldn't be hashing extra zeros

[DvirYo-starkware]

Reviewed 1 of 6 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ShahakShama and @TzahiTaub)

---

crates/starknet_client/src/reader/objects/block.rs line 62 at r2 (raw file):

    pub l1_data_gas_price: GasPricePerToken,
    // New field in V0.13.3.
    #[serde(default)]

If I understand this PR correctly, this field will be retroactively present in old blocks. If this is the case, you can remove the #[serde(default)].

Code quote:

#[serde(default)]

[ShahakShama]

crates/starknet_client/src/reader/objects/block.rs line 62 at r2 (raw file):

Previously, DvirYo-starkware wrote…

If I understand this PR correctly, this field will be retroactively present in old blocks. If this is the case, you can remove the #[serde(default)].

No, because we want the client to support 0.13.2 and 0.13.3 during the time period when 0.13.3 is in integration but not in mainnet

[ShahakShama]

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

---

crates/papyrus_protobuf/src/converters/header.rs line at r2 (raw file):

Previously, dorimedini-starkware wrote…

is the block hash computation of old block backward-compatible? we shouldn't be hashing extra zeros

Is your comment on the wrong file?

---

crates/papyrus_protobuf/src/proto/p2p/proto/header.proto line 25 at r1 (raw file):

Previously, dorimedini-starkware wrote…

the order may have meaning here, I want shahak's input

Move it down, and add a TODO on me to move it back up and change the numbering to be grouped by context once we insert l2 gas fields to the p2p specs

---

crates/papyrus_storage/src/header.rs line 74 at r2 (raw file):

    pub l1_gas_price: GasPricePerToken,
    pub l1_data_gas_price: GasPricePerToken,
    pub l2_gas_price: GasPricePerToken,

@dan-starkware @DvirYo-starkware Do we need to increase the storage version for this or was the version broken recently and we can do breaking changes without increasing the version?

---

crates/starknet_api/src/block_hash/block_hash_calculator_test.rs line 76 at r2 (raw file):

            price_in_wei: GasPrice(9),
        },
        l2_gas_price: GasPricePerToken { price_in_fri: GasPrice(1), price_in_wei: GasPrice(1) },

I think @yoavGrs should review this, right?

---

crates/starknet_client/src/reader/objects/block_test.rs line 27 at r2 (raw file):

#[test]
fn load_block_succeeds() {
    // TODO(Tzahi): Replace block_post_0_13_3 (copied from 0_13_2) with live data once available.

copied from 0_13_2 -> copied from 0_13_2 and added additional fields

[dorimedini-starkware]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @DvirYo-starkware, @ShahakShama, and @TzahiTaub)

---

crates/papyrus_protobuf/src/converters/header.rs line at r2 (raw file):

Previously, ShahakShama wrote…

Is your comment on the wrong file?

referring to the change in impl From<(BlockHeader, Vec<BlockSignature>)> for protobuf::SignedBlockHeader, here below

[DvirYo-starkware]

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

---

crates/papyrus_storage/src/header.rs line 74 at r2 (raw file):

Previously, ShahakShama wrote…

@dan-starkware @DvirYo-starkware Do we need to increase the storage version for this or was the version broken recently and we can do breaking changes without increasing the version?

We will break the storage in 13.3. There is already a PR that does that

---

crates/starknet_client/src/reader/objects/block.rs line 62 at r2 (raw file):

Previously, ShahakShama wrote…

No, because we want the client to support 0.13.2 and 0.13.3 during the time period when 0.13.3 is in integration but not in mainnet

I don't see a reason for that when Papyrus version is coupled with Starknet version.

[yoavGrs]

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

---

crates/starknet_api/src/block_hash/block_hash_calculator_test.rs line 136 at r2 (raw file):

            parent_hash,
            block_number,
            l1_gas_price,

Consider adding a TODO for testing l2_gas_price

[DvirYo-starkware]

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

---

crates/starknet_client/src/reader/objects/block.rs line 62 at r2 (raw file):

Previously, DvirYo-starkware wrote…

I don't see a reason for that when Papyrus version is coupled with Starknet version.

I talked with Dan; we need this quality for syncing backup.

[yoavGrs]

Reviewable status: 14 of 16 files reviewed, 6 unresolved discussions (waiting on @dan-starkware, @dorimedini-starkware, @ShahakShama, and @TzahiTaub)

---

crates/papyrus_protobuf/src/converters/header.rs line at r2 (raw file):

Previously, dorimedini-starkware wrote…

referring to the change in impl From<(BlockHeader, Vec<BlockSignature>)> for protobuf::SignedBlockHeader, here below

This PR doesn't change the block hash at all.

---

crates/papyrus_protobuf/src/proto/p2p/proto/header.proto line 25 at r1 (raw file):

Previously, ShahakShama wrote…

Move it down, and add a TODO on me to move it back up and change the numbering to be grouped by context once we insert l2 gas fields to the p2p specs

Done.

---

crates/starknet_api/src/block_hash/block_hash_calculator_test.rs line 76 at r2 (raw file):

Previously, ShahakShama wrote…

I think @yoavGrs should review this, right?

I take these PRs

[ShahakShama]

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

---

crates/papyrus_protobuf/src/converters/header.rs line at r2 (raw file):

Previously, yoavGrs wrote…

This PR doesn't change the block hash at all.

@yoavGrs is right, but we need to add l2_gas to the header hash at some point in the future

[dorimedini-starkware]

Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @TzahiTaub)

[yoavGrs]

Reviewable status: 13 of 16 files reviewed, 2 unresolved discussions (waiting on @DvirYo-starkware and @TzahiTaub)

---

crates/papyrus_protobuf/src/converters/header.rs line at r2 (raw file):

Previously, ShahakShama wrote…

@yoavGrs is right, but we need to add l2_gas to the header hash at some point in the future

https://reviewable.io/reviews/starkware-libs/sequencer/683

[dorimedini-starkware]

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @TzahiTaub)

[yoavGrs]

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

---

crates/starknet_client/src/reader/objects/block_test.rs line 27 at r2 (raw file):

Previously, ShahakShama wrote…

copied from 0_13_2 -> copied from 0_13_2 and added additional fields

Done.

[ShahakShama]

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

[dorimedini-starkware]

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

[dorimedini-starkware]

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)

[yoavGrs]

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

[dorimedini-starkware]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)

[yoavGrs]

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

Approved, reviewable bug didn't write it in the right location