test(starknet_integration_tests): test commit blocks by running multiple heights

[yair-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[yair-starkware]

• test(starknet_integration_tests): test mempool syncing in e2e flow test #2628
• test(starknet_integration_tests): test proposer-validator setup in flow test #2348
• fix(starknet_integration_tests): temporary fixes to make the e2e flow test pass #2591
• fix(starknet_integration_tests): fix config when creating multiple nodes #2426
• refactor(starknet_integration_tests): separate sequencer setup from test setup #2299
• test(starknet_integration_tests): test commit blocks by running multiple heights #2242 👈 (View in Graphite)
• feat(tests_integration): one to many network config #2253
• main

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.60%. Comparing base (e3165c4) to head (8404158).
Report is 674 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2242       +/-   ##
===========================================
+ Coverage   40.10%   77.60%   +37.49%     
===========================================
  Files          26      389      +363     
  Lines        1895    41184    +39289     
  Branches     1895    41184    +39289     
===========================================
+ Hits          760    31961    +31201     
- Misses       1100     6962     +5862     
- Partials       35     2261     +2226     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[alonh5]

Nice PR! One semantic comment.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dan-starkware and @yair-starkware)

---

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 37 at r1 (raw file):

    let first_height = INITIAL_HEIGHT.unchecked_next();
    let last_height = first_height.unchecked_next();

First and initial mean the same, but they are different here, so it's a bit confusing. WDYT of this?
Also, the last height is only used for the iterator, so it is better to initialize it as it will be used.

Another option is const END_HEIGHT: BlockNumber = BlockNumber(3); Or you could implement for BlockNumber fn iter_up(usize).

Any of these is fine IMO.

Suggestion:

    const START_HEIGHT: BlockNumber = BASE_HEIGHT.unchecked_next();
    const END_HEIGHT: BlockNumber = START_HEIGHT.unchecked_next().unchecked_next();

[alonh5]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dan-starkware and @yair-starkware)

---

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 38 at r1 (raw file):

    let first_height = INITIAL_HEIGHT.unchecked_next();
    let last_height = first_height.unchecked_next();
    let expected_content_ids = [

Suggestion:

expected_block_hashes

---

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 43 at r1 (raw file):

        ),
        Felt::from_hex_unchecked(
            "0x7e2c0e448bea6bbf00962017d8addd56c6146d5beb5a273b2e02f5fb862d20f",

Did you make sure the txs are successful in the second block also? It must be checked manually once in order for us to "trust" this block hash.

Code quote:

"0x7e2c0e448bea6bbf00962017d8addd56c6146d5beb5a273b2e02f5fb862d20f"

[yair-starkware]

Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @alonh5 and @dan-starkware)

---

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 37 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

First and initial mean the same, but they are different here, so it's a bit confusing. WDYT of this?
Also, the last height is only used for the iterator, so it is better to initialize it as it will be used.

Another option is const END_HEIGHT: BlockNumber = BlockNumber(3); Or you could implement for BlockNumber fn iter_up(usize).

Any of these is fine IMO.

Changed the variables, WDYT?

---

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 43 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Did you make sure the txs are successful in the second block also? It must be checked manually once in order for us to "trust" this block hash.

I didn't.
What do you regard as a sufficient check? I need to calc the StateDiff imposed by the test txs, and calculate its StateDiffCommitment.
I think it is not in the scope of the e2e test to validate these calculations.
Perhaps it doesn't need to check the ContentId at all, just the tx hashes.

---

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 38 at r1 (raw file):

    let first_height = INITIAL_HEIGHT.unchecked_next();
    let last_height = first_height.unchecked_next();
    let expected_content_ids = [

I don't know why they convert to BlockHash in the consensus, that's a mistake IMO.
ContentId is the correct term.

[alonh5]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dan-starkware and @yair-starkware)

---

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 37 at r1 (raw file):

Previously, yair-starkware (Yair) wrote…

Changed the variables, WDYT?

Looks good.

---

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 43 at r1 (raw file):

Previously, yair-starkware (Yair) wrote…

I didn't.
What do you regard as a sufficient check? I need to calc the StateDiff imposed by the test txs, and calculate its StateDiffCommitment.
I think it is not in the scope of the e2e test to validate these calculations.
Perhaps it doesn't need to check the ContentId at all, just the tx hashes.

I meant just to check in the logs that the 3 txs in the iteration go into that second block.

---

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

    for (height, expected_content_id) in
        next_height.iter_up_to(LAST_HEIGHT.unchecked_next()).zip(expected_content_ids.iter())
    {

Check out zip_eq from iter_tools. I think you should use it here (and in general).

Suggestion:

    for (height, expected_content_id) in
        next_height.iter_up_to(LAST_HEIGHT).zip(expected_content_ids.iter())
    {

[alonh5]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dan-starkware and @yair-starkware)

---

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 43 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

I meant just to check in the logs that the 3 txs in the iteration go into that second block.

in the second* iteration

[yair-starkware]

Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @alonh5 and @dan-starkware)

---

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 43 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

I meant just to check in the logs that the 3 txs in the iteration go into that second block.

Oh yes I checked this

---

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

Previously, alonh5 (Alon Haramati) wrote…

Check out zip_eq from iter_tools. I think you should use it here (and in general).

Done.

[github-actions]

Artifacts upload workflows:

• Started at 2024-12-01T09:29:34Z
• Started at 2024-12-02T08:06:01Z
• Started at 2024-12-02T08:17:04Z
• Started at 2024-12-02T08:18:45Z
• Started at 2024-12-02T09:29:17Z
• Started at 2024-12-03T09:19:47Z
• Started at 2024-12-03T11:43:26Z

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.860 ms 34.914 ms 34.970 ms]
change: [-4.4188% -2.9505% -1.7035%] (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

[alonh5]

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

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.662 ms 34.745 ms 34.843 ms]
change: [-4.8273% -3.2811% -1.8970%] (p = 0.00 < 0.05)
Performance has improved.
Found 13 outliers among 100 measurements (13.00%)
3 (3.00%) high mild
10 (10.00%) high severe

[yair-starkware]

Merge activity

• Dec 2, 7:56 AM EST: Graphite disabled "merge when ready" on this PR due to: a merge conflict with the target branch; resolve the conflict and try again..
• Dec 3, 7:21 AM EST: A user merged this pull request with Graphite.

[yair-starkware]

Reviewed 2 of 5 files at r4, 3 of 3 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dan-starkware)

[alonh5]

Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dan-starkware)