Handling snapshot requests is now required (#27537)

2022-09-07 10:08:42 -04:00 · 2022-09-07 10:08:42 -04:00 · 93a4f80a2c
parent acd846b90e
commit 93a4f80a2c
6 changed files with 151 additions and 108 deletions
--- a/core/src/validator.rs
+++ b/core/src/validator.rs
@ -194,7 +194,7 @@ impl Default for ValidatorConfig {
            geyser_plugin_config_files: None,
            rpc_addrs: None,
            pubsub_config: PubSubConfig::default(),
-            snapshot_config: None,
+            snapshot_config: Some(SnapshotConfig::new_load_only()),
            broadcast_stage_type: BroadcastStageType::Standard,
            turbine_disabled: Arc::<AtomicBool>::default(),
            enforce_ulimit_nofile: true,
@ -575,29 +575,17 @@ impl Validator {
        cluster_info.restore_contact_info(ledger_path, config.contact_save_interval);
        let cluster_info = Arc::new(cluster_info);

-        let (
-            accounts_background_service,
-            accounts_hash_verifier,
-            snapshot_packager_service,
-            accounts_background_request_sender,
-        ) = {
-            let pending_accounts_package = PendingAccountsPackage::default();
-            let (
-                accounts_background_request_sender,
-                snapshot_request_handler,
-                pending_snapshot_package,
-                snapshot_packager_service,
-            ) = if let Some(snapshot_config) = config.snapshot_config.clone() {
-                if !is_snapshot_config_valid(
-                    snapshot_config.full_snapshot_archive_interval_slots,
-                    snapshot_config.incremental_snapshot_archive_interval_slots,
-                    config.accounts_hash_interval_slots,
-                ) {
-                    error!("Snapshot config is invalid");
-                }
+        // A snapshot config is required.  Remove the Option<> wrapper in the future.
+        assert!(config.snapshot_config.is_some());
+        let snapshot_config = config.snapshot_config.clone().unwrap();

-                let pending_snapshot_package = PendingSnapshotPackage::default();
+        assert!(is_snapshot_config_valid(
+            &snapshot_config,
+            config.accounts_hash_interval_slots,
+        ));

+        let (pending_snapshot_package, snapshot_packager_service) =
+            if snapshot_config.should_generate_snapshots() {
                // filler accounts make snapshots invalid for use
                // so, do not publish that we have snapshots
                let enable_gossip_push = config
@ -605,7 +593,7 @@ impl Validator {
                    .as_ref()
                    .map(|config| config.filler_accounts_config.count == 0)
                    .unwrap_or(true);
-
+                let pending_snapshot_package = PendingSnapshotPackage::default();
                let snapshot_packager_service = SnapshotPackagerService::new(
                    pending_snapshot_package.clone(),
                    starting_snapshot_hashes,
@ -614,56 +602,48 @@ impl Validator {
                    snapshot_config.clone(),
                    enable_gossip_push,
                );
-
-                let (snapshot_request_sender, snapshot_request_receiver) = unbounded();
                (
-                    AbsRequestSender::new(snapshot_request_sender),
-                    Some(SnapshotRequestHandler {
-                        snapshot_config,
-                        snapshot_request_receiver,
-                        pending_accounts_package: pending_accounts_package.clone(),
-                    }),
                    Some(pending_snapshot_package),
                    Some(snapshot_packager_service),
                )
            } else {
-                (AbsRequestSender::default(), None, None, None)
+                (None, None)
            };

-            let accounts_hash_verifier = AccountsHashVerifier::new(
-                Arc::clone(&pending_accounts_package),
-                pending_snapshot_package,
-                &exit,
-                &cluster_info,
-                config.known_validators.clone(),
-                config.halt_on_known_validators_accounts_hash_mismatch,
-                config.accounts_hash_fault_injection_slots,
-                config.snapshot_config.clone(),
-            );
+        let pending_accounts_package = PendingAccountsPackage::default();
+        let accounts_hash_verifier = AccountsHashVerifier::new(
+            Arc::clone(&pending_accounts_package),
+            pending_snapshot_package,
+            &exit,
+            &cluster_info,
+            config.known_validators.clone(),
+            config.halt_on_known_validators_accounts_hash_mismatch,
+            config.accounts_hash_fault_injection_slots,
+            config.snapshot_config.clone(),
+        );

-            let pruned_banks_request_handler = PrunedBanksRequestHandler {
-                pruned_banks_receiver,
-            };
-            let last_full_snapshot_slot = starting_snapshot_hashes.map(|x| x.full.hash.0);
-            let accounts_background_service = AccountsBackgroundService::new(
-                bank_forks.clone(),
-                &exit,
-                AbsRequestHandlers {
-                    snapshot_request_handler,
-                    pruned_banks_request_handler,
-                },
-                config.accounts_db_caching_enabled,
-                config.accounts_db_test_hash_calculation,
-                last_full_snapshot_slot,
-            );
-
-            (
-                accounts_background_service,
-                accounts_hash_verifier,
-                snapshot_packager_service,
-                accounts_background_request_sender,
-            )
+        let (snapshot_request_sender, snapshot_request_receiver) = unbounded();
+        let accounts_background_request_sender = AbsRequestSender::new(snapshot_request_sender);
+        let snapshot_request_handler = SnapshotRequestHandler {
+            snapshot_config,
+            snapshot_request_receiver,
+            pending_accounts_package,
        };
+        let pruned_banks_request_handler = PrunedBanksRequestHandler {
+            pruned_banks_receiver,
+        };
+        let last_full_snapshot_slot = starting_snapshot_hashes.map(|x| x.full.hash.0);
+        let accounts_background_service = AccountsBackgroundService::new(
+            bank_forks.clone(),
+            &exit,
+            AbsRequestHandlers {
+                snapshot_request_handler,
+                pruned_banks_request_handler,
+            },
+            config.accounts_db_caching_enabled,
+            config.accounts_db_test_hash_calculation,
+            last_full_snapshot_slot,
+        );

        let leader_schedule_cache = Arc::new(leader_schedule_cache);
        let mut process_blockstore = ProcessBlockStore::new(
@ -2158,15 +2138,18 @@ fn cleanup_accounts_paths(config: &ValidatorConfig) {
 }

 pub fn is_snapshot_config_valid(
-    full_snapshot_interval_slots: Slot,
-    incremental_snapshot_interval_slots: Slot,
+    snapshot_config: &SnapshotConfig,
    accounts_hash_interval_slots: Slot,
 ) -> bool {
-    // if full snapshot interval is MAX, that means snapshots are turned off, so yes, valid
-    if full_snapshot_interval_slots == Slot::MAX {
+    // if the snapshot config is configured to *not* take snapshots, then it is valid
+    if !snapshot_config.should_generate_snapshots() {
        return true;
    }

+    let full_snapshot_interval_slots = snapshot_config.full_snapshot_archive_interval_slots;
+    let incremental_snapshot_interval_slots =
+        snapshot_config.incremental_snapshot_archive_interval_slots;
+
    let is_incremental_config_valid = if incremental_snapshot_interval_slots == Slot::MAX {
        true
    } else {
@ -2413,40 +2396,97 @@ mod tests {

    #[test]
    fn test_interval_check() {
-        assert!(is_snapshot_config_valid(300, 200, 100));
+        fn new_snapshot_config(
+            full_snapshot_archive_interval_slots: Slot,
+            incremental_snapshot_archive_interval_slots: Slot,
+        ) -> SnapshotConfig {
+            SnapshotConfig {
+                full_snapshot_archive_interval_slots,
+                incremental_snapshot_archive_interval_slots,
+                ..SnapshotConfig::default()
+            }
+        }
+
+        assert!(is_snapshot_config_valid(
+            &new_snapshot_config(300, 200),
+            100
+        ));

        let default_accounts_hash_interval =
            snapshot_utils::DEFAULT_INCREMENTAL_SNAPSHOT_ARCHIVE_INTERVAL_SLOTS;
        assert!(is_snapshot_config_valid(
-            snapshot_utils::DEFAULT_FULL_SNAPSHOT_ARCHIVE_INTERVAL_SLOTS,
-            snapshot_utils::DEFAULT_INCREMENTAL_SNAPSHOT_ARCHIVE_INTERVAL_SLOTS,
+            &new_snapshot_config(
+                snapshot_utils::DEFAULT_FULL_SNAPSHOT_ARCHIVE_INTERVAL_SLOTS,
+                snapshot_utils::DEFAULT_INCREMENTAL_SNAPSHOT_ARCHIVE_INTERVAL_SLOTS
+            ),
            default_accounts_hash_interval,
        ));
-
        assert!(is_snapshot_config_valid(
-            Slot::MAX,
-            snapshot_utils::DEFAULT_INCREMENTAL_SNAPSHOT_ARCHIVE_INTERVAL_SLOTS,
+            &new_snapshot_config(
+                snapshot_utils::DEFAULT_FULL_SNAPSHOT_ARCHIVE_INTERVAL_SLOTS,
+                Slot::MAX
+            ),
            default_accounts_hash_interval
        ));
        assert!(is_snapshot_config_valid(
-            snapshot_utils::DEFAULT_FULL_SNAPSHOT_ARCHIVE_INTERVAL_SLOTS,
-            Slot::MAX,
+            &new_snapshot_config(
+                snapshot_utils::DEFAULT_INCREMENTAL_SNAPSHOT_ARCHIVE_INTERVAL_SLOTS,
+                Slot::MAX
+            ),
            default_accounts_hash_interval
        ));
        assert!(is_snapshot_config_valid(
-            snapshot_utils::DEFAULT_INCREMENTAL_SNAPSHOT_ARCHIVE_INTERVAL_SLOTS,
-            Slot::MAX,
-            default_accounts_hash_interval
+            &new_snapshot_config(Slot::MAX, Slot::MAX),
+            Slot::MAX
        ));

-        assert!(!is_snapshot_config_valid(0, 100, 100));
-        assert!(!is_snapshot_config_valid(100, 0, 100));
-        assert!(!is_snapshot_config_valid(42, 100, 100));
-        assert!(!is_snapshot_config_valid(100, 42, 100));
-        assert!(!is_snapshot_config_valid(100, 100, 100));
-        assert!(!is_snapshot_config_valid(100, 200, 100));
-        assert!(!is_snapshot_config_valid(444, 200, 100));
-        assert!(!is_snapshot_config_valid(400, 222, 100));
+        assert!(!is_snapshot_config_valid(&new_snapshot_config(0, 100), 100));
+        assert!(!is_snapshot_config_valid(&new_snapshot_config(100, 0), 100));
+        assert!(!is_snapshot_config_valid(
+            &new_snapshot_config(42, 100),
+            100
+        ));
+        assert!(!is_snapshot_config_valid(
+            &new_snapshot_config(100, 42),
+            100
+        ));
+        assert!(!is_snapshot_config_valid(
+            &new_snapshot_config(100, 100),
+            100
+        ));
+        assert!(!is_snapshot_config_valid(
+            &new_snapshot_config(100, 200),
+            100
+        ));
+        assert!(!is_snapshot_config_valid(
+            &new_snapshot_config(444, 200),
+            100
+        ));
+        assert!(!is_snapshot_config_valid(
+            &new_snapshot_config(400, 222),
+            100
+        ));
+
+        assert!(is_snapshot_config_valid(
+            &SnapshotConfig::new_load_only(),
+            100
+        ));
+        assert!(is_snapshot_config_valid(
+            &SnapshotConfig {
+                full_snapshot_archive_interval_slots: 41,
+                incremental_snapshot_archive_interval_slots: 37,
+                ..SnapshotConfig::new_load_only()
+            },
+            100
+        ));
+        assert!(is_snapshot_config_valid(
+            &SnapshotConfig {
+                full_snapshot_archive_interval_slots: Slot::MAX,
+                incremental_snapshot_archive_interval_slots: Slot::MAX,
+                ..SnapshotConfig::new_load_only()
+            },
+            100
+        ));
    }

    #[test]
--- a/core/tests/snapshots.rs
+++ b/core/tests/snapshots.rs
@ -925,11 +925,11 @@ fn test_snapshots_with_background_services(
    }

    let abs_request_sender = AbsRequestSender::new(snapshot_request_sender);
-    let snapshot_request_handler = Some(SnapshotRequestHandler {
+    let snapshot_request_handler = SnapshotRequestHandler {
        snapshot_config: snapshot_test_config.snapshot_config.clone(),
        snapshot_request_receiver,
        pending_accounts_package: Arc::clone(&pending_accounts_package),
-    });
+    };
    let pruned_banks_request_handler = PrunedBanksRequestHandler {
        pruned_banks_receiver,
    };
--- a/ledger-tool/src/main.rs
+++ b/ledger-tool/src/main.rs
@ -38,7 +38,7 @@ use {
    solana_runtime::{
        accounts_background_service::{
            AbsRequestHandlers, AbsRequestSender, AccountsBackgroundService,
-            PrunedBanksRequestHandler,
+            PrunedBanksRequestHandler, SnapshotRequestHandler,
        },
        accounts_db::{AccountsDbConfig, FillerAccountsConfig},
        accounts_index::{AccountsIndexConfig, IndexLimitMb, ScanConfig},
@ -53,6 +53,7 @@ use {
        snapshot_config::SnapshotConfig,
        snapshot_hash::StartingSnapshotHashes,
        snapshot_minimizer::SnapshotMinimizer,
+        snapshot_package::PendingAccountsPackage,
        snapshot_utils::{
            self, ArchiveFormat, SnapshotVersion, DEFAULT_ARCHIVE_COMPRESSION,
            DEFAULT_MAX_FULL_SNAPSHOT_ARCHIVES_TO_RETAIN,
@ -1095,13 +1096,20 @@ fn load_bank_forks(
            accounts_update_notifier,
        );

+    let (snapshot_request_sender, snapshot_request_receiver) = crossbeam_channel::unbounded();
+    let accounts_background_request_sender = AbsRequestSender::new(snapshot_request_sender);
+    let snapshot_request_handler = SnapshotRequestHandler {
+        snapshot_config: SnapshotConfig::new_load_only(),
+        snapshot_request_receiver,
+        pending_accounts_package: PendingAccountsPackage::default(),
+    };
    let pruned_banks_receiver =
        AccountsBackgroundService::setup_bank_drop_callback(bank_forks.clone());
    let pruned_banks_request_handler = PrunedBanksRequestHandler {
        pruned_banks_receiver,
    };
    let abs_request_handler = AbsRequestHandlers {
-        snapshot_request_handler: None,
+        snapshot_request_handler,
        pruned_banks_request_handler,
    };
    let exit = Arc::new(AtomicBool::new(false));
@ -1121,7 +1129,7 @@ fn load_bank_forks(
        &process_options,
        None,
        None,
-        &AbsRequestSender::default(),
+        &accounts_background_request_sender,
    )
    .map(|_| (bank_forks, starting_snapshot_hashes));

--- a/local-cluster/src/local_cluster.rs
+++ b/local-cluster/src/local_cluster.rs
@ -24,7 +24,7 @@ use {
    solana_sdk::{
        account::{Account, AccountSharedData},
        client::SyncClient,
-        clock::{Slot, DEFAULT_DEV_SLOTS_PER_EPOCH, DEFAULT_TICKS_PER_SLOT},
+        clock::{DEFAULT_DEV_SLOTS_PER_EPOCH, DEFAULT_TICKS_PER_SLOT},
        commitment_config::CommitmentConfig,
        epoch_schedule::EpochSchedule,
        genesis_config::{ClusterType, GenesisConfig},
@ -741,8 +741,6 @@ impl LocalCluster {
        // node lifecycle.
        // There must be some place holder for now...
        SnapshotConfig {
-            full_snapshot_archive_interval_slots: Slot::MAX,
-            incremental_snapshot_archive_interval_slots: Slot::MAX,
            full_snapshot_archives_dir: DUMMY_SNAPSHOT_CONFIG_PATH_MARKER.into(),
            bank_snapshots_dir: DUMMY_SNAPSHOT_CONFIG_PATH_MARKER.into(),
            ..SnapshotConfig::new_load_only()
--- a/runtime/src/accounts_background_service.rs
+++ b/runtime/src/accounts_background_service.rs
@ -394,7 +394,7 @@ impl PrunedBanksRequestHandler {
 }

 pub struct AbsRequestHandlers {
-    pub snapshot_request_handler: Option<SnapshotRequestHandler>,
+    pub snapshot_request_handler: SnapshotRequestHandler,
    pub pruned_banks_request_handler: PrunedBanksRequestHandler,
 }

@ -407,16 +407,12 @@ impl AbsRequestHandlers {
        non_snapshot_time_us: u128,
        last_full_snapshot_slot: &mut Option<Slot>,
    ) -> Option<Result<u64, SnapshotError>> {
-        self.snapshot_request_handler
-            .as_ref()
-            .and_then(|snapshot_request_handler| {
-                snapshot_request_handler.handle_snapshot_requests(
-                    accounts_db_caching_enabled,
-                    test_hash_calculation,
-                    non_snapshot_time_us,
-                    last_full_snapshot_slot,
-                )
-            })
+        self.snapshot_request_handler.handle_snapshot_requests(
+            accounts_db_caching_enabled,
+            test_hash_calculation,
+            non_snapshot_time_us,
+            last_full_snapshot_slot,
+        )
    }
 }

--- a/validator/src/main.rs
+++ b/validator/src/main.rs
@ -2957,8 +2957,9 @@ pub fn main() {
    validator_config.accounts_hash_interval_slots =
        value_t_or_exit!(matches, "accounts-hash-interval-slots", u64);
    if !is_snapshot_config_valid(
-        full_snapshot_archive_interval_slots,
-        incremental_snapshot_archive_interval_slots,
+        // SAFETY: Calling `.unwrap()` is safe here because `validator_config.snapshot_config` must
+        // be `Some`. The Option<> wrapper will be removed later to solidify this requirement.
+        validator_config.snapshot_config.as_ref().unwrap(),
        validator_config.accounts_hash_interval_slots,
    ) {
        eprintln!("Invalid snapshot configuration provided: snapshot intervals are incompatible. \