feat(starknet_integration_tests): add update revert config

[noamsp-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[noamsp-starkware]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• WIP_INTEGRATION_REVER_FLOW_TEST #4114
• test(starknet_integration_tests): add revert flow integration test #4076
• feat(starknet_integration_tests): add snapshot tx generator fn #4075
• feat(starknet_integration_tests): add update revert config #4074 👈 (View in Graphite)
• feat(starknet_state_sync): add revert_up_to_and_including option #3989
• feat(apollo_reverts): add revert blocks functionality #3988
• feat(papyrus_storage): add revert class manager marker #3987
• main

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

[ShahakShama]

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed (commit messages unreviewed), 4 unresolved discussions (waiting on @noamsp-starkware)

---

crates/starknet_integration_tests/src/sequencer_manager.rs line 220 at r1 (raw file):

    }

    pub fn update_revert_executables(&self, value: Option<BlockNumber>) {

Rename to update_revert_config_to_all_nodes

---

crates/starknet_integration_tests/src/sequencer_manager.rs line 221 at r1 (raw file):

    pub fn update_revert_executables(&self, value: Option<BlockNumber>) {
        self.idle_nodes.values().for_each(|idle_node| {

This function assumes that all nodes are shut down, which is not clear to the caller.
I suggest instead to move the shutdown and restart code to this function. Then you should rename the function by adding _and_restart at the end

Another alternative is to rename this function to update_revert_config_to_all_idle_nodes

---

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

    }

    pub fn update_revert_config(&self, value: Option<BlockNumber>) {

Add TODO to change this into change_config once we need to change something else in the config

---

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

            }
            None => {
                json_data["revert_config.revert_up_to_and_including"] = Value::from(u64::MAX);

Remove this line, so that we'll see that just changing should_revert to false will turn off reverts
This placeholder is for production where mistakes cost us heavily. Here it's ok if there will be mistakes

[noamsp-starkware]

Reviewable status: all files reviewed (commit messages unreviewed), 4 unresolved discussions (waiting on @ShahakShama)

---

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

Previously, ShahakShama wrote…

Add TODO to change this into change_config once we need to change something else in the config

Done.

---

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

Previously, ShahakShama wrote…

Remove this line, so that we'll see that just changing should_revert to false will turn off reverts
This placeholder is for production where mistakes cost us heavily. Here it's ok if there will be mistakes

Done.

---

crates/starknet_integration_tests/src/sequencer_manager.rs line 220 at r1 (raw file):

Previously, ShahakShama wrote…

Rename to update_revert_config_to_all_nodes

Done.

---

crates/starknet_integration_tests/src/sequencer_manager.rs line 221 at r1 (raw file):

Previously, ShahakShama wrote…

This function assumes that all nodes are shut down, which is not clear to the caller.
I suggest instead to move the shutdown and restart code to this function. Then you should rename the function by adding _and_restart at the end

Another alternative is to rename this function to update_revert_config_to_all_idle_nodes

I need the ability to assign the tx_generator between shutdown and running the nodes again, I'll go with the alternative.

[ShahakShama]

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

[ShahakShama]

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

[nadin-Starkware]

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

[Itay-Tsabary-Starkware]

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

---

crates/starknet_integration_tests/src/integration_test_setup.rs line 196 at r3 (raw file):

        data = serde_json::to_string_pretty(&json_data).unwrap();
        fs::write(config_path, data).unwrap();
    }

Please:

1. Add the config and the required params to the executable setup struct:

   sequencer/crates/starknet_integration_tests/src/integration_test_setup.rs

   Lines 62 to 86 in e99555e

   	pub struct ExecutableSetup {
   	// Node test identifier.
   	pub node_execution_id: NodeExecutionId,
   	// Client for adding transactions to the sequencer node.
   	pub add_tx_http_client: HttpTestClient,
   	// Client for checking liveness of the sequencer node.
   	pub monitoring_client: MonitoringClient,
   	// Path to the node configuration file.
   	pub node_config_path: PathBuf,
   	// Storage reader for the batcher.
   	pub batcher_storage_config: StorageConfig,
   	// Storage reader for the state sync.
   	pub state_sync_storage_config: StorageConfig,
   	// Handlers for the storage and config files, maintained so the files are not deleted. Since
   	// these are only maintained to avoid dropping the handlers, private visibility suffices, and
   	// as such, the '#[allow(dead_code)]' attributes are used to suppress the warning.
   	#[allow(dead_code)]
   	batcher_storage_handle: Option<TempDir>,
   	#[allow(dead_code)]
   	node_config_dir_handle: TempDir,
   	#[allow(dead_code)]
   	state_sync_storage_handle: Option<TempDir>,
   	#[allow(dead_code)]
   	class_manager_storage_handles: Option<FileHandles>,
   	}

2. Wrap these as a self fn:

   sequencer/crates/starknet_integration_tests/src/integration_test_setup.rs

   Lines 141 to 145 in e99555e

   	let node_config_path = dump_config_file_changes(
   	&config,
   	required_params,
   	node_config_dir_handle.path().to_path_buf(),
   	);

3. change update_revert_config to update_config that takes two args: config and required params. This function should set the new values within the struct, and then call the dump fn from bullet fix(format): run format fix on all repo #2. Specifically, avoid duplicating the writing-to-file functionality.

Code quote:

    // TODO(noamsp): Change this into change_config once we need to change other values in the
    // config.
    pub fn update_revert_config(&self, value: Option<BlockNumber>) {
        let config_path = self.node_config_path.clone();
        let config_path = config_path.to_str().unwrap();
        let mut data = fs::read_to_string(config_path).unwrap();
        let mut json_data: Value = serde_json::from_str(&data).unwrap();

        match value {
            Some(value) => {
                json_data["revert_config.revert_up_to_and_including"] = Value::from(value.0);
                json_data["revert_config.should_revert"] = Value::from(true);
            }
            // If should revert is false, the revert_up_to_and_including value is irrelevant.
            None => {
                json_data["revert_config.should_revert"] = Value::from(false);
            }
        }

        data = serde_json::to_string_pretty(&json_data).unwrap();
        fs::write(config_path, data).unwrap();
    }

[ShahakShama]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Itay-Tsabary-Starkware)

---

crates/starknet_integration_tests/src/integration_test_setup.rs line 196 at r3 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please:

1. Add the config and the required params to the executable setup struct:

   sequencer/crates/starknet_integration_tests/src/integration_test_setup.rs

   Lines 62 to 86 in e99555e

   	pub struct ExecutableSetup {
   	// Node test identifier.
   	pub node_execution_id: NodeExecutionId,
   	// Client for adding transactions to the sequencer node.
   	pub add_tx_http_client: HttpTestClient,
   	// Client for checking liveness of the sequencer node.
   	pub monitoring_client: MonitoringClient,
   	// Path to the node configuration file.
   	pub node_config_path: PathBuf,
   	// Storage reader for the batcher.
   	pub batcher_storage_config: StorageConfig,
   	// Storage reader for the state sync.
   	pub state_sync_storage_config: StorageConfig,
   	// Handlers for the storage and config files, maintained so the files are not deleted. Since
   	// these are only maintained to avoid dropping the handlers, private visibility suffices, and
   	// as such, the '#[allow(dead_code)]' attributes are used to suppress the warning.
   	#[allow(dead_code)]
   	batcher_storage_handle: Option<TempDir>,
   	#[allow(dead_code)]
   	node_config_dir_handle: TempDir,
   	#[allow(dead_code)]
   	state_sync_storage_handle: Option<TempDir>,
   	#[allow(dead_code)]
   	class_manager_storage_handles: Option<FileHandles>,
   	}

2. Wrap these as a self fn:

   sequencer/crates/starknet_integration_tests/src/integration_test_setup.rs

   Lines 141 to 145 in e99555e

   	let node_config_path = dump_config_file_changes(
   	&config,
   	required_params,
   	node_config_dir_handle.path().to_path_buf(),
   	);

3. change update_revert_config to update_config that takes two args: config and required params. This function should set the new values within the struct, and then call the dump fn from bullet fix(format): run format fix on all repo #2. Specifically, avoid duplicating the writing-to-file functionality.

Why? This is the only place we change the config and doing it this way requires much less changes to the API
There's also a TODO to do what you suggested if we have another place that changes the config

[Itay-Tsabary-Starkware]

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

---

crates/starknet_integration_tests/src/integration_test_setup.rs line 196 at r3 (raw file):

Previously, ShahakShama wrote…

Why? This is the only place we change the config and doing it this way requires much less changes to the API
There's also a TODO to do what you suggested if we have another place that changes the config

The functionality of dumping config files should be located in a single function.
Additionally, creating a config struct, dumping it, and then reading it, instead of the simpler alternative of maintaing it results in a messy code.
Please follow through with the requested changes ^