Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(blockifier): update compiler version #2709

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

meship-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

@meship-starkware meship-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 4 files at r1.
Reviewable status: 1 of 4 files reviewed, all discussions resolved

@meship-starkware meship-starkware force-pushed the meship/update_compiler_ver_to_2.9.2 branch from 9c05b9e to b618cf8 Compare December 17, 2024 08:09
Copy link
Contributor Author

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


crates/blockifier/src/execution/syscalls/hint_processor.rs line 667 at r2 (raw file):

            Hint::Core(hint) => execute_core_hint_base(vm, exec_scopes, hint, false),
            Hint::Starknet(hint) => self.execute_next_syscall(vm, hint),
            Hint::External(_) => todo!("Handle External Hints."),

This is not the right solution, but I am not sure what external hints represent and how I should handle them

Code quote:

            Hint::Core(hint) => execute_core_hint_base(vm, exec_scopes, hint, false),
            Hint::Starknet(hint) => self.execute_next_syscall(vm, hint),
            Hint::External(_) => todo!("Handle External Hints."),

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 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @meship-starkware)


crates/blockifier/src/execution/syscalls/hint_processor.rs line 665 at r2 (raw file):

        let hint = hint_data.downcast_ref::<Hint>().ok_or(HintError::WrongHintData)?;
        match hint {
            Hint::Core(hint) => execute_core_hint_base(vm, exec_scopes, hint, false),

What does the false mean?

Code quote:

            Hint::Core(hint) => execute_core_hint_base(vm, exec_scopes, hint, false),

@meship-starkware meship-starkware force-pushed the meship/update_compiler_ver_to_2.9.2 branch from b618cf8 to 001b236 Compare December 17, 2024 08:47
Copy link
Contributor Author

@meship-starkware meship-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 12 of 16 files at r3.
Reviewable status: 15 of 19 files reviewed, 2 unresolved discussions (waiting on @noaov1)

Copy link
Contributor Author

@meship-starkware meship-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: 15 of 19 files reviewed, 4 unresolved discussions (waiting on @noaov1)


crates/blockifier/src/execution/syscalls/hint_processor.rs line 665 at r2 (raw file):

Previously, noaov1 (Noa Oved) wrote…

What does the false mean?

I am not sure this is the comment in the Cairo compiler:
/// Avoid allocating memory segments so that the finalization of the segment arena may not occur.
So, to my understanding, false here means we do not avoid allocating memory segments, but I am not sure I will ask someone from the compiler team.


crates/blockifier/src/transaction/transactions_test.rs line 2438 at r3 (raw file):

        computation: ComputationResources {
            vm_resources: expected_execution_resources,
            sierra_gas: gas_consumed.into(),

The cosmetic change for Cllipy does not relate to the Cairo version update.

Code quote:

sierra_gas: gas_consumed.into(),

crates/blockifier/src/execution/syscalls/syscall_tests/keccak.rs line 29 at r3 (raw file):

    pretty_assertions::assert_eq!(
        entry_point_call.execute_directly(&mut state).unwrap().execution,
        CallExecution { gas_consumed: 254240, ..CallExecution::from_retdata(retdata![]) }

@ilyalesokhin-starkware, Can you make sure this gas change looks logical after the compiler version update?

Code quote:

CallExecution { gas_consumed: 254240, ..CallExecution::from_retdata(retdata![]) }

@meship-starkware meship-starkware force-pushed the meship/update_compiler_ver_to_2.9.2 branch from 001b236 to c54523a Compare December 17, 2024 09:19
Copy link
Contributor Author

@meship-starkware meship-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: 15 of 19 files reviewed, 4 unresolved discussions (waiting on @noaov1)


crates/blockifier/src/execution/syscalls/hint_processor.rs line 665 at r2 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

I am not sure this is the comment in the Cairo compiler:
/// Avoid allocating memory segments so that the finalization of the segment arena may not occur.
So, to my understanding, false here means we do not avoid allocating memory segments, but I am not sure I will ask someone from the compiler team.

Talked with Ori Ziv, and I understood correctly but as the segment arena finalization is only relevant to the proofs, so we do want to avoid allocating temporary segments


crates/blockifier/src/execution/syscalls/hint_processor.rs line 667 at r2 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

This is not the right solution, but I am not sure what external hints represent and how I should handle them

The sequencer shouldn't get external hints

Copy link

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [35.370 ms 35.852 ms 36.421 ms]
change: [+1.8157% +3.3087% +4.7272%] (p = 0.00 < 0.05)
Performance has regressed.
Found 10 outliers among 100 measurements (10.00%)
2 (2.00%) high mild
8 (8.00%) high severe

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 3 of 16 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions

@meship-starkware meship-starkware force-pushed the meship/update_compiler_ver_to_2.9.2 branch from c54523a to 4955476 Compare December 17, 2024 11:10
Copy link
Contributor Author

@meship-starkware meship-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 4 files at r5.
Reviewable status: 16 of 19 files reviewed, 2 unresolved discussions (waiting on @noaov1)

Copy link
Contributor Author

@meship-starkware meship-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 4 files at r5.
Reviewable status: 17 of 19 files reviewed, 2 unresolved discussions (waiting on @noaov1)

Copy link
Contributor Author

@meship-starkware meship-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 4 files at r5.
Reviewable status: 18 of 19 files reviewed, 2 unresolved discussions (waiting on @noaov1)

@ilyalesokhin-starkware
Copy link
Contributor

crates/blockifier/src/execution/syscalls/syscall_tests/keccak.rs line 29 at r3 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

@ilyalesokhin-starkware, Can you make sure this gas change looks logical after the compiler version update?

Looks fine

Copy link

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [35.111 ms 35.633 ms 36.238 ms]
change: [+1.2879% +2.8270% +4.3503%] (p = 0.00 < 0.05)
Performance has regressed.
Found 13 outliers among 100 measurements (13.00%)
3 (3.00%) high mild
10 (10.00%) high severe

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 4 of 16 files at r3, 1 of 4 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @meship-starkware)

@meship-starkware meship-starkware merged commit bfd97e2 into main Dec 17, 2024
21 checks passed
@meship-starkware meship-starkware deleted the meship/update_compiler_ver_to_2.9.2 branch December 17, 2024 13: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.

4 participants