Add sierra gas resource tracking #3049

THenry14 · 2025-03-06T07:58:44Z

Related to #2977

This PR stack aims to add support for new resource tracking (sierra-gas) and new gas representation (GasVector triplet instead of single l1 gas value).

--> (This PR) Add sierra gas tracking
-- (#3050) Run already existing tests with CairoSteps set explicitly
-- (#3051) Add support for sierra gas in --detailed-resources
-- (#3052) Add support for sierra gas in --save-trace-data
-- (#3053) Add sierra version assertion
-- (#3054) Introduce new gas representation
-- (#3055) Add gas tests for sierra-gas tracking
-- (#3056) Update docs with sierra-gas related changes

...heatnet/src/runtime_extensions/call_to_blockifier_runtime_extension/execution/entry_point.rs

crates/cheatnet/src/state.rs

crates/forge-runner/src/forge_config.rs

crates/cheatnet/src/runtime_extensions/forge_runtime_extension/mod.rs

ddoktorski · 2025-03-07T18:20:32Z

crates/cheatnet/src/runtime_extensions/forge_runtime_extension/mod.rs

+fn n_steps_to_sierra_gas(n_steps: usize, versioned_constants: &VersionedConstants) -> GasAmount {
+    let n_steps_u64 = u64_from_usize(n_steps);
+    let gas_per_step = versioned_constants
+        .os_constants
+        .gas_costs
+        .base
+        .step_gas_cost;
+    let n_steps_gas_cost = n_steps_u64.checked_mul(gas_per_step).unwrap_or_else(|| {
+        panic!(
+            "Multiplication overflow while converting steps to gas. steps: {n_steps}, gas per step: {gas_per_step}."
+        )
+    });
+    GasAmount(n_steps_gas_cost)
+}
+
+#[must_use]
+pub fn vm_resources_to_sierra_gas(
+    resources: &ExecutionResources,
+    versioned_constants: &VersionedConstants,
+) -> GasAmount {
+    let builtins_gas_cost =
+        builtins_to_sierra_gas(&resources.prover_builtins(), versioned_constants);
+    let n_steps_gas_cost = n_steps_to_sierra_gas(resources.total_n_steps(), versioned_constants);
+    n_steps_gas_cost.checked_add(builtins_gas_cost).unwrap_or_else(|| {
+        panic!(
+            "Addition overflow while converting vm resources to gas. steps gas: {n_steps_gas_cost}, \
+            builtins gas: {builtins_gas_cost}."
+        )
+    })
+}


Is Sierra gas calculated this way exactly the same as real Sierra gas calculated when executing with Cairo-native?

My question was not related to the conversion logic. I was wondering because what we generally do is not exactly tracking Sierra resources, but rather tracking VM resources and then converting them to Sierra resources. So my question is, are we sure that executing the same transaction with cairo-native will result in exactly the same Sierra gas as calculated in snforge with the current logic? According to the starknet documentation, it seems that this may not necessarily be the case. Please correct me if I am wrong.

This is not really true - we are tracking sierra resources, but the execution comes through the vm (because at the moment, we do not support cairo native execution in forge; I believe it is not even yet planned). We are converting things here, because vm has some pre-defined "spin-up" cost that is given in steps and builtins - which are not the way to describe execution cost when tracking sierra gas; when going through native the same cost would be described in sierra gas.

Regarding the docs you've linked (and answering the question) - we are as sure as we can be running thrugh vm. The cost when tracking sierra gas should not differ regardless of the environment, but we'll never be 100% sure as long as we do not support native, I guess. To reiterate my point from the previous paragraph - we are not tracking cairo steps and converting them; we are tracking sierra gas, and take them as they are reported by the vm, no conversion there. The docs state that there will be some mismatch because of the cost of the resources and max vs sum approach - which is true and you can actually observe it in the tests I added in #3055, where identical tests yield different costs depending on the tracked resource.

Hope that answers your question, but if something still seems off please let me know 🙏

cptartur · 2025-03-10T10:09:35Z

...heatnet/src/runtime_extensions/call_to_blockifier_runtime_extension/execution/entry_point.rs

+    match &context
+        .tracked_resource_stack
+        .last()
+        .expect("Unexpected empty tracked resource.")


Nit, but maybe these should be passed as an argument to this function instead of taking them of the stack? For me it's like a post execution callback

Quite the contrast from this comment by Darek #3049 (comment) :D

My rationale for doing this here was that we already have a context, so there was really no need to do it this way, but I'm fine with either one.

Don't have a strong opinion on this, we can keep this as is

cptartur · 2025-03-10T10:10:54Z

...heatnet/src/runtime_extensions/call_to_blockifier_runtime_extension/execution/entry_point.rs

+            resources -= &versioned_constants.get_additional_os_syscall_resources(syscall_counter);
+        }
+        TrackedResource::SierraGas => {
+            let syscalls_consumed_gas: u64 = syscall_counter


I think we should already rebase ourselves to new changes to main-0.13.4 branch in blockifier (rebase my fork).

They've removed the syscall_counter and replaced it with a similar structure with different name and layout.

Are there any changes on our side regarding this? if not I'd rather not, as this is out od scope of what this PR is supposed to do. If yes, I can add a PR to this stack with required changes.

I think just this but didn't verify in details. They actually already have main-0.13.5 branch now.

crates/cheatnet/src/runtime_extensions/forge_runtime_extension/mod.rs

cptartur · 2025-03-10T10:16:01Z

crates/cheatnet/src/runtime_extensions/forge_runtime_extension/mod.rs

        l1_handler_payload_lengths,
        l2_to_l1_payload_lengths,
    }
 }
+
+fn n_steps_to_sierra_gas(n_steps: usize, versioned_constants: &VersionedConstants) -> GasAmount {
+    let n_steps_u64 = u64_from_usize(n_steps);


What it does exactly? Why not just unwrap/expect here?

This does exactly what you said it should. But hey have a helper for this in blockifier (and use it), so I figured - what the heck I'll use it too. If you rather not, I can change it to regular expect(). 👍

I mean maybe there is a reason they do that. I'm fine with either.

cptartur · 2025-03-10T10:19:12Z

crates/cheatnet/src/runtime_extensions/forge_runtime_extension/mod.rs

+            let syscalls_consumed_gas: u64 = top_call_syscalls
+                .iter()
+                .map(|(name, count)| {
+                    versioned_constants.get_syscall_gas_cost(name) * (*count as u64)
+                })
+                .sum();
+            sierra_gas_consumed += syscalls_consumed_gas;


Similar code is used in remove_syscall_resources_and_exit_success_call. Maybe it'd make sense to reuse it as a function?

To be sure - You only mean the part of fetching consumed gas by syscalls, right?
I can do it if you think this will bring us some gains, but personaly I find it too trivial and not really worth it to make a separate function out of 🤔

This whole logic. It's not necessary, but we are kind of repeating ourselves with this.

Suggested change

let syscalls_consumed_gas: u64 = top_call_syscalls

.iter()

.map(|(name, count)| {

versioned_constants.get_syscall_gas_cost(name) * (*count as u64)

})

.sum();

sierra_gas_consumed += syscalls_consumed_gas;

let syscalls_consumed_gas: u64 = top_call_syscalls

.iter()

.map(|(name, count)| {

versioned_constants.get_syscall_gas_cost(name) * (*count as u64)

})

.sum();

sierra_gas_consumed += syscalls_consumed_gas;

crates/forge-runner/src/forge_config.rs

crates/forge-runner/src/running.rs

cptartur · 2025-03-10T10:23:50Z

crates/forge-runner/src/running/config_run.rs

@@ -83,7 +87,7 @@ pub fn run_config_pass(
    let string_to_hint = hints_by_representation(&assembled_program);
    let hints_dict = hints_to_params(&assembled_program);

-    let mut context = build_context(&block_info, None);
+    let mut context = build_context(&block_info, None, &TrackedResource::from(tracked_resource));


Not sure if it matters for config pass anyway. But if you already added the logic to provide correct resource here, we can keep it.

Sorry, I'm not sure I follow :( Can you rephrase or specify please?

This part is specifically for the so called 'configuration run' which doesn't execute any tests and only collect configuration from attributes. Tracking resources here does not matter as they are not counted for anything.

That is true 👍 But I still don't know if any/what action is needed from me here :D

What I meant is that you probably didn't need to provide correct tracked_resource here and could just hardcode some value. But no need to change that.

crates/cheatnet/src/runtime_extensions/forge_runtime_extension/mod.rs

crates/forge-runner/src/forge_config.rs

cptartur

Looks good, just please address the comments if needed

cptartur · 2025-03-11T17:15:22Z

...heatnet/src/runtime_extensions/call_to_blockifier_runtime_extension/execution/entry_point.rs

+    match &context
+        .tracked_resource_stack
+        .last()
+        .expect("Unexpected empty tracked resource.")


Don't have a strong opinion on this, we can keep this as is

cptartur · 2025-03-11T17:15:57Z

...heatnet/src/runtime_extensions/call_to_blockifier_runtime_extension/execution/entry_point.rs

+            resources -= &versioned_constants.get_additional_os_syscall_resources(syscall_counter);
+        }
+        TrackedResource::SierraGas => {
+            let syscalls_consumed_gas: u64 = syscall_counter


I think just this but didn't verify in details. They actually already have main-0.13.5 branch now.

cptartur · 2025-03-11T17:19:11Z

crates/cheatnet/src/runtime_extensions/forge_runtime_extension/mod.rs

+            let syscalls_consumed_gas: u64 = top_call_syscalls
+                .iter()
+                .map(|(name, count)| {
+                    versioned_constants.get_syscall_gas_cost(name) * (*count as u64)
+                })
+                .sum();
+            sierra_gas_consumed += syscalls_consumed_gas;


This whole logic. It's not necessary, but we are kind of repeating ourselves with this.

Suggested change

let syscalls_consumed_gas: u64 = top_call_syscalls

.iter()

.map(|(name, count)| {

versioned_constants.get_syscall_gas_cost(name) * (*count as u64)

})

.sum();

sierra_gas_consumed += syscalls_consumed_gas;

let syscalls_consumed_gas: u64 = top_call_syscalls

.iter()

.map(|(name, count)| {

versioned_constants.get_syscall_gas_cost(name) * (*count as u64)

})

.sum();

sierra_gas_consumed += syscalls_consumed_gas;

cptartur · 2025-03-11T17:34:23Z

crates/cheatnet/src/runtime_extensions/forge_runtime_extension/mod.rs

        l1_handler_payload_lengths,
        l2_to_l1_payload_lengths,
    }
 }
+
+fn n_steps_to_sierra_gas(n_steps: usize, versioned_constants: &VersionedConstants) -> GasAmount {
+    let n_steps_u64 = u64_from_usize(n_steps);


I mean maybe there is a reason they do that. I'm fine with either.

cptartur · 2025-03-11T17:35:38Z

crates/cheatnet/src/runtime_extensions/forge_runtime_extension/mod.rs

+// region: Modified blockifier code
+// https://github.com/starkware-libs/sequencer/blob/main-v0.13.4/crates/blockifier/src/bouncer.rs#L320


Nit but maybe we can just do that

Suggested change

// region: Modified blockifier code

// https://github.com/starkware-libs/sequencer/blob/main-v0.13.4/crates/blockifier/src/bouncer.rs#L320

// Based on https://github.com/starkware-libs/sequencer/blob/main-v0.13.4/crates/blockifier/src/bouncer.rs#L320

if we aren't marking specific modified parts anyway. Or you can mark the modified parts in code

cptartur · 2025-03-11T17:37:38Z

crates/forge-runner/src/running/config_run.rs

@@ -83,7 +87,7 @@ pub fn run_config_pass(
    let string_to_hint = hints_by_representation(&assembled_program);
    let hints_dict = hints_to_params(&assembled_program);

-    let mut context = build_context(&block_info, None);
+    let mut context = build_context(&block_info, None, &TrackedResource::from(tracked_resource));


What I meant is that you probably didn't need to provide correct tracked_resource here and could just hardcode some value. But no need to change that.

THenry14 marked this pull request as ready for review March 6, 2025 08:51

THenry14 requested a review from a team as a code owner March 6, 2025 08:51

THenry14 requested review from ddoktorski, kkawula and cptartur March 6, 2025 08:51

THenry14 added 2 commits March 6, 2025 13:34

Add sierra gas resource tracking

d4020ed

[skip ci] Pin cairo and smoll_str versions, rebase

6169638

THenry14 force-pushed the szymczyk/sg-1-add-sierra-gas-tracking branch from 56e4694 to 6169638 Compare March 6, 2025 12:56

[skip ci] Fix changelog after rebase

0a92b0b

ddoktorski reviewed Mar 7, 2025

View reviewed changes

cptartur reviewed Mar 10, 2025

View reviewed changes

ksew1 reviewed Mar 10, 2025

View reviewed changes

crates/cheatnet/src/runtime_extensions/forge_runtime_extension/mod.rs Outdated Show resolved Hide resolved

crates/cheatnet/src/runtime_extensions/forge_runtime_extension/mod.rs Show resolved Hide resolved

crates/forge-runner/src/forge_config.rs Outdated Show resolved Hide resolved

Review changes

d7fd230

THenry14 requested review from ddoktorski, cptartur and ksew1 March 10, 2025 12:39

ksew1 approved these changes Mar 11, 2025

View reviewed changes

cptartur approved these changes Mar 11, 2025

View reviewed changes

ddoktorski approved these changes Mar 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sierra gas resource tracking #3049

Add sierra gas resource tracking #3049

THenry14 commented Mar 6, 2025 •

edited

Loading

ddoktorski Mar 7, 2025

THenry14 Mar 10, 2025

ddoktorski Mar 10, 2025

THenry14 Mar 11, 2025

cptartur Mar 10, 2025

THenry14 Mar 10, 2025

cptartur Mar 11, 2025

cptartur Mar 10, 2025

THenry14 Mar 10, 2025

cptartur Mar 11, 2025

cptartur Mar 10, 2025

THenry14 Mar 10, 2025

cptartur Mar 11, 2025

cptartur Mar 10, 2025

THenry14 Mar 10, 2025

cptartur Mar 11, 2025

cptartur Mar 10, 2025

THenry14 Mar 10, 2025

ddoktorski Mar 10, 2025

THenry14 Mar 11, 2025

cptartur Mar 11, 2025

cptartur left a comment

cptartur Mar 11, 2025

cptartur Mar 11, 2025

cptartur Mar 11, 2025

cptartur Mar 11, 2025

cptartur Mar 11, 2025

cptartur Mar 11, 2025

		// region: Modified blockifier code
		// https://github.com/starkware-libs/sequencer/blob/main-v0.13.4/crates/blockifier/src/bouncer.rs#L320

	// region: Modified blockifier code
	// https://github.com/starkware-libs/sequencer/blob/main-v0.13.4/crates/blockifier/src/bouncer.rs#L320
	// Based on https://github.com/starkware-libs/sequencer/blob/main-v0.13.4/crates/blockifier/src/bouncer.rs#L320

Add sierra gas resource tracking #3049

Are you sure you want to change the base?

Add sierra gas resource tracking #3049

Conversation

THenry14 commented Mar 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cptartur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

THenry14 commented Mar 6, 2025 •

edited

Loading