Skip to content

Conversation

@nimrod-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

nimrod-starkware commented Jul 1, 2025

@nimrod-starkware nimrod-starkware marked this pull request as ready for review July 1, 2025 07:43
@nimrod-starkware nimrod-starkware force-pushed the nimrod/aliases_test/empty_storage branch from 343bbb5 to c2ddcba Compare July 1, 2025 09:57
@nimrod-starkware nimrod-starkware force-pushed the nimrod/python_tests/run_from_rust branch from 60073ba to ab9acfb Compare July 1, 2025 10:43
@nimrod-starkware nimrod-starkware force-pushed the nimrod/aliases_test/empty_storage branch from c2ddcba to 4c54803 Compare July 1, 2025 10:43
@nimrod-starkware nimrod-starkware force-pushed the nimrod/python_tests/run_from_rust branch from ab9acfb to d07b00f Compare July 1, 2025 11:21
@nimrod-starkware nimrod-starkware force-pushed the nimrod/aliases_test/empty_storage branch from 4c54803 to e6c0fb5 Compare July 1, 2025 11:21
Copy link
Collaborator

@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.

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @amosStarkware)


crates/starknet_os/src/tests/aliases.rs line 56 at r1 (raw file):

#[case(Vec::from_iter((0..128).map(Felt::from)), (0..128).collect::<Vec<_>>(), HashMap::from_iter([(0.into(), 128.into())]))]
#[case(Vec::from_iter((0..129).map(Felt::from)), (0..129).collect::<Vec<_>>(), HashMap::from_iter([(0.into(), 129.into()), (128.into(), 128.into())]))]
#[case(vec![13.into(), 500.into(), 11.into(), 2000.into(), 2001.into(), 13.into(), 501.into(), 98.into(), 222.into(), 2000.into(), 127.into(), 128.into()], vec![13, 128, 11, 129, 130, 13, 131, 98, 132, 129, 127, 133], HashMap::from([(0.into(), 134.into()), (128.into(), 133.into()), (222.into(), 132.into()), (500.into(), 128.into()), (501.into(), 131.into()), (2000.into(), 129.into()), (2001.into(), 130.into())]))]

can you break long lines here? (no autoformatter for attributes..)

Code quote:

#[rstest]
#[case(Vec::new(), Vec::new(), HashMap::from([(0.into(), 128.into())]))]
#[case(vec![Felt::from(&*L2_ADDRESS_UPPER_BOUND)], vec![128], HashMap::from([(0.into(), 129.into()), (Felt::from(&*L2_ADDRESS_UPPER_BOUND), 128.into())]))]
#[case(vec![2000.into(), 1999999999.into(), 3000.into(), 2000.into()], vec![128, 129, 130, 128], HashMap::from([(0.into(), 131.into()), (2000.into(), 128.into()), (3000.into(), 130.into()), (1999999999.into(), 129.into())]))]
#[case(Vec::from_iter((0..128).map(Felt::from)), (0..128).collect::<Vec<_>>(), HashMap::from_iter([(0.into(), 128.into())]))]
#[case(Vec::from_iter((0..129).map(Felt::from)), (0..129).collect::<Vec<_>>(), HashMap::from_iter([(0.into(), 129.into()), (128.into(), 128.into())]))]
#[case(vec![13.into(), 500.into(), 11.into(), 2000.into(), 2001.into(), 13.into(), 501.into(), 98.into(), 222.into(), 2000.into(), 127.into(), 128.into()], vec![13, 128, 11, 129, 130, 13, 131, 98, 132, 129, 127, 133], HashMap::from([(0.into(), 134.into()), (128.into(), 133.into()), (222.into(), 132.into()), (500.into(), 128.into()), (501.into(), 131.into()), (2000.into(), 129.into()), (2001.into(), 130.into())]))]

crates/starknet_os/src/tests/aliases.rs line 80 at r1 (raw file):

    let unique_keys: HashSet<Felt> = HashSet::from_iter(
        keys.iter()
            .filter(|key| key >= &&INITIAL_AVAILABLE_ALIAS)

this is not in the original test, why is it needed here?

Code quote:

.filter(|key| key >= &&INITIAL_AVAILABLE_ALIAS)

crates/starknet_os/src/tests/aliases.rs line 82 at r1 (raw file):

            .filter(|key| key >= &&INITIAL_AVAILABLE_ALIAS)
            .copied()
            .chain([*ALIAS_COUNTER_STORAGE_KEY.key()]),

this is not in the original test, why is it needed here?

Code quote:

.chain([*ALIAS_COUNTER_STORAGE_KEY.key()]),

crates/starknet_os/src/tests/aliases.rs line 90 at r1 (raw file):

            (unique_keys.len()) * DICT_ACCESS_SIZE
            ])),
        EndpointArg::Pointer(PointerArg::Array(vec![Felt::ZERO; keys.len()])),

Suggestion:

// Aliases per-key ptr.
EndpointArg::Pointer(PointerArg::Array(vec![Felt::ZERO; keys.len()])),

crates/starknet_os/src/tests/aliases.rs line 95 at r1 (raw file):

    let keys_arg = EndpointArg::Pointer(PointerArg::Array(keys));
    let explicit_args = vec![n_keys_arg, keys_arg];
    let alias_contract_address: ContractAddress = Felt::TWO.try_into().unwrap();

don't we have a named constant for this?
maybe about time we have one... but maybe already defined in the blockifier?

Code quote:

Felt::TWO

crates/starknet_os/src/tests/aliases.rs line 129 at r1 (raw file):

            let new_value = aliases_storage_updates[i * DICT_ACCESS_SIZE + new_value_offset];
            actual_alias_storage.insert(key, new_value);
        }

maybe this should be a util - dictaccess vector to hashmap. non-blocking

Code quote:

        let n_aliases = felt_to_usize(n_aliases).unwrap();
        assert!(aliases_storage_updates.len() % DICT_ACCESS_SIZE == 0);
        assert!(aliases_storage_updates.len() / DICT_ACCESS_SIZE == n_aliases);
        let key_offset = 0;
        let new_value_offset = 2;
        for i in 0..n_aliases {
            let key = aliases_storage_updates[i * DICT_ACCESS_SIZE + key_offset];
            let new_value = aliases_storage_updates[i * DICT_ACCESS_SIZE + new_value_offset];
            actual_alias_storage.insert(key, new_value);
        }

crates/starknet_os/src/test_utils/utils.rs line 43 at r1 (raw file):

        hint_locals,
        None,
    )?;

for clarity of reading

Suggestion:

    let state_reader = None;
    let (actual_implicit_retdata, actual_explicit_retdata, _) = run_cairo_0_entry_point(
        runner_config,
        program_bytes,
        function_name,
        explicit_args,
        implicit_args,
        expected_explicit_retdata,
        hint_locals,
        state_reader,
    )?;

crates/starknet_os/src/tests/bls_field.rs line 154 at r1 (raw file):

            HashMap::new(),
            None,
        )

for clarity

Suggestion:

        let state_reader = None;
        let (_, explicit_retdata, _) = run_cairo_0_entry_point(
            &entrypoint_runner_config,
            OS_PROGRAM_BYTES,
            "starkware.starknet.core.os.data_availability.bls_field.horner_eval",
            &explicit_args,
            &implicit_args,
            &[EndpointArg::Value(ValueArg::Array(vec![Felt::ZERO, Felt::ZERO, Felt::ZERO]))],
            HashMap::new(),
            state_reader,
        )

crates/starknet_os/src/hint_processor/snos_hint_processor.rs line 353 at r1 (raw file):

            vec![state_reader],
        )?;
        hint_processor.execution_helpers_manager.increment_current_helper_index();

why is this needed here and not in run_os?

Code quote:

hint_processor.execution_helpers_manager.increment_current_helper_index();

crates/starknet_os/src/test_utils/cairo_runner.rs line 566 at r1 (raw file):

    expected_explicit_return_values: &[EndpointArg],
    hint_locals: HashMap<String, Box<dyn Any>>,
    state_reader: Option<DictStateReader>,

can this be generic?

Suggestion:

pub fn run_cairo_0_entry_point<S: StateReader>(
    runner_config: &EntryPointRunnerConfig,
    program_bytes: &[u8],
    entrypoint: &str,
    explicit_args: &[EndpointArg],
    implicit_args: &[ImplicitArg],
    expected_explicit_return_values: &[EndpointArg],
    hint_locals: HashMap<String, Box<dyn Any>>,
    state_reader: Option<S>,

Copy link
Contributor Author

@nimrod-starkware nimrod-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: 4 of 5 files reviewed, 9 unresolved discussions (waiting on @amosStarkware and @dorimedini-starkware)


crates/starknet_os/src/hint_processor/snos_hint_processor.rs line 353 at r1 (raw file):

Previously, dorimedini-starkware wrote…

why is this needed here and not in run_os?

Inrun_os it's called by a hint, and here not. A similar trick is done in python by injecting the hint scope variable aliases.


crates/starknet_os/src/test_utils/cairo_runner.rs line 566 at r1 (raw file):

Previously, dorimedini-starkware wrote…

can this be generic?

The function new_for_testing is not generic ATM, it's impossible to make it generic because of unwrap_or_default. we can add another constraint by S: StateReader + Default


crates/starknet_os/src/test_utils/utils.rs line 43 at r1 (raw file):

Previously, dorimedini-starkware wrote…

for clarity of reading

Done.


crates/starknet_os/src/tests/aliases.rs line 56 at r1 (raw file):

Previously, dorimedini-starkware wrote…

can you break long lines here? (no autoformatter for attributes..)

Done.


crates/starknet_os/src/tests/aliases.rs line 80 at r1 (raw file):

Previously, dorimedini-starkware wrote…

this is not in the original test, why is it needed here?

This calculation is for knowing the length of result.
It's needed here because the rust cairo runner should now in advance what is the expected length of the array returned.
In python we don't have to know in advance.


crates/starknet_os/src/tests/aliases.rs line 82 at r1 (raw file):

Previously, dorimedini-starkware wrote…

this is not in the original test, why is it needed here?

same as above. it's for calculating the length of the result.


crates/starknet_os/src/tests/aliases.rs line 95 at r1 (raw file):

Previously, dorimedini-starkware wrote…

don't we have a named constant for this?
maybe about time we have one... but maybe already defined in the blockifier?

We have it defined in the blockifier in a test file. It's impossible to import from there because it's gated under #[cfg(test)]


crates/starknet_os/src/tests/bls_field.rs line 154 at r1 (raw file):

Previously, dorimedini-starkware wrote…

for clarity

Done.

@nimrod-starkware nimrod-starkware force-pushed the nimrod/aliases_test/empty_storage branch from f32e267 to a02e05c Compare July 1, 2025 13:59
@github-actions
Copy link

github-actions bot commented Jul 1, 2025

@nimrod-starkware nimrod-starkware force-pushed the nimrod/aliases_test/empty_storage branch from a02e05c to 87c4177 Compare July 1, 2025 14:00
Copy link
Collaborator

@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.

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


crates/starknet_os/src/test_utils/cairo_runner.rs line 566 at r1 (raw file):

Previously, nimrod-starkware wrote…

The function new_for_testing is not generic ATM, it's impossible to make it generic because of unwrap_or_default. we can add another constraint by S: StateReader + Default

out of scope for now... I guess that in tests we'll always use the DictStateReader


crates/starknet_os/src/tests/aliases.rs line 95 at r1 (raw file):

Previously, nimrod-starkware wrote…

We have it defined in the blockifier in a test file. It's impossible to import from there because it's gated under #[cfg(test)]

hmmm... can you either

  1. move it to a non-test, public place, or
  2. add a TODO to do it?

crates/starknet_os/src/tests/aliases.rs line 87 at r3 (raw file):

    ])
)]
#[case(

what about this test case?

(
            [128 + 2**i for i in range(150)] + [128 + 2**i for i in range(150)],
            [128 + i for i in range(150)] + [128 + i for i in range(150)],
            [(0, 128 + 150)] + [(128 + 2**i, 128 + i) for i in range(150)],
        ),

Code quote:

)]
#[case(

@github-actions
Copy link

github-actions bot commented Jul 1, 2025

Benchmark movements: No major performance changes detected.

@nimrod-starkware nimrod-starkware force-pushed the nimrod/aliases_test/empty_storage branch from 87c4177 to d2492d4 Compare July 1, 2025 14:15
@nimrod-starkware nimrod-starkware force-pushed the nimrod/python_tests/run_from_rust branch from 8c182da to 5202af2 Compare July 1, 2025 14:15
Copy link
Collaborator

@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.

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @amosStarkware and @nimrod-starkware)

@nimrod-starkware nimrod-starkware changed the base branch from nimrod/python_tests/run_from_rust to graphite-base/7680 July 2, 2025 05:04
@nimrod-starkware nimrod-starkware force-pushed the nimrod/aliases_test/empty_storage branch from d2492d4 to bef6c71 Compare July 2, 2025 05:04
@nimrod-starkware nimrod-starkware changed the base branch from graphite-base/7680 to main-v0.14.0 July 2, 2025 05:04
@nimrod-starkware nimrod-starkware force-pushed the nimrod/aliases_test/empty_storage branch from bef6c71 to 5f88e8b Compare July 2, 2025 05:05
Copy link
Contributor Author

@nimrod-starkware nimrod-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: all files reviewed, 3 unresolved discussions (waiting on @amosStarkware and @dorimedini-starkware)


crates/starknet_os/src/tests/aliases.rs line 95 at r1 (raw file):

Previously, dorimedini-starkware wrote…

hmmm... can you either

  1. move it to a non-test, public place, or
  2. add a TODO to do it?

Done.


crates/starknet_os/src/tests/aliases.rs line 90 at r1 (raw file):

            (unique_keys.len()) * DICT_ACCESS_SIZE
            ])),
        EndpointArg::Pointer(PointerArg::Array(vec![Felt::ZERO; keys.len()])),

Done.

Copy link
Collaborator

@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.

Reviewed 4 of 4 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @amosStarkware and @nimrod-starkware)

@nimrod-starkware nimrod-starkware force-pushed the nimrod/aliases_test/empty_storage branch from 5f88e8b to 37951c0 Compare July 2, 2025 06:54
Copy link
Contributor Author

@nimrod-starkware nimrod-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: all files reviewed, 1 unresolved discussion (waiting on @amosStarkware and @dorimedini-starkware)


crates/starknet_os/src/tests/aliases.rs line 129 at r1 (raw file):

Previously, dorimedini-starkware wrote…

maybe this should be a util - dictaccess vector to hashmap. non-blocking

Done


crates/starknet_os/src/tests/aliases.rs line 87 at r3 (raw file):

Previously, dorimedini-starkware wrote…

what about this test case?

(
            [128 + 2**i for i in range(150)] + [128 + 2**i for i in range(150)],
            [128 + i for i in range(150)] + [128 + i for i in range(150)],
            [(0, 128 + 150)] + [(128 + 2**i, 128 + i) for i in range(150)],
        ),

Done.

Copy link
Collaborator

@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.

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @amosStarkware)


crates/starknet_os/src/test_utils/utils.rs line 81 at r6 (raw file):

    }
    dict
}

does this work? I think using chunks will make this nicer

Suggestion:

    assert!(squashed_dict.len() % DICT_ACCESS_SIZE == 0, "Invalid squashed dict length");
    squashed_dict
        .iter()
        .chunks(DICT_ACCESS_SIZE)
        .map(|[key, _old_val, new_val]| (key, new_val))
        .collect()
}

@nimrod-starkware nimrod-starkware force-pushed the nimrod/aliases_test/empty_storage branch from 37951c0 to a050ae6 Compare July 2, 2025 07:01
Copy link
Collaborator

@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.

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

@nimrod-starkware nimrod-starkware force-pushed the nimrod/aliases_test/empty_storage branch from a050ae6 to 71cc436 Compare July 2, 2025 07:09
Copy link
Contributor Author

@nimrod-starkware nimrod-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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @amosStarkware)


crates/starknet_os/src/test_utils/utils.rs line 81 at r6 (raw file):

Previously, dorimedini-starkware wrote…

does this work? I think using chunks will make this nicer

derefernence the chunk as [key, _old_val, new_val] doesn't work

Copy link
Collaborator

@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.

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

@nimrod-starkware nimrod-starkware added this pull request to the merge queue Jul 2, 2025
Merged via the queue into main-v0.14.0 with commit 2cac124 Jul 2, 2025
19 checks passed
victorkstarkware pushed a commit that referenced this pull request Jul 3, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Jul 3, 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