feat(blockifier, starknet_api): add explcit typing and arithmetic to gas amount/price, and fee

[dorimedini-starkware]

This change is 

[dorimedini-starkware]

• feat(blockifier): panic or saturate GasAmount arithmetic depending on 'natural' source of values #1281
• feat(blockifier): panic on GasVector arithmetic overflow #1280
• feat(blockifier): panic or saturate Fee arithmetic depending on 'natural' source of values #1268
• feat(blockifier): safe addition of fee amounts in business logic #1255
• feat(blockifier): event fields converted from u128 to u64 #1250
• feat(starknet_api): resource bounds struct now has typed fields #1180
• refactor(blockifier,starknet_api): migrate GasVector from blockifier to starknet_api #1179
• refactor(blockifier): refactor to_discounted_l1_gas #1178
• refactor(blockifier): indirection in GasPrices struct #1177
• refactor(starknet_api): gas vector to contain fields of type GasAmount #1176
• refactor(blockifier,starknet_api): gas amount is now a u64, not a u128 #1175
• refactor(blockifier): resource cost ratio type changed to u64 ratio from u128 ratio #1174
• feat(blockifier, starknet_api): add explcit typing and arithmetic to gas amount/price, and fee #1173 👈
• refactor(blockifier,starknet_api): add GasAmount (u128) type and use in blockifier's GasVector #1172
• main

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

Join @dorimedini-starkware and the rest of your teammates on Graphite

[codecov]

Codecov Report

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

Project coverage is 67.87%. Comparing base (b0cfe82) to head (3c2d2d7).
Report is 255 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/native_blockifier/src/py_state_diff.rs	33.33%	10 Missing and 10 partials ⚠️
crates/papyrus_protobuf/src/converters/header.rs	84.21%	0 Missing and 6 partials ⚠️
crates/starknet_api/src/block.rs	85.71%	6 Missing ⚠️
crates/starknet_api/src/execution_resources.rs	75.00%	3 Missing ⚠️
crates/batcher/src/block_builder.rs	0.00%	1 Missing ⚠️
crates/blockifier/src/fee/resources.rs	95.83%	1 Missing ⚠️
crates/gateway/src/rpc_objects.rs	66.66%	0 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (b0cfe82) and HEAD (3c2d2d7). Click for more details.

HEAD has 2 uploads less than BASE

Flag	BASE (b0cfe82)	HEAD (3c2d2d7)
	3	1

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1173      +/-   ##
==========================================
- Coverage   74.18%   67.87%   -6.32%     
==========================================
  Files         359      254     -105     
  Lines       36240    28462    -7778     
  Branches    36240    28462    -7778     
==========================================
- Hits        26886    19319    -7567     
- Misses       7220     7530     +310     
+ Partials     2134     1613     -521     

Flag	Coverage Δ
	?

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[dorimedini-starkware]

Reviewable status: 0 of 42 files reviewed, all discussions resolved

---

crates/blockifier/src/utils.rs line 71 at r1 (raw file):

    }
    result
}

was only used for nonzero- gas price; implementation moved

Code quote:

/// Returns the ceiling of the division of two u128 numbers.
pub fn u128_div_ceil(a: u128, b: NonZeroU128) -> u128 {
    let mut result = a / b;
    if result * b.get() < a {
        result += 1;
    }
    result
}

---

crates/blockifier/src/utils_test.rs line 77 at r1 (raw file):

    assert_eq!(9, u128_div_ceil(27, NonZeroU128::new(3).unwrap()));
    assert_eq!(10, u128_div_ceil(28, NonZeroU128::new(3).unwrap()));
}

this test was moved (see above)

Code quote:

#[test]
fn test_u128_div_ceil() {
    assert_eq!(1, u128_div_ceil(1, NonZeroU128::new(1).unwrap()));
    assert_eq!(0, u128_div_ceil(0, NonZeroU128::new(1).unwrap()));
    assert_eq!(1, u128_div_ceil(1, NonZeroU128::new(2).unwrap()));
    assert_eq!(9, u128_div_ceil(27, NonZeroU128::new(3).unwrap()));
    assert_eq!(10, u128_div_ceil(28, NonZeroU128::new(3).unwrap()));
}

---

crates/starknet_api/src/transaction.rs line 752 at r1 (raw file):

        }
        result
    }

logic was moved (see above)

Code quote:

    pub fn div_ceil(self, rhs: NonzeroGasPrice) -> GasAmount {
        let mut result: GasAmount = self / rhs;
        if result.0 * rhs.get().0 < self.0 {
            result = GasAmount(result.0 + 1);
        }
        result
    }

---

crates/starknet_api/src/transaction_test.rs line 12 at r1 (raw file):

    assert_eq!(GasAmount(9), Fee(27).div_ceil(NonzeroGasPrice::new(GasPrice(3)).unwrap()));
    assert_eq!(GasAmount(10), Fee(28).div_ceil(NonzeroGasPrice::new(GasPrice(3)).unwrap()));
}

this test was moved (see above)

Code quote:

#[test]
fn test_fee_div_ceil() {
    assert_eq!(GasAmount(1), Fee(1).div_ceil(NonzeroGasPrice::new(GasPrice(1)).unwrap()));
    assert_eq!(GasAmount(0), Fee(0).div_ceil(NonzeroGasPrice::new(GasPrice(1)).unwrap()));
    assert_eq!(GasAmount(1), Fee(1).div_ceil(NonzeroGasPrice::new(GasPrice(2)).unwrap()));
    assert_eq!(GasAmount(9), Fee(27).div_ceil(NonzeroGasPrice::new(GasPrice(3)).unwrap()));
    assert_eq!(GasAmount(10), Fee(28).div_ceil(NonzeroGasPrice::new(GasPrice(3)).unwrap()));
}

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.837 ms 34.890 ms 34.946 ms]
change: [-3.9715% -2.4155% -1.0954%] (p = 0.00 < 0.05)
Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mild

[Yoni-Starkware]

Reviewed 2 of 42 files at r1.
Reviewable status: 2 of 42 files reviewed, all discussions resolved

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.582 ms 34.705 ms 34.864 ms]
change: [-6.2971% -4.1117% -2.1555%] (p = 0.00 < 0.05)
Performance has improved.
Found 11 outliers among 100 measurements (11.00%)
1 (1.00%) high mild
10 (10.00%) high severe

[Yoni-Starkware]

Reviewed 6 of 42 files at r1, 2 of 11 files at r3, all commit messages.
Reviewable status: 10 of 42 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)

---

crates/blockifier/src/execution/entry_point.rs line 205 at r3 (raw file):

            }
            TransactionInfo::Current(context) => {
                GasAmount(u128::from(context.l1_resource_bounds().max_amount))

Throughout this PR as well

Suggestion:

  context.l1_resource_bounds().max_amount.into()

[Yoni-Starkware]

Reviewed 4 of 42 files at r1.
Reviewable status: 14 of 42 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)

[Yoni-Starkware]

Reviewed 3 of 42 files at r1, 1 of 13 files at r2, 3 of 11 files at r3.
Reviewable status: 21 of 42 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware)

---

crates/blockifier/src/transaction/errors.rs line 60 at r3 (raw file):

    #[error(
        "Max {resource} price ({max_gas_price:?}) is lower than the actual gas price: \
         {actual_gas_price:?}."

Remove :?

Code quote:

        "Max {resource} price ({max_gas_price:?}) is lower than the actual gas price: \
         {actual_gas_price:?}."

[Yoni-Starkware]

Reviewed 1 of 11 files at r3.
Reviewable status: 22 of 42 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware)

---

crates/starknet_api/src/block.rs line 242 at r3 (raw file):

#[derive(Copy, Clone, Debug)]
pub struct NonzeroGasPrice(GasPrice);

Please document why this struct is needed (since we also have GasPrice in this file)
Also, I guess that the nonzero variant is not part of the block header, so maybe it's not the right file for it (this part is not blocking)

Code quote:

pub struct NonzeroGasPrice(GasPrice);

[Yoni-Starkware]

Reviewed 2 of 42 files at r1.
Reviewable status: 24 of 42 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware)

---

crates/starknet_api/src/transaction.rs line 812 at r3 (raw file):

    pub fn checked_mul(self, rhs: GasPrice) -> Option<Fee> {
        rhs.checked_mul(self)
    }

Why here? I think the convention is having a single impl block next to the struct definition.

Also, consider gathering all gas/price/fee structs in a separate file (non blocking)

Code quote:

impl Div<NonzeroGasPrice> for Fee {
    type Output = GasAmount;

    fn div(self, rhs: NonzeroGasPrice) -> Self::Output {
        GasAmount(self.0 / GasPrice::from(rhs).0)
    }
}

impl GasPrice {
    pub const fn saturating_mul(self, rhs: GasAmount) -> Fee {
        Fee(self.0.saturating_mul(rhs.0))
    }

    pub fn checked_mul(self, rhs: GasAmount) -> Option<Fee> {
        self.0.checked_mul(rhs.0).map(Fee)
    }
}

impl NonzeroGasPrice {
    pub const fn saturating_mul(self, rhs: GasAmount) -> Fee {
        self.get().saturating_mul(rhs)
    }

    pub fn checked_mul(self, rhs: GasAmount) -> Option<Fee> {
        self.get().checked_mul(rhs)
    }
}

impl GasAmount {
    pub const fn saturating_mul(self, rhs: GasPrice) -> Fee {
        rhs.saturating_mul(self)
    }

    pub const fn nonzero_saturating_mul(self, rhs: NonzeroGasPrice) -> Fee {
        rhs.saturating_mul(self)
    }

    pub fn checked_mul(self, rhs: GasPrice) -> Option<Fee> {
        rhs.checked_mul(self)
    }

[Yoni-Starkware]

Reviewed 1 of 11 files at r3.
Reviewable status: 25 of 42 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware)

[dorimedini-starkware]

Reviewable status: 17 of 50 files reviewed, 4 unresolved discussions (waiting on @Yoni-Starkware)

---

crates/blockifier/src/execution/entry_point.rs line 205 at r3 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Throughout this PR as well

Done.

---

crates/blockifier/src/transaction/errors.rs line 60 at r3 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Remove :?

Done.

---

crates/starknet_api/src/block.rs line 242 at r3 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Please document why this struct is needed (since we also have GasPrice in this file)
Also, I guess that the nonzero variant is not part of the block header, so maybe it's not the right file for it (this part is not blocking)

1. It should be in starknet-api so we can have impl Fee { fn div_ceil(self, rhs: NonzeroGasPrice) -> GasAmount; }
2. I put it in this module so it's next to GasPrice

documented

---

crates/starknet_api/src/transaction.rs line 812 at r3 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Why here? I think the convention is having a single impl block next to the struct definition.

Also, consider gathering all gas/price/fee structs in a separate file (non blocking)

Done.

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.904 ms 34.945 ms 34.988 ms]
change: [-3.7705% -2.3841% -1.2148%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
3 (3.00%) high mild
1 (1.00%) high severe

[Yoni-Starkware]

Reviewed 1 of 42 files at r1, 1 of 13 files at r2, 1 of 11 files at r3, 7 of 23 files at r4, all commit messages.
Reviewable status: 27 of 50 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware)

---

crates/blockifier/src/execution/entry_point.rs line 205 at r3 (raw file):

Previously, dorimedini-starkware wrote…

Done.

Not done :(

[Yoni-Starkware]

Reviewed 1 of 13 files at r2, 3 of 23 files at r4.
Reviewable status: 31 of 50 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware)

---

crates/blockifier/src/transaction/test_utils.rs line 361 at r4 (raw file):

) -> ValidResourceBounds {
    ValidResourceBounds::AllResources(AllResourceBounds {
        l1_gas: ResourceBounds {

Are you going to change its types in a future PR?

Code quote:

ResourceBounds {

[Yoni-Starkware]

Reviewed 3 of 42 files at r1, 6 of 23 files at r4.
Reviewable status: 40 of 50 files reviewed, 5 unresolved discussions (waiting on @dorimedini-starkware)

---

crates/gateway/src/rpc_objects.rs line 102 at r4 (raw file):

fn parse_gas_price(gas_price: GasPrice) -> Result<NonzeroGasPrice, RPCStateReaderError> {
    NonzeroGasPrice::new(gas_price)
        .map_err(|_| RPCStateReaderError::GasPriceParsingFailure(gas_price))

What's the difference?

Code quote:

.map_err(|_|

[dorimedini-starkware]

Reviewable status: 31 of 51 files reviewed, 3 unresolved discussions (waiting on @Yoni-Starkware)

---

crates/blockifier/src/execution/entry_point.rs line 205 at r3 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Not done :(

oops... now done, I hope

---

crates/blockifier/src/transaction/test_utils.rs line 361 at r4 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Are you going to change its types in a future PR?

yes

---

crates/gateway/src/rpc_objects.rs line 102 at r4 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

What's the difference?

NonZeroU128::new returns Option, while NonzeroGasPrice::new returns Result

[Yoni-Starkware]

Reviewed 1 of 42 files at r1, 2 of 13 files at r2, 5 of 23 files at r4, 12 of 12 files at r5, 5 of 5 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dorimedini-starkware)

[dorimedini-starkware]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dorimedini-starkware)