Skip to content

starknet_os: proper base measurement in OS resources test #14294

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dorimedini-starkware wants to merge 1 commit into from
Closed

starknet_os: proper base measurement in OS resources test #14294

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions crates/blockifier/resources/blockifier_versioned_constants_0_14_4.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,15 +247,15 @@
},
"Deploy": {
"constant": {
"n_steps": 1181,
"n_steps": 1171,
"n_memory_holes": 0,
"builtin_instance_counter": {
"range_check_builtin": 21,
"pedersen_builtin": 8
"pedersen_builtin": 7
}
},
"calldata_factor": {
"n_steps": 8,
"n_steps": 10,
"n_memory_holes": 0,
"builtin_instance_counter": {
"pedersen_builtin": 1
Expand Down Expand Up @@ -361,11 +361,11 @@
},
"MetaTxV0": {
"constant": {
"n_steps": 1315,
"n_steps": 1307,
"n_memory_holes": 0,
"builtin_instance_counter": {
"range_check_builtin": 20,
"pedersen_builtin": 10
"pedersen_builtin": 9
}
},
"calldata_factor": {
Expand Down
31 changes: 25 additions & 6 deletions crates/starknet_os_flow_tests/src/os_resources_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use std::collections::HashSet;
use std::collections::{HashMap, HashSet};
use std::sync::LazyLock;

use assert_matches::assert_matches;
use blockifier::blockifier_versioned_constants::{
Expand Down Expand Up @@ -76,10 +77,22 @@ const UNMEASURABLE_SYSCALLS: [Selector; 12] = [
Selector::StorageWrite,
];

/// Keccak does not store the linear factor in the same entry in the versioned constants, but it
/// does have a measurable linear factor stored under [Selector::KeccakRound].
const SYSCALLS_WITH_LINEAR_FACTOR: [Selector; 3] =
[Selector::Deploy, Selector::Keccak, Selector::MetaTxV0];
/// Store a mapping from a linearly-charged syscall, with the number of "linear elements" in it's
/// base measurement. For example, if we measure the base and linear costs of a [Selector::Deploy]
/// by running:
/// ```
/// deploy_syscall(stable_class_hash, 3, array![2, 0, 0].span(), true).unwrap_syscall();
/// deploy_syscall(stable_class_hash, 3, array![3, 0, 0, 0].span(), true).unwrap_syscall();
/// ```
/// ... then the linear factor is exactly the difference between the two measurements, because the
/// second measurement has one more linear element (4) than the first measurement (3). However, the
/// first call has 3 elements in it's calldata, so to compute the estimated cost of a zero-element
/// call (base) we need to subtract three times the linear factor from the first measurement.
/// Note: Keccak does not store the linear factor in the same entry in the versioned constants, but
/// it does have a measurable linear factor stored under [Selector::KeccakRound].
const SYSCALLS_WITH_LINEAR_FACTOR: LazyLock<HashMap<Selector, usize>> = LazyLock::new(|| {
HashMap::from([(Selector::Deploy, 1), (Selector::Keccak, 0), (Selector::MetaTxV0, 1)])
});

@cursor cursor Bot Jun 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const with LazyLock defeats lazy caching purpose

Low Severity

SYSCALLS_WITH_LINEAR_FACTOR is declared as const but wraps a LazyLock<HashMap<...>>. In Rust, const items are inlined at each use site, so every reference to this identifier creates a brand-new LazyLock and re-allocates the HashMap — completely defeating the lazy-initialization caching that LazyLock provides. Since it's referenced inside a while loop (at line 436), a fresh HashMap is built on every iteration. Every other LazyLock in this crate correctly uses static. This is the well-known declare_interior_mutable_const anti-pattern; changing const to static fixes it.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit c8fdcf3. Configure here.


/// Syscalls that are implemented using virtual builtins. Such syscalls have their "heavy lifting"
/// executed after the execute_syscalls part of the OS, so the consumed resources are not captured
Expand Down Expand Up @@ -420,7 +433,7 @@ async fn test_os_resources_regression() {

// If this if a syscall with a linear factor, the next syscall should be the linear cost.
// Otherwise, this syscall has a constant cost.
if SYSCALLS_WITH_LINEAR_FACTOR.contains(&selector) {
if let Some(linear_count_in_base) = SYSCALLS_WITH_LINEAR_FACTOR.get(&selector) {
let next_syscall_trace = syscalls_iter.next().unwrap();
assert_eq!(
selector,
Expand All @@ -434,6 +447,12 @@ async fn test_os_resources_regression() {
- &next_inner_overhead)
.filter_unused_builtins();
let linear_factor_resources = (&next_resources - &resources).filter_unused_builtins();

// Linear factor is computed; deduct the linear overhead from the base cost to get the
// real base cost.
resources = (&resources - &(&linear_factor_resources * *linear_count_in_base))
.filter_unused_builtins();

// Keccak is a special case - we store the linear cost as a separate syscall.
if selector == Selector::Keccak {
// TODO(Dori): Currently, the Keccak base cost is enforced in the OS to equal the
Expand Down
Loading