Skip to content

Conversation

@dorimedini-starkware
Copy link
Collaborator

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@github-actions
Copy link

github-actions bot commented Apr 15, 2025

Artifacts upload workflows:

@dorimedini-starkware dorimedini-starkware self-assigned this Apr 15, 2025
@dorimedini-starkware dorimedini-starkware marked this pull request as ready for review April 15, 2025 14:16
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_define_and_implement_static_program branch from 97349df to 608a692 Compare April 15, 2025 14:31
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_define_and_implement_static_programhash branch from 00e05d9 to ff0b371 Compare April 15, 2025 14:31
@dorimedini-starkware dorimedini-starkware marked this pull request as draft April 15, 2025 14:32
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_define_and_implement_static_programhash branch from ff0b371 to 4d1905e Compare April 15, 2025 14:55
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_define_and_implement_static_program branch from 608a692 to 7c2ee53 Compare April 15, 2025 15:08
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_define_and_implement_static_programhash branch from 4d1905e to 52df3de Compare April 15, 2025 15:08
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_define_and_implement_static_program branch from 7c2ee53 to f5f2e37 Compare April 15, 2025 15:48
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_define_and_implement_static_programhash branch from 52df3de to 2746f5a Compare April 15, 2025 15:49
@github-actions
Copy link

github-actions bot commented Apr 15, 2025

Benchmark movements: tree_computation_flow performance regressed! tree_computation_flow time: [35.606 ms 36.143 ms 36.758 ms] change: [+2.7374% +4.2896% +6.3430%] (p = 0.00 < 0.05) Performance has regressed. Found 11 outliers among 100 measurements (11.00%) 1 (1.00%) high mild 10 (10.00%) high severe

@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_define_and_implement_static_program branch from f5f2e37 to a9cbf18 Compare April 15, 2025 16:15
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_define_and_implement_static_programhash branch from 2746f5a to 433717f Compare April 15, 2025 16:15
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_define_and_implement_static_program branch from a9cbf18 to ae04ec1 Compare April 15, 2025 16:32
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_define_and_implement_static_programhash branch from 433717f to aa3ae7a Compare April 15, 2025 16:32
@github-actions
Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.573 ms 34.665 ms 34.772 ms]
change: [-4.4529% -2.9354% -1.5644%] (p = 0.00 < 0.05)
Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
3 (3.00%) high mild
4 (4.00%) high severe

@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_define_and_implement_static_program branch from 5954e4d to 84eeaec Compare May 19, 2025 22:10
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_define_and_implement_static_programhash branch from 8a8817e to 7e5d772 Compare May 19, 2025 22:11
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_define_and_implement_static_program branch from 84eeaec to 3265e5d Compare May 25, 2025 14:54
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_define_and_implement_static_programhash branch from 7e5d772 to 81cecce Compare May 25, 2025 14:54
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_define_and_implement_static_program branch from 3265e5d to 8ad4e5b Compare May 25, 2025 15:26
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_define_and_implement_static_programhash branch from 81cecce to 6a6017c Compare May 25, 2025 15:26
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_define_and_implement_static_program branch from 8ad4e5b to 1500f5b Compare May 25, 2025 17:09
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_define_and_implement_static_programhash branch from 6a6017c to e4e2c5a Compare May 25, 2025 17:10
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_define_and_implement_static_program branch from 1500f5b to eedaf6d Compare May 26, 2025 08:01
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_define_and_implement_static_programhash branch from e4e2c5a to 724b31e Compare May 26, 2025 08:02
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_define_and_implement_static_program branch from eedaf6d to 7c19baf Compare May 26, 2025 09:50
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_define_and_implement_static_programhash branch from 724b31e to 7fd8590 Compare May 26, 2025 09:50
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_define_and_implement_static_program branch from 7c19baf to 1216f60 Compare May 26, 2025 11:51
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_define_and_implement_static_programhash branch from 7fd8590 to 694999d Compare May 26, 2025 11:52
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_define_and_implement_static_program branch from 1216f60 to 30e8ec6 Compare May 26, 2025 13:10
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_define_and_implement_static_programhash branch from 694999d to 2d08ecc Compare May 26, 2025 13:10
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_define_and_implement_static_program branch from 30e8ec6 to ea3a188 Compare May 26, 2025 13:43
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_define_and_implement_static_programhash branch 2 times, most recently from ef70b43 to d37e6be Compare May 26, 2025 14:51
@dorimedini-starkware dorimedini-starkware changed the base branch from 04-13-feat_apollo_starknet_os_program_define_and_implement_static_program to main-v0.14.0 May 26, 2025 14:51
Copy link
Contributor

@TzahiTaub TzahiTaub left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 6 files at r1, 4 of 4 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @amosStarkware and @nimrod-starkware)


crates/apollo_starknet_os_program/src/program_hash.rs line 26 at r2 (raw file):

pub struct ProgramHash {
    os: Felt,
}

?

Suggestion:

pub struct OsProgramHash(pub Felt);

crates/apollo_starknet_os_program/src/program_hash.rs line 46 at r3 (raw file):

            let builtin_bytes = builtin.to_str().to_string().into_bytes();
            if builtin_bytes.len() > 32 {
                Err(ProgramHashError::BuiltinNameTooLong(*builtin))

Consider (as the problem is with the str repr and not the enum)

Suggestion:

builtin.to_str()

crates/apollo_starknet_os_program/src/program_hash.rs line 61 at r3 (raw file):

    let program_header = vec![
        Felt::from(BOOTLOADER_VERSION),
        Felt::from(OS_PROGRAM.get_stripped_program()?.main),

This seems like a too heavy function call (copies everything) just to get this field. I propose making this field public in the cairo-vm crate if possible, so we can do this (or a getter)

Suggestion:

(OS_PROGRAM.shared_program_data.main

Copy link
Contributor

@TzahiTaub TzahiTaub left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @amosStarkware, @dorimedini-starkware, and @nimrod-starkware)

@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_define_and_implement_static_programhash branch from d37e6be to 97979f0 Compare May 27, 2025 07:56
Copy link
Collaborator Author

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 5 of 6 files reviewed, all discussions resolved (waiting on @amosStarkware, @nimrod-starkware, and @TzahiTaub)


crates/apollo_starknet_os_program/src/program_hash.rs line 26 at r2 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

?

next PRs add aggregator program hashes (here)

Copy link
Contributor

@TzahiTaub TzahiTaub left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @amosStarkware and @nimrod-starkware)

@dorimedini-starkware dorimedini-starkware added this pull request to the merge queue May 27, 2025
Merged via the queue into main-v0.14.0 with commit 0f2dc42 May 27, 2025
24 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators May 28, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants