feat(fee): add events gas cost

[barak-b-starkware]

This change is 

[avi-starkware]

crates/blockifier/src/fee/eth_gas_constants.rs line 31 at r1 (raw file):

// Every key and every data word is a StarkFelt which contains 32 bytes.
pub const MILLI_GAS_PER_KEY: usize = 32 * MILLI_GAS_PER_KEY_BYTE;
pub const MILLI_GAS_PER_DATA_WORD: usize = 32 * MILLI_GAS_PER_DATA_BYTE;

These constants should be moved to versioned_constants.json (see #1426).
The file eth_gas_constants.rs is supposed to only include ETH constants that don't change (are part of the ETH protocol).
Constants that change between versions should be in versioned_constants.json.

Code quote:

// L2 Events.
pub const MILLI_GAS_PER_KEY_BYTE: usize = 40;
pub const MILLI_GAS_PER_DATA_BYTE: usize = 2 * MILLI_GAS_PER_KEY_BYTE;
// TODO(barak, 18/03/2024): Remove calculation per StarkFelt.
// Every key and every data word is a StarkFelt which contains 32 bytes.
pub const MILLI_GAS_PER_KEY: usize = 32 * MILLI_GAS_PER_KEY_BYTE;
pub const MILLI_GAS_PER_DATA_WORD: usize = 32 * MILLI_GAS_PER_DATA_BYTE;

[avi-starkware]

crates/blockifier/src/fee/gas_usage.rs line 33 at r1 (raw file):

    Ok(get_messages_gas_cost(call_infos.clone(), l1_handler_payload_size)?
        + get_da_gas_cost(state_changes_count, use_kzg_da)
        + get_events_gas_cost(call_infos))

There should be a test where this number is not 0.

Code quote:

        + get_events_gas_cost(call_infos))

[Yoni-Starkware]

How come no test is modified?
Can you add a short unit test?

Reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware, @avi-starkware, and @barak-b-starkware)

---

crates/blockifier/src/fee/gas_usage.rs line 31 at r1 (raw file):

) -> TransactionExecutionResult<GasVector> {
    // TODO(barak, 18/03/2024): Iterate over call_infos once without cloning.
    Ok(get_messages_gas_cost(call_infos.clone(), l1_handler_payload_size)?

This does not clone the call infos, right?

Code quote:

call_infos.clone()

[Yoni-Starkware]

Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @ArniStarkware, @avi-starkware, and @barak-b-starkware)

---

crates/blockifier/src/fee/gas_usage.rs line 48 at r1 (raw file):

                                + ordered_event.event.data.0.len()
                                    * eth_gas_constants::MILLI_GAS_PER_DATA_WORD,
                        )

Split into a function, get_event_gas_cost

Code quote:

            + call_info.execution.events.iter().fold(
                GasVector { l1_gas: 0, blob_gas: 0 },
                |sum, ordered_event| {
                    sum + GasVector {
                        l1_gas: u128_from_usize(
                            ordered_event.event.keys.len() * eth_gas_constants::MILLI_GAS_PER_KEY
                                + ordered_event.event.data.0.len()
                                    * eth_gas_constants::MILLI_GAS_PER_DATA_WORD,
                        )

[barak-b-starkware]

crates/blockifier/src/fee/gas_usage.rs line 48 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Split into a function, get_event_gas_cost

Done.

[barak-b-starkware]

crates/blockifier/src/fee/gas_usage.rs line 31 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

This does not clone the call infos, right?

AFAIU it doesn't look here, but I can't say I understand 100% what happens there.

[barak-b-starkware]

IIUC the test that runs calculate_tx_gas_usage_vector uses CallInfo without events.
You can see here.

[barak-b-starkware]

crates/blockifier/src/fee/eth_gas_constants.rs line 31 at r1 (raw file):

Previously, avi-starkware wrote…

These constants should be moved to versioned_constants.json (see #1426).
The file eth_gas_constants.rs is supposed to only include ETH constants that don't change (are part of the ETH protocol).
Constants that change between versions should be in versioned_constants.json.

Done.

[barak-b-starkware]

A unit test was added.

Reviewable status: 0 of 7 files reviewed, 4 unresolved discussions (waiting on @ArniStarkware, @avi-starkware, and @Yoni-Starkware)

---

crates/blockifier/src/fee/gas_usage.rs line 33 at r1 (raw file):

Previously, avi-starkware wrote…

There should be a test where this number is not 0.

Done.

[codecov-commenter]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (94a81f3) 69.23% compared to head (5207874) 69.35%.

Files	Patch %	Lines
crates/blockifier/src/fee/gas_usage.rs	97.05%	0 Missing and 1 partial ⚠️
crates/blockifier/src/transaction/objects.rs	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@               Coverage Diff                @@
##           main-v0.13.1    #1458      +/-   ##
================================================
+ Coverage         69.23%   69.35%   +0.11%     
================================================
  Files                59       59              
  Lines              8059     8089      +30     
  Branches           8059     8089      +30     
================================================
+ Hits               5580     5610      +30     
  Misses             2021     2021              
  Partials            458      458              

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

[giladchase]

Reviewed 6 of 7 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @ArniStarkware, @avi-starkware, @barak-b-starkware, and @Yoni-Starkware)

---

crates/blockifier/resources/versioned_constants.json line 11 at r3 (raw file):

    "milligas_per_data_word": 2560,
    "invoke_tx_max_n_steps": 3000000,
    "max_recursion_depth": 50,

Top level keys are alphabetized

Suggestion:

    "invoke_tx_max_n_steps": 3000000,
    "max_recursion_depth": 50,
    "milligas_per_key_byte": 40,
    "milligas_per_data_byte": 80,
    "milligas_per_key": 1280,
    "milligas_per_data_word": 2560,

---

crates/blockifier/src/versioned_constants.rs line 41 at r3 (raw file):

    pub milligas_per_data_byte: usize,
    pub milligas_per_key: usize,
    pub milligas_per_data_word: usize,

I'm getting confused about the types 😅.

Gas is u128 (based on GasVector's inner types) but miligas is usize? Isn't GasVector also used to represent miligas?

Code quote:

    pub milligas_per_key_byte: usize,
    pub milligas_per_data_byte: usize,
    pub milligas_per_key: usize,
    pub milligas_per_data_word: usize,

---

crates/blockifier/src/versioned_constants.rs line 41 at r3 (raw file):

    pub milligas_per_data_byte: usize,
    pub milligas_per_key: usize,
    pub milligas_per_data_word: usize,

These smell like tight coupling.

If that's the case plz pack into an appropriate struct with the same derives of VersionedConstants --- in the json this will add a nesting level under the key with the chosen name of the field that holds the new struct.

Code quote:

    pub milligas_per_key_byte: usize,
    pub milligas_per_data_byte: usize,
    pub milligas_per_key: usize,
    pub milligas_per_data_word: usize,

---

crates/blockifier/src/fee/gas_usage.rs line 31 at r1 (raw file):

Previously, barak-b-starkware wrote…

AFAIU it doesn't look here, but I can't say I understand 100% what happens there.

Yep it's just cloning the iterator, which is a cheap op.
In general though, passing by & is preferred unless the type is Copy or ownership is explicitly needed.

But if it requires changing a bunch of out of scope stuff clone is fine here, otherwise, if passing by & is easy to change, remove the + Clone trait bound.

---

crates/blockifier/src/fee/gas_usage.rs line 25 at r3 (raw file):

/// e.g., a message from L2 to L1 is followed by a storage write operation in Starknet L1 contract
/// which requires gas.
pub fn calculate_tx_gas_usage_vector<'a>(

Consider moving this as a method on ActualCostBuilder.
It is the only user, and two args appearing multiple times here are its fields.

Code quote:

pub fn calculate_tx_gas_usage_vector<'a>(

---

crates/blockifier/src/fee/gas_usage.rs line 51 at r3 (raw file):

        l1_gas: milli_gas_vector.l1_gas / 1000,
        l1_data_gas: milli_gas_vector.l1_data_gas / 1000,
    }

Add derive_more::Div to GasVector for this to work (it already has Add/Sum).
Also add Copy, it's applicable.

Prefer sum over fold for summing.

BTW, if this were part of a method on ActualCostBuilder, the map could have been

map(self.get_events_milligas_cost_from_events)

Suggestion:

    let milli_gas_vector: GasVector = call_infos
        .map(|call_info| get_events_milligas_cost_from_events(call_info, versioned_constants))
        .sum();
        
    milli_gas_vector / 1000

---

crates/blockifier/src/fee/gas_usage.rs line 51 at r3 (raw file):

        l1_gas: milli_gas_vector.l1_gas / 1000,
        l1_data_gas: milli_gas_vector.l1_data_gas / 1000,
    }

GasVector is used to represent both gas units and milli-gas units? Can we use different types?

Code quote:

    GasVector {
        l1_gas: milli_gas_vector.l1_gas / 1000,
        l1_data_gas: milli_gas_vector.l1_data_gas / 1000,
    }

---

crates/blockifier/src/fee/gas_usage.rs line 70 at r3 (raw file):

                l1_data_gas: 0_u128,
            }
        },

Prefer chaining and iterators whenever possible (unless it's forced), and try keeping nesting depth low.

Rust iterators are lazy: they do nothing until they are consumed (so this iterates only once even though there are two maps).

Also, if the lengths of both of these fields has meaning (i'm not well acquainted with this type),
consider adding to OrderedEvent something like pub fn length(&self) -> (usize, usize) to hide the len logic.

Suggestion:

let total_gas_vector = call_info
    .execution
    .events
    .iter()
    .map(|OrderedEvent { event, .. }| {
        let keys_gas = event.keys.len() * versioned_constants.milligas_per_key;
        let data_gas = event.data.0.len() * versioned_constants.milligas_per_data_word;
        u128_from_usize(keys_gas + data_gas)
            .expect("Could not convert starknet gas usage from usize to u128.")
    })
    .map(|l1_gas| GasVector { l1_gas, ..Default::default() })
    .sum();

---

crates/blockifier/src/fee/gas_usage_test.rs line 25 at r3 (raw file):

    serde_json::from_str(DEFAULT_CONSTANTS_JSON)
        .expect("Versioned constants JSON file is malformed")
});

or if you need ownership:

#[fixture]
#[once]
fn versioned_constants() -> VersionedConstants {
    VersionedConstants::latest_constants().clone()
}

Suggestion:

#[fixture]
fn versioned_constants() -> &'static VersionedConstants {
    VersionedConstants::latest_constants()
}

---

crates/blockifier/src/fee/gas_usage_test.rs line 113 at r3 (raw file):

)
]
fn test_get_events_gas_cost(#[case] call_infos: Vec<CallInfo>, #[case] expected: GasVector) {

Plz split into two separate tests without parameterization.

Unparameterized tests are easier to debug in general (less macros == easier debugging), and if there isn't too much duplication in the test itself (which is the case here) the complicated parameterization isn't worth it.

[giladchase]

I added a bunch of blocking comments, but due to the time constraints for this release, if you have no time to implement/challange the feel free to dismiss them all and add a monday task to revisit them, even for the sake of discussion.

Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @ArniStarkware, @avi-starkware, @barak-b-starkware, and @Yoni-Starkware)

---

crates/blockifier/src/fee/gas_usage.rs line 51 at r3 (raw file):

Previously, giladchase wrote…

Add derive_more::Div to GasVector for this to work (it already has Add/Sum).
Also add Copy, it's applicable.

Prefer sum over fold for summing.

BTW, if this were part of a method on ActualCostBuilder, the map could have been

map(self.get_events_milligas_cost_from_events)

maybe even

...
let gas_vector = milli_gas_vector / 1000
gas_vector

for more readability

[Yoni-Starkware]

Reviewed 6 of 7 files at r2.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @ArniStarkware, @avi-starkware, and @barak-b-starkware)

---

crates/blockifier/src/fee/gas_usage.rs line 42 at r2 (raw file):

    versioned_constants: &VersionedConstants,
) -> GasVector {
    let milli_gas_vector = call_infos.fold(

I think you should work here with an integer (rather than gas vector)
And wrap it up just to return

Suggestion:

milligas_vector = call_infos.fold(

[barak-b-starkware]

Reviewable status: 1 of 10 files reviewed, 11 unresolved discussions (waiting on @ArniStarkware, @avi-starkware, @giladchase, and @Yoni-Starkware)

---

crates/blockifier/resources/versioned_constants.json line 11 at r3 (raw file):

Previously, giladchase wrote…

Top level keys are alphabetized

Done.

---

crates/blockifier/src/versioned_constants.rs line 41 at r3 (raw file):

Previously, giladchase wrote…

I'm getting confused about the types 😅.

Gas is u128 (based on GasVector's inner types) but miligas is usize? Isn't GasVector also used to represent miligas?

Done.

---

crates/blockifier/src/versioned_constants.rs line 41 at r3 (raw file):

Previously, giladchase wrote…

These smell like tight coupling.

If that's the case plz pack into an appropriate struct with the same derives of VersionedConstants --- in the json this will add a nesting level under the key with the chosen name of the field that holds the new struct.

Done.

---

crates/blockifier/src/fee/gas_usage.rs line 31 at r1 (raw file):

Previously, giladchase wrote…

Yep it's just cloning the iterator, which is a cheap op.
In general though, passing by & is preferred unless the type is Copy or ownership is explicitly needed.

But if it requires changing a bunch of out of scope stuff clone is fine here, otherwise, if passing by & is easy to change, remove the + Clone trait bound.

Passing by reference is possible, but it should be passed twice, and the first time will consume the iterator, so what do you prefer here?

---

crates/blockifier/src/fee/gas_usage.rs line 42 at r2 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

I think you should work here with an integer (rather than gas vector)
And wrap it up just to return

Changed according to @giladchase's suggestion. LMK if you like it or not (:

---

crates/blockifier/src/fee/gas_usage.rs line 25 at r3 (raw file):

Previously, giladchase wrote…

Consider moving this as a method on ActualCostBuilder.
It is the only user, and two args appearing multiple times here are its fields.

Done.

---

crates/blockifier/src/fee/gas_usage.rs line 51 at r3 (raw file):

Previously, giladchase wrote…

maybe even

...
let gas_vector = milli_gas_vector / 1000
gas_vector

for more readability

Done.
I added Copy but why to do that if it's not necessary right now?
Regarding your second suggestion, Clippy complains of unnecessary 'let' binding.

---

crates/blockifier/src/fee/gas_usage.rs line 51 at r3 (raw file):

Previously, giladchase wrote…

GasVector is used to represent both gas units and milli-gas units? Can we use different types?

Done.

---

crates/blockifier/src/fee/gas_usage.rs line 70 at r3 (raw file):

Previously, giladchase wrote…

Prefer chaining and iterators whenever possible (unless it's forced), and try keeping nesting depth low.

Rust iterators are lazy: they do nothing until they are consumed (so this iterates only once even though there are two maps).

Also, if the lengths of both of these fields has meaning (i'm not well acquainted with this type),
consider adding to OrderedEvent something like pub fn length(&self) -> (usize, usize) to hide the len logic.

Done.

---

crates/blockifier/src/fee/gas_usage_test.rs line 25 at r3 (raw file):

Previously, giladchase wrote…

or if you need ownership:

#[fixture]
#[once]
fn versioned_constants() -> VersionedConstants {
    VersionedConstants::latest_constants().clone()
}

Done.
Didn't find any documentation or examples in our repo for #[once], WDYM?

---

crates/blockifier/src/fee/gas_usage_test.rs line 113 at r3 (raw file):

Previously, giladchase wrote…

Plz split into two separate tests without parameterization.

Unparameterized tests are easier to debug in general (less macros == easier debugging), and if there isn't too much duplication in the test itself (which is the case here) the complicated parameterization isn't worth it.

Done.

[barak-b-starkware]

crates/blockifier/src/fee/gas_usage.rs line 51 at r3 (raw file):

Previously, barak-b-starkware wrote…

Done.
I added Copy but why to do that if it's not necessary right now?
Regarding your second suggestion, Clippy complains of unnecessary 'let' binding.

I agree that adding the Copy derivation is a good thing to do, it saves some redundant "clones" and it holds only two u128 which are Copy too... good idea (:

[Yoni-Starkware]

Reviewable status: 1 of 10 files reviewed, 17 unresolved discussions (waiting on @ArniStarkware, @avi-starkware, @barak-b-starkware, and @giladchase)

---

crates/blockifier/resources/versioned_constants.json line 8 at r5 (raw file):

    "event_gas_cost": {
        "milligas_per_data_word": 2560,
        "milligas_per_key": 1280

Suggestion:

"event_key_factor": 2

---

crates/blockifier/resources/versioned_constants.json line 9 at r5 (raw file):

        "milligas_per_data_word": 2560,
        "milligas_per_key": 1280
    },

L2_resources

Code quote:

    "event_gas_cost": {
        "milligas_per_data_word": 2560,
        "milligas_per_key": 1280
    },

---

crates/blockifier/src/versioned_constants.rs line 55 at r6 (raw file):

    os_constants: Arc<OSConstants>,

    event_gas_cost: Arc<EventGasCost>,

Suggestion:

 l2_resource_gas_costs: Arc<EventGasCost>,

---

crates/blockifier/src/fee/gas_usage.rs line 21 at r5 (raw file):

pub mod test;

pub fn get_events_gas_cost<'a>(

Suggestion:

pub fn get_tx_events_gas_cost<'a>(

---

crates/blockifier/src/fee/gas_usage.rs line 31 at r5 (raw file):

}

pub fn get_events_milligas_cost_from_events(

Suggestion:

pub fn get_events_milligas_cost(

---

crates/blockifier/src/fee/gas_usage.rs line 32 at r5 (raw file):

pub fn get_events_milligas_cost_from_events(
    call_info: &CallInfo,

Suggestion:

events: &Vec<...>,

[barak-b-starkware]

crates/blockifier/src/fee/gas_usage_test.rs line 54 at r7 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Move this call to be an inner call of the first one (to check iteration)

Done.

[barak-b-starkware]

crates/blockifier/src/versioned_constants.rs line 55 at r4 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Avi changed it to pub l2_resource_gas_costs: L2ResourceGasCosts

Done.

[barak-b-starkware]

crates/blockifier/src/fee/actual_cost.rs line 163 at r4 (raw file):

            + get_da_gas_cost(state_changes_count, use_kzg_da)
            + get_events_gas_cost(call_infos, versioned_constants))
    }

At @Yoni-Starkware 's request, it moved back, so the changes you requested can't be done (using self).

[Yoni-Starkware]

Reviewed 1 of 5 files at r10.
Reviewable status: 2 of 12 files reviewed, 8 unresolved discussions (waiting on @ArniStarkware, @avi-starkware, @barak-b-starkware, and @giladchase)

---

crates/blockifier/src/fee/gas_usage.rs line 24 at r15 (raw file):

/// Returns an estimation of the L1 gas amount that will be used (by Starknet's state update and
/// the Verifier) following the addition of a transaction with the given parameters to a batch;
/// e.g., a message from L2 to L1 is followed by a storage write operation in Starknet L1 contract

Document as follows:

Suggestion:

/// Returns the gas usage of a transaction, specifically: 
/// * L1 gas, used by Starknet's state update and the Verifier, 
///   e.g., a message from L2 to L1 is followed by a storage write operation on L1.
/// * L1 data gas, for publishing data availability.
/// * L2 resources cost, e.g., for storing transaction calldata.

[barak-b-starkware]

crates/blockifier/src/fee/gas_usage.rs line 24 at r15 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Document as follows:

Done.

[Yoni-Starkware]

Reviewed 1 of 3 files at r15.
Reviewable status: 2 of 12 files reviewed, 9 unresolved discussions (waiting on @ArniStarkware, @avi-starkware, @barak-b-starkware, and @giladchase)

---

crates/blockifier/src/fee/gas_usage_test.rs line 37 at r17 (raw file):

    assert_eq!(GasVector::default(), get_tx_events_gas_cost(call_infos, versioned_constants));

    let create_trivial_ordered_event_from_keys_and_data_sizes =

Suggestion:

create_event

[barak-b-starkware]

crates/blockifier/src/fee/gas_usage_test.rs line 37 at r17 (raw file):

    assert_eq!(GasVector::default(), get_tx_events_gas_cost(call_infos, versioned_constants));

    let create_trivial_ordered_event_from_keys_and_data_sizes =

Done.

[Yoni-Starkware]

Reviewed 1 of 6 files at r7, 1 of 5 files at r10, 6 of 7 files at r12, 1 of 3 files at r15, 1 of 1 files at r16, 1 of 1 files at r17, 1 of 1 files at r18, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ArniStarkware, @avi-starkware, and @giladchase)

The base branch was changed.

[Yoni-Starkware]

Reviewed 3 of 3 files at r19, all commit messages.
Dismissed @giladchase from 4 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ArniStarkware, @avi-starkware, and @giladchase)

[Yoni-Starkware]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware, @avi-starkware, and @giladchase)

.