feat(fee): calculates messages size field

[ayeletstarkware]

This change is 

[codecov-commenter]

Codecov Report

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

Comparison is base (174a0c9) 71.10% compared to head (201dfa7) 71.01%.

Files	Patch %	Lines
...ates/native_blockifier/src/transaction_executor.rs	0.00%	13 Missing ⚠️
crates/blockifier/src/execution/call_info.rs	88.23%	1 Missing and 1 partial ⚠️
crates/blockifier/src/fee/gas_usage.rs	90.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@               Coverage Diff                @@
##           main-v0.13.1    #1290      +/-   ##
================================================
- Coverage         71.10%   71.01%   -0.10%     
================================================
  Files                59       59              
  Lines              7639     7651      +12     
  Branches           7639     7651      +12     
================================================
+ Hits               5432     5433       +1     
- Misses             1780     1791      +11     
  Partials            427      427              

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

[elintul]

Reviewed 1 of 3 files at r1, all commit messages.
Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware and @noaov1)

---

crates/blockifier/src/transaction/transaction_execution.rs line 90 at r1 (raw file):

    }

    pub fn get_l1_handler_payload_size(&self) -> Option<usize> {

It's a bit weird to put this method on Transaction, when most of types don't have it.
I'd do an if let in the native_blockifier code.

Code quote:

get_l1_handler_payload_size

[ayeletstarkware]

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

---

crates/blockifier/src/transaction/transaction_execution.rs line 90 at r1 (raw file):

Previously, elintul (Elin) wrote…

It's a bit weird to put this method on Transaction, when most of types don't have it.
I'd do an if let in the native_blockifier code.

Done.

[noaov1]

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

---

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

}

pub fn calculate_messages_size(

Suggestion:

pub fn calculate_message_segment_size(

---

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

pub fn calculate_messages_size(
    tx_info: &TransactionExecutionInfo,

Why not pass directly [&tx_info.validate_call_info, &tx_info.execute_call_info]? It's the only information that you need from the TransactionExecutionInfo.

Code quote:

    tx_info: &TransactionExecutionInfo,

---

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

    for call_info in &call_infos {
        l2_to_l1_payloads_length
            .extend(call_info.get_sorted_l2_to_l1_payloads_length().into_iter().flatten());

What does it mean to use faltten() on a result object?

Code quote:

            .extend(call_info.get_sorted_l2_to_l1_payloads_length().into_iter().flatten());

---

crates/native_blockifier/src/transaction_executor.rs line 74 at r2 (raw file):

        let l1_handler_payload_size: Option<usize> =
            if let Transaction::L1HandlerTransaction(l1_handler_tx) = &tx {
                Some(l1_handler_tx.tx.calldata.0.len() - 1)

Consider defining a method for L1HandlerTransaction to calculate the payload size.

Code quote:

                Some(l1_handler_tx.tx.calldata.0.len() - 1)

---

crates/native_blockifier/src/transaction_executor.rs line 87 at r2 (raw file):

            Ok(tx_execution_info) => {
                tx_executed_class_hashes.extend(tx_execution_info.get_executed_class_hashes());
                let messages_size =

Suggestion:

let message_segment_size =

[ayeletstarkware]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @elintul and @noaov1)

---

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

Previously, noaov1 (Noa Oved) wrote…

Why not pass directly [&tx_info.validate_call_info, &tx_info.execute_call_info]? It's the only information that you need from the TransactionExecutionInfo.

Done.

---

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

Previously, noaov1 (Noa Oved) wrote…

What does it mean to use faltten() on a result object?

changed it to filter map

---

crates/native_blockifier/src/transaction_executor.rs line 74 at r2 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Consider defining a method for L1HandlerTransaction to calculate the payload size.

hope that i understood correctly

[noaov1]

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

---

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

Previously, ayeletstarkware (Ayelet Zilber) wrote…

changed it to filter map

I meant the flatten() inside the extend. Notice that get_sorted_l2_to_l1_payloads_length returns a result object.

[ayeletstarkware]

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

---

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

Previously, noaov1 (Noa Oved) wrote…

I meant the flatten() inside the extend. Notice that get_sorted_l2_to_l1_payloads_length returns a result object.

because the result get_sorted_l2_to_l1_payloads_length of is of type Vec. if I don't flatten the result, we get Vec<Vec>. do you know another way to do so?

[ayeletstarkware]

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

---

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

Previously, ayeletstarkware (Ayelet Zilber) wrote…

because the result get_sorted_l2_to_l1_payloads_length of is of type Vec. if I don't flatten the result, we get Vec<Vec>. do you know another way to do so?

we get Vec<Vec>*

[Yael-Starkware]

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

---

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

            Ok(vec) => l2_to_l1_payloads_length.extend(vec),
            Err(err) => return Err(err),
        }

l2_to_l1_payloads_length.extend(call_info.get_sorted_l2_to_l1_payloads_length()?);

Code quote:

        match call_info.get_sorted_l2_to_l1_payloads_length() {
            Ok(vec) => l2_to_l1_payloads_length.extend(vec),
            Err(err) => return Err(err),
        }

[ayeletstarkware]

Reviewable status: 1 of 4 files reviewed, 5 unresolved discussions (waiting on @elintul, @noaov1, and @Yael-Starkware)

---

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

Previously, Yael-Starkware (YaelD) wrote…

l2_to_l1_payloads_length.extend(call_info.get_sorted_l2_to_l1_payloads_length()?);

Done.

[noaov1]

Reviewed 1 of 3 files at r3, all commit messages.
Reviewable status: 2 of 4 files reviewed, 5 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @Yael-Starkware)

---

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

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Done.

Can you please align your code with the original one? See here.

---

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

Previously, ayeletstarkware (Ayelet Zilber) wrote…

we get Vec<Vec>*

Not relevant anymore (given you recent change). I would try to understand the meaning of faltten() on a Result object :)

---

crates/blockifier/src/transaction/transactions.rs line 470 at r6 (raw file):

    }

    pub fn get_payload_size(&self) -> Option<usize> {

Why return an Option?

Suggestion:

pub fn get_payload_size(&self) -> usize {

[ayeletstarkware]

Reviewable status: 2 of 4 files reviewed, 5 unresolved discussions (waiting on @elintul, @noaov1, and @Yael-Starkware)

---

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

Previously, noaov1 (Noa Oved) wrote…

Can you please align your code with the original one? See here.

Done.

---

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

Previously, noaov1 (Noa Oved) wrote…

Not relevant anymore (given you recent change). I would try to understand the meaning of faltten() on a Result object :)

Done.

---

crates/blockifier/src/transaction/transactions.rs line 470 at r6 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Why return an Option?

Done.

[noaov1]

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

---

crates/blockifier/src/fee/gas_usage.rs line 63 at r9 (raw file):

    call_infos: &[&CallInfo],
    l1_handler_payload_size: Option<usize>,
) -> Result<usize, TransactionExecutionError> {

Suggestion:

) -> TransactionExecutionResult<usize> {

---

crates/blockifier/src/transaction/transactions.rs line 471 at r9 (raw file):

    pub fn get_payload_size(&self) -> usize {
        self.tx.calldata.0.len() - 1

Please use it here.

Suggestion:

    pub fn payload_size(&self) -> usize {
         // The calldata includes the "from" field, which is not a part of the payload.
        self.tx.calldata.0.len() - 1

---

crates/native_blockifier/src/transaction_executor.rs line 101 at r9 (raw file):

                        .iter()
                        .filter_map(|&call_info| call_info.as_ref())
                        .collect::<Vec<&CallInfo>>();

Consider extracting into a method to be used here as well.

Code quote:

                let call_infos: Vec<&CallInfo> =
                    [&tx_execution_info.validate_call_info, &tx_execution_info.execute_call_info]
                        .iter()
                        .filter_map(|&call_info| call_info.as_ref())
                        .collect::<Vec<&CallInfo>>();

[ayeletstarkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @elintul and @noaov1)

---

crates/native_blockifier/src/transaction_executor.rs line 101 at r9 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Consider extracting into a method to be used here as well.

Will be added in a different PR

[noaov1]

Reviewed 3 of 3 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @elintul)

[noaov1]

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

[elintul]

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

---

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

pub mod test;

pub fn calculate_l2_to_l1_payloads_length_and_message_segment_length<'a>(

Please add a small struct MessageL1CostInfo with those two fields and convert this function to its c-tor; can be done in another PR if too big of an addition.

Code quote:

calculate_l2_to_l1_payloads_length_and_message_segment_length

---

crates/native_blockifier/src/transaction_executor.rs line 97 at r14 (raw file):

                tx_executed_class_hashes.extend(tx_execution_info.get_executed_class_hashes());
                tx_visited_storage_entries.extend(tx_execution_info.get_visited_storage_entries());
                let call_infos: Vec<&CallInfo> =

This function is getting too long.
Please define a c-tor for PyBouncerInfo receiving all it needs and move all related code (inc. your new code) there.

[ayeletstarkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @elintul)

---

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

Previously, elintul (Elin) wrote…

Please add a small struct MessageL1CostInfo with those two fields and convert this function to its c-tor; can be done in another PR if too big of an addition.

Done.

---

crates/native_blockifier/src/transaction_executor.rs line 97 at r14 (raw file):

Previously, elintul (Elin) wrote…

This function is getting too long.
Please define a c-tor for PyBouncerInfo receiving all it needs and move all related code (inc. your new code) there.

Will be done in a different PR

[elintul]

Reviewed 3 of 3 files at r15, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ayeletstarkware)

---

crates/blockifier/src/execution/call_info.rs line 32 at r15 (raw file):

}

#[cfg_attr(test, derive(Clone))]

Why?

Code quote:

#[cfg_attr(test, derive(Clone))]

---

crates/blockifier/src/execution/call_info.rs line 35 at r15 (raw file):

#[derive(Debug, Default, Eq, PartialEq)]
pub struct MessageL1CostInfo {
    pub l2_to_l1_payloads_length: Vec<usize>,

Suggestion:

l2_to_l1_payload_lengths

---

crates/blockifier/src/execution/call_info.rs line 36 at r15 (raw file):

pub struct MessageL1CostInfo {
    pub l2_to_l1_payloads_length: Vec<usize>,
    pub message_segment_length: usize,

We have a new c-tor; no need to leave the default one public.

Suggestion:

    l2_to_l1_payloads_length: Vec<usize>,
    message_segment_length: usize,

---

crates/blockifier/src/execution/call_info.rs line 40 at r15 (raw file):

impl MessageL1CostInfo {
    pub fn new<'a>(

Rename to create/calculate, so it's clear to the reader work is done here.

Code quote:

new

---

crates/blockifier/src/execution/call_info.rs line 47 at r15 (raw file):

        for call_info in call_infos {
            l2_to_l1_payloads_length.extend(call_info.get_sorted_l2_to_l1_payloads_length()?);
        }

Is there a more idiomatic way to write this loop? Perhaps ChatGPT would suggest one.

Code quote:

        let mut l2_to_l1_payloads_length = Vec::new();
        for call_info in call_infos {
            l2_to_l1_payloads_length.extend(call_info.get_sorted_l2_to_l1_payloads_length()?);
        }

---

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

    l1_handler_payload_size: Option<usize>,
) -> TransactionExecutionResult<usize> {
    let message_l1_cost_info = MessageL1CostInfo::new(call_infos, l1_handler_payload_size)?;

x2

Suggestion:

let MessageL1CostInfo { l2_to_l1_payloads_length, message_segment_length } = 
    MessageL1CostInfo::new(call_infos, l1_handler_payload_size)?;

[ayeletstarkware]

Reviewable status: 2 of 6 files reviewed, 4 unresolved discussions (waiting on @elintul, @noaov1, and @Yael-Starkware)

---

crates/blockifier/src/execution/call_info.rs line 32 at r15 (raw file):

Previously, elintul (Elin) wrote…

Why?

not needed. deleted.

---

crates/blockifier/src/execution/call_info.rs line 36 at r15 (raw file):

Previously, elintul (Elin) wrote…

We have a new c-tor; no need to leave the default one public.

But i'm calling these fields in gas_usage.rs

---

crates/blockifier/src/execution/call_info.rs line 47 at r15 (raw file):

Previously, elintul (Elin) wrote…

Is there a more idiomatic way to write this loop? Perhaps ChatGPT would suggest one.

Is flat_map better?

---

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

Previously, elintul (Elin) wrote…

x2

Done.

[elintul]

Reviewed 1 of 3 files at r12, 4 of 4 files at r16, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ayeletstarkware)

---

crates/blockifier/src/execution/call_info.rs line 36 at r15 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

But i'm calling these fields in gas_usage.rs

Where? Please link to the code.

---

crates/blockifier/src/execution/call_info.rs line 47 at r15 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Is flat_map better?

What exactly does it do?

---

crates/blockifier/src/execution/call_info.rs line 44 at r16 (raw file):

    ) -> TransactionExecutionResult<Self> {
        let l2_to_l1_payload_lengths: Vec<usize> = call_infos
            .flat_map(|call_info| call_info.l2_to_l1_payload_lengths().unwrap())

Will cause panic; instead, add the error variant to the appropriate error enum, and return a result - as before.

Code quote:

unwrap()

---

crates/native_blockifier/src/transaction_executor.rs line 103 at r16 (raw file):

                let MessageL1CostInfo { l2_to_l1_payload_lengths: _, message_segment_length } =
                    MessageL1CostInfo::calculate(
                        call_infos.into_iter(),

Can we move this up to the call_infos line?

Code quote:

.into_iter()

[ayeletstarkware]

Reviewable status: 3 of 6 files reviewed, 4 unresolved discussions (waiting on @elintul)

---

crates/blockifier/src/execution/call_info.rs line 36 at r15 (raw file):

Previously, elintul (Elin) wrote…

Where? Please link to the code.

here:

blockifier/crates/blockifier/src/fee/gas_usage.rs

Line 48 in 9c065aa

let MessageL1CostInfo { l2_to_l1_payload_lengths, message_segment_length } =

it doesn't work without the public visibality.

---

crates/blockifier/src/execution/call_info.rs line 47 at r15 (raw file):

Previously, elintul (Elin) wrote…

What exactly does it do?

from rust-lang:
flat_map creates an iterator that works like map, but flattens nested structure.
The mapadapter is very useful, but only when the closure argument produces values. If it produces an iterator instead, there’s an extra layer of indirection. flat_map() will remove this extra layer on its own.

In this case, where 'l2_to_l1_payload_lengths' returns a 'TransactionExecutionResult<Vec>', using 'flat_map' ensures that the final result is a flattened 'Vec'

---

crates/blockifier/src/execution/call_info.rs line 44 at r16 (raw file):

Previously, elintul (Elin) wrote…

Will cause panic; instead, add the error variant to the appropriate error enum, and return a result - as before.

I tried using 'flat_map' but wasn't successful, so I went back to the previous version.

---

crates/native_blockifier/src/transaction_executor.rs line 103 at r16 (raw file):

Previously, elintul (Elin) wrote…

Can we move this up to the call_infos line?

I tried but couldn't make it work.

[ayeletstarkware]

Reviewable status: 2 of 6 files reviewed, 4 unresolved discussions (waiting on @elintul)

---

crates/native_blockifier/src/transaction_executor.rs line 103 at r16 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

I tried but couldn't make it work.

Fixed!

[elintul]

Reviewed 1 of 3 files at r17, 1 of 1 files at r18, 2 of 2 files at r20, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ayeletstarkware)

---

crates/blockifier/src/execution/call_info.rs line 36 at r15 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

here:

blockifier/crates/blockifier/src/fee/gas_usage.rs

Line 48 in 9c065aa

let MessageL1CostInfo { l2_to_l1_payload_lengths, message_segment_length } =

it doesn't work without the public visibality.

Okay, didn't know that unpacking to the struct means the regular c-tor must be public.

---

crates/blockifier/src/execution/call_info.rs line 102 at r19 (raw file):

    /// Returns a list of Starknet L2ToL1Payload length collected during the execution, sorted
    /// by the order in which they were sent.
    pub fn get_sorted_l2_to_l1_payloads_length(&self) -> TransactionExecutionResult<Vec<usize>> {

Restore?

Code quote:

get_sorted_

The base branch was changed.

[elintul]

Reviewed 2 of 2 files at r21, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ayeletstarkware)

[elintul]

Reviewed 2 of 2 files at r22, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware)

a discussion (no related file):
Please link to the Python-side PR.

---

[ayeletstarkware]

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

a discussion (no related file):

Previously, elintul (Elin) wrote…

Please link to the Python-side PR.

https://reviewable.io/reviews/starkware-industries/starkware/33090

---

[elintul]

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