chore(consensus): remove default value for chain_id config parameter

[guy-starkware]

I'm trying to make changes to the consensus config. Starting with removing the default values for some of the parameters.

[reviewable-StarkWare]

This change is 

[guy-starkware]

• chore(consensus): remove default value for chain_id config parameter #2898 : 2 dependent PRs (#3130 , #3268 ) 👈 (View in Graphite)
• main

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

[github-actions]

Artifacts upload workflows:

• Started at 2024-12-26T10:54:06Z
• Started at 2024-12-29T10:42:47Z
• Started at 2024-12-29T11:02:34Z
• Started at 2024-12-30T14:00:16Z
• Started at 2024-12-30T14:03:34Z
• Started at 2024-12-30T14:07:38Z
• Started at 2024-12-30T15:04:22Z
• Started at 2024-12-30T16:11:18Z
• Started at 2025-01-05T11:14:10Z
• Started at 2025-01-08T14:25:56Z
• Started at 2025-01-09T07:37:13Z
• Started at 2025-01-12T11:09:36Z
• Started at 2025-01-12T15:23:45Z
• Started at 2025-01-14T13:14:52Z
• Started at 2025-01-14T13:23:57Z
• Started at 2025-01-15T09:12:43Z
• Started at 2025-01-15T10:06:55Z
• Started at 2025-01-15T13:07:45Z

[asmaastarkware]

Reviewed 4 of 8 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @guy-starkware)

---

-- commits line 2 at r2:

Suggestion:

chore(consensus): remove default value for chain_id config parameter

[guy-starkware]

Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @asmaastarkware)

---

-- commits line 2 at r2:
Done.

[giladchase]

Reviewable status: 5 of 9 files reviewed, 2 unresolved discussions (waiting on @asmaastarkware and @guy-starkware)

---

crates/papyrus_node/src/bin/central_source_integration_test.rs line 20 at r4 (raw file):

    fs::create_dir_all(path.clone()).expect("Should make a temporary `data` directory");
    let config = NodeConfig::load_and_process(vec![
        "Placeholder-binary-name".to_owned(),

Config boilerplate?

Code quote:

      "Placeholder-binary-name".to_owned(),

---

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

    let broadcast_channels = network_config_into_broadcast_channels(
        channels_network_config,
        papyrus_network::gossipsub_impl::Topic::new(TEST_CONSENSUS_PROPOSALS_TOPIC),

Is this the same value as before? If so then is there a reason we can't import it?

Code quote:

        papyrus_network::gossipsub_impl::Topic::new(TEST_CONSENSUS_PROPOSALS_TOPIC),

---

crates/papyrus_node/src/config/config_test.rs line 72 at r4 (raw file):

    args.append(&mut additional_args.into_iter().map(|s| s.to_owned()).collect());
    args
}

and remove all the vec![] from the callsites.

It's out of scope, so this is suggestion is optional, but better not have all of those unnecessary allocations.

Suggestion:

fn get_args(additional_args: impl IntoIterator<Item = impl ToString>) -> Vec<String> {
    let mut args = vec!["Papyrus".to_owned()];
    args.extend(required_args());
    args.extend(additional_args.into_iter().map(|s| s.to_string()));
    args
}

[guy-starkware]

Reviewable status: 5 of 9 files reviewed, 2 unresolved discussions (waiting on @asmaastarkware and @giladchase)

---

crates/papyrus_node/src/bin/central_source_integration_test.rs line 20 at r4 (raw file):

Previously, giladchase wrote…

Config boilerplate?

Yes, the parser expects the first word to be the binary name.

---

crates/papyrus_node/src/config/config_test.rs line 72 at r4 (raw file):

Previously, giladchase wrote…

and remove all the vec![] from the callsites.

It's out of scope, so this is suggestion is optional, but better not have all of those unnecessary allocations.

Cool. I didn't know you could do that!

---

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

Previously, giladchase wrote…

Is this the same value as before? If so then is there a reason we can't import it?

I'm not sure if it is the same value, but the other const is going to disappear in one of the next PRs, but the config it is going into is not going to be available in this test, so I'm just putting a replacement.

I'm not sure when this went into this PR, maybe I can remove it for now.

[giladchase]

Reviewed 2 of 8 files at r1, 3 of 4 files at r2, 2 of 4 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @asmaastarkware)

---

crates/papyrus_node/src/config/config_test.rs line 72 at r4 (raw file):

Previously, guy-starkware wrote…

Cool. I didn't know you could do that!

😬

In general this trick is applicable whenever someone passes an array/vec only for the sake of iterating over it. This type is way more flexible for callers, and doesn't force callers to allocate anything in order to use it.

In fact it appears that this is applicable up the stack in load_and_process, which keep passing vecs around only for the sake of consuming them through iteration, but that one's already too much out of scope here.

---

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

Previously, guy-starkware wrote…

I'm not sure if it is the same value, but the other const is going to disappear in one of the next PRs, but the config it is going into is not going to be available in this test, so I'm just putting a replacement.

I'm not sure when this went into this PR, maybe I can remove it for now.

If it is removed later then leaving it here is fine.

---

crates/papyrus_node/src/config/config_test.rs line 78 at r5 (raw file):

    env::set_current_dir(resolve_project_relative_path("").unwrap())
        .expect("Couldn't set working dir.");
    NodeConfig::load_and_process(get_args(vec!["--chain_id", "SN_MAIN"]))

Now that get_args accepts anything that supports into_iter, you no longer have to allocate a vec and an on-stack array suffices (so this spares an extra heap allocation).

Ditto for the rest of the callsites.

Suggestion:

["--chain_id", "SN_MAIN"]))

[guy-starkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @asmaastarkware)

---

crates/papyrus_node/src/config/config_test.rs line 72 at r4 (raw file):

Previously, giladchase wrote…

😬

In general this trick is applicable whenever someone passes an array/vec only for the sake of iterating over it. This type is way more flexible for callers, and doesn't force callers to allocate anything in order to use it.

In fact it appears that this is applicable up the stack in load_and_process, which keep passing vecs around only for the sake of consuming them through iteration, but that one's already too much out of scope here.

I can fix that in a separate PR if you think it is worth the effort.

---

crates/papyrus_node/src/config/config_test.rs line 78 at r5 (raw file):

Previously, giladchase wrote…

Now that get_args accepts anything that supports into_iter, you no longer have to allocate a vec and an on-stack array suffices (so this spares an extra heap allocation).

Ditto for the rest of the callsites.

Got it!

---

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

Previously, giladchase wrote…

If it is removed later then leaving it here is fine.

Too late. It's gone. It's a few PRs up the stack before we reach where I remove it, so it's better to keep it for now.

[giladchase]

Reviewable status: 8 of 10 files reviewed, 1 unresolved discussion (waiting on @asmaastarkware and @dan-starkware)

---

crates/papyrus_node/src/config/config_test.rs line 72 at r4 (raw file):

Previously, guy-starkware wrote…

I can fix that in a separate PR if you think it is worth the effort.

Nah it's fine to leave it, it's too much into "creating our own tasks" territory.

[asmaastarkware]

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

[guy-starkware]

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