Skip to content

starknet_os: proper base measurement in OS resources test#14294

Closed
dorimedini-starkware wants to merge 1 commit into
05-27-starknet_os_os_resources_test_-_add_sha512from
06-02-starknet_os_proper_base_measurement_in_os_resources_test
Closed

starknet_os: proper base measurement in OS resources test#14294
dorimedini-starkware wants to merge 1 commit into
05-27-starknet_os_os_resources_test_-_add_sha512from
06-02-starknet_os_proper_base_measurement_in_os_resources_test

Conversation

@dorimedini-starkware

Copy link
Copy Markdown
Collaborator

No description provided.

@reviewable-StarkWare

Copy link
Copy Markdown

This change is Reviewable

dorimedini-starkware commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator Author

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown

Artifacts upload workflows:

@dorimedini-starkware dorimedini-starkware marked this pull request as ready for review June 2, 2026 10:22
@cursor

cursor Bot commented Jun 2, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Test methodology and fee/resource constant JSON only; no runtime execution path changes beyond aligned syscall cost tables.

Overview
Fixes how the OS resources regression test derives constant (base) costs for syscalls with a calldata linear factor (Deploy, MetaTxV0, and the Keccak path).

The test now tracks how many linear elements are baked into the first paired measurement (e.g. deploy calldata length) and subtracts linear_factor × count from that trace before recording the base cost. Previously it treated the first syscall trace as the base even when it still included per-element overhead.

Versioned constants for 0.14.4 are refreshed to match the corrected measurements: lower Deploy / MetaTxV0 constant steps and Pedersen counts, and a higher Deploy per-calldata step factor.

Reviewed by Cursor Bugbot for commit c8fdcf3. Bugbot is set up for automated code reviews on this repo. Configure here.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit c8fdcf3. Configure here.

/// 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)])
});

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants