build(blockifier): add l2_gas_price and refactor

[aner-starkware]

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 63.10680% with 38 lines in your changes missing coverage. Please review.

Project coverage is 76.61%. Comparing base (13ae058) to head (61ba588).
Report is 93 commits behind head on main.

Files	Patch %	Lines
crates/native_blockifier/src/py_state_diff.rs	0.00%	20 Missing and 8 partials ⚠️
crates/blockifier/src/blockifier/block.rs	87.17%	5 Missing ⚠️
crates/gateway/src/rpc_objects.rs	20.00%	0 Missing and 4 partials ⚠️
crates/papyrus_execution/src/objects.rs	92.30%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #322      +/-   ##
==========================================
- Coverage   76.62%   76.61%   -0.01%     
==========================================
  Files         346      348       +2     
  Lines       36263    36339      +76     
  Branches    36263    36339      +76     
==========================================
+ Hits        27786    27841      +55     
- Misses       6175     6195      +20     
- Partials     2302     2303       +1     

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

[dorimedini-starkware]

Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @aner-starkware)

---

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

#[path = "block_test.rs"]
pub mod block_test;
pub const L2_GAS_FOR_CAIRO_STEP: u128 = 100;

I think you should take this value from here - it should be part of the versioned constants.

Code quote:

L2_GAS_FOR_CAIRO_STEP: u128 = 100;

---

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

pub mod block_test;
pub const L2_GAS_FOR_CAIRO_STEP: u128 = 100;
pub const CAIRO_STEPS_PER_L1_GAS: u128 = 400;

this value should be taken from vm_resource_fee_cost, see here

Code quote:

CAIRO_STEPS_PER_L1_GAS: u128 = 400;

---

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

pub const L2_GAS_FOR_CAIRO_STEP: u128 = 100;
pub const CAIRO_STEPS_PER_L1_GAS: u128 = 400;
pub const L2_TO_L1_GAS_PRICE_RATIO: u128 = L2_GAS_FOR_CAIRO_STEP * CAIRO_STEPS_PER_L1_GAS;

shouldn't this be a ratio type?

Code quote:

L2_TO_L1_GAS_PRICE_RATIO

---

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

pub const L2_TO_L1_GAS_PRICE_RATIO: u128 = L2_GAS_FOR_CAIRO_STEP * CAIRO_STEPS_PER_L1_GAS;

pub type L2Cost = Ratio<u128>;

no need to redefine the ResourceCost type

Code quote:

pub type L2Cost = Ratio<u128>;

---

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

    strk_l1_data_gas_price: NonZeroU128, // In fri.
    eth_l2_gas_price: L2Cost,            // In wei.
    strk_l2_gas_price: L2Cost,           // In fri.

Suggestion:

    eth_l2_gas_price: ResourceCost,            // In wei.
    strk_l2_gas_price: ResourceCost,           // In fri.

---

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

        }
    }
    pub fn get_gas_price_by_fee_type(&self, fee_type: &FeeType) -> NonZeroU128 {

newline between function implementations

Suggestion:

    }
    
    pub fn get_gas_price_by_fee_type(&self, fee_type: &FeeType) -> NonZeroU128 {

---

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

    }

    pub fn get_data_gas_price_by_fee_type(&self, fee_type: &FeeType) -> NonZeroU128 {

these should be renamed now (in a separate PR)

Suggestion:

    pub fn get_l1_gas_price_by_fee_type(&self, fee_type: &FeeType) -> NonZeroU128 {
        match fee_type {
            FeeType::Strk => self.strk_l1_gas_price,
            FeeType::Eth => self.eth_l1_gas_price,
        }
    }

    pub fn get_l1_data_gas_price_by_fee_type(&self, fee_type: &FeeType) -> NonZeroU128 {

---

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

    #[case] expect_failure: bool,
) {
    use crate::blockifier::block::GasPrices;

move import to top

Code quote:

use crate::blockifier::block::GasPrices;

---

crates/blockifier/src/test_utils/struct_impls.rs line 186 at r1 (raw file):

    }

    pub fn create_for_testing_w_strk_gas_prices(

rename this function... not sure to what, but (a) please don't use single-letter abbreviations and (b) what does strk have to do with this function? both eth and strk appear in the inputs.

better yet, delete this function completely - instead of passing None just use new and pass the defaults explicitly

Code quote:

create_for_testing_w_strk_gas_prices

---

crates/papyrus_execution/src/objects.rs line 164 at r1 (raw file):

        PriceUnit::Wei => FeeType::Eth,
        PriceUnit::Fri => FeeType::Strk,
    };

what is PriceUnit?
how about adding impl From<PriceUnit> for FeeType? seems natural, and removes the need for this preceding match (just do price_unit.into())

Code quote:

    let fee_type = match tx_execution_output.price_unit {
        PriceUnit::Wei => FeeType::Eth,
        PriceUnit::Fri => FeeType::Strk,
    };

[dorimedini-starkware]

Reviewed 5 of 14 files at r3, 9 of 9 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @aner-starkware)

[aner-starkware]

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

---

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

Previously, dorimedini-starkware wrote…

shouldn't this be a ratio type?

How can I create a const ratio type?

---

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

Previously, dorimedini-starkware wrote…

no need to redefine the ResourceCost type

Done.

---

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

    strk_l1_data_gas_price: NonZeroU128, // In fri.
    eth_l2_gas_price: L2Cost,            // In wei.
    strk_l2_gas_price: L2Cost,           // In fri.

Done.

---

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

Previously, dorimedini-starkware wrote…

newline between function implementations

Done.

---

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

Previously, dorimedini-starkware wrote…

these should be renamed now (in a separate PR)

Done.

---

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

Previously, dorimedini-starkware wrote…

move import to top

Done.

---

crates/blockifier/src/test_utils/struct_impls.rs line 186 at r1 (raw file):

Previously, dorimedini-starkware wrote…

rename this function... not sure to what, but (a) please don't use single-letter abbreviations and (b) what does strk have to do with this function? both eth and strk appear in the inputs.

better yet, delete this function completely - instead of passing None just use new and pass the defaults explicitly

Done.

---

crates/papyrus_execution/src/objects.rs line 164 at r1 (raw file):

Previously, dorimedini-starkware wrote…

what is PriceUnit?
how about adding impl From<PriceUnit> for FeeType? seems natural, and removes the need for this preceding match (just do price_unit.into())

Done.

[dorimedini-starkware]

Reviewed 5 of 5 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @aner-starkware)

---

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

Previously, aner-starkware wrote…

How can I create a const ratio type?

hmmm... maybe the type of the const should be [u128; 2], and you should convert it to a Ratio<u128> at runtime?

[aner-starkware]

Reviewable status: 6 of 15 files reviewed, 3 unresolved discussions (waiting on @amosStarkware and @dorimedini-starkware)

---

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

Previously, dorimedini-starkware wrote…

I think you should take this value from here - it should be part of the versioned constants.

Done. Now using a function (until next PR, where it's changed to receiving it from python)

---

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

Previously, dorimedini-starkware wrote…

this value should be taken from vm_resource_fee_cost, see here

Done.

---

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

Previously, dorimedini-starkware wrote…

hmmm... maybe the type of the const should be [u128; 2], and you should convert it to a Ratio<u128> at runtime?

I guess if it's taken from the file, it can't be a const.

[dorimedini-starkware]

Reviewed 7 of 7 files at r6, 2 of 2 files at r9, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @amosStarkware and @aner-starkware)

---

crates/blockifier/src/versioned_constants.rs line 130 at r9 (raw file):

    pub fn l1_to_l2_gas_price_conversion(l1_gas_price: u128) -> u128 {
        let versioned_constants = Self::latest_constants();
        let l1_to_l2_gas_price_ratio: Ratio<u128> = Ratio::new(

don't get the latest constants: call this as a method please.
the actual versioned constants used at runtime should be configurable

Suggestion:

    /// Converts from l1 gas cost to l2 gas cost with **upward rounding**
    pub fn l1_to_l2_gas_price_conversion(&self, l1_gas_price: u128) -> u128 {
        let l1_to_l2_gas_price_ratio: Ratio<u128> = Ratio::new(

---

crates/blockifier/src/versioned_constants.rs line 134 at r9 (raw file):

            std::convert::Into::<u128>::into(
                versioned_constants.os_constants.gas_costs.step_gas_cost,
            ),

isn't this equivalent?
or just .into() without the u128::from(...)?

Suggestion:

            u128::from(versioned_constants.os_constants.gas_costs.step_gas_cost),

---

crates/blockifier/src/versioned_constants.rs line 138 at r9 (raw file):

            ["n_steps"];
        (l1_gas_price * l1_to_l2_gas_price_ratio.numer())
            .div_ceil(*l1_to_l2_gas_price_ratio.denom())

doesn't Ratio support multiplication by u128? and has it's own "round up" method? I don't think you need to call numer and denom explicitly here

Code quote:

        (l1_gas_price * l1_to_l2_gas_price_ratio.numer())
            .div_ceil(*l1_to_l2_gas_price_ratio.denom())

---

crates/blockifier/src/blockifier/block.rs line 49 at r9 (raw file):

            eth_l1_gas_price.into(),
        ))
        .unwrap();

no unwrap in business logic; use expect if you need to

Code quote:

.unwrap();

---

crates/blockifier/src/blockifier/block.rs line 53 at r9 (raw file):

            VersionedConstants::l1_to_l2_gas_price_conversion(strk_l1_gas_price.into()),
        )
        .unwrap();

the problem with this is that you implicitly assume the "latest" versioned constants are used; we shouldn't assume this.
either get the versioned constants as input, or get the L2 gas price(s) as explicit input..

Code quote:

    pub fn new(
        eth_l1_gas_price: NonZeroU128,
        strk_l1_gas_price: NonZeroU128,
        eth_l1_data_gas_price: NonZeroU128,
        strk_l1_data_gas_price: NonZeroU128,
    ) -> Self {
        let eth_l2_gas_price = NonZeroU128::new(VersionedConstants::l1_to_l2_gas_price_conversion(
            eth_l1_gas_price.into(),
        ))
        .unwrap();
        let strk_l2_gas_price = NonZeroU128::new(
            VersionedConstants::l1_to_l2_gas_price_conversion(strk_l1_gas_price.into()),
        )
        .unwrap();

---

crates/blockifier/src/blockifier/block.rs line 53 at r9 (raw file):

            VersionedConstants::l1_to_l2_gas_price_conversion(strk_l1_gas_price.into()),
        )
        .unwrap();

no unwrap in business logic; use expect if you need to

Code quote:

.unwrap();

[aner-starkware]

Reviewable status: 13 of 15 files reviewed, 6 unresolved discussions (waiting on @amosStarkware and @dorimedini-starkware)

---

crates/blockifier/src/versioned_constants.rs line 130 at r9 (raw file):

Previously, dorimedini-starkware wrote…

don't get the latest constants: call this as a method please.
the actual versioned constants used at runtime should be configurable

Done.

---

crates/blockifier/src/versioned_constants.rs line 134 at r9 (raw file):

Previously, dorimedini-starkware wrote…

isn't this equivalent?
or just .into() without the u128::from(...)?

.into() didn't work for some reason, so I took the compiler's suggestion. u128::from does work, thanks

---

crates/blockifier/src/versioned_constants.rs line 138 at r9 (raw file):

Previously, dorimedini-starkware wrote…

doesn't Ratio support multiplication by u128? and has it's own "round up" method? I don't think you need to call numer and denom explicitly here

Done.

---

crates/blockifier/src/blockifier/block.rs line 49 at r9 (raw file):

Previously, dorimedini-starkware wrote…

no unwrap in business logic; use expect if you need to

Done.

---

crates/blockifier/src/blockifier/block.rs line 53 at r9 (raw file):

Previously, dorimedini-starkware wrote…

no unwrap in business logic; use expect if you need to

Done.

---

crates/blockifier/src/blockifier/block.rs line 53 at r9 (raw file):

Previously, dorimedini-starkware wrote…

the problem with this is that you implicitly assume the "latest" versioned constants are used; we shouldn't assume this.
either get the versioned constants as input, or get the L2 gas price(s) as explicit input..

It's already changed in the next PR; added a TODO and changed how we get versioned constants.

[dorimedini-starkware]

Reviewed 2 of 2 files at r10, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @amosStarkware and @aner-starkware)

---

crates/blockifier/src/versioned_constants.rs line 132 at r10 (raw file):

            Ratio::new(1, u128::from(self.os_constants.gas_costs.step_gas_cost))
                * self.vm_resource_fee_cost()["n_steps"];
        *(l1_to_l2_gas_price_ratio * l1_gas_price).ceil().numer()

Q: ceil() doesn't return an integer? it returns a Ratio? is it guaranteed to have denom() == 1?

Code quote:

.ceil().numer()

[dorimedini-starkware]

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

[aner-starkware]

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

---

crates/blockifier/src/versioned_constants.rs line 132 at r10 (raw file):

Previously, dorimedini-starkware wrote…

Q: ceil() doesn't return an integer? it returns a Ratio? is it guaranteed to have denom() == 1?

Seems so - it uses from_ingteger, which has the following implementation:

/// Creates a `Ratio` representing the integer `t`.
    #[inline]
    pub fn from_integer(t: T) -> Ratio<T> {
        Ratio::new_raw(t, One::one())
    }

[dorimedini-starkware]

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

[TzahiTaub]

Reviewed 4 of 9 files at r4, 6 of 7 files at r6, 1 of 2 files at r10, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @amosStarkware)