Merge branch 'rumenov/r23' into 'master'

fix: make the local store path required in the IC config

The local store path is required in theory but not in practice. Enforcing that we require this value reduces bloat and removes a bunch of unwrap statements. 

See merge request dfinity-lab/public/ic!11867

Author: rumenov

ic-os/guestos/rootfs/opt/ic/share/ic.json5.template

@@ -17,7 +17,7 @@
     // Configuration of registry client
     // ============================================
     registry_client: {
-        // The default is not to specify it.
+        // The directory that should be used to persist registry content.
         local_store: "/var/lib/ic/data/ic_registry_local_store/"
     },
     // ============================================

rs/config/src/config_sample.rs

@@ -86,7 +86,8 @@ pub const SAMPLE_CONFIG: &str = r#"
     // Configuration of registry client
     // ============================================
     registry_client: {
-        // The default is not to specify it.
+        // The directory that should be used to persist registry content.
+        local_store: "/var/lib/ic/data/ic_registry_local_store/"
     },
     // ============================================
     // Configuration of the node state persistence.

rs/config/src/registry_client.rs

@@ -1,21 +1,9 @@
 use serde::{Deserialize, Serialize};
 use std::path::PathBuf;
 
-/// Eventually, the replica will only read registry data from the local store
-/// and the orchestrator will both read from and write to the registry local
-/// store.
-///
-/// I.e. all data provider variants except for the variant `LocalStore` are
-/// considered deprecated.
+/// The replica only reads registry data from the local store.
+/// The orchestrator both reads from and writes to the registry local store.
 #[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)]
 pub struct Config {
-    #[serde(flatten)]
-    pub data_provider: Option<DataProviderConfig>,
-}
-
-#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
-#[serde(rename_all = "snake_case")]
-pub enum DataProviderConfig {
-    /// A path to a directory that containing the locally persisted registry.
-    LocalStore(PathBuf),
+    pub local_store: PathBuf,
 }

rs/monitoring/onchain_observability/adapter/src/main.rs

@@ -9,7 +9,6 @@ use ic_adapter_metrics_server::start_metrics_grpc;
 use ic_async_utils::{abort_on_panic, incoming_from_nth_systemd_socket};
 use ic_base_types::{CanisterId, NodeId};
 use ic_canister_client::{Agent, Sender};
-use ic_config::registry_client::DataProviderConfig;
 use ic_crypto::CryptoComponent;
 use ic_interfaces::crypto::{BasicSigner, ErrorReproducibility, KeyManager};
 use ic_logger::{error, info, new_replica_logger_from_config, warn, ReplicaLogger};
@@ -340,13 +339,7 @@ async fn create_registry_client_and_crypto_component(
     config: Config,
     rt_handle: Handle,
 ) -> (Arc<RegistryClientImpl>, Arc<CryptoComponent>) {
-    let DataProviderConfig::LocalStore(local_store_from_config) = config
-        .registry_config
-        .data_provider
-        .as_ref()
-        .expect("No registry provider found");
-
-    let data_provider = Arc::new(LocalStoreImpl::new(local_store_from_config));
+    let data_provider = Arc::new(LocalStoreImpl::new(config.registry_config.local_store));
     let registry_client = Arc::new(RegistryClientImpl::new(
         data_provider,
         Some(&metrics_registry),

rs/orchestrator/registry_replicator/src/args.rs

@@ -1,5 +1,5 @@
 use clap::Parser;
-use ic_config::{logger::LogFormat, registry_client::DataProviderConfig, Config};
+use ic_config::{logger::LogFormat, Config};
 use slog::Level;
 use std::{
     net::{Ipv4Addr, SocketAddr, SocketAddrV4},
@@ -67,9 +67,7 @@ impl RegistryReplicatorArgs {
         config.logger.dc_id = self.dc_id;
         config.registration.nns_pub_key_pem = Some(self.nns_pub_key_pem.clone());
         config.registration.nns_url = Some(self.nns_url.clone());
-        config.registry_client.data_provider = Some(DataProviderConfig::LocalStore(
-            self.local_store_path.clone(),
-        ));
+        config.registry_client.local_store = self.local_store_path.clone();
         config.nns_registry_replicator.poll_delay_duration_ms = self.poll_delay_duration_ms;
 
         (config, _dir)

rs/orchestrator/registry_replicator/src/lib.rs

@@ -30,7 +30,7 @@
 
 use crate::internal_state::InternalState;
 use ic_config::metrics::{Config as MetricsConfig, Exporter};
-use ic_config::{registry_client::DataProviderConfig, Config};
+use ic_config::Config;
 use ic_crypto_utils_threshold_sig_der::parse_threshold_sig_key;
 use ic_http_endpoints_metrics::MetricsHttpEndpoint;
 use ic_interfaces_registry::{RegistryClient, RegistryDataProvider, ZERO_REGISTRY_VERSION};
@@ -92,12 +92,7 @@ impl RegistryReplicator {
         config: &Config,
     ) -> Self {
         // We only support the local store data provider
-        let DataProviderConfig::LocalStore(local_store_path) = config
-            .registry_client
-            .data_provider
-            .clone()
-            .expect("registry data provider is not configured");
-
+        let local_store_path = &config.registry_client.local_store;
         let local_store = Arc::new(LocalStoreImpl::new(local_store_path.clone()));
         std::fs::create_dir_all(local_store_path)
             .expect("Could not create directory for registry local store.");

rs/registry/client/src/client.rs

@@ -2,7 +2,6 @@
 //! immediately. The provided data provider is polled periodically in the
 //! background when start_polling() is called.
 use crossbeam_channel::{RecvTimeoutError, Sender, TrySendError};
-pub use ic_config::registry_client::DataProviderConfig;
 pub use ic_interfaces_registry::{
     empty_zero_registry_record, RegistryClient, RegistryClientVersionedResult,
     RegistryDataProvider, RegistryTransportRecord, POLLING_PERIOD, ZERO_REGISTRY_VERSION,

rs/replay/src/lib.rs

@@ -19,7 +19,6 @@ use crate::ingress::*;
 use crate::player::{Player, ReplayResult};
 
 use ic_canister_client::{Agent, Sender};
-use ic_config::registry_client::DataProviderConfig;
 use ic_config::{Config, ConfigSource};
 use ic_nns_constants::GOVERNANCE_CANISTER_ID;
 use ic_protobuf::registry::subnet::v1::InitialNiDkgTranscriptRecord;
@@ -98,9 +97,7 @@ pub fn replay(args: ReplayToolArgs) -> ReplayResult {
 
         // Override config
         if let Some(path) = args.data_root {
-            cfg.registry_client.data_provider = Some(DataProviderConfig::LocalStore(
-                path.join("ic_registry_local_store"),
-            ));
+            cfg.registry_client.local_store = path.join("ic_registry_local_store");
             cfg.state_manager = ic_config::state_manager::Config::new(path.join("ic_state"));
             cfg.artifact_pool.consensus_pool_path = path.join("ic_consensus_pool");
         }

rs/replay/src/player.rs

@@ -7,10 +7,7 @@ use ic_artifact_pool::{
     certification_pool::CertificationPoolImpl,
     consensus_pool::{ConsensusPoolImpl, UncachedConsensusPoolImpl},
 };
-use ic_config::{
-    artifact_pool::ArtifactPoolConfig, registry_client::DataProviderConfig,
-    subnet_config::SubnetConfigs, Config,
-};
+use ic_config::{artifact_pool::ArtifactPoolConfig, subnet_config::SubnetConfigs, Config};
 use ic_consensus::{
     certification::VerifierImpl,
     consensus::{
@@ -118,7 +115,7 @@ pub struct Player {
     ingress_history_reader: Box<dyn IngressHistoryReader>,
     certification_pool: Option<CertificationPoolImpl>,
     pub registry: Arc<RegistryClientImpl>,
-    local_store_path: Option<PathBuf>,
+    local_store_path: PathBuf,
     replica_version: ReplicaVersion,
     pub log: ReplicaLogger,
     _async_log_guard: AsyncGuard,
@@ -253,14 +250,6 @@ impl Player {
             registry.get_latest_version(),
             &log,
         );
-        let local_store_path = if let Some(DataProviderConfig::LocalStore(path)) =
-            cfg.registry_client.data_provider.clone()
-        {
-            Some(path)
-        } else {
-            None
-        };
-
         let metrics_registry = MetricsRegistry::new();
         let subnet_config = SubnetConfigs::default().own_subnet_config(subnet_type);
 
@@ -315,7 +304,7 @@ impl Player {
                 metrics_registry.clone(),
             )
         });
-
+        let local_store_path = cfg.registry_client.local_store.clone();
         let validator = consensus_pool.as_ref().map(|pool| {
             ReplayValidator::new(
                 cfg,
@@ -330,7 +319,6 @@ impl Player {
                 log.clone(),
             )
         });
-
         Player {
             state_manager,
             message_routing,
@@ -631,9 +619,7 @@ impl Player {
     /// Fetch registry records from the given `nns_url`, and update the local
     /// registry store with the new records.
     pub fn update_registry_local_store(&self) {
-        let local_store_path = self.local_store_path.clone().expect(
-           "update_registry_local_store can only be used with registry configured with local store");
-        println!("RegistryLocalStore path: {:?}", local_store_path);
+        println!("RegistryLocalStore path: {:?}", &self.local_store_path);
         let latest_version = self.registry.get_latest_version();
         println!("RegistryLocalStore latest version: {}", latest_version);
         let records = self
@@ -642,7 +628,7 @@ impl Player {
                 current_time() + Duration::from_secs(60),
             )
             .unwrap_or_else(|err| panic!("Error in get_certified_changes_since: {}", err));
-        write_records_to_local_store(&local_store_path, latest_version, records)
+        write_records_to_local_store(&self.local_store_path, latest_version, records)
     }
 
     /// Deliver finalized batches since last expected batch height.
@@ -1232,13 +1218,7 @@ fn setup_registry(
     config: Config,
     metrics_registry: Option<&MetricsRegistry>,
 ) -> std::sync::Arc<RegistryClientImpl> {
-    let data_provider = match config
-        .registry_client
-        .data_provider
-        .expect("Data provider required")
-    {
-        DataProviderConfig::LocalStore(path) => Arc::new(LocalStoreImpl::new(path)),
-    };
+    let data_provider = Arc::new(LocalStoreImpl::new(config.registry_client.local_store));
 
     let registry = Arc::new(RegistryClientImpl::new(data_provider, metrics_registry));
     if let Err(e) = registry.fetch_and_start_polling() {

rs/replica/src/main.rs

@@ -1,10 +1,7 @@
 //! Replica -- Internet Computer
 
 use ic_async_utils::{abort_on_panic, shutdown_signal};
-use ic_config::{
-    registry_client::DataProviderConfig,
-    {subnet_config::SubnetConfigs, Config},
-};
+use ic_config::{subnet_config::SubnetConfigs, Config};
 use ic_crypto_sha::Sha256;
 use ic_crypto_tls_interfaces::TlsHandshake;
 use ic_http_endpoints_metrics::MetricsHttpEndpoint;
@@ -261,11 +258,9 @@ fn main() -> io::Result<()> {
         &logger.inner_logger.root,
     );
 
-    let registry_certified_time_reader: Arc<dyn LocalStoreCertifiedTimeReader> = {
-        let DataProviderConfig::LocalStore(path) =
-            config.registry_client.data_provider.clone().unwrap();
-        Arc::new(LocalStoreImpl::new(path))
-    };
+    let registry_certified_time_reader: Arc<dyn LocalStoreCertifiedTimeReader> = Arc::new(
+        LocalStoreImpl::new(config.registry_client.local_store.clone()),
+    );
 
     info!(logger, "Constructing IC stack");
     let (_, _, _p2p_thread_joiner, _, _xnet_endpoint) =