From 8d0eee7ca8fe117b2ee286c6080bfa0057ee31ae Mon Sep 17 00:00:00 2001 From: perekopskiy <53865202+perekopskiy@users.noreply.github.com> Date: Fri, 23 Aug 2024 12:47:37 +0300 Subject: [PATCH] feat: Change default_protective_reads_persistence_enabled to false (#2716) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What ❔ Changes default_protective_reads_persistence_enabled to false both for main and external node ## Why ❔ For EN: it was confirmed that it works well without protective reads For main node: it's expected that vm_runner_protective_reads is run by default ## Checklist - [ ] PR title corresponds to the body of PR (we generate changelog entries from PRs). - [ ] Tests for the changes have been added / updated. - [ ] Documentation comments have been added / updated. - [ ] Code has been formatted via `zk fmt` and `zk lint`. --- core/bin/external_node/src/config/mod.rs | 9 ++------- core/lib/config/src/configs/chain.rs | 9 +++------ core/lib/config/src/configs/experimental.rs | 12 ++++-------- core/lib/protobuf_config/src/chain.rs | 7 +++---- .../layers/state_keeper/output_handler.rs | 9 +++------ 5 files changed, 15 insertions(+), 31 deletions(-) diff --git a/core/bin/external_node/src/config/mod.rs b/core/bin/external_node/src/config/mod.rs index 568d3195bbea..cd4e845b8f3e 100644 --- a/core/bin/external_node/src/config/mod.rs +++ b/core/bin/external_node/src/config/mod.rs @@ -391,8 +391,7 @@ pub(crate) struct OptionalENConfig { /// Configures whether to persist protective reads when persisting L1 batches in the state keeper. /// Protective reads are never required by full nodes so far, not until such a node runs a full Merkle tree /// (presumably, to participate in L1 batch proving). - /// By default, set to `true` as a temporary safety measure. - #[serde(default = "OptionalENConfig::default_protective_reads_persistence_enabled")] + #[serde(default)] pub protective_reads_persistence_enabled: bool, /// Address of the L1 diamond proxy contract used by the consistency checker to match with the origin of logs emitted /// by commit transactions. If not set, it will not be verified. @@ -645,7 +644,7 @@ impl OptionalENConfig { .db_config .as_ref() .map(|a| a.experimental.protective_reads_persistence_enabled) - .unwrap_or(true), + .unwrap_or_default(), merkle_tree_processing_delay_ms: load_config_or_default!( general_config.db_config, experimental.processing_delay_ms, @@ -769,10 +768,6 @@ impl OptionalENConfig { 10 } - const fn default_protective_reads_persistence_enabled() -> bool { - true - } - const fn default_mempool_cache_update_interval_ms() -> u64 { 50 } diff --git a/core/lib/config/src/configs/chain.rs b/core/lib/config/src/configs/chain.rs index 6ac70b27b84a..7e33f6964bb7 100644 --- a/core/lib/config/src/configs/chain.rs +++ b/core/lib/config/src/configs/chain.rs @@ -127,8 +127,9 @@ pub struct StateKeeperConfig { /// Configures whether to persist protective reads when persisting L1 batches in the state keeper. /// Protective reads can be written asynchronously in VM runner instead. - /// By default, set to `true` as a temporary safety measure. - #[serde(default = "StateKeeperConfig::default_protective_reads_persistence_enabled")] + /// By default, set to `false` as it is expected that a separate `vm_runner_protective_reads` component + /// which is capable of saving protective reads is run. + #[serde(default)] pub protective_reads_persistence_enabled: bool, // Base system contract hashes, required only for generating genesis config. @@ -143,10 +144,6 @@ pub struct StateKeeperConfig { } impl StateKeeperConfig { - fn default_protective_reads_persistence_enabled() -> bool { - true - } - /// Creates a config object suitable for use in unit tests. /// Values mostly repeat the values used in the localhost environment. pub fn for_tests() -> Self { diff --git a/core/lib/config/src/configs/experimental.rs b/core/lib/config/src/configs/experimental.rs index 8309b36e7f22..097f3c4112b3 100644 --- a/core/lib/config/src/configs/experimental.rs +++ b/core/lib/config/src/configs/experimental.rs @@ -16,8 +16,9 @@ pub struct ExperimentalDBConfig { /// Configures whether to persist protective reads when persisting L1 batches in the state keeper. /// Protective reads are never required by full nodes so far, not until such a node runs a full Merkle tree /// (presumably, to participate in L1 batch proving). - /// By default, set to `true` as a temporary safety measure. - #[serde(default = "ExperimentalDBConfig::default_protective_reads_persistence_enabled")] + /// By default, set to `false` as it is expected that a separate `vm_runner_protective_reads` component + /// which is capable of saving protective reads is run. + #[serde(default)] pub protective_reads_persistence_enabled: bool, // Merkle tree config /// Processing delay between processing L1 batches in the Merkle tree. @@ -36,8 +37,7 @@ impl Default for ExperimentalDBConfig { state_keeper_db_block_cache_capacity_mb: Self::default_state_keeper_db_block_cache_capacity_mb(), state_keeper_db_max_open_files: None, - protective_reads_persistence_enabled: - Self::default_protective_reads_persistence_enabled(), + protective_reads_persistence_enabled: false, processing_delay_ms: Self::default_merkle_tree_processing_delay_ms(), include_indices_and_filters_in_block_cache: false, } @@ -53,10 +53,6 @@ impl ExperimentalDBConfig { self.state_keeper_db_block_cache_capacity_mb * super::BYTES_IN_MEGABYTE } - const fn default_protective_reads_persistence_enabled() -> bool { - true - } - const fn default_merkle_tree_processing_delay_ms() -> u64 { 100 } diff --git a/core/lib/protobuf_config/src/chain.rs b/core/lib/protobuf_config/src/chain.rs index fafecc0131cd..f91bf07e43f8 100644 --- a/core/lib/protobuf_config/src/chain.rs +++ b/core/lib/protobuf_config/src/chain.rs @@ -78,10 +78,9 @@ impl ProtoRepr for proto::StateKeeper { max_circuits_per_batch: required(&self.max_circuits_per_batch) .and_then(|x| Ok((*x).try_into()?)) .context("max_circuits_per_batch")?, - protective_reads_persistence_enabled: *required( - &self.protective_reads_persistence_enabled, - ) - .context("protective_reads_persistence_enabled")?, + protective_reads_persistence_enabled: self + .protective_reads_persistence_enabled + .unwrap_or_default(), // We need these values only for instantiating configs from environmental variables, so it's not // needed during the initialization from files diff --git a/core/node/node_framework/src/implementations/layers/state_keeper/output_handler.rs b/core/node/node_framework/src/implementations/layers/state_keeper/output_handler.rs index f639d72fe40a..5f63e4e19475 100644 --- a/core/node/node_framework/src/implementations/layers/state_keeper/output_handler.rs +++ b/core/node/node_framework/src/implementations/layers/state_keeper/output_handler.rs @@ -42,8 +42,8 @@ pub struct OutputHandlerLayer { /// before they are included into L2 blocks. pre_insert_txs: bool, /// Whether protective reads persistence is enabled. - /// Must be `true` for any node that maintains a full Merkle Tree (e.g. any instance of main node). - /// May be set to `false` for nodes that do not participate in the sequencing process (e.g. external nodes). + /// May be set to `false` for nodes that do not participate in the sequencing process (e.g. external nodes) + /// or run `vm_runner_protective_reads` component. protective_reads_persistence_enabled: bool, } @@ -68,7 +68,7 @@ impl OutputHandlerLayer { l2_shared_bridge_addr, l2_block_seal_queue_capacity, pre_insert_txs: false, - protective_reads_persistence_enabled: true, + protective_reads_persistence_enabled: false, } } @@ -112,9 +112,6 @@ impl WiringLayer for OutputHandlerLayer { persistence = persistence.with_tx_insertion(); } if !self.protective_reads_persistence_enabled { - // **Important:** Disabling protective reads persistence is only sound if the node will never - // run a full Merkle tree OR an accompanying protective-reads-writer is being run. - tracing::warn!("Disabling persisting protective reads; this should be safe, but is considered an experimental option at the moment"); persistence = persistence.without_protective_reads(); }