refactor(starknet_integration_tests): separate sequencer setup from test setup

[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 👈 (View in Graphite)
• test(starknet_integration_tests): test commit blocks by running multiple heights #2242
• 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 37.38%. Comparing base (e3165c4) to head (60b70c2).
Report is 782 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2299      +/-   ##
==========================================
- Coverage   40.10%   37.38%   -2.73%     
==========================================
  Files          26      280     +254     
  Lines        1895    32301   +30406     
  Branches     1895    32301   +30406     
==========================================
+ Hits          760    12075   +11315     
- Misses       1100    19208   +18108     
- Partials       35     1018     +983     

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

[Itay-Tsabary-Starkware]

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

---

crates/starknet_integration_tests/src/integration_test_setup.rs line 55 at r1 (raw file):

        let (mut consensus_manager_configs, _consensus_proposals_channels) =
            create_consensus_manager_configs_and_channels(1);

Could you please explain where this values come from?

Code quote:

1

---

crates/starknet_integration_tests/src/integration_test_setup.rs line 59 at r1 (raw file):

        // Derive the configuration for the sequencer node.
        let (config, required_params) = create_config(
            0,

Could you please explain where this values come from?

Code quote:

0

---

crates/starknet_integration_tests/src/flow_test_setup.rs line 42 at r1 (raw file):

    pub async fn new_from_tx_generator(tx_generator: &MultiAccountTransactionGenerator) -> Self {
        let handle = Handle::current();
        let task_executor = TokioExecutor::new(handle);

Unrelated to this pr, but you can drop this completely.

Code quote:

        let handle = Handle::current();
        let task_executor = TokioExecutor::new(handle);

---

crates/starknet_integration_tests/src/flow_test_setup.rs line 47 at r1 (raw file):

        let accounts = tx_generator.accounts();
        let (mut consensus_manager_configs, consensus_proposals_channels) =
            create_consensus_manager_configs_and_channels(1);

Could you please explain where this values come from?

Code quote:

1

---

crates/starknet_integration_tests/src/flow_test_setup.rs line 49 at r1 (raw file):

            create_consensus_manager_configs_and_channels(1);

        let proposer_consensus_manager_config = consensus_manager_configs.remove(0);

Could you please explain where this values come from? Should this be PROPOSER_ID?

Code quote:

0

---

crates/starknet_integration_tests/src/utils.rs line 49 at r1 (raw file):

}

// TODO(yair, itay): Create config presets for tests.

Please change to Tsabary, otherwise I won't find it :-)

Code quote:

 itay

---

crates/starknet_integration_tests/src/utils.rs line 57 at r1 (raw file):

    mut consensus_manager_config: ConsensusManagerConfig,
) -> (SequencerNodeConfig, RequiredParams) {
    set_validator_id(&mut consensus_manager_config, sequencer_id);

Could we please unify the validator id and sequencer address config parameters, assuming they have the same meaning?
@matan-starkware @alonh5

Code quote:

set_validator_id(&mut consensus_manager_config, sequencer_id);

---

crates/starknet_integration_tests/src/utils.rs line 63 at r1 (raw file):

    let http_server_config = create_http_server_config().await;
    let rpc_state_reader_config = test_rpc_state_reader_config(rpc_server_addr);
    let mempool_p2p_config = create_mempool_p2p_config(sequencer_id, chain_info.chain_id.clone());

Why do you need to explicitly mention chain id? The config pointer mapping should address this, no?

Code quote:

    let mempool_p2p_config = create_mempool_p2p_config(sequencer_id, chain_info.chain_id.clone());

---

crates/starknet_integration_tests/src/utils.rs line 259 at r1 (raw file):

    )
    .unwrap();
}

Can this be passed as an arg through the preset file?

Code quote:

fn set_validator_id(consensus_manager_config: &mut ConsensusManagerConfig, sequencer_id: usize) {
    consensus_manager_config.consensus_config.validator_id = ValidatorId::try_from(
        Felt::from(consensus_manager_config.consensus_config.validator_id)
            + Felt::from(sequencer_id),
    )
    .unwrap();
}

---

crates/starknet_integration_tests/src/utils.rs line 268 at r1 (raw file):

    let mut config = MempoolP2pConfig::default();
    config.network_config.tcp_port += u16::try_from(sequencer_id).unwrap();
    config.network_config.quic_port += u16::try_from(sequencer_id).unwrap();

Could you please explain what is this part doing?

Code quote:

    config.network_config.tcp_port += u16::try_from(sequencer_id).unwrap();
    config.network_config.quic_port += u16::try_from(sequencer_id).unwrap();

[yair-starkware]

Reviewable status: 0 of 6 files reviewed, 9 unresolved discussions (waiting on @alonh5, @Itay-Tsabary-Starkware, and @matan-starkware)

---

crates/starknet_integration_tests/src/flow_test_setup.rs line 42 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Unrelated to this pr, but you can drop this completely.

Added TODO

---

crates/starknet_integration_tests/src/flow_test_setup.rs line 47 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Could you please explain where this values come from?

Done.

---

crates/starknet_integration_tests/src/flow_test_setup.rs line 49 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Could you please explain where this values come from? Should this be PROPOSER_ID?

Done.

---

crates/starknet_integration_tests/src/integration_test_setup.rs line 55 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Could you please explain where this values come from?

Done.

---

crates/starknet_integration_tests/src/integration_test_setup.rs line 59 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Could you please explain where this values come from?

Replaced with a const

---

crates/starknet_integration_tests/src/utils.rs line 49 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please change to Tsabary, otherwise I won't find it :-)

Done.

---

crates/starknet_integration_tests/src/utils.rs line 57 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Could we please unify the validator id and sequencer address config parameters, assuming they have the same meaning?
@matan-starkware @alonh5

Added a todo on @matan-starkware

---

crates/starknet_integration_tests/src/utils.rs line 63 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Why do you need to explicitly mention chain id? The config pointer mapping should address this, no?

In the test, we are not loading the configuration using the config crate; we create the instance directly.
There is a TODO to use presets.

---

crates/starknet_integration_tests/src/utils.rs line 259 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Can this be passed as an arg through the preset file?

It will automatically once we have it

---

crates/starknet_integration_tests/src/utils.rs line 268 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Could you please explain what is this part doing?

Added comment.
This will be resolved too when we use presets

[yair-starkware]

Reviewable status: 1 of 6 files reviewed, 6 unresolved discussions (waiting on @alonh5, @Itay-Tsabary-Starkware, and @matan-starkware)

---

a discussion (no related file):

Previously, alonh5 (Alon Haramati) wrote…

Could you please split this PR? Looks like there are a few parts that can and should be reviewed separately.

Splitted.
Sorry @Itay-Tsabary-Starkware, PTAL again

[Itay-Tsabary-Starkware]

Reviewable status: 1 of 6 files reviewed, 2 unresolved discussions (waiting on @alonh5, @matan-starkware, and @yair-starkware)

---

crates/starknet_integration_tests/src/utils.rs line 63 at r1 (raw file):

Previously, yair-starkware (Yair) wrote…

Perhaps a day.
I will know better when the multiple sequencers test succeeds (already in a PR: #2348).
I think the sequencer address/validator id issue might cause some problems because IIUC the validator ids must be a range [0, num sequencers)

I highly suggest to use RequiredParams then, its mere purpose is to detail the required parameters that need to be explicitly passed or set n the map. WDYT?

[yair-starkware]

Reviewable status: 1 of 6 files reviewed, 2 unresolved discussions (waiting on @alonh5, @Itay-Tsabary-Starkware, and @matan-starkware)

---

crates/starknet_integration_tests/src/utils.rs line 63 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

I highly suggest to use RequiredParams then, its mere purpose is to detail the required parameters that need to be explicitly passed or set n the map. WDYT?

In the config crate, required params are parameters that the user must provide (they don't have a default value).
The ports are not required params in this notion.

Do you want to have a separate concept of required params for the integration tests? The function arguments make it so that the user has to provide them.

Once we have presets everything will be simpler.

[Itay-Tsabary-Starkware]

Reviewable status: 1 of 6 files reviewed, 2 unresolved discussions (waiting on @alonh5, @matan-starkware, and @yair-starkware)

---

crates/starknet_integration_tests/src/utils.rs line 63 at r1 (raw file):

Previously, yair-starkware (Yair) wrote…

In the config crate, required params are parameters that the user must provide (they don't have a default value).
The ports are not required params in this notion.

Do you want to have a separate concept of required params for the integration tests? The function arguments make it so that the user has to provide them.

Once we have presets everything will be simpler.

PTAL
https://github.com/starkware-libs/sequencer/blob/main/crates/starknet_sequencer_node/src/config/test_utils.rs#L8-L15
I was suggesting to use this struct to handle the passing of required parameters to the test.

[yair-starkware]

crates/starknet_integration_tests/src/utils.rs line 63 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

PTAL
https://github.com/starkware-libs/sequencer/blob/main/crates/starknet_sequencer_node/src/config/test_utils.rs#L8-L15
I was suggesting to use this struct to handle the passing of required parameters to the test.

I think it is unnecessary once we use the config crate

[Itay-Tsabary-Starkware]

Reviewed 1 of 5 files at r1, 1 of 4 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alonh5 and @matan-starkware)

[Itay-Tsabary-Starkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alonh5 and @matan-starkware)

---

a discussion (no related file):

Previously, yair-starkware (Yair) wrote…

Splitted.
Sorry @Itay-Tsabary-Starkware, PTAL again

Unblocked

[alonh5]

Reviewed 1 of 4 files at r3, 4 of 4 files at r4, 5 of 5 files at r5, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @matan-starkware and @yair-starkware)

---

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

#[rstest]
#[tokio::test]
async fn end_to_end(mut tx_generator: MultiAccountTransactionGenerator) {

Suggestion:

end_to_end_flow

---

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

#[tokio::test]
async fn end_to_end(mut tx_generator: MultiAccountTransactionGenerator) {
    starknet_sequencer_infra::trace_util::configure_tracing();

import?

Suggestion:

configure_tracing();

---

crates/starknet_integration_tests/src/flow_test_setup.rs line 74 at r4 (raw file):

}

pub struct SequencerTestSetup {

Suggestion:

SequencerSetup

---

crates/starknet_integration_tests/src/flow_test_setup.rs line 110 at r4 (raw file):

        .await;

        debug!("Rpc server spawned at: {}", rpc_server_addr);

Why do we need the address?

Code quote:

debug!("Rpc server spawned at: {}", rpc_server_addr);

[yair-starkware]

Reviewable status: 2 of 6 files reviewed, 2 unresolved discussions (waiting on @alonh5 and @matan-starkware)

---

crates/starknet_integration_tests/src/flow_test_setup.rs line 110 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Why do we need the address?

It goes to the config of RpcStateReader.
It needs to be changed as part of making all of the ports configurable.

---

crates/starknet_integration_tests/src/flow_test_setup.rs line 74 at r4 (raw file):

}

pub struct SequencerTestSetup {

Done.

[alonh5]

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

---

crates/starknet_integration_tests/src/flow_test_setup.rs line 110 at r4 (raw file):

Previously, yair-starkware (Yair) wrote…

It goes to the config of RpcStateReader.
It needs to be changed as part of making all of the ports configurable.

I meant why do we need to log it?

[yair-starkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alonh5 and @matan-starkware)

---

crates/starknet_integration_tests/src/flow_test_setup.rs line 110 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

I meant why do we need to log it?

Removed

[alonh5]

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

[alonh5]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @matan-starkware)