Skip to content

starknet_os: os resources test - support virtual builtins, add sha256#14141

Open
dorimedini-starkware wants to merge 1 commit into
05-23-starknet_os_define_sha256_batch_resources_constantsfrom
05-23-starknet_os_os_resources_test_-_support_virtual_builtins_add_sha256
Open

starknet_os: os resources test - support virtual builtins, add sha256#14141
dorimedini-starkware wants to merge 1 commit into
05-23-starknet_os_define_sha256_batch_resources_constantsfrom
05-23-starknet_os_os_resources_test_-_support_virtual_builtins_add_sha256

Conversation

@dorimedini-starkware

Copy link
Copy Markdown
Collaborator

No description provided.

dorimedini-starkware commented May 24, 2026

Copy link
Copy Markdown
Collaborator Author

@reviewable-StarkWare

Copy link
Copy Markdown

This change is Reviewable

@dorimedini-starkware dorimedini-starkware self-assigned this May 24, 2026
@dorimedini-starkware dorimedini-starkware marked this pull request as ready for review May 24, 2026 07:03
@cursor

cursor Bot commented May 24, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Touches versioned OS program hashes, syscall gas costs, and fee/resource accounting used by blockifier and proving; incorrect virtual-builtin math would misreport Sha256 OS overhead in regression tests and constants.

Overview
Enables OS syscall resource regression for Sha256ProcessBlock by treating it as a virtual builtin whose batch work runs outside OsLogger, and rebases OS / fee constants after the OS rebuild.

The flow test harness adds SYSCALLS_WITH_VIRTUAL_BUILTINS and update_resources_for_virtual_builtin_syscall, which adds a per-block share of SHA256_BATCH_RESOURCES_LINEAR_UNSCALED (using SHA256_BATCH_SIZE = 7) to measured syscall overhead. Sha256ProcessBlock is removed from UNMEASURABLE_SYSCALLS, and OsResourcesTestContract now invokes sha256_process_block_syscall during __execute__.

Constants and expectations move together: allowed virtual OS program hash slot 2, program_hash.json (os / virtual_os), SHA256_PROCESS_BLOCK_GAS_COST (839995), Sha256ProcessBlock n_steps 1854, blockifier sha256 syscall test gas (850445), and the 0.14.3→0.14.4 diff regression file. starknet_os test utils expose unscaled vs scaled SHA256 batch linear resources and make BUILTIN_INSTANCE_SIZES public for sharing with flow tests.

Reviewed by Cursor Bugbot for commit 07d79f0. 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 035e95f. Configure here.

Comment thread crates/starknet_os_flow_tests/src/os_resources_test.rs Outdated
@dorimedini-starkware dorimedini-starkware force-pushed the 05-23-starknet_os_define_sha256_batch_resources_constants branch from fd6b95e to 062b998 Compare May 29, 2026 14:04
@dorimedini-starkware dorimedini-starkware force-pushed the 05-23-starknet_os_os_resources_test_-_support_virtual_builtins_add_sha256 branch from 61ef5ef to 2144fa8 Compare May 29, 2026 14:04
@dorimedini-starkware dorimedini-starkware force-pushed the 05-23-starknet_os_os_resources_test_-_support_virtual_builtins_add_sha256 branch from 2144fa8 to 1ba5ff7 Compare June 1, 2026 08:37
@dorimedini-starkware dorimedini-starkware force-pushed the 05-23-starknet_os_define_sha256_batch_resources_constants branch 2 times, most recently from 09cbdec to c5b45e5 Compare June 1, 2026 10:24
@dorimedini-starkware dorimedini-starkware force-pushed the 05-23-starknet_os_os_resources_test_-_support_virtual_builtins_add_sha256 branch 2 times, most recently from c190265 to fbcb731 Compare June 2, 2026 10:21
@dorimedini-starkware dorimedini-starkware force-pushed the 05-23-starknet_os_define_sha256_batch_resources_constants branch from c5b45e5 to a1487a4 Compare June 2, 2026 10:21

@yoavGrs yoavGrs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@yoavGrs reviewed 8 files and made 4 comments.
Reviewable status: 8 of 10 files reviewed, 4 unresolved discussions (waiting on dorimedini-starkware and Yoni-Starkware).


crates/starknet_os/src/test_utils.rs line 22 at r2 (raw file):

// Resources consumed by the SHA-256 batch phase, separated into linear and constant factors.
pub const SHA256_BLOCK_TO_ROUND: usize = 7;

Or how you called it in the previous PR.

Suggestion:

SHA256_BLOCKS_IN_ROUND

crates/starknet_os_flow_tests/src/os_resources_test.rs line 114 at r2 (raw file):

                n_steps: linear_steps,
                builtin_instance_counter: linear_builtin_instance_counter,
                n_memory_holes: linear_memory_holes,

Please

Suggestion:

                round_n_steps: linear_steps,
                round_builtin_instance_counter: linear_builtin_instance_counter,
                round_n_memory_holes: linear_memory_holes,

crates/starknet_os_flow_tests/src/os_resources_test.rs line 120 at r2 (raw file):

            for (builtin, count) in linear_builtin_instance_counter.iter() {
                *new_resources.builtin_instance_counter.entry(*builtin).or_insert(0) +=
                    BUILTIN_INSTANCE_SIZES.get(builtin).unwrap() * count / SHA256_BLOCK_TO_ROUND;

In SHA256_BATCH_RESOURCES_LINEAR the count was the number of instances, now you count memory cells.
There, the resources are divided by the builtin size, and here you multiply by the same number.

Code quote:

BUILTIN_INSTANCE_SIZES.get(builtin).unwrap() * count

crates/blockifier/resources/blockifier_versioned_constants_0_14_4.json line 468 at r2 (raw file):

                "builtin_instance_counter": {
                    "range_check_builtin": 65,
                    "bitwise_builtin": 1114

Number of builtins changes.

Code quote:

"bitwise_builtin": 1114

@yoavGrs yoavGrs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@yoavGrs reviewed 2 files and all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on dorimedini-starkware and Yoni-Starkware).

@dorimedini-starkware dorimedini-starkware changed the base branch from 05-23-starknet_os_define_sha256_batch_resources_constants to graphite-base/14141 June 3, 2026 07:50
@dorimedini-starkware dorimedini-starkware force-pushed the 05-23-starknet_os_os_resources_test_-_support_virtual_builtins_add_sha256 branch from fbcb731 to 4d23c23 Compare June 3, 2026 07:57
@dorimedini-starkware dorimedini-starkware changed the base branch from graphite-base/14141 to 05-23-starknet_os_define_sha256_batch_resources_constants June 3, 2026 07:58

@dorimedini-starkware dorimedini-starkware left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@dorimedini-starkware made 4 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on yoavGrs and Yoni-Starkware).


crates/blockifier/resources/blockifier_versioned_constants_0_14_4.json line 468 at r2 (raw file):

Previously, yoavGrs wrote…

Number of builtins changes.

keep this blocking until this PR is close to main, I'll rerun the numbers...


crates/starknet_os/src/test_utils.rs line 22 at r2 (raw file):

Previously, yoavGrs wrote…

Or how you called it in the previous PR.

changed to batch size, to match the cairo-common terminology


crates/starknet_os_flow_tests/src/os_resources_test.rs line 114 at r2 (raw file):

Previously, yoavGrs wrote…

Please

Done.


crates/starknet_os_flow_tests/src/os_resources_test.rs line 120 at r2 (raw file):

Previously, yoavGrs wrote…

In SHA256_BATCH_RESOURCES_LINEAR the count was the number of instances, now you count memory cells.
There, the resources are divided by the builtin size, and here you multiply by the same number.

correct, this is for visibility - the numbers in cairo-common are given in builtin instances. I hard-code the numbers scaled thus so that they can be compared to the cairo-common impl

@yoavGrs yoavGrs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@yoavGrs reviewed 8 files and all commit messages, and resolved 3 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on Yoni-Starkware).

@dorimedini-starkware dorimedini-starkware changed the base branch from 05-23-starknet_os_define_sha256_batch_resources_constants to graphite-base/14141 June 7, 2026 15:51
@dorimedini-starkware dorimedini-starkware force-pushed the 05-23-starknet_os_os_resources_test_-_support_virtual_builtins_add_sha256 branch from 4d23c23 to e1d301a Compare June 8, 2026 10:23
@dorimedini-starkware dorimedini-starkware changed the base branch from graphite-base/14141 to 05-23-starknet_os_define_sha256_batch_resources_constants June 8, 2026 10:23
@dorimedini-starkware dorimedini-starkware force-pushed the 05-23-starknet_os_os_resources_test_-_support_virtual_builtins_add_sha256 branch from e1d301a to 07d79f0 Compare June 9, 2026 11:02
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.

3 participants