Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Change default_protective_reads_persistence_enabled to false #2716

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions core/bin/external_node/src/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
}
Expand Down
9 changes: 3 additions & 6 deletions core/lib/config/src/configs/chain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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 {
Expand Down
12 changes: 4 additions & 8 deletions core/lib/config/src/configs/experimental.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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,
}
Expand All @@ -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
}
Expand Down
7 changes: 3 additions & 4 deletions core/lib/protobuf_config/src/chain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}

Expand All @@ -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,
}
}

Expand Down Expand Up @@ -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();
}

Expand Down
Loading