feat(blockifier): don't count Sierra gas in CairoSteps mode

[Yoni-Starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[github-actions]

Artifacts upload workflows:

• Started at 2024-12-03T17:36:49Z
• Started at 2024-12-04T11:20:54Z
• Started at 2024-12-04T11:21:11Z

[Yoni-Starkware]

Py side: https://github.com/starkware-industries/starkware/pull/36379

Reviewable status: 0 of 7 files reviewed, all discussions resolved (waiting on @dorimedini-starkware)

[codecov]

Codecov Report

Attention: Patch coverage is 97.14286% with 1 line in your changes missing coverage. Please review.

Project coverage is 71.21%. Comparing base (e3165c4) to head (ca92676).
Report is 694 commits behind head on main.

Files with missing lines	Patch %	Lines
.../blockifier/src/execution/entry_point_execution.rs	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2440       +/-   ##
===========================================
+ Coverage   40.10%   71.21%   +31.11%     
===========================================
  Files          26      102       +76     
  Lines        1895    13681    +11786     
  Branches     1895    13681    +11786     
===========================================
+ Hits          760     9743     +8983     
- Misses       1100     3524     +2424     
- Partials       35      414      +379     

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

[dorimedini-starkware]

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Yoni-Starkware)

---

crates/blockifier/src/execution/contract_class.rs line 149 at r1 (raw file):

            Some(TrackedResource::CairoSteps) => TrackedResource::CairoSteps,
            Some(TrackedResource::SierraGas) => contract_tracked_resource,
            None => contract_tracked_resource,

Suggestion:

            Some(TrackedResource::SierraGas) | None => contract_tracked_resource,

---

crates/blockifier/src/execution/contract_class.rs line 150 at r1 (raw file):

            Some(TrackedResource::SierraGas) => contract_tracked_resource,
            None => contract_tracked_resource,
        }

I think we are not ready yet to make this change.
we currently assume sierra gas consumption is zero if the user didn't sign on L1 data gas,
so you cna't run in gas tracking mode without changing this assumption first

Code quote:

    ) -> TrackedResource {
        let contract_tracked_resource = match self {
            Self::V0(_) => TrackedResource::CairoSteps,
            Self::V1(contract_class) => contract_class.tracked_resource(min_sierra_version),
            #[cfg(feature = "cairo_native")]
            Self::V1Native(contract_class) => {
                contract_class.casm().tracked_resource(min_sierra_version)
            }
        };
        match last_tracked_resource {
            // Once we ran with CairoSteps, we will continue to run using it for all nested calls.
            Some(TrackedResource::CairoSteps) => TrackedResource::CairoSteps,
            Some(TrackedResource::SierraGas) => contract_tracked_resource,
            None => contract_tracked_resource,
        }

---

crates/blockifier/src/execution/syscalls/syscall_tests/call_contract.rs line 227 at r1 (raw file):

    assert_eq!(main_call_info.tracked_resource, TrackedResource::SierraGas);
    assert!(main_call_info.execution.gas_consumed != 0);

Suggestion:

assert_ne!(main_call_info.execution.gas_consumed, 0);

---

crates/blockifier/src/execution/syscalls/syscall_tests/call_contract.rs line 238 at r1 (raw file):

    let second_inner_call = main_call_info.inner_calls.get(1).unwrap();
    assert_eq!(second_inner_call.tracked_resource, TrackedResource::SierraGas);
    assert!(second_inner_call.execution.gas_consumed != 0);

Suggestion:

assert_ne!(second_inner_call.execution.gas_consumed, 0);

---

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

        TrackedResource::CairoSteps => 0,
        TrackedResource::SierraGas => syscall_handler.base.call.initial_gas - gas,
    };

I have a suggestion: since gas_for_fee already exists and is implemented, why not have 2 PRs:

1. First, delete gas_consumed. when needed, use gas_for_fee (logically should be the same, but easier to review)
2. If you want, delete gas_for_fee (rename it to gas_consumed) as a second PR

WDYT?

Code quote:

    let gas_consumed = match tracked_resource {
        // Do not count Sierra gas in CairoSteps mode.
        TrackedResource::CairoSteps => 0,
        TrackedResource::SierraGas => syscall_handler.base.call.initial_gas - gas,
    };

---

crates/blockifier/src/transaction/transactions_test.rs line 0 at r1 (raw file):
when you set None as the last tracked resource in these tests - is it accurate? are you only calling the tracked_resource method on outermost calls (validate/execute)?

[dorimedini-starkware]

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Yoni-Starkware)

[Yoni-Starkware]

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

---

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

Previously, dorimedini-starkware wrote…

I have a suggestion: since gas_for_fee already exists and is implemented, why not have 2 PRs:

1. First, delete gas_consumed. when needed, use gas_for_fee (logically should be the same, but easier to review)
2. If you want, delete gas_for_fee (rename it to gas_consumed) as a second PR

WDYT?

I don't want to mess with gas_for_fee for now. I aim to align the blockifier and the OS regarding gas_consumed (the OS needs this field).

I'll let Tzahi handle gas_for_fee once he is back (can be removed)

---

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

Previously, dorimedini-starkware wrote…

when you set None as the last tracked resource in these tests - is it accurate? are you only calling the tracked_resource method on outermost calls (validate/execute)?

Right.

---

crates/blockifier/src/execution/contract_class.rs line 149 at r1 (raw file):

            Some(TrackedResource::CairoSteps) => TrackedResource::CairoSteps,
            Some(TrackedResource::SierraGas) => contract_tracked_resource,
            None => contract_tracked_resource,

Done.

---

crates/blockifier/src/execution/syscalls/syscall_tests/call_contract.rs line 227 at r1 (raw file):

    assert_eq!(main_call_info.tracked_resource, TrackedResource::SierraGas);
    assert!(main_call_info.execution.gas_consumed != 0);

Done.

---

crates/blockifier/src/execution/syscalls/syscall_tests/call_contract.rs line 238 at r1 (raw file):

    let second_inner_call = main_call_info.inner_calls.get(1).unwrap();
    assert_eq!(second_inner_call.tracked_resource, TrackedResource::SierraGas);
    assert!(second_inner_call.execution.gas_consumed != 0);

Done.

[Yoni-Starkware]

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

---

crates/blockifier/src/execution/contract_class.rs line 150 at r1 (raw file):

Previously, dorimedini-starkware wrote…

I think we are not ready yet to make this change.
we currently assume sierra gas consumption is zero if the user didn't sign on L1 data gas,
so you cna't run in gas tracking mode without changing this assumption first

Hey, I think we should keep it as-is. Let's talk F2F

[Yoni-Starkware]

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

---

crates/blockifier/src/execution/contract_class.rs line 150 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Hey, I think we should keep it as-is. Let's talk F2F

Oh no

[Yoni-Starkware]

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

---

crates/blockifier/src/execution/contract_class.rs line 150 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Oh no

Done in a separate PR

[dorimedini-starkware]

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