feat(starknet_integration_tests): refactor e2e flow test #4787

yair-starkware · 2025-03-10T09:22:50Z

Check success on aggregated txs instead of per height

yair-starkware · 2025-03-10T09:23:03Z

feat(starknet_integration_tests): refactor e2e flow test #4787 : 2 dependent PRs (#5164 , #5223 ) 👈 (View in Graphite)
feat(starknet_integration_tests): struct to collect batched txs #4786
feat(starknet_integration_tests): struct to collect streamed txs #4785
refactor(starknet_integration_tests): copy listen_to_broadcasted_messages to test setup #4784
main

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

reviewable-StarkWare · 2025-03-10T09:23:09Z

This change is

graphite-app · 2025-03-10T09:25:23Z

Graphite Automations

"Yair - Auto-assign" took an action on this PR • (03/10/25)

1 assignee was added to this PR based on Yair's automation.

github-actions · 2025-03-10T09:46:18Z

alonh5

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: 2 of 4 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware and @Yael-Starkware)

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 100 at r2 (raw file):

        tokio::time::timeout(TEST_SENARIO_TIMOUT, async {
            for tx in expected_batched_tx_hashes {

In the many_txs_scenario we need to check that only the expected txs were batched.

Yael-Starkware

Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @alonh5 and @ArniStarkware)

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 100 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

In the many_txs_scenario we need to check that only the expected txs were batched.

what stops the test from producing another block with the remaining txs?

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 109 at r2 (raw file):

                    }
                    tokio::time::sleep(Duration::from_millis(2000)).await;
                }

Suggestion:

            while !expected_batched_tx_hashes.is_empty() {
                let batched_txs =
                    mock_running_system.aggregated_txs.lock().await.get_all_txs_so_far();

                expected_batched_tx_hashes.retain(|tx| !batched_txs.contains(tx));

                tokio::time::sleep(Duration::from_millis(2000)).await;
            }

Yael-Starkware

Reviewable status: 2 of 4 files reviewed, 3 unresolved discussions (waiting on @alonh5, @ArniStarkware, and @yair-starkware)

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 59 at r2 (raw file):

    configure_tracing().await;

    const TEST_SENARIO_TIMOUT: std::time::Duration = std::time::Duration::from_secs(50);

Suggestion:

TEST_SCENARIO_TIMOUT

Yael-Starkware

Reviewable status: 2 of 4 files reviewed, 3 unresolved discussions (waiting on @alonh5, @ArniStarkware, and @yair-starkware)

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 109 at r2 (raw file):

                    }
                    tokio::time::sleep(Duration::from_millis(2000)).await;
                }

Suggestion:

            loop {
                println!(
                    "Waiting for txs {} to be included in a block.",
                    expected_batched_tx_hashes
                );

                let batched_txs =
                    mock_running_system.aggregated_txs.lock().await.get_all_txs_so_far();
                expected_batched_tx_hashes.retain(|tx| !batched_txs.contains(tx));
                if expected_batched_tx_hashes.is_empty() {
                    break;
                }

                tokio::time::sleep(Duration::from_millis(2000)).await;
            }

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 109 at r2 (raw file):

                    }
                    tokio::time::sleep(Duration::from_millis(2000)).await;
                }

retracting this one and suggesting a refined version in the next comment.

alonh5

Reviewable status: 2 of 4 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware, @Yael-Starkware, and @yair-starkware)

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 100 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

what stops the test from producing another block with the remaining txs?

Nothing, we should test that at some point only the expected txs where batched. Or can you think of another way to verify the block was closed on size and not time?

yair-starkware

Reviewable status: 2 of 4 files reviewed, 3 unresolved discussions (waiting on @alonh5, @ArniStarkware, and @Yael-Starkware)

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 100 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Nothing, we should test that at some point only the expected txs where batched. Or can you think of another way to verify the block was closed on size and not time?

How does this verify that it was closed on size and not on time?

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 109 at r2 (raw file):

                    }
                    tokio::time::sleep(Duration::from_millis(2000)).await;
                }

Why is one better than the other?

github-actions · 2025-03-16T12:10:10Z

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [35.903 ms 36.371 ms 36.917 ms]
change: [+1.3800% +2.6707% +4.2010%] (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

alonh5

Reviewable status: 1 of 4 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware, @Yael-Starkware, and @yair-starkware)

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 100 at r2 (raw file):

Previously, yair-starkware (Yair) wrote…

How does this verify that it was closed on size and not on time?

Actually nothing, you're right. Maybe we should just decrease the max gas amount even more and send a lot of txs.
Also maybe we should add a metric for batches closed on time/size for the dashboard, and then we could use it here. (in a different PR). WDYT?

yair-starkware

Reviewable status: 0 of 5 files reviewed, 4 unresolved discussions (waiting on @alonh5, @ArniStarkware, and @Yael-Starkware)

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 108 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

This is cool that this way you can print which txs we're still waiting for. Can you use a new remaining_expected_batched_tx_hashes variable so in the end you can also assert the other direction:
expected_batched_tx_hashes == batched_txs

It's a problem in the too_many_txs test because the additional txs get to the accumulated txs too.

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 162 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

This can be removed now.

Done.

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 163 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Also these aren't blocks anymore.

Done.

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 160 at r6 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

There is no longer a need to split it into two test cases. (other than the fact that closing a block on size must have more than 15 txs).
This test case can be squashed into the first test case.

Non-blocking, as this may be complicated.

Can't because limiting the block_max_capacity_sierra_gas for the too_many_txs is causing the l1handler tx to fail in the batcher

alonh5

Reviewed 3 of 5 files at r7, 2 of 3 files at r8, 1 of 1 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ArniStarkware, @Yael-Starkware, and @yair-starkware)

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 108 at r4 (raw file):

Previously, yair-starkware (Yair) wrote…

It's a problem in the too_many_txs test because the additional txs get to the accumulated txs too.

Like we said we aren't checking those 12 tx are in one block anyways here right?
You can decrease even more the gas amount and send more txs just to be sure the block will close on size, then you can add this assertion.
In a separate PR can we check the logs we're getting a block full log?

ArniStarkware

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yael-Starkware and @yair-starkware)

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 160 at r6 (raw file):

Previously, yair-starkware (Yair) wrote…

Can't because limiting the block_max_capacity_sierra_gas for the too_many_txs is causing the l1handler tx to fail in the batcher

Can you increase the number of Txs be more than 15?
Also not sure - because that case the block might be closed on time before it is closed on size.
But, as I said - out of scope.

yair-starkware

Reviewable status: 1 of 5 files reviewed, 1 unresolved discussion (waiting on @alonh5 and @Yael-Starkware)

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 108 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Like we said we aren't checking those 12 tx are in one block anyways here right?
You can decrease even more the gas amount and send more txs just to be sure the block will close on size, then you can add this assertion.
In a separate PR can we check the logs we're getting a block full log?

Added a todo and will try it in a separate PR

yair-starkware · 2025-03-23T09:51:02Z

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 108 at r4 (raw file):

Previously, yair-starkware (Yair) wrote…

Added a todo and will try it in a separate PR

Added the check of the other direction

Check success on aggregated txs instead of per height

alonh5

Reviewed 5 of 5 files at r11, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Yael-Starkware)

This was referenced Mar 10, 2025

refactor(starknet_integration_tests): copy listen_to_broadcasted_messages to test setup #4784

Merged

feat(starknet_integration_tests): struct to collect streamed txs #4785

Merged

feat(starknet_integration_tests): struct to collect batched txs #4786

Merged

yair-starkware marked this pull request as ready for review March 10, 2025 09:23

graphite-app bot assigned yair-starkware Mar 10, 2025

yair-starkware requested a review from alonh5 March 10, 2025 09:28

yair-starkware force-pushed the yair/refactor_e2e branch from 9951e7d to 525642b Compare March 10, 2025 09:46

yair-starkware requested review from ArniStarkware and Yael-Starkware March 10, 2025 09:49

yair-starkware force-pushed the yair/tx_collector branch from cb2f624 to b29b947 Compare March 10, 2025 09:52

yair-starkware force-pushed the yair/refactor_e2e branch from 525642b to 9c8f12c Compare March 10, 2025 09:52

alonh5 requested changes Mar 10, 2025

View reviewed changes

Yael-Starkware requested changes Mar 11, 2025

View reviewed changes

alonh5 requested changes Mar 11, 2025

View reviewed changes

yair-starkware requested review from alonh5 and Yael-Starkware March 16, 2025 11:57

yair-starkware commented Mar 16, 2025

View reviewed changes

yair-starkware force-pushed the yair/tx_collector branch from b29b947 to cf7afc2 Compare March 16, 2025 11:57

yair-starkware force-pushed the yair/refactor_e2e branch from 9c8f12c to 9e532a7 Compare March 16, 2025 11:57

alonh5 requested changes Mar 16, 2025

View reviewed changes

yair-starkware force-pushed the yair/tx_collector branch from cf7afc2 to 0b924a0 Compare March 16, 2025 12:37

yair-starkware force-pushed the yair/refactor_e2e branch from 9e532a7 to 30c8f25 Compare March 16, 2025 12:37

yair-starkware requested a review from alonh5 March 16, 2025 12:51

yair-starkware changed the base branch from yair/tx_collector to graphite-base/4787 March 20, 2025 09:21

yair-starkware force-pushed the graphite-base/4787 branch from 1edddd9 to cce21a7 Compare March 20, 2025 09:21

yair-starkware force-pushed the yair/refactor_e2e branch from ae41407 to f14a08c Compare March 20, 2025 09:21

yair-starkware changed the base branch from graphite-base/4787 to main March 20, 2025 09:22

yair-starkware requested review from alonh5 and Yael-Starkware March 20, 2025 09:22

yair-starkware commented Mar 20, 2025

View reviewed changes

yair-starkware force-pushed the yair/refactor_e2e branch 2 times, most recently from 2f87781 to 89f9261 Compare March 20, 2025 09:29

alonh5 requested changes Mar 20, 2025

View reviewed changes

yair-starkware force-pushed the yair/refactor_e2e branch from 89f9261 to 3cac284 Compare March 20, 2025 13:18

ArniStarkware reviewed Mar 20, 2025

View reviewed changes

yair-starkware force-pushed the yair/refactor_e2e branch from 3cac284 to 7324887 Compare March 23, 2025 08:54

yair-starkware requested a review from alonh5 March 23, 2025 09:00

yair-starkware commented Mar 23, 2025

View reviewed changes

yair-starkware mentioned this pull request Mar 23, 2025

refactor(starknet_integration_tests): consolidate e2e flow test cases #5164

Closed

yair-starkware force-pushed the yair/refactor_e2e branch from 7324887 to 3d2f9f7 Compare March 24, 2025 12:28

yair-starkware mentioned this pull request Mar 24, 2025

feat(starknet_batcher): metric for blocks closed on capacity #5223

Merged

yair-starkware force-pushed the yair/refactor_e2e branch from 3d2f9f7 to 4b27ecd Compare March 24, 2025 12:50

yair-starkware mentioned this pull request Mar 24, 2025

test(starknet_integration_tests): assert the full blocks flow #5229

Merged

feat(starknet_integration_tests): refactor e2e flow test

56359b8

Check success on aggregated txs instead of per height

yair-starkware force-pushed the yair/refactor_e2e branch from 4b27ecd to 56359b8 Compare March 25, 2025 11:36

alonh5 approved these changes Mar 27, 2025

View reviewed changes

yair-starkware added this pull request to the merge queue Mar 27, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 27, 2025

yair-starkware added this pull request to the merge queue Mar 27, 2025

Merged via the queue into main with commit 94ae504 Mar 27, 2025
23 checks passed

github-actions bot locked and limited conversation to collaborators Mar 29, 2025

feat(starknet_integration_tests): refactor e2e flow test #4787

feat(starknet_integration_tests): refactor e2e flow test #4787

Uh oh!

Conversation

yair-starkware commented Mar 10, 2025

Uh oh!

yair-starkware commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

reviewable-StarkWare commented Mar 10, 2025

Uh oh!

graphite-app bot commented Mar 10, 2025

Graphite Automations

Uh oh!

github-actions bot commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alonh5 left a comment

Choose a reason for hiding this comment

Uh oh!

Yael-Starkware left a comment

Choose a reason for hiding this comment

Uh oh!

Yael-Starkware left a comment

Choose a reason for hiding this comment

Uh oh!

Yael-Starkware left a comment

Choose a reason for hiding this comment

Uh oh!

alonh5 left a comment

Choose a reason for hiding this comment

Uh oh!

yair-starkware left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 16, 2025

Uh oh!

alonh5 left a comment

Choose a reason for hiding this comment

Uh oh!

yair-starkware left a comment

Choose a reason for hiding this comment

Uh oh!

alonh5 left a comment

Choose a reason for hiding this comment

Uh oh!

ArniStarkware left a comment

Choose a reason for hiding this comment

Uh oh!

yair-starkware left a comment

Choose a reason for hiding this comment

Uh oh!

yair-starkware commented Mar 23, 2025

Uh oh!

alonh5 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yair-starkware commented Mar 10, 2025 •

edited

Loading

github-actions bot commented Mar 10, 2025 •

edited

Loading