feat(mempool): impl calculate-gas-price according to EIP 1559

[MohammadNassar1]

This change is 

[MohammadNassar1]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @elintul)

a discussion (no related file):
This PR implements the calculation of gap price according to EIP 1559.
High level explanation of EIP 1559:
https://eips.ethereum.org/EIPS/eip-1559

---

[giladchase]

Can you plz describe a flow that will use this function?

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @MohammadNassar1)

---

crates/blockifier/src/blockifier/block.rs line 32 at r1 (raw file):

    ///
    /// # Parameters
    /// - `current_price`: The base fee of the current block.

Question: is this really the current price? isn't it the previous base price, whereas the current price is the result of this block? Maybe it's not clear to me exactly when this should be called...

Code quote:

    /// - `current_price`: The base fee of the current block.

---

crates/blockifier/src/blockifier/block.rs line 38 at r1 (raw file):

    /// # Returns
    /// The price for the next block.
    pub fn calculate_gas_price(

Suggestion:

    pub fn calculate_estimated_gas_price(

---

crates/blockifier/src/blockifier/block.rs line 38 at r1 (raw file):

    /// # Returns
    /// The price for the next block.
    pub fn calculate_gas_price(

Make into a free function, this doesn't use BlockInfo.
Keeping this as a free function inside a block makes sense.

---

crates/blockifier/src/blockifier/block.rs line 43 at r1 (raw file):

        current_gas_target: u64,
    ) -> u64 {
        const BASE_FEE_MAX_CHANGE_DENOMINATOR: u64 = 2;

add docstring to explain this, perhaps describe it as a "sensitivity parameter" and mention that it limits the max rate of change between consecutive blocks.
Also why are we using 2 and not 8 like the eip? is that a requirement?

Code quote:

       const BASE_FEE_MAX_CHANGE_DENOMINATOR: u64 = 2;

[elintul]

Reviewed all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ayeletstarkware, @giladchase, and @MohammadNassar1)

---

crates/blockifier/src/blockifier/block.rs line 32 at r1 (raw file):

Previously, giladchase wrote…

Question: is this really the current price? isn't it the previous base price, whereas the current price is the result of this block? Maybe it's not clear to me exactly when this should be called...

Let's go with no prefix for input, and "next" for the output.

---

crates/blockifier/src/blockifier/block.rs line 38 at r1 (raw file):

    /// # Returns
    /// The price for the next block.
    pub fn calculate_gas_price(

Maybe calculate_next_base_gas_price?

---

crates/blockifier/src/blockifier/block.rs line 38 at r1 (raw file):

Previously, giladchase wrote…

Make into a free function, this doesn't use BlockInfo.
Keeping this as a free function inside a block makes sense.

I'm actually not sure this is the right location, it's related to BlockContext creation. Also, it might be an independent logic that might be invoked from code that isn't related to execuion.

---

crates/blockifier/src/blockifier/block.rs line 43 at r1 (raw file):

        current_gas_target: u64,
    ) -> u64 {
        const BASE_FEE_MAX_CHANGE_DENOMINATOR: u64 = 2;

Should this number be configurable?

Code quote:

const BASE_FEE_MAX_CHANGE_DENOMINATOR: u64 = 2;

---

crates/blockifier/src/blockifier/block.rs line 44 at r1 (raw file):

    ) -> u64 {
        const BASE_FEE_MAX_CHANGE_DENOMINATOR: u64 = 2;

Are we okay with this integer division?

[elintul]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ayeletstarkware, @giladchase, and @MohammadNassar1)

---

crates/blockifier/src/blockifier/block.rs line 44 at r1 (raw file):

Previously, elintul (Elin) wrote…

Are we okay with this integer division?

Maybe we should work with floats and only round in the end?

---

crates/blockifier/src/blockifier/block.rs line 54 at r1 (raw file):

        } else {
            current_price - price_change
        }

WDYT about calculating the adjustment factor, and returning base_price * adjustment_factor?

[elintul]

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ayeletstarkware, @giladchase, and @MohammadNassar1)

[MohammadNassar1]

Reviewable status: 0 of 1 files reviewed, 7 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)

---

crates/blockifier/src/blockifier/block.rs line 32 at r1 (raw file):

Previously, elintul (Elin) wrote…

Let's go with no prefix for input, and "next" for the output.

Done

---

crates/blockifier/src/blockifier/block.rs line 38 at r1 (raw file):

Previously, elintul (Elin) wrote…

Maybe calculate_next_base_gas_price?

Done.

---

crates/blockifier/src/blockifier/block.rs line 43 at r1 (raw file):

Previously, giladchase wrote…

add docstring to explain this, perhaps describe it as a "sensitivity parameter" and mention that it limits the max rate of change between consecutive blocks.
Also why are we using 2 and not 8 like the eip? is that a requirement?

Product should provide the required value, added a todo.

---

crates/blockifier/src/blockifier/block.rs line 43 at r1 (raw file):

Previously, elintul (Elin) wrote…

Should this number be configurable?

wdym?

---

crates/blockifier/src/blockifier/block.rs line 44 at r1 (raw file):

Previously, elintul (Elin) wrote…

Maybe we should work with floats and only round in the end?

It appears that we do not. Added a docstring, which explains why.

[MohammadNassar1]

Reviewable status: 0 of 1 files reviewed, 7 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)

---

crates/blockifier/src/blockifier/block.rs line 38 at r1 (raw file):

Previously, elintul (Elin) wrote…

I'm actually not sure this is the right location, it's related to BlockContext creation. Also, it might be an independent logic that might be invoked from code that isn't related to execuion.

"As I understand, this should ideally be part of the consensus unit, which we currently do not have. Therefore, I've added it to the blockifier. I believe that BlockInfo is an appropriate place for this, as it contains relevant information about the block. wdyt?

---

crates/blockifier/src/blockifier/block.rs line 54 at r1 (raw file):

Previously, elintul (Elin) wrote…

WDYT about calculating the adjustment factor, and returning base_price * adjustment_factor?

In any case, we still need to retrieve the old price.
Where is the suggested returned value used? I believe it's only utilized after updating to the next price

[MohammadNassar1]

Sure.
As part of block creation, the blockifier calculates the gas used. To manage network congestion, the gas price is adjusted based on the current congestion level.
At the end of the block creation round, the consensus unit (or possibly the blockifier) needs to calculate the gas price for the next block using this function. This updated gas price is then sent to other components, such as other blockifiers, mempools, gateways, etc.

Reviewable status: 0 of 1 files reviewed, 7 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)

[giladchase]

Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @MohammadNassar1)

---

crates/blockifier/src/blockifier/block.rs line 43 at r1 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Product should provide the required value, added a todo.

I think we can check with product on this value now instead of using a TODO. Should be a quick question

---

crates/blockifier/src/blockifier/block.rs line 43 at r1 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

wdym?

Through a config, rather than as a const, i'm leaning towards const but perhaps we should ask product to be sure.

---

crates/blockifier/src/blockifier/block.rs line 55 at r2 (raw file):

        // Calculate the gas change as u128 to handle potential overflow during multiplication.
        let gas = price_u128.checked_mul(gas_delta_u128).unwrap();
        let price_change_u128 = gas / gas_target_u128 / BASE_FEE_MAX_CHANGE_DENOMINATOR;

Add something to ward off anyone from switching the order of operations, specifically that div must happen after mul and why

Code quote:

        // Calculate the gas change as u128 to handle potential overflow during multiplication.
        let gas = price_u128.checked_mul(gas_delta_u128).unwrap();
        let price_change_u128 = gas / gas_target_u128 / BASE_FEE_MAX_CHANGE_DENOMINATOR;

[MohammadNassar1]

Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)

---

crates/blockifier/src/blockifier/block.rs line 43 at r1 (raw file):

Previously, giladchase wrote…

I think we can check with product on this value now instead of using a TODO. Should be a quick question

Asked them in a #mempool-dev channel.

---

crates/blockifier/src/blockifier/block.rs line 43 at r1 (raw file):

Previously, giladchase wrote…

Through a config, rather than as a const, i'm leaning towards const but perhaps we should ask product to be sure.

asked produce

[elintul]

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @ayeletstarkware and @giladchase)

[elintul]

Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @ayeletstarkware, @giladchase, and @MohammadNassar1)

---

crates/blockifier/src/blockifier/block.rs line 43 at r1 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

asked produce

Any updates?

[elintul]

Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @ayeletstarkware, @giladchase, and @MohammadNassar1)

---

crates/blockifier/src/blockifier/block.rs line 43 at r1 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Asked them in a #mempool-dev channel.

Any updates?

[MohammadNassar1]

Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)

---

crates/blockifier/src/blockifier/block.rs line 43 at r1 (raw file):

Previously, elintul (Elin) wrote…

Any updates?

not yet

---

crates/blockifier/src/blockifier/block.rs line 43 at r1 (raw file):

Previously, elintul (Elin) wrote…

Any updates?

not yet

---

crates/blockifier/src/blockifier/block.rs line 55 at r2 (raw file):

Previously, giladchase wrote…

Add something to ward off anyone from switching the order of operations, specifically that div must happen after mul and why

Done.

[giladchase]

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @MohammadNassar1)

---

crates/blockifier/src/blockifier/block.rs line 43 at r1 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

not yet

In favor of resolving, if prod don't know yet then we can keep it at a TODO level.

Only thing is, why did you pick 2 as the default rather than 8, as is in ethereum?

---

crates/blockifier/src/blockifier/block.rs line 55 at r2 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Done.

where? 😅

i meant something like:

let scaled_gas_price = price_u128.checked_mul(gas_delta_u128).unwrap();
// Calculate the price change, maintaining precision by dividing after multiplication.
// This avoids significant precision loss that would occur if dividing before multiplication.
let price_change_u128 = scaled_gas_price / gas_target_u128 / BASE_FEE_MAX_CHANGE_DENOMINATOR;

[elintul]

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @ayeletstarkware, @giladchase, and @MohammadNassar1)

[MohammadNassar1]

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)

a discussion (no related file):
Once you approve this PR, I'll move the method to the fee_market.rs file under the more batch crate.

---

---

crates/blockifier/src/blockifier/block.rs line 55 at r2 (raw file):

Previously, giladchase wrote…

where? 😅

i meant something like:

let scaled_gas_price = price_u128.checked_mul(gas_delta_u128).unwrap();
// Calculate the price change, maintaining precision by dividing after multiplication.
// This avoids significant precision loss that would occur if dividing before multiplication.
let price_change_u128 = scaled_gas_price / gas_target_u128 / BASE_FEE_MAX_CHANGE_DENOMINATOR;

Ooh, I thought you were referring to adding to the docstring.
Do you think I should also include it in the docstring?

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 39 lines in your changes missing coverage. Please review.

Project coverage is 1.62%. Comparing base (72b1115) to head (7bab5d4).
Report is 35 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/batcher/src/fee_market.rs	0.00%	39 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #203       +/-   ##
==========================================
- Coverage   76.63%   1.62%   -75.01%     
==========================================
  Files         353      48      -305     
  Lines       37273    3014    -34259     
  Branches    37273    3014    -34259     
==========================================
- Hits        28563      49    -28514     
+ Misses       6390    2955     -3435     
+ Partials     2320      10     -2310     

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

[giladchase]

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @MohammadNassar1)

---

crates/blockifier/src/blockifier/block.rs line 55 at r2 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Ooh, I thought you were referring to adding to the docstring.
Do you think I should also include it in the docstring?

Nope, code comment suffices i think

[MohammadNassar1]

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)

---

crates/blockifier/src/blockifier/block.rs line 55 at r2 (raw file):

Previously, giladchase wrote…

Nope, code comment suffices i think

Updated.

[elintul]

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 7 unresolved discussions (waiting on @ayeletstarkware, @giladchase, and @MohammadNassar1)

a discussion (no related file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Question: Noam suggests returning the maximum between the calculation result and a minimum gas price value. Should this be integrated into the function's calculation?

Yes.

---

---

crates/blockifier/src/blockifier/block.rs line 38 at r1 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

"As I understand, this should ideally be part of the consensus unit, which we currently do not have. Therefore, I've added it to the blockifier. I believe that BlockInfo is an appropriate place for this, as it contains relevant information about the block. wdyt?

Move to blockifier/fee/fee_market.rs towards merging this.

[giladchase]

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @MohammadNassar1)

---

crates/blockifier/src/blockifier/block.rs line 60 at r7 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Added as a comment after the division.
Let me know what do you think about it.

I remember Noam bringing up specific bounds on these variables, have you spoken with Elin about these bounds and discussed how to incorporate them into the project?

The comment is OK, but it can go stale, especially if the gas_target isn't defined here, but only given as an arg (if the gas_target changes at someplace else, no one will know that this comment got stale).

So I'd like to know what the plan for these consts is before we add the comment, so I'll be able to know how reasonable it is to put it here.

[MohammadNassar1]

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)

---

crates/blockifier/src/blockifier/block.rs line 38 at r1 (raw file):

Previously, elintul (Elin) wrote…

Move to blockifier/fee/fee_market.rs towards merging this.

Once you approve it, I'll move to batcher/fee/fee_market.rs.

[MohammadNassar1]

Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)

a discussion (no related file):

Previously, elintul (Elin) wrote…

Yes.

Done

---

[MohammadNassar1]

Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)

---

crates/blockifier/src/blockifier/block.rs line 60 at r7 (raw file):

Previously, giladchase wrote…

I remember Noam bringing up specific bounds on these variables, have you spoken with Elin about these bounds and discussed how to incorporate them into the project?

The comment is OK, but it can go stale, especially if the gas_target isn't defined here, but only given as an arg (if the gas_target changes at someplace else, no one will know that this comment got stale).

So I'd like to know what the plan for these consts is before we add the comment, so I'll be able to know how reasonable it is to put it here.

I added an assert for a specific target value and another assert for the minimum gas price.

Since these values are bounded, the division will be safe. With the target fixed, the delta is bounded by max_block_size / 2, ensuring that the division remains within safe limits. Therefore, converting back to u64 will also be safe.

[MohammadNassar1]

Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)

---

crates/blockifier/src/blockifier/block.rs line 60 at r7 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

I added an assert for a specific target value and another assert for the minimum gas price.

Since these values are bounded, the division will be safe. With the target fixed, the delta is bounded by max_block_size / 2, ensuring that the division remains within safe limits. Therefore, converting back to u64 will also be safe.

safe = valid

[giladchase]

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @MohammadNassar1)

---

crates/blockifier/src/blockifier/block.rs line 48 at r10 (raw file):

    pub fn calculate_next_base_gas_price(price: u64, gas_used: u64, gas_target: u64) -> u64 {
        assert_eq!(gas_target, MAX_BLOCK_SIZE / 2, "Gas target must be half MAX_BLOCK_SIZE");
        assert!(price >= MIN_GAS_PRICE, "Gas price should be greater than the MIN_GAS_PRICE");

I mentioned this before, assert messages should document assumptions/invariants.
In other words, never translate readable code into words, readable code is better than words.

Both of these bounds had a reason why there chosen, so here is the spot to (succinctly) explain why they were chosen.

Code quote:

        assert_eq!(gas_target, MAX_BLOCK_SIZE / 2, "Gas target must be half MAX_BLOCK_SIZE");
        assert!(price >= MIN_GAS_PRICE, "Gas price should be greater than the MIN_GAS_PRICE");

[MohammadNassar1]

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)

---

crates/blockifier/src/blockifier/block.rs line 48 at r10 (raw file):

Previously, giladchase wrote…

I mentioned this before, assert messages should document assumptions/invariants.
In other words, never translate readable code into words, readable code is better than words.

Both of these bounds had a reason why there chosen, so here is the spot to (succinctly) explain why they were chosen.

I added a comment above it, and tried to add a succinct assertion message.
Let me know what do you think now.

[ayeletstarkware]

Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @elintul, @giladchase, and @MohammadNassar1)

---

crates/blockifier/src/blockifier/block.rs line 24 at r10 (raw file):

// between consecutive blocks.
const GAS_PRICE_MAX_CHANGE_DENOMINATOR: u128 = 48;
const MIN_GAS_PRICE: u64 = 100000; // In fri.

Why is there a minimum gas price?

[MohammadNassar1]

Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)

---

crates/blockifier/src/blockifier/block.rs line 24 at r10 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Why is there a minimum gas price?

We want to avoid too low gas prices; it's not profitable.

[ayeletstarkware]

Reviewed 1 of 1 files at r11, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @elintul and @giladchase)

[MohammadNassar1]

Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)

---

crates/blockifier/src/blockifier/block.rs line 57 at r12 (raw file):

        // To prevent precision loss during multiplication and division, we set a minimum gas price.
        // Additionally, a minimum gas price is established to prevent prolonged periods before the
        // price reaches a higher value.

This was added after Gilad's input in today's meeting.
@giladchase, Could you please confirm if this aligns with what you intended?

Code quote:

        // Additionally, a minimum gas price is established to prevent prolonged periods before the
        // price reaches a higher value.

[giladchase]

Reviewed 1 of 1 files at r13, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ayeletstarkware and @elintul)

[MohammadNassar1]

Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)

a discussion (no related file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Once you approve this PR, I'll move the method to the fee_market.rs file under the more batch crate.

Done

---

[elintul]

Reviewed 2 of 3 files at r14, all commit messages.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @ayeletstarkware, @giladchase, and @MohammadNassar1)

---

crates/batcher/src/fee_market.rs line 44 at r14 (raw file):

    let price_u128 = u128::from(price);
    let gas_delta_u128 = u128::from(gas_delta);
    let gas_target_u128 = u128::from(gas_target);

Can we use map? Can we shadow the previous variables?

Code quote:

    let price_u128 = u128::from(price);
    let gas_delta_u128 = u128::from(gas_delta);
    let gas_target_u128 = u128::from(gas_target);

[MohammadNassar1]

Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)

---

crates/batcher/src/fee_market.rs line 44 at r14 (raw file):

Previously, elintul (Elin) wrote…

Can we use map? Can we shadow the previous variables?

I don't think map is appropriate to use here.

Regarding shadowing the previous parameters: It can be applied to some, such as gas_delta and price_change.
However, it should not be used for price and gas_target since their original u64 values are needed later in the code.

Do you want me to shadow some of the variables?

[giladchase]

Reviewed 3 of 3 files at r14, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @MohammadNassar1)

---

crates/batcher/src/fee_market.rs line 44 at r14 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

I don't think map is appropriate to use here.

Regarding shadowing the previous parameters: It can be applied to some, such as gas_delta and price_change.
However, it should not be used for price and gas_target since their original u64 values are needed later in the code.

Do you want me to shadow some of the variables?

For comparison:

    let [price_u128, gas_delta_u128, gas_target_u128] =
        [price, gas_delta, gas_target].map(u128::from);

I don't feel strongly in either direction on this one

Elin suggested to dismiss her review.

[MohammadNassar1]

Dismissed @elintul from a discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware, @elintul, and @giladchase)