Refactor calculate_tx_gas_usage

[ayeletstarkware]

This change is 

[codecov-commenter]

Codecov Report

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

Comparison is base (d6dd88b) 70.88% compared to head (b405d20) 70.85%.

Files	Patch %	Lines
crates/blockifier/src/fee/gas_usage.rs	90.90%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1320      +/-   ##
==========================================
- Coverage   70.88%   70.85%   -0.03%     
==========================================
  Files          59       59              
  Lines        7350     7347       -3     
  Branches     7350     7347       -3     
==========================================
- Hits         5210     5206       -4     
  Misses       1726     1726              
- Partials      414      415       +1     

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

[giladchase]

Can you explain what changes have been made plz?

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

---

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

    }

    // Calculate the addition of the transaction to the output messages segment.

Shouldn't this be "length of the addition"? or something of that nature

Code quote:

  // Calculate the addition of the transaction to the output messages segment.

---

crates/blockifier/src/fee/gas_usage_test.rs line 210 at r1 (raw file):

            })
        })
    });

you can unwrap freely in tests, no need for and_then (which is rarely used, since it isn't too intuitive), unless you want to explicitly assert something being OK as part of the test logic.

Code quote:

    // the combined calculation got it once.
    let expected_gas_usage = l1_handler_gas_usage.and_then(|l1_handler_gas_usage_val| {
        l2_to_l1_messages_gas_usage.and_then(|l2_to_l1_messages_gas_usage_val| {
            storage_writings_gas_usage.map(|storage_writings_gas_usage_val| {
                l1_handler_gas_usage_val
                    + l2_to_l1_messages_gas_usage_val
                    + storage_writings_gas_usage_val
                    + fee_balance_discount
            })
        })
    });

[ayeletstarkware]

I want to share code from the "calculate_tx_gas_usage" function for the next PR (I will use "calculate_l2_to_l1_payloads_length_and_message_segment_length"). While doing that, I realized "calculate_l1_gas_usage" is no longer needed. This change has affected many tests.

Reviewable status: 2 of 5 files reviewed, 2 unresolved discussions (waiting on @giladchase and @noaov1)

---

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

Previously, giladchase wrote…

Shouldn't this be "length of the addition"? or something of that nature

not sure this comment is still relevant. was moved from "calculate_tx_gas_usage" func.

---

crates/blockifier/src/fee/gas_usage_test.rs line 210 at r1 (raw file):

Previously, giladchase wrote…

you can unwrap freely in tests, no need for and_then (which is rarely used, since it isn't too intuitive), unless you want to explicitly assert something being OK as part of the test logic.

Done.

[giladchase]

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

---

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

// TODO(Ayelet, 10/1/2024): Use to calculate message segment length in transaction_executer's
// execute
pub fn calculate_l2_to_l1_payloads_length_and_message_segment_length<'a>(

Plz make private until it is actually used outside of this module.
Also, once it is reused, consider bundling the payload length and segment length into some struct (for 1-2 usages returning the 2-tuple is fine).

Code quote:

// TODO(Ayelet, 10/1/2024): Use to calculate message segment length in transaction_executer's
// execute
pub fn calculate_l2_to_l1_payloads_length_and_message_segment_length<'a>(

[ayeletstarkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @giladchase and @noaov1)

---

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

Previously, giladchase wrote…

Plz make private until it is actually used outside of this module.
Also, once it is reused, consider bundling the payload length and segment length into some struct (for 1-2 usages returning the 2-tuple is fine).

Done.

[giladchase]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1)

[noaov1]

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

---

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

    let residual_message_segment_length =
        get_message_segment_length(&l2_to_l1_payloads_length, l1_handler_payload_size);

What is the meaning of residual in this context?

Code quote:

    let residual_message_segment_length =
        get_message_segment_length(&l2_to_l1_payloads_length, l1_handler_payload_size);

---

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

            ..Default::default()
        },
    ];

Consider using a loop

Code quote:

        CallInfo {
            execution: CallExecution {
                l2_to_l1_messages: vec![OrderedL2ToL1Message {
                    message: Default::default(),
                    ..Default::default()
                }],
                ..Default::default()
            },
            ..Default::default()
        },
        CallInfo {
            execution: CallExecution {
                l2_to_l1_messages: vec![OrderedL2ToL1Message {
                    message: MessageToL1 {
                        payload: L2ToL1Payload(vec![stark_felt!(0_u16)]),
                        ..Default::default()
                    },
                    ..Default::default()
                }],
                ..Default::default()
            },
            ..Default::default()
        },
        CallInfo {
            execution: CallExecution {
                l2_to_l1_messages: vec![OrderedL2ToL1Message {
                    message: MessageToL1 {
                        payload: L2ToL1Payload(vec![stark_felt!(0_u16), stark_felt!(0_u16)]),
                        ..Default::default()
                    },
                    ..Default::default()
                }],
                ..Default::default()
            },
            ..Default::default()
        },
        CallInfo {
            execution: CallExecution {
                l2_to_l1_messages: vec![OrderedL2ToL1Message {
                    message: MessageToL1 {
                        payload: L2ToL1Payload(vec![
                            stark_felt!(0_u16),
                            stark_felt!(0_u16),
                            stark_felt!(0_u16),
                        ]),
                        ..Default::default()
                    },
                    ..Default::default()
                }],
                ..Default::default()
            },
            ..Default::default()
        },
    ];

---

crates/blockifier/src/transaction/transactions_test.rs line 1450 at r3 (raw file):

    let l1_gas_usage = calculate_tx_gas_usage(std::iter::empty(), state_changes_count, None);
    assert!(l1_gas_usage.is_ok());
    assert_eq!(tx_execution_info.actual_resources.gas_usage(), l1_gas_usage.unwrap());

Can you please change it in all the calls to calculate_tx_gas_usage in test files?

Suggestion:

    let l1_gas_usage = calculate_tx_gas_usage(std::iter::empty(), state_changes_count, None).unwrap();
    assert_eq!(tx_execution_info.actual_resources.gas_usage(), l1_gas_usage);

[noaov1]

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

---

crates/blockifier/src/fee/gas_usage_test.rs line 98 at r4 (raw file):

    };
    let l2_to_l1_messages_gas_usage =
        calculate_tx_gas_usage(call_infos.iter(), l2_to_l1_state_changes_count, None).unwrap();

Consider defining let call_infos_iter = call_infos.iter() (you can also use it when calculating l2_to_l1_payloads_length).

Code quote:

call_infos.iter()

---

crates/blockifier/src/fee/gas_usage_test.rs line 152 at r4 (raw file):

    // l2_to_l1_messages_gas_usage and storage_writings_gas_usage got a discount each, while
    // the combined calculation got it once.

Why did you move this comment?

Code quote:

    // l2_to_l1_messages_gas_usage and storage_writings_gas_usage got a discount each, while
    // the combined calculation got it once.

[ayeletstarkware]

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

---

crates/blockifier/src/fee/gas_usage_test.rs line 98 at r4 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Consider defining let call_infos_iter = call_infos.iter() (you can also use it when calculating l2_to_l1_payloads_length).

Done

---

crates/blockifier/src/fee/gas_usage_test.rs line 152 at r4 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Why did you move this comment?

Fixed

[noaov1]

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

---

crates/blockifier/src/fee/gas_usage_test.rs line 98 at r4 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Done

Previously it looked better, right? Anyway, next PR.