Skip to content

Commit

Permalink
feat: Change default_protective_reads_persistence_enabled to false (m…
Browse files Browse the repository at this point in the history
…atter-labs#2716)

## 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

<!-- Check your PR fulfills the following items. -->
<!-- For draft PRs check the boxes as you complete them. -->

- [ ] 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`.
  • Loading branch information
perekopskiy authored Aug 23, 2024
1 parent 835aec3 commit 8d0eee7
Show file tree
Hide file tree
Showing 5 changed files with 15 additions and 31 deletions.
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

0 comments on commit 8d0eee7

Please sign in to comment.