reserialize bank in ahv by first writing to temp file in abs (#23947)

core/src/accounts_hash_verifier.rs

@@ -145,6 +145,10 @@ impl AccountsHashVerifier {
             assert_eq!(expected_hash, hash);
         };
         measure_hash.stop();
+        solana_runtime::serde_snapshot::reserialize_bank(
+            accounts_package.snapshot_links.path(),
+            accounts_package.slot,
+        );
         datapoint_info!(
             "accounts_hash_verifier",
             ("calculate_hash", measure_hash.as_us(), i64),

core/tests/snapshots.rs

@@ -166,10 +166,10 @@ mod tests {
         old_genesis_config: &GenesisConfig,
         account_paths: &[PathBuf],
     ) {
-        let (snapshot_path, snapshot_archives_dir) = old_bank_forks
+        let snapshot_archives_dir = old_bank_forks
             .snapshot_config
             .as_ref()
-            .map(|c| (&c.bank_snapshots_dir, &c.snapshot_archives_dir))
+            .map(|c| &c.snapshot_archives_dir)
             .unwrap();
 
         let old_last_bank = old_bank_forks.get(old_last_slot).unwrap();
@@ -213,12 +213,6 @@ mod tests {
             .unwrap()
             .clone();
         assert_eq!(*bank, deserialized_bank);
-
-        let bank_snapshots = snapshot_utils::get_bank_snapshots(&snapshot_path);
-
-        for p in bank_snapshots {
-            snapshot_utils::remove_bank_snapshot(p.slot, &snapshot_path).unwrap();
-        }
     }
 
     // creates banks up to "last_slot" and runs the input function `f` on each bank created
@@ -274,7 +268,7 @@ mod tests {
         let snapshot_config = &snapshot_test_config.snapshot_config;
         let bank_snapshots_dir = &snapshot_config.bank_snapshots_dir;
         let last_bank_snapshot_info =
-            snapshot_utils::get_highest_bank_snapshot_info(bank_snapshots_dir)
+            snapshot_utils::get_highest_bank_snapshot_pre(bank_snapshots_dir)
                 .expect("no bank snapshots found in path");
         let accounts_package = AccountsPackage::new(
             last_bank,
@@ -289,6 +283,10 @@ mod tests {
             Some(SnapshotType::FullSnapshot),
         )
         .unwrap();
+        solana_runtime::serde_snapshot::reserialize_bank(
+            accounts_package.snapshot_links.path(),
+            accounts_package.slot,
+        );
         let snapshot_package = SnapshotPackage::from(accounts_package);
         snapshot_utils::archive_snapshot_package(
             &snapshot_package,
@@ -472,7 +470,7 @@ mod tests {
         // currently sitting in the channel
         snapshot_utils::purge_old_bank_snapshots(bank_snapshots_dir);
 
-        let mut bank_snapshots = snapshot_utils::get_bank_snapshots(&bank_snapshots_dir);
+        let mut bank_snapshots = snapshot_utils::get_bank_snapshots_pre(&bank_snapshots_dir);
         bank_snapshots.sort_unstable();
         assert!(bank_snapshots
             .into_iter()
@@ -511,6 +509,10 @@ mod tests {
                     .unwrap()
                     .take()
                     .unwrap();
+                solana_runtime::serde_snapshot::reserialize_bank(
+                    accounts_package.snapshot_links.path(),
+                    accounts_package.slot,
+                );
                 let snapshot_package = SnapshotPackage::from(accounts_package);
                 pending_snapshot_package
                     .lock()
@@ -548,6 +550,9 @@ mod tests {
         )
         .unwrap();
 
+        // files were saved off before we reserialized the bank in the hacked up accounts_hash_verifier stand-in.
+        solana_runtime::serde_snapshot::reserialize_bank(saved_snapshots_dir.path(), saved_slot);
+
         snapshot_utils::verify_snapshot_archive(
             saved_archive_path.unwrap(),
             saved_snapshots_dir.path(),
@@ -751,7 +756,7 @@ mod tests {
         let slot = bank.slot();
         info!("Making full snapshot archive from bank at slot: {}", slot);
         let bank_snapshot_info =
-            snapshot_utils::get_bank_snapshots(&snapshot_config.bank_snapshots_dir)
+            snapshot_utils::get_bank_snapshots_pre(&snapshot_config.bank_snapshots_dir)
                 .into_iter()
                 .find(|elem| elem.slot == slot)
                 .ok_or_else(|| {
@@ -786,7 +791,7 @@ mod tests {
             slot, incremental_snapshot_base_slot,
         );
         let bank_snapshot_info =
-            snapshot_utils::get_bank_snapshots(&snapshot_config.bank_snapshots_dir)
+            snapshot_utils::get_bank_snapshots_pre(&snapshot_config.bank_snapshots_dir)
                 .into_iter()
                 .find(|elem| elem.slot == slot)
                 .ok_or_else(|| {

runtime/src/serde_snapshot.rs

@@ -14,6 +14,7 @@ use {
         epoch_stakes::EpochStakes,
         hardened_unpack::UnpackedAppendVecMap,
         rent_collector::RentCollector,
+        snapshot_utils::{self, BANK_SNAPSHOT_PRE_FILENAME_EXTENSION},
         stakes::Stakes,
     },
     bincode::{self, config::Options, Error},
@@ -285,6 +286,21 @@ where
     })
 }
 
+pub fn reserialize_bank(bank_snapshots_dir: impl AsRef<Path>, slot: Slot) {
+    let bank_post = snapshot_utils::get_bank_snapshots_dir(bank_snapshots_dir, slot);
+    let bank_post = bank_post.join(snapshot_utils::get_snapshot_file_name(slot));
+    let mut bank_pre = bank_post.clone();
+    bank_pre.set_extension(BANK_SNAPSHOT_PRE_FILENAME_EXTENSION);
+
+    // some tests don't create the file
+    if bank_pre.is_file() {
+        // replace the original file with the newly serialized one
+        // atm, this just copies the file and deletes the old one
+        std::fs::copy(&bank_pre, bank_post).unwrap();
+        std::fs::remove_file(bank_pre).unwrap();
+    }
+}
+
 struct SerializableBankAndStorage<'a, C> {
     bank: &'a Bank,
     snapshot_storages: &'a [SnapshotStorage],

runtime/src/snapshot_utils.rs

@@ -54,6 +54,7 @@ const MAX_SNAPSHOT_VERSION_FILE_SIZE: u64 = 8; // byte
 const VERSION_STRING_V1_2_0: &str = "1.2.0";
 pub(crate) const TMP_BANK_SNAPSHOT_PREFIX: &str = "tmp-bank-snapshot-";
 pub const TMP_SNAPSHOT_ARCHIVE_PREFIX: &str = "tmp-snapshot-archive-";
+pub const BANK_SNAPSHOT_PRE_FILENAME_EXTENSION: &str = "pre";
 pub const MAX_BANK_SNAPSHOTS_TO_RETAIN: usize = 8; // Save some bank snapshots but not too many
 pub const DEFAULT_MAX_FULL_SNAPSHOT_ARCHIVES_TO_RETAIN: usize = 2;
 pub const DEFAULT_MAX_INCREMENTAL_SNAPSHOT_ARCHIVES_TO_RETAIN: usize = 4;
@@ -115,11 +116,16 @@ impl SnapshotVersion {
     }
 }
 
-/// A slot and the path to its bank snapshot
+/// Information about a bank snapshot. Namely the slot of the bank, the path to the snapshot, and
+/// the type of the snapshot.
 #[derive(PartialEq, Eq, Debug)]
 pub struct BankSnapshotInfo {
+    /// Slot of the bank
     pub slot: Slot,
+    /// Path to the snapshot
     pub snapshot_path: PathBuf,
+    /// Type of the snapshot
+    pub snapshot_type: BankSnapshotType,
 }
 
 impl PartialOrd for BankSnapshotInfo {
@@ -135,6 +141,22 @@ impl Ord for BankSnapshotInfo {
     }
 }
 
+/// Bank snapshots traditionally had their accounts hash calculated prior to serialization.  Since
+/// the hash calculation takes a long time, an optimization has been put in to offload the accounts
+/// hash calculation.  The bank serialization format has not changed, so we need another way to
+/// identify if a bank snapshot contains the calculated accounts hash or not.
+///
+/// When a bank snapshot is first taken, it does not have the calculated accounts hash.  It is said
+/// that this bank snapshot is "pre" accounts hash.  Later, when the accounts hash is calculated,
+/// the bank snapshot is re-serialized, and is now "post" accounts hash.
+#[derive(Debug, Copy, Clone, Eq, PartialEq)]
+pub enum BankSnapshotType {
+    /// This bank snapshot has *not* yet had its accounts hash calculated
+    Pre,
+    /// This bank snapshot *has* had its accounts hash calculated
+    Post,
+}
+
 /// Helper type when rebuilding from snapshots.  Designed to handle when rebuilding from just a
 /// full snapshot, or from both a full snapshot and an incremental snapshot.
 #[derive(Debug)]
@@ -388,54 +410,102 @@ pub fn archive_snapshot_package(
     Ok(())
 }
 
-/// Get a list of bank snapshots in a directory
+/// Get the bank snapshots in a directory
-pub fn get_bank_snapshots<P>(bank_snapshots_dir: P) -> Vec<BankSnapshotInfo>
+pub fn get_bank_snapshots(bank_snapshots_dir: impl AsRef<Path>) -> Vec<BankSnapshotInfo> {
-where
+    let mut bank_snapshots = Vec::default();
-    P: AsRef<Path>,
-{
     match fs::read_dir(&bank_snapshots_dir) {
-        Ok(paths) => paths
-            .filter_map(|entry| {
-                entry.ok().and_then(|e| {
-                    e.path()
-                        .file_name()
-                        .and_then(|n| n.to_str().map(|s| s.parse::<Slot>().ok()))
-                        .unwrap_or(None)
-                })
-            })
-            .map(|slot| {
-                let snapshot_file_name = get_snapshot_file_name(slot);
-                // So nice I join-ed it twice!  The redundant `snapshot_file_name` is unintentional
-                // and should be simplified.  Kept for compatibility.
-                let snapshot_path = bank_snapshots_dir
-                    .as_ref()
-                    .join(&snapshot_file_name)
-                    .join(snapshot_file_name);
-                BankSnapshotInfo {
-                    slot,
-                    snapshot_path,
-                }
-            })
-            .collect::<Vec<BankSnapshotInfo>>(),
         Err(err) => {
             info!(
-                "Unable to read snapshots directory {}: {}",
+                "Unable to read bank snapshots directory {}: {}",
                 bank_snapshots_dir.as_ref().display(),
                 err
             );
-            vec![]
         }
+        Ok(paths) => paths
+            .filter_map(|entry| {
+                // check if this entry is a directory and only a Slot
+                // bank snapshots are bank_snapshots_dir/slot/slot(BANK_SNAPSHOT_PRE_FILENAME_EXTENSION)
+                entry
+                    .ok()
+                    .filter(|entry| entry.path().is_dir())
+                    .and_then(|entry| {
+                        entry
+                            .path()
+                            .file_name()
+                            .and_then(|file_name| file_name.to_str())
+                            .and_then(|file_name| file_name.parse::<Slot>().ok())
+                    })
+            })
+            .for_each(|slot| {
+                // check this directory to see if there is a BankSnapshotPre and/or
+                // BankSnapshotPost file
+                let bank_snapshot_outer_dir = get_bank_snapshots_dir(&bank_snapshots_dir, slot);
+                let bank_snapshot_post_path =
+                    bank_snapshot_outer_dir.join(get_snapshot_file_name(slot));
+                let mut bank_snapshot_pre_path = bank_snapshot_post_path.clone();
+                bank_snapshot_pre_path.set_extension(BANK_SNAPSHOT_PRE_FILENAME_EXTENSION);
+
+                if bank_snapshot_pre_path.is_file() {
+                    bank_snapshots.push(BankSnapshotInfo {
+                        slot,
+                        snapshot_path: bank_snapshot_pre_path,
+                        snapshot_type: BankSnapshotType::Pre,
+                    });
+                }
+
+                if bank_snapshot_post_path.is_file() {
+                    bank_snapshots.push(BankSnapshotInfo {
+                        slot,
+                        snapshot_path: bank_snapshot_post_path,
+                        snapshot_type: BankSnapshotType::Post,
+                    });
+                }
+            }),
     }
+    bank_snapshots
+}
+
+/// Get the bank snapshots in a directory
+///
+/// This function retains only the bank snapshots of type BankSnapshotType::Pre
+pub fn get_bank_snapshots_pre(bank_snapshots_dir: impl AsRef<Path>) -> Vec<BankSnapshotInfo> {
+    let mut bank_snapshots = get_bank_snapshots(bank_snapshots_dir);
+    bank_snapshots.retain(|bank_snapshot| bank_snapshot.snapshot_type == BankSnapshotType::Pre);
+    bank_snapshots
+}
+
+/// Get the bank snapshots in a directory
+///
+/// This function retains only the bank snapshots of type BankSnapshotType::Post
+pub fn get_bank_snapshots_post(bank_snapshots_dir: impl AsRef<Path>) -> Vec<BankSnapshotInfo> {
+    let mut bank_snapshots = get_bank_snapshots(bank_snapshots_dir);
+    bank_snapshots.retain(|bank_snapshot| bank_snapshot.snapshot_type == BankSnapshotType::Post);
+    bank_snapshots
 }
 
 /// Get the bank snapshot with the highest slot in a directory
-pub fn get_highest_bank_snapshot_info<P>(bank_snapshots_dir: P) -> Option<BankSnapshotInfo>
+///
-where
+/// This function gets the highest bank snapshot of type BankSnapshotType::Pre
-    P: AsRef<Path>,
+pub fn get_highest_bank_snapshot_pre(
-{
+    bank_snapshots_dir: impl AsRef<Path>,
-    let mut bank_snapshot_infos = get_bank_snapshots(bank_snapshots_dir);
+) -> Option<BankSnapshotInfo> {
-    bank_snapshot_infos.sort_unstable();
+    do_get_highest_bank_snapshot(get_bank_snapshots_pre(bank_snapshots_dir))
-    bank_snapshot_infos.into_iter().rev().next()
+}
+
+/// Get the bank snapshot with the highest slot in a directory
+///
+/// This function gets the highest bank snapshot of type BankSnapshotType::Post
+pub fn get_highest_bank_snapshot_post(
+    bank_snapshots_dir: impl AsRef<Path>,
+) -> Option<BankSnapshotInfo> {
+    do_get_highest_bank_snapshot(get_bank_snapshots_post(bank_snapshots_dir))
+}
+
+fn do_get_highest_bank_snapshot(
+    mut bank_snapshots: Vec<BankSnapshotInfo>,
+) -> Option<BankSnapshotInfo> {
+    bank_snapshots.sort_unstable();
+    bank_snapshots.into_iter().rev().next()
 }
 
 pub fn serialize_snapshot_data_file<F>(data_file_path: &Path, serializer: F) -> Result<u64>
@@ -619,11 +689,14 @@ pub fn add_bank_snapshot<P: AsRef<Path>>(
     let bank_snapshots_dir = get_bank_snapshots_dir(bank_snapshots_dir, slot);
     fs::create_dir_all(&bank_snapshots_dir)?;
 
-    // the bank snapshot is stored as bank_snapshots_dir/slot/slot
+    // the bank snapshot is stored as bank_snapshots_dir/slot/slot.BANK_SNAPSHOT_PRE_FILENAME_EXTENSION
-    let snapshot_bank_file_path = bank_snapshots_dir.join(get_snapshot_file_name(slot));
+    let mut bank_snapshot_path = bank_snapshots_dir.join(get_snapshot_file_name(slot));
+    bank_snapshot_path.set_extension(BANK_SNAPSHOT_PRE_FILENAME_EXTENSION);
+
     info!(
-        "Creating snapshot for slot {}, path: {:?}",
+        "Creating bank snapshot for slot {}, path: {}",
-        slot, snapshot_bank_file_path,
+        slot,
+        bank_snapshot_path.display(),
     );
 
     let mut bank_serialize = Measure::start("bank-serialize-ms");
@@ -635,7 +708,7 @@ pub fn add_bank_snapshot<P: AsRef<Path>>(
         Ok(())
     };
     let consumed_size =
-        serialize_snapshot_data_file(&snapshot_bank_file_path, bank_snapshot_serializer)?;
+        serialize_snapshot_data_file(&bank_snapshot_path, bank_snapshot_serializer)?;
     bank_serialize.stop();
 
     // Monitor sizes because they're capped to MAX_SNAPSHOT_DATA_FILE_SIZE
@@ -648,13 +721,16 @@ pub fn add_bank_snapshot<P: AsRef<Path>>(
     inc_new_counter_info!("bank-serialize-ms", bank_serialize.as_ms() as usize);
 
     info!(
-        "{} for slot {} at {:?}",
+        "{} for slot {} at {}",
-        bank_serialize, slot, snapshot_bank_file_path,
+        bank_serialize,
+        slot,
+        bank_snapshot_path.display(),
     );
 
     Ok(BankSnapshotInfo {
         slot,
-        snapshot_path: snapshot_bank_file_path,
+        snapshot_path: bank_snapshot_path,
+        snapshot_type: BankSnapshotType::Pre,
     })
 }
 
@@ -1442,13 +1518,13 @@ fn verify_unpacked_snapshots_dir_and_version(
                     &unpacked_snapshots_dir_and_version.snapshot_version,
                 ))
             })?;
-    let mut bank_snapshot_infos =
+    let mut bank_snapshots =
-        get_bank_snapshots(&unpacked_snapshots_dir_and_version.unpacked_snapshots_dir);
+        get_bank_snapshots_post(&unpacked_snapshots_dir_and_version.unpacked_snapshots_dir);
-    if bank_snapshot_infos.len() > 1 {
+    if bank_snapshots.len() > 1 {
         return Err(get_io_error("invalid snapshot format"));
     }
-    bank_snapshot_infos.sort_unstable();
+    bank_snapshots.sort_unstable();
-    let root_paths = bank_snapshot_infos
+    let root_paths = bank_snapshots
         .pop()
         .ok_or_else(|| get_io_error("No snapshots found in snapshots directory"))?;
     Ok((snapshot_version, root_paths))
@@ -1560,11 +1636,11 @@ fn rebuild_bank_from_snapshots(
     Ok(bank)
 }
 
-fn get_snapshot_file_name(slot: Slot) -> String {
+pub(crate) fn get_snapshot_file_name(slot: Slot) -> String {
     slot.to_string()
 }
 
-fn get_bank_snapshots_dir<P: AsRef<Path>>(path: P, slot: Slot) -> PathBuf {
+pub(crate) fn get_bank_snapshots_dir<P: AsRef<Path>>(path: P, slot: Slot) -> PathBuf {
     path.as_ref().join(slot.to_string())
 }
 
@@ -1604,25 +1680,26 @@ pub fn verify_snapshot_archive<P, Q, R>(
 }
 
 /// Remove outdated bank snapshots
-pub fn purge_old_bank_snapshots<P>(bank_snapshots_dir: P)
+pub fn purge_old_bank_snapshots(bank_snapshots_dir: impl AsRef<Path>) {
-where
+    let do_purge = |mut bank_snapshots: Vec<BankSnapshotInfo>| {
-    P: AsRef<Path>,
+        bank_snapshots.sort_unstable();
-{
+        bank_snapshots
-    let mut bank_snapshot_infos = get_bank_snapshots(&bank_snapshots_dir);
+            .into_iter()
-    bank_snapshot_infos.sort_unstable();
+            .rev()
-    bank_snapshot_infos
+            .skip(MAX_BANK_SNAPSHOTS_TO_RETAIN)
-        .into_iter()
+            .for_each(|bank_snapshot| {
-        .rev()
+                let r = remove_bank_snapshot(bank_snapshot.slot, &bank_snapshots_dir);
-        .skip(MAX_BANK_SNAPSHOTS_TO_RETAIN)
+                if r.is_err() {
-        .for_each(|bank_snapshot_info| {
+                    warn!(
-            let r = remove_bank_snapshot(bank_snapshot_info.slot, &bank_snapshots_dir);
+                        "Couldn't remove bank snapshot at: {}",
-            if r.is_err() {
+                        bank_snapshot.snapshot_path.display()
-                warn!(
+                    );
-                    "Couldn't remove bank snapshot at: {}",
+                }
-                    bank_snapshot_info.snapshot_path.display()
+            })
-                );
+    };
-            }
+
-        })
+    do_purge(get_bank_snapshots_pre(&bank_snapshots_dir));
+    do_purge(get_bank_snapshots_post(&bank_snapshots_dir));
 }
 
 /// Gather the necessary elements for a snapshot of the given `root_bank`.
@@ -1819,6 +1896,11 @@ pub fn package_and_archive_full_snapshot(
         Some(SnapshotType::FullSnapshot),
     )?;
 
+    crate::serde_snapshot::reserialize_bank(
+        accounts_package.snapshot_links.path(),
+        accounts_package.slot,
+    );
+
     let snapshot_package = SnapshotPackage::from(accounts_package);
     archive_snapshot_package(
         &snapshot_package,
@@ -1860,6 +1942,11 @@ pub fn package_and_archive_incremental_snapshot(
         )),
     )?;
 
+    crate::serde_snapshot::reserialize_bank(
+        accounts_package.snapshot_links.path(),
+        accounts_package.slot,
+    );
+
     let snapshot_package = SnapshotPackage::from(accounts_package);
     archive_snapshot_package(
         &snapshot_package,
@@ -2279,28 +2366,28 @@ mod tests {
     }
 
     #[test]
-    fn test_get_bank_snapshot_infos() {
+    fn test_get_bank_snapshots() {
         solana_logger::setup();
         let temp_snapshots_dir = tempfile::TempDir::new().unwrap();
         let min_slot = 10;
         let max_slot = 20;
         common_create_bank_snapshot_files(temp_snapshots_dir.path(), min_slot, max_slot);
 
-        let bank_snapshot_infos = get_bank_snapshots(temp_snapshots_dir.path());
+        let bank_snapshots = get_bank_snapshots(temp_snapshots_dir.path());
-        assert_eq!(bank_snapshot_infos.len() as Slot, max_slot - min_slot);
+        assert_eq!(bank_snapshots.len() as Slot, max_slot - min_slot);
     }
 
     #[test]
-    fn test_get_highest_bank_snapshot_info() {
+    fn test_get_highest_bank_snapshot_post() {
         solana_logger::setup();
         let temp_snapshots_dir = tempfile::TempDir::new().unwrap();
         let min_slot = 99;
         let max_slot = 123;
         common_create_bank_snapshot_files(temp_snapshots_dir.path(), min_slot, max_slot);
 
-        let highest_bank_snapshot_info = get_highest_bank_snapshot_info(temp_snapshots_dir.path());
+        let highest_bank_snapshot = get_highest_bank_snapshot_post(temp_snapshots_dir.path());
-        assert!(highest_bank_snapshot_info.is_some());
+        assert!(highest_bank_snapshot.is_some());
-        assert_eq!(highest_bank_snapshot_info.unwrap().slot, max_slot - 1);
+        assert_eq!(highest_bank_snapshot.unwrap().slot, max_slot - 1);
     }
 
     /// A test helper function that creates full and incremental snapshot archive files.  Creates