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

feat(starknet_integration_tests): sequencer simulator binary #4032

Merged
merged 1 commit into from
Feb 17, 2025

Conversation

nadin-Starkware
Copy link
Collaborator

@nadin-Starkware nadin-Starkware commented Feb 9, 2025

commit-id:9e5db078


Stack:

⚠️ Part of a stack created by spr. Do not merge manually using the UI - doing so may have unexpected results.

@reviewable-StarkWare
Copy link

This change is Reviewable

@nadin-Starkware nadin-Starkware force-pushed the spr/main/9e5db078 branch 2 times, most recently from 7705719 to 5aa46ca Compare February 9, 2025 10:54
@nadin-Starkware nadin-Starkware force-pushed the spr/main/9e5db078 branch 3 times, most recently from aedfbc6 to 6d14144 Compare February 9, 2025 12:04
Copy link

github-actions bot commented Feb 9, 2025

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [35.549 ms 35.772 ms 36.038 ms]
change: [+1.4429% +2.1071% +2.9320%] (p = 0.00 < 0.05)
Performance has regressed.
Found 14 outliers among 100 measurements (14.00%)
3 (3.00%) high mild
11 (11.00%) high severe

Copy link
Contributor

@lev-starkware lev-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 5 files reviewed, 1 unresolved discussion


crates/starknet_integration_tests/src/sequencer_simulator_utils.rs line 54 at r1 (raw file):

    pub async fn await_execution(&self, expected_block_number: BlockNumber) {
        await_execution(&self.monitoring_client, expected_block_number, 0, 0).await;

I prefer a fully qualified name for distinction.

Code quote:

await_execution

@nadin-Starkware nadin-Starkware force-pushed the spr/main/9e5db078 branch 2 times, most recently from 7210400 to 63f869d Compare February 10, 2025 09:39
Copy link
Collaborator Author

@nadin-Starkware nadin-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 5 files reviewed, 1 unresolved discussion (waiting on @lev-starkware)


crates/starknet_integration_tests/src/sequencer_simulator_utils.rs line 54 at r1 (raw file):

Previously, lev-starkware wrote…

I prefer a fully qualified name for distinction.

Done.

Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.816 ms 34.839 ms 34.864 ms]
change: [-3.9934% -2.4396% -1.0631%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
2 (2.00%) high mild
2 (2.00%) high severe

Copy link
Contributor

@lev-starkware lev-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 2 of 5 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @nadin-Starkware)

Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [35.466 ms 35.500 ms 35.538 ms]
change: [-4.4357% -3.0196% -1.7844%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
3 (3.00%) high mild
1 (1.00%) high severe

Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-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 5 files reviewed, 1 unresolved discussion (waiting on @lev-starkware)


crates/starknet_integration_tests/src/bin/sequencer_simulator.rs line 23 at r3 (raw file):

        default_panic(info);
        std::process::exit(1);
    }));

The other stack iirc wrapped this as a function. Add a todo (on you) to use the fn after it is merged.

Code quote:

    // TODO(Tsabary): remove the hook definition once we transition to proper usage of task
    // spawning.
    let default_panic = std::panic::take_hook();
    std::panic::set_hook(Box::new(move |info| {
        default_panic(info);
        std::process::exit(1);
    }));

Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-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 5 files reviewed, 2 unresolved discussions (waiting on @lev-starkware and @nadin-Starkware)


crates/starknet_integration_tests/src/bin/sequencer_simulator.rs line 24 at r3 (raw file):

        std::process::exit(1);
    }));
    const SENDER_ACCOUNT: AccountId = 0;

This should be consistent with the setup binary, right? If so, please define them in a unified place.

Code quote:

const SENDER_ACCOUNT: AccountId = 0;

@nadin-Starkware nadin-Starkware changed the base branch from spr/main/7c11e54a to main February 16, 2025 14:03
Copy link
Collaborator Author

@nadin-Starkware nadin-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 5 files reviewed, 2 unresolved discussions (waiting on @Itay-Tsabary-Starkware and @lev-starkware)


crates/starknet_integration_tests/src/bin/sequencer_simulator.rs line 23 at r3 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

The other stack iirc wrapped this as a function. Add a todo (on you) to use the fn after it is merged.

Done.


crates/starknet_integration_tests/src/bin/sequencer_simulator.rs line 24 at r3 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

This should be consistent with the setup binary, right? If so, please define them in a unified place.

Done.

@nadin-Starkware nadin-Starkware force-pushed the spr/main/9e5db078 branch 2 times, most recently from 70a0c2b to 230eb41 Compare February 16, 2025 14:15
Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-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 3 files at r2, 1 of 4 files at r3, 1 of 3 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @nadin-Starkware)

Copy link
Collaborator Author

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

@nadin-Starkware nadin-Starkware added this pull request to the merge queue Feb 17, 2025
Merged via the queue into main with commit 9b20f37 Feb 17, 2025
26 of 28 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Feb 19, 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