Skip to content

Conversation

@avivg-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@avivg-starkware avivg-starkware marked this pull request as ready for review August 6, 2025 15:16
Copy link
Contributor Author

avivg-starkware commented Aug 6, 2025

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

@avivg-starkware avivg-starkware changed the base branch from avivg/blockifier/use_bytecode_segment_felt_sizes to graphite-base/8462 August 7, 2025 07:55
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/replace_segment_length_w_felt_size branch from 05b347d to b3ae4c1 Compare August 7, 2025 07:58
@avivg-starkware avivg-starkware changed the base branch from graphite-base/8462 to avivg/blockifier/use_bytecode_segment_felt_sizes August 7, 2025 07:58
@avivg-starkware avivg-starkware changed the base branch from avivg/blockifier/use_bytecode_segment_felt_sizes to graphite-base/8462 August 7, 2025 08:22
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/replace_segment_length_w_felt_size branch from b3ae4c1 to 7899d22 Compare August 7, 2025 08:43
@avivg-starkware avivg-starkware changed the base branch from graphite-base/8462 to avivg/blockifier/use_bytecode_segment_felt_sizes August 7, 2025 08:44
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/use_bytecode_segment_felt_sizes branch from 3714347 to bb2a656 Compare August 7, 2025 09:50
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/replace_segment_length_w_felt_size branch from 7899d22 to 7e152d9 Compare August 7, 2025 09:50
Copy link
Collaborator

@noaov1 noaov1 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 r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware)


crates/native_blockifier/src/py_testing_wrappers.rs line 28 at r2 (raw file):

    let node = NestedMultipleIntList::Leaf(
        bytecode_segment_lengths,
        FeltSizeGroups { small: 0, large: 0 },

Consider deriving the default trait.

Code quote:

FeltSizeGroups { small: 0, large: 0 },

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/use_bytecode_segment_felt_sizes branch from bb2a656 to ac1be6d Compare August 10, 2025 08:33
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/replace_segment_length_w_felt_size branch from 7e152d9 to f1a364a Compare August 10, 2025 08:33
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/use_bytecode_segment_felt_sizes branch from ac1be6d to 4f7f1e8 Compare August 10, 2025 10:04
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/replace_segment_length_w_felt_size branch from f1a364a to 4dbfcfb Compare August 10, 2025 10:04
Copy link
Contributor Author

@avivg-starkware avivg-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: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware and @noaov1)


crates/native_blockifier/src/py_testing_wrappers.rs line 28 at r2 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Consider deriving the default trait.

yes thanks, added

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/use_bytecode_segment_felt_sizes branch from 4f7f1e8 to f9ffea8 Compare August 10, 2025 10:32
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/replace_segment_length_w_felt_size branch from 4dbfcfb to e04770e Compare August 10, 2025 10:32
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/use_bytecode_segment_felt_sizes branch from f9ffea8 to 0f703aa Compare August 10, 2025 12:47
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/replace_segment_length_w_felt_size branch from e04770e to 097fa12 Compare August 10, 2025 12:47
Copy link
Contributor

@AvivYossef-starkware AvivYossef-starkware left a comment

Choose a reason for hiding this comment

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

you should open a python PR for it.
Consider deleting this test if @Yoni-Starkware agrees, we have a similar test in Rust.
estimate_casm_hash_computation_resources_for_testing_list
estimate_casm_hash_computation_resources_for_testing_single

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noaov1)

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/replace_segment_length_w_felt_size branch from 097fa12 to 9f6a62e Compare August 10, 2025 17:14
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/use_bytecode_segment_felt_sizes branch from 0f703aa to 1616c8c Compare August 10, 2025 17:14
Copy link
Contributor Author

@avivg-starkware avivg-starkware left a comment

Choose a reason for hiding this comment

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

seems like this doesn't break, see Py PR: https://reviewable.io/reviews/starkware-industries/starkware/38860

Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware and @noaov1)

Copy link
Contributor

@AvivYossef-starkware AvivYossef-starkware 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: all files reviewed, 1 unresolved discussion (waiting on @noaov1)

Copy link
Collaborator

@noaov1 noaov1 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 2 of 3 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @avivg-starkware)

@avivg-starkware avivg-starkware changed the base branch from avivg/blockifier/use_bytecode_segment_felt_sizes to graphite-base/8462 August 11, 2025 11:29
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/replace_segment_length_w_felt_size branch from 9f6a62e to 8eb3f10 Compare August 11, 2025 11:29
@avivg-starkware avivg-starkware changed the base branch from graphite-base/8462 to main-v0.14.1 August 11, 2025 11:29
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/replace_segment_length_w_felt_size branch from 8eb3f10 to ceb85ba Compare August 11, 2025 12:33
@graphite-app
Copy link

graphite-app bot commented Aug 11, 2025

Merge activity

  • Aug 11, 12:37 PM UTC: This pull request can not be added to the Graphite merge queue. Please try rebasing and resubmitting to merge when ready.
  • Aug 11, 12:37 PM UTC: Graphite disabled "merge when ready" on this PR due to: a merge conflict with the target branch; resolve the conflict and try again..

Copy link
Contributor Author

@avivg-starkware avivg-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: 2 of 4 files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware and @noaov1)


crates/starknet_os/src/hints/hint_implementation/compiled_class/compiled_class_test.rs line 318 at r5 (raw file):

            &contract_class.get_bytecode_segment_lengths(),
            &contract_class.bytecode,
        ));

alternative approach:

Suggestion:

fn test_compiled_class_hash_resources_estimation() {
    // TODO(Aviv): Parameterize this test to run for both V1 and V2.
    let hash_version = HashVersion::V1;
    let feature_contract =
        FeatureContract::TestContract(CairoVersion::Cairo1(RunnableCairo1::Casm));
    let class = feature_contract.get_class();
    let (mut casm, compiled_class_v1) = match class {
        ContractClass::V1(class) => {
            let (casm, _sierra_version) = class.clone();
            let compiled_class_v1: CompiledClassV1 = class.try_into().unwrap();
            (casm, compiled_class_v1)
        }
        _ => panic!("Expected ContractClass::V1"),
    };

    // TODO(Aviv): Remove this once we estimate correctly compiled class hash with entry-points.
    contract_class.entry_points_by_type = Default::default();

    // Run the compiled class hash entry point with full contract loading.
    let (mut actual_execution_resources, _hash_computed_by_cairo) =
        run_compiled_class_hash_entry_point(&contract_class, true, &hash_version);

    // Compare the actual execution resources with the estimation with some allowed margin.
    let mut execution_resources_estimation =
        estimate_casm_poseidon_hash_computation_resources(compiled_class_v1.bytecode_segment_felt_sizes());

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/replace_segment_length_w_felt_size branch from ceb85ba to b0f69ca Compare August 11, 2025 12:51
Copy link
Contributor

@AvivYossef-starkware AvivYossef-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 2 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noaov1)

@avivg-starkware avivg-starkware added this pull request to the merge queue Aug 11, 2025
Merged via the queue into main-v0.14.1 with commit a3c7d1a Aug 11, 2025
21 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 13, 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.

5 participants