Add snapshot_utils::bank_from_latest_snapshot_archives()

While reviewing PR solana-labs#18565, as issue was brought up to refactor some code around verifying the bank after rebuilding from snapshots. A new top-level function has been added to get the latest snapshot archives and load the bank then verify. Additionally, new tests have been written and existing tests have been updated to use this new function. Fixes solana-labs#18973 While resolving the issue, it became clear there was some additional low-hanging fruit this change enabled. Specifically, the functions `bank_to_xxx_snapshot_archive()` now return their respective `SnapshotArchiveInfo`. And on the flip side, `bank_from_snapshot_archives()` now takes `SnapshotArchiveInfo`s instead of separate paths and archive formats. This bundling simplifies bank rebuilding.
brooksprumo · Aug 6, 2021 · 9985929 · 9985929
1 parent 41f4973
commit 9985929
Show file tree

Hide file tree

Showing 6 changed files with 359 additions and 212 deletions.
diff --git a/core/src/validator.rs b/core/src/validator.rs
@@ -1194,20 +1194,24 @@ fn new_banks_from_ledger(
         );
         leader_schedule_cache.set_root(&bank_forks.root_bank());
 
-        let archive_file = solana_runtime::snapshot_utils::bank_to_full_snapshot_archive(
-            ledger_path,
-            &bank_forks.root_bank(),
-            None,
-            &snapshot_config.snapshot_package_output_path,
-            snapshot_config.archive_format,
-            Some(bank_forks.root_bank().get_thread_pool()),
-            snapshot_config.maximum_snapshots_to_retain,
-        )
-        .unwrap_or_else(|err| {
-            error!("Unable to create snapshot: {}", err);
-            abort();
-        });
-        info!("created snapshot: {}", archive_file.display());
+        let full_snapshot_archive_info =
+            solana_runtime::snapshot_utils::bank_to_full_snapshot_archive(
+                ledger_path,
+                &bank_forks.root_bank(),
+                None,
+                &snapshot_config.snapshot_package_output_path,
+                snapshot_config.archive_format,
+                Some(bank_forks.root_bank().get_thread_pool()),
+                snapshot_config.maximum_snapshots_to_retain,
+            )
+            .unwrap_or_else(|err| {
+                error!("Unable to create snapshot: {}", err);
+                abort();
+            });
+        info!(
+            "created snapshot: {}",
+            full_snapshot_archive_info.path().display()
+        );
     }
 
     let tower = post_process_restored_tower(

diff --git a/core/tests/snapshots.rs b/core/tests/snapshots.rs
@@ -160,7 +160,16 @@ mod tests {
 
         let old_last_bank = old_bank_forks.get(old_last_slot).unwrap();
 
-        let check_hash_calculation = false;
+        let full_snapshot_archive_path = snapshot_utils::build_full_snapshot_archive_path(
+            snapshot_package_output_path.to_path_buf(),
+            old_last_bank.slot(),
+            &old_last_bank.get_accounts_hash(),
+            ArchiveFormat::TarBzip2,
+        );
+        let full_snapshot_archive_info =
+            snapshot_utils::FullSnapshotArchiveInfo::new_from_path(full_snapshot_archive_path)
+                .unwrap();
+
         let (deserialized_bank, _timing) = snapshot_utils::bank_from_snapshot_archives(
             account_paths,
             &[],
@@ -169,22 +178,16 @@ mod tests {
                 .as_ref()
                 .unwrap()
                 .snapshot_path,
-            snapshot_utils::build_full_snapshot_archive_path(
-                snapshot_package_output_path.to_path_buf(),
-                old_last_bank.slot(),
-                &old_last_bank.get_accounts_hash(),
-                ArchiveFormat::TarBzip2,
-            ),
+            &full_snapshot_archive_info,
             None,
-            ArchiveFormat::TarBzip2,
             old_genesis_config,
             None,
             None,
             AccountSecondaryIndexes::default(),
             false,
             None,
             accounts_db::AccountShrinkThreshold::default(),
-            check_hash_calculation,
+            false,
             false,
             false,
         )
@@ -716,9 +719,8 @@ mod tests {
                 )
                 .unwrap();
 
-                restore_from_incremental_snapshot_and_check_banks_are_equal(
+                restore_from_snapshots_and_check_banks_are_equal(
                     &bank,
-                    last_full_snapshot_slot.unwrap(),
                     &snapshot_test_config.snapshot_config,
                     snapshot_test_config.accounts_dir.path().to_path_buf(),
                     &snapshot_test_config.genesis_config_info.genesis_config,
@@ -783,57 +785,17 @@ mod tests {
         Ok(())
     }
 
-    fn restore_from_incremental_snapshot_and_check_banks_are_equal(
+    fn restore_from_snapshots_and_check_banks_are_equal(
         bank: &Bank,
-        last_full_snapshot_slot: Slot,
         snapshot_config: &SnapshotConfig,
         accounts_dir: PathBuf,
         genesis_config: &GenesisConfig,
     ) -> snapshot_utils::Result<()> {
-        let (
-            full_snapshot_archive_slot,
-            (incremental_snapshot_archive_base_slot, incremental_snapshot_archive_slot),
-            deserialized_bank,
-        ) = restore_from_incremental_snapshot(snapshot_config, accounts_dir, genesis_config)?;
-
-        assert_eq!(
-            full_snapshot_archive_slot,
-            incremental_snapshot_archive_base_slot
-        );
-        assert_eq!(full_snapshot_archive_slot, last_full_snapshot_slot);
-        assert_eq!(incremental_snapshot_archive_slot, bank.slot(),);
-        assert_eq!(*bank, deserialized_bank);
-
-        Ok(())
-    }
-
-    fn restore_from_incremental_snapshot(
-        snapshot_config: &SnapshotConfig,
-        accounts_dir: PathBuf,
-        genesis_config: &GenesisConfig,
-    ) -> snapshot_utils::Result<(Slot, (Slot, Slot), Bank)> {
-        let full_snapshot_archive_info = snapshot_utils::get_highest_full_snapshot_archive_info(
+        let (deserialized_bank, _) = snapshot_utils::bank_from_latest_snapshot_archives(
+            &snapshot_config.snapshot_path,
             &snapshot_config.snapshot_package_output_path,
-        )
-        .ok_or_else(|| Error::new(ErrorKind::Other, "no full snapshot"))?;
-
-        let incremental_snapshot_archive_info =
-            snapshot_utils::get_highest_incremental_snapshot_archive_info(
-                &snapshot_config.snapshot_package_output_path,
-                *full_snapshot_archive_info.slot(),
-            )
-            .ok_or_else(|| Error::new(ErrorKind::Other, "no incremental snapshot"))?;
-
-        info!("Restoring bank from full snapshot slot: {}, and incremental snapshot slot: {} (with base slot: {})",
-        full_snapshot_archive_info.slot(), incremental_snapshot_archive_info.slot(), incremental_snapshot_archive_info.base_slot());
-
-        let (deserialized_bank, _) = snapshot_utils::bank_from_snapshot_archives(
             &[accounts_dir],
             &[],
-            &snapshot_config.snapshot_path,
-            full_snapshot_archive_info.path(),
-            Some(incremental_snapshot_archive_info.path()),
-            snapshot_config.archive_format,
             genesis_config,
             None,
             None,
@@ -846,13 +808,8 @@ mod tests {
             false,
         )?;
 
-        Ok((
-            *full_snapshot_archive_info.slot(),
-            (
-                *incremental_snapshot_archive_info.base_slot(),
-                *incremental_snapshot_archive_info.slot(),
-            ),
-            deserialized_bank,
-        ))
+        assert_eq!(bank, &deserialized_bank);
+
+        Ok(())
     }
 }
diff --git a/ledger-tool/src/main.rs b/ledger-tool/src/main.rs
@@ -2199,7 +2199,7 @@ fn main() {
                         bank.slot(),
                     );
 
-                    let archive_file = snapshot_utils::bank_to_full_snapshot_archive(
+                    let full_snapshot_archive_info = snapshot_utils::bank_to_full_snapshot_archive(
                         ledger_path,
                         &bank,
                         Some(snapshot_version),
@@ -2217,7 +2217,7 @@ fn main() {
                         "Successfully created snapshot for slot {}, hash {}: {}",
                         bank.slot(),
                         bank.hash(),
-                        archive_file.display(),
+                        full_snapshot_archive_info.path().display(),
                     );
                     println!(
                         "Shred version: {}",

diff --git a/ledger/src/bank_forks_utils.rs b/ledger/src/bank_forks_utils.rs
@@ -8,11 +8,7 @@ use crate::{
 };
 use log::*;
 use solana_entry::entry::VerifyRecyclers;
-use solana_runtime::{
-    bank_forks::BankForks,
-    snapshot_config::SnapshotConfig,
-    snapshot_utils::{self, FullSnapshotArchiveInfo},
-};
+use solana_runtime::{bank_forks::BankForks, snapshot_config::SnapshotConfig, snapshot_utils};
 use solana_sdk::{clock::Slot, genesis_config::GenesisConfig, hash::Hash};
 use std::{fs, path::PathBuf, process, result};
 
@@ -44,19 +40,19 @@ pub fn load(
     transaction_status_sender: Option<&TransactionStatusSender>,
     cache_block_meta_sender: Option<&CacheBlockMetaSender>,
 ) -> LoadResult {
-    if let Some(snapshot_config) = snapshot_config.as_ref() {
+    if let Some(snapshot_config) = snapshot_config {
         info!(
-            "Initializing snapshot path: {:?}",
-            snapshot_config.snapshot_path
+            "Initializing snapshot path: {}",
+            snapshot_config.snapshot_path.display()
         );
         let _ = fs::remove_dir_all(&snapshot_config.snapshot_path);
         fs::create_dir_all(&snapshot_config.snapshot_path)
             .expect("Couldn't create snapshot directory");
 
-        if let Some(full_snapshot_archive_info) =
-            snapshot_utils::get_highest_full_snapshot_archive_info(
-                &snapshot_config.snapshot_package_output_path,
-            )
+        if snapshot_utils::get_highest_full_snapshot_archive_info(
+            &snapshot_config.snapshot_package_output_path,
+        )
+        .is_some()
         {
             return load_from_snapshot(
                 genesis_config,
@@ -67,7 +63,6 @@ pub fn load(
                 process_options,
                 transaction_status_sender,
                 cache_block_meta_sender,
-                &full_snapshot_archive_info,
             );
         } else {
             info!("No snapshot package available; will load from genesis");
@@ -115,26 +110,18 @@ fn load_from_snapshot(
     process_options: ProcessOptions,
     transaction_status_sender: Option<&TransactionStatusSender>,
     cache_block_meta_sender: Option<&CacheBlockMetaSender>,
-    full_snapshot_archive_info: &FullSnapshotArchiveInfo,
 ) -> LoadResult {
-    info!(
-        "Loading snapshot package: {:?}",
-        full_snapshot_archive_info.path()
-    );
-
     // Fail hard here if snapshot fails to load, don't silently continue
     if account_paths.is_empty() {
         error!("Account paths not present when booting from snapshot");
         process::exit(1);
     }
 
-    let (deserialized_bank, timings) = snapshot_utils::bank_from_snapshot_archives(
+    let (deserialized_bank, timings) = snapshot_utils::bank_from_latest_snapshot_archives(
+        &snapshot_config.snapshot_path,
+        &snapshot_config.snapshot_package_output_path,
         &account_paths,
         &process_options.frozen_accounts,
-        &snapshot_config.snapshot_path,
-        full_snapshot_archive_info.path(),
-        None,
-        *full_snapshot_archive_info.archive_format(),
         genesis_config,
         process_options.debug_keys.clone(),
         Some(&crate::builtins::get(process_options.bpf_jit)),
@@ -147,30 +134,14 @@ fn load_from_snapshot(
         process_options.verify_index,
     )
     .expect("Load from snapshot failed");
-    if let Some(shrink_paths) = shrink_paths {
-        deserialized_bank.set_shrink_paths(shrink_paths);
-    }
 
     let deserialized_bank_slot_and_hash = (
         deserialized_bank.slot(),
         deserialized_bank.get_accounts_hash(),
     );
 
-    if deserialized_bank_slot_and_hash
-        != (
-            *full_snapshot_archive_info.slot(),
-            *full_snapshot_archive_info.hash(),
-        )
-    {
-        error!(
-            "Snapshot has mismatch:\narchive: {:?}\ndeserialized: {:?}",
-            (
-                full_snapshot_archive_info.slot(),
-                full_snapshot_archive_info.hash()
-            ),
-            deserialized_bank_slot_and_hash
-        );
-        process::exit(1);
+    if let Some(shrink_paths) = shrink_paths {
+        deserialized_bank.set_shrink_paths(shrink_paths);
     }
 
     to_loadresult(

diff --git a/replica-node/src/replica_node.rs b/replica-node/src/replica_node.rs
@@ -115,9 +115,8 @@ fn initialize_from_snapshot(
         &replica_config.account_paths,
         &[],
         &snapshot_config.snapshot_path,
-        archive_info.path(),
+        &archive_info,
         None,
-        *archive_info.archive_format(),
         genesis_config,
         process_options.debug_keys.clone(),
         None,