feat(blockifier): gas_for_fee computation

[TzahiTaub]

No description provided.

[reviewable-StarkWare]

This change is 

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.73%. Comparing base (e3165c4) to head (7442ad4).
Report is 244 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1804       +/-   ##
===========================================
+ Coverage   40.10%   68.73%   +28.62%     
===========================================
  Files          26      102       +76     
  Lines        1895    13634    +11739     
  Branches     1895    13634    +11739     
===========================================
+ Hits          760     9371     +8611     
- Misses       1100     3862     +2762     
- Partials       35      401      +366     

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

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[dorimedini-starkware]

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @TzahiTaub)

---

crates/blockifier/src/execution/entry_point_execution.rs line 370 at r1 (raw file):

}

/// Calculates the gas for fee consumed in the current call.

Suggestion:

/// Calculates the total gas for fee consumed in the current call + subtree.

---

crates/blockifier/src/execution/entry_point_execution.rs line 387 at r1 (raw file):

    // is 2 (upper branch) + 4 (lower branch).
    // For every branch, the first child embeds its branch "VM mode calls gas_consumed" in the
    // difference between its gas_consumed and gas_for_fee.

How about this? Clearer, IMO

// The total sierra gas consumed in this specific call is `gas_consumed` (total gas including self + subtree),
// minus the sum of all inner calls total sierra gas consumed.
// To compute the total sierra gas to charge (if the sierra gas resource is the tracked resource), we add this
// amount to the total gas to charge for in the subtree:
// total_gas_to_charge = gas_consumed - total_subtree_gas_consumed + total_subtree_gas_to_charge.

Code quote:

    // For CairoSteps the gas_for_fee is 0.
    // For SierraGas it is the gas_consumed of the call minus the gas_consumed of CairoSteps (VM)
    // mode inner calls.
    // E.g., for the following call topology (marked as TrackedResource(gas_consumed, gas_for_fee)):
    //       Gas(8,2) -> Gas(3,1) -> VM(2,0) -> VM(1,0)
    //          \
    //           --> VM(4,0)
    // The fee_for_gas for the first call is 8-2-4 = 2. As the gas_consumed of derived VM mode calls
    // is 2 (upper branch) + 4 (lower branch).
    // For every branch, the first child embeds its branch "VM mode calls gas_consumed" in the
    // difference between its gas_consumed and gas_for_fee.

---

crates/blockifier/src/execution/entry_point_execution.rs line 389 at r1 (raw file):

    // difference between its gas_consumed and gas_for_fee.
    GasAmount(match tracked_resource {
        TrackedResource::CairoSteps => 0,

is my comment correct?
if so, please add it; if not, why not?

Suggestion:

// If the tracked resource is steps, then all tracked resources of all calls in
// the subtree are also steps. Thus, the total gas to charge in this subtree is zero. 
TrackedResource::CairoSteps => 0,

---

crates/blockifier/src/execution/entry_point_execution_test.rs line 5 at r1 (raw file):

use super::to_gas_for_fee;
use crate::execution::call_info::{CallExecution, CallInfo, ChargedResources};
use crate::execution::contract_class::TrackedResource;

be consistent: super or crate::execution?
i prefer crate but this is a test module so super is also ok

Code quote:

use super::to_gas_for_fee;
use crate::execution::call_info::{CallExecution, CallInfo, ChargedResources};
use crate::execution::contract_class::TrackedResource;

---

crates/blockifier/src/execution/entry_point_execution_test.rs line 51 at r1 (raw file):

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

can you merge this with the for loop above? adding a fourth boolean argument so you know how to construct the CallInfo, and whether you want to push or reassign to inner_calls?

Code quote:

    // Second branch - 1 call.
    let (tracked_resource, gas_consumed, expected_gas_for_fee) =
        (TrackedResource::CairoSteps, 4, 0);
    assert_eq!(to_gas_for_fee(&tracked_resource, gas_consumed, &[]).0, expected_gas_for_fee);

    inner_calls.push(CallInfo {
        execution: CallExecution { gas_consumed, ..Default::default() },
        tracked_resource,
        charged_resources: ChargedResources {
            gas_for_fee: GasAmount(expected_gas_for_fee),
            ..Default::default()
        },
        ..Default::default()
    });

---

crates/blockifier/src/execution/entry_point_execution_test.rs line 58 at r1 (raw file):

        to_gas_for_fee(&tracked_resource, gas_consumed, &inner_calls).0,
        expected_gas_for_fee
    );

no point in intermediate vars here IMO... your docstring is clear

Suggestion:

    assert_eq!(to_gas_for_fee(&TrackedResource::SierraGas, 8, &inner_calls).0, 2);

[TzahiTaub]

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

---

crates/blockifier/src/execution/entry_point_execution.rs line 370 at r1 (raw file):

}

/// Calculates the gas for fee consumed in the current call.

Done. Removed the consumed as it's not clear what it means.

---

crates/blockifier/src/execution/entry_point_execution.rs line 387 at r1 (raw file):

Previously, dorimedini-starkware wrote…

How about this? Clearer, IMO

// The total sierra gas consumed in this specific call is `gas_consumed` (total gas including self + subtree),
// minus the sum of all inner calls total sierra gas consumed.
// To compute the total sierra gas to charge (if the sierra gas resource is the tracked resource), we add this
// amount to the total gas to charge for in the subtree:
// total_gas_to_charge = gas_consumed - total_subtree_gas_consumed + total_subtree_gas_to_charge.

Changed it a little (specifically removed most of the total as I think it should only be when talking about self + subtree). Thanks.

---

crates/blockifier/src/execution/entry_point_execution.rs line 389 at r1 (raw file):

Previously, dorimedini-starkware wrote…

is my comment correct?
if so, please add it; if not, why not?

Done.

---

crates/blockifier/src/execution/entry_point_execution_test.rs line 5 at r1 (raw file):

Previously, dorimedini-starkware wrote…

be consistent: super or crate::execution?
i prefer crate but this is a test module so super is also ok

Do you mean to change everything to super::super?

---

crates/blockifier/src/execution/entry_point_execution_test.rs line 51 at r1 (raw file):

Previously, dorimedini-starkware wrote…

can you merge this with the for loop above? adding a fourth boolean argument so you know how to construct the CallInfo, and whether you want to push or reassign to inner_calls?

No :)
Became too ugly.

---

crates/blockifier/src/execution/entry_point_execution_test.rs line 58 at r1 (raw file):

Previously, dorimedini-starkware wrote…

no point in intermediate vars here IMO... your docstring is clear

Done.

[github-actions]

Artifacts upload triggered. View details here

[dorimedini-starkware]

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

---

crates/blockifier/src/execution/entry_point_execution_test.rs line 5 at r1 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Do you mean to change everything to super::super?

super and crate::execution are interchangeable in this module.
I mean: pick one :)
I prefer crate::.. always

[TzahiTaub]

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

---

crates/blockifier/src/execution/entry_point_execution_test.rs line 5 at r1 (raw file):

Previously, dorimedini-starkware wrote…

super and crate::execution are interchangeable in this module.
I mean: pick one :)
I prefer crate::.. always

Not exactly - super == crate::execution::entry_point_execution and super::super == crate::execution