From 6a322de845ec0671cf8fbde64073b84452851581 Mon Sep 17 00:00:00 2001 From: Brooks Prumo Date: Wed, 7 Sep 2022 16:41:40 -0400 Subject: [PATCH] Make Accounts Background Services aware of Epoch Accounts Hash (#27626) --- core/src/accounts_hash_verifier.rs | 33 ++- core/tests/snapshots.rs | 8 +- runtime/src/accounts_background_service.rs | 55 ++-- runtime/src/accounts_db.rs | 2 +- runtime/src/bank_forks.rs | 3 +- runtime/src/lib.rs | 2 +- runtime/src/snapshot_package.rs | 114 ++++---- runtime/src/snapshot_utils.rs | 295 ++++++++++++++------- 8 files changed, 326 insertions(+), 186 deletions(-) diff --git a/core/src/accounts_hash_verifier.rs b/core/src/accounts_hash_verifier.rs index 33bf4e3f24..040e894506 100644 --- a/core/src/accounts_hash_verifier.rs +++ b/core/src/accounts_hash_verifier.rs @@ -9,10 +9,11 @@ use { solana_measure::measure::Measure, solana_runtime::{ accounts_hash::{CalcAccountsHashConfig, HashStats}, + epoch_accounts_hash::EpochAccountsHash, snapshot_config::SnapshotConfig, snapshot_package::{ - retain_max_n_elements, AccountsPackage, PendingAccountsPackage, PendingSnapshotPackage, - SnapshotPackage, SnapshotType, + retain_max_n_elements, AccountsPackage, AccountsPackageType, PendingAccountsPackage, + PendingSnapshotPackage, SnapshotPackage, SnapshotType, }, sorted_storages::SortedStorages, }, @@ -98,6 +99,8 @@ impl AccountsHashVerifier { ) { let accounts_hash = Self::calculate_and_verify_accounts_hash(&accounts_package); + Self::save_epoch_accounts_hash(&accounts_package, accounts_hash); + Self::push_accounts_hashes_to_cluster( &accounts_package, cluster_info, @@ -228,6 +231,21 @@ impl AccountsHashVerifier { accounts_hash } + fn save_epoch_accounts_hash(accounts_package: &AccountsPackage, accounts_hash: Hash) { + if accounts_package.package_type == AccountsPackageType::EpochAccountsHash { + let new_epoch_accounts_hash = EpochAccountsHash::new(accounts_hash); + let old_epoch_accounts_hash = accounts_package + .accounts + .accounts_db + .epoch_accounts_hash + .lock() + .unwrap() + .replace(new_epoch_accounts_hash); + // Old epoch accounts hash must be NONE, because a previous bank must have taken it to hash into itself + assert!(old_epoch_accounts_hash.is_none()); + } + } + fn generate_fault_hash(original_hash: &Hash) -> Hash { use { rand::{thread_rng, Rng}, @@ -280,14 +298,17 @@ impl AccountsHashVerifier { snapshot_config: Option<&SnapshotConfig>, accounts_hash: Hash, ) { - if accounts_package.snapshot_type.is_none() - || pending_snapshot_package.is_none() + if pending_snapshot_package.is_none() || !snapshot_config .map(|snapshot_config| snapshot_config.should_generate_snapshots()) .unwrap_or(false) + || !matches!( + accounts_package.package_type, + AccountsPackageType::Snapshot(_) + ) { return; - }; + } let snapshot_package = SnapshotPackage::new(accounts_package, accounts_hash); let pending_snapshot_package = pending_snapshot_package.unwrap(); @@ -444,6 +465,7 @@ mod tests { let expected_hash = Hash::from_str("GKot5hBsd81kMupNCXHaqbhv3huEbxAFMLnpcX2hniwn").unwrap(); for i in 0..MAX_SNAPSHOT_HASHES + 1 { let accounts_package = AccountsPackage { + package_type: AccountsPackageType::AccountsHashVerifier, slot: full_snapshot_archive_interval_slots + i as u64, block_height: full_snapshot_archive_interval_slots + i as u64, slot_deltas: vec![], @@ -456,7 +478,6 @@ mod tests { expected_capitalization: 0, accounts_hash_for_testing: None, cluster_type: ClusterType::MainnetBeta, - snapshot_type: None, accounts: Arc::clone(&accounts), epoch_schedule: EpochSchedule::default(), rent_collector: RentCollector::default(), diff --git a/core/tests/snapshots.rs b/core/tests/snapshots.rs index d83fce5c25..55f5a74dfa 100644 --- a/core/tests/snapshots.rs +++ b/core/tests/snapshots.rs @@ -25,8 +25,8 @@ use { snapshot_archive_info::FullSnapshotArchiveInfo, snapshot_config::SnapshotConfig, snapshot_package::{ - AccountsPackage, PendingAccountsPackage, PendingSnapshotPackage, SnapshotPackage, - SnapshotType, + AccountsPackage, AccountsPackageType, PendingAccountsPackage, PendingSnapshotPackage, + SnapshotPackage, SnapshotType, }, snapshot_utils::{ self, ArchiveFormat, @@ -240,6 +240,7 @@ fn run_bank_forks_snapshot_n( .expect("no bank snapshots found in path"); let slot_deltas = last_bank.status_cache.read().unwrap().root_slot_deltas(); let accounts_package = AccountsPackage::new( + AccountsPackageType::Snapshot(SnapshotType::FullSnapshot), &last_bank, &last_bank_snapshot_info, bank_snapshots_dir, @@ -250,7 +251,6 @@ fn run_bank_forks_snapshot_n( ArchiveFormat::TarBzip2, snapshot_version, None, - Some(SnapshotType::FullSnapshot), ) .unwrap(); solana_runtime::serde_snapshot::reserialize_bank_with_new_accounts_hash( @@ -391,7 +391,7 @@ fn test_concurrent_snapshot_packaging( snapshot_config.snapshot_version, snapshot_config.archive_format, None, - Some(SnapshotType::FullSnapshot), + AccountsPackageType::Snapshot(SnapshotType::FullSnapshot), ) .unwrap(); diff --git a/runtime/src/accounts_background_service.rs b/runtime/src/accounts_background_service.rs index 7b70980dc3..e5e3708bec 100644 --- a/runtime/src/accounts_background_service.rs +++ b/runtime/src/accounts_background_service.rs @@ -9,7 +9,7 @@ use { bank::{Bank, BankSlotDelta, DropCallback}, bank_forks::BankForks, snapshot_config::SnapshotConfig, - snapshot_package::{PendingAccountsPackage, SnapshotType}, + snapshot_package::{AccountsPackageType, PendingAccountsPackage, SnapshotType}, snapshot_utils::{self, SnapshotError}, }, crossbeam_channel::{Receiver, SendError, Sender}, @@ -112,6 +112,18 @@ impl SendDroppedBankCallback { pub struct SnapshotRequest { pub snapshot_root_bank: Arc, pub status_cache_slot_deltas: Vec, + pub request_type: SnapshotRequestType, +} + +/// What type of request is this? +/// +/// The snapshot request has been expanded to support more than just snapshots. This is +/// confusing, but can be resolved by renaming this type; or better, by creating an enum with +/// variants that wrap the fields-of-interest for each request. +#[derive(Debug, Copy, Clone, Eq, PartialEq)] +pub enum SnapshotRequestType { + Snapshot, + EpochAccountsHash, } pub struct SnapshotRequestHandler { @@ -137,6 +149,7 @@ impl SnapshotRequestHandler { let SnapshotRequest { snapshot_root_bank, status_cache_slot_deltas, + request_type, } = snapshot_request; // we should not rely on the state of this validator until startup verification is complete @@ -216,23 +229,19 @@ impl SnapshotRequestHandler { } let block_height = snapshot_root_bank.block_height(); - let snapshot_type = if snapshot_utils::should_take_full_snapshot( - block_height, - self.snapshot_config.full_snapshot_archive_interval_slots, - ) { - *last_full_snapshot_slot = Some(snapshot_root_bank.slot()); - Some(SnapshotType::FullSnapshot) - } else if snapshot_utils::should_take_incremental_snapshot( - block_height, - self.snapshot_config - .incremental_snapshot_archive_interval_slots, - *last_full_snapshot_slot, - ) { - Some(SnapshotType::IncrementalSnapshot( - last_full_snapshot_slot.unwrap(), - )) - } else { - None + let accounts_package_type = match request_type { + SnapshotRequestType::EpochAccountsHash => AccountsPackageType::EpochAccountsHash, + _ => { + if snapshot_utils::should_take_full_snapshot(block_height, self.snapshot_config.full_snapshot_archive_interval_slots) { + *last_full_snapshot_slot = Some(snapshot_root_bank.slot()); + AccountsPackageType::Snapshot(SnapshotType::FullSnapshot) + } else if snapshot_utils::should_take_incremental_snapshot(block_height, self.snapshot_config .incremental_snapshot_archive_interval_slots, *last_full_snapshot_slot) { + AccountsPackageType::Snapshot(SnapshotType::IncrementalSnapshot( last_full_snapshot_slot.unwrap())) + } else { + AccountsPackageType::AccountsHashVerifier + } + }, + }; // Snapshot the bank and send over an accounts package @@ -247,13 +256,13 @@ impl SnapshotRequestHandler { self.snapshot_config.snapshot_version, self.snapshot_config.archive_format, hash_for_testing, - snapshot_type, + accounts_package_type, ); if let Err(e) = result { warn!( - "Error taking bank snapshot. slot: {}, snapshot type: {:?}, err: {:?}", + "Error taking bank snapshot. slot: {}, accounts package type: {:?}, err: {:?}", snapshot_root_bank.slot(), - snapshot_type, + accounts_package_type, e, ); @@ -262,8 +271,8 @@ impl SnapshotRequestHandler { } } snapshot_time.stop(); - info!("Took bank snapshot. snapshot type: {:?}, slot: {}, accounts hash: {}, bank hash: {}", - snapshot_type, + info!("Took bank snapshot. accounts package type: {:?}, slot: {}, accounts hash: {}, bank hash: {}", + accounts_package_type, snapshot_root_bank.slot(), snapshot_root_bank.get_accounts_hash(), snapshot_root_bank.hash(), diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index 44da3a11f5..968e701a46 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -1195,7 +1195,7 @@ pub struct AccountsDb { /// The cadence is once per epoch, all nodes calculate a full accounts hash as of a known slot calculated using 'N' /// Some time later (to allow for slow calculation time), the bank hash at a slot calculated using 'M' includes the full accounts hash. /// Thus, the state of all accounts on a validator is known to be correct at least once per epoch. - pub(crate) epoch_accounts_hash: Mutex>, + pub epoch_accounts_hash: Mutex>, } #[derive(Debug, Default)] diff --git a/runtime/src/bank_forks.rs b/runtime/src/bank_forks.rs index 6785b962aa..fbc2efbcde 100644 --- a/runtime/src/bank_forks.rs +++ b/runtime/src/bank_forks.rs @@ -2,7 +2,7 @@ use { crate::{ - accounts_background_service::{AbsRequestSender, SnapshotRequest}, + accounts_background_service::{AbsRequestSender, SnapshotRequest, SnapshotRequestType}, bank::Bank, snapshot_config::SnapshotConfig, }, @@ -290,6 +290,7 @@ impl BankForks { SnapshotRequest { snapshot_root_bank, status_cache_slot_deltas, + request_type: SnapshotRequestType::Snapshot, }, ) { warn!( diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index 247df17fe1..9a86795eb7 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -34,7 +34,7 @@ pub mod commitment; pub mod contains; pub mod cost_model; pub mod cost_tracker; -mod epoch_accounts_hash; +pub mod epoch_accounts_hash; pub mod epoch_stakes; pub mod execute_cost_table; mod expected_rent_collection; diff --git a/runtime/src/snapshot_package.rs b/runtime/src/snapshot_package.rs index 8dec38f18a..a34a5393db 100644 --- a/runtime/src/snapshot_package.rs +++ b/runtime/src/snapshot_package.rs @@ -32,6 +32,7 @@ pub type PendingSnapshotPackage = Arc>>; #[derive(Debug)] pub struct AccountsPackage { + pub package_type: AccountsPackageType, pub slot: Slot, pub block_height: Slot, pub slot_deltas: Vec, @@ -44,7 +45,6 @@ pub struct AccountsPackage { pub expected_capitalization: u64, pub accounts_hash_for_testing: Option, pub cluster_type: ClusterType, - pub snapshot_type: Option, pub accounts: Arc, pub epoch_schedule: EpochSchedule, pub rent_collector: RentCollector, @@ -54,6 +54,7 @@ impl AccountsPackage { /// Package up bank files, storages, and slot deltas for a snapshot #[allow(clippy::too_many_arguments)] pub fn new( + package_type: AccountsPackageType, bank: &Bank, bank_snapshot_info: &BankSnapshotInfo, bank_snapshots_dir: impl AsRef, @@ -64,22 +65,21 @@ impl AccountsPackage { archive_format: ArchiveFormat, snapshot_version: SnapshotVersion, accounts_hash_for_testing: Option, - snapshot_type: Option, ) -> Result { - info!( - "Package snapshot for bank {} has {} account storage entries (snapshot type: {:?})", - bank.slot(), - snapshot_storages.len(), - snapshot_type, - ); - - if let Some(SnapshotType::IncrementalSnapshot(incremental_snapshot_base_slot)) = - snapshot_type - { - assert!( - bank.slot() > incremental_snapshot_base_slot, - "Incremental snapshot base slot must be less than the bank being snapshotted!" + if let AccountsPackageType::Snapshot(snapshot_type) = package_type { + info!( + "Package snapshot for bank {} has {} account storage entries (snapshot type: {:?})", + bank.slot(), + snapshot_storages.len(), + snapshot_type, ); + if let SnapshotType::IncrementalSnapshot(incremental_snapshot_base_slot) = snapshot_type + { + assert!( + bank.slot() > incremental_snapshot_base_slot, + "Incremental snapshot base slot must be less than the bank being snapshotted!" + ); + } } // Hard link the snapshot into a tmpdir, to ensure its not removed prior to packaging. @@ -100,6 +100,7 @@ impl AccountsPackage { } Ok(Self { + package_type, slot: bank.slot(), block_height: bank.block_height(), slot_deltas, @@ -114,7 +115,6 @@ impl AccountsPackage { expected_capitalization: bank.capitalization(), accounts_hash_for_testing, cluster_type: bank.cluster_type(), - snapshot_type, accounts: bank.accounts(), epoch_schedule: *bank.epoch_schedule(), rent_collector: bank.rent_collector().clone(), @@ -122,6 +122,16 @@ impl AccountsPackage { } } +/// Accounts packages are sent to the Accounts Hash Verifier for processing. There are multiple +/// types of accounts packages, which are specified as variants in this enum. All accounts +/// packages do share some processing: such as calculating the accounts hash. +#[derive(Debug, Copy, Clone, Eq, PartialEq)] +pub enum AccountsPackageType { + AccountsHashVerifier, + Snapshot(SnapshotType), + EpochAccountsHash, +} + pub struct SnapshotPackage { pub snapshot_archive_info: SnapshotArchiveInfo, pub block_height: Slot, @@ -134,40 +144,46 @@ pub struct SnapshotPackage { impl SnapshotPackage { pub fn new(accounts_package: AccountsPackage, accounts_hash: Hash) -> Self { - assert!( - accounts_package.snapshot_type.is_some(), - "Cannot make a SnapshotPackage from an AccountsPackage when SnapshotType is None!" - ); - let mut snapshot_storages = accounts_package.snapshot_storages; - let snapshot_archive_path = match accounts_package.snapshot_type.unwrap() { - SnapshotType::FullSnapshot => snapshot_utils::build_full_snapshot_archive_path( - accounts_package.full_snapshot_archives_dir, - accounts_package.slot, - &accounts_hash, - accounts_package.archive_format, - ), - SnapshotType::IncrementalSnapshot(incremental_snapshot_base_slot) => { - snapshot_storages.retain(|storages| { - storages - .first() // storages are grouped by slot in the outer Vec, so all storages will have the same slot as the first - .map(|storage| storage.slot() > incremental_snapshot_base_slot) - .unwrap_or_default() - }); - assert!( - snapshot_storages.iter().all(|storage| storage - .iter() - .all(|entry| entry.slot() > incremental_snapshot_base_slot)), - "Incremental snapshot package must only contain storage entries where slot > incremental snapshot base slot (i.e. full snapshot slot)!" + let (snapshot_type, snapshot_archive_path) = match accounts_package.package_type { + AccountsPackageType::Snapshot(snapshot_type) => match snapshot_type { + SnapshotType::FullSnapshot => ( + snapshot_type, + snapshot_utils::build_full_snapshot_archive_path( + accounts_package.full_snapshot_archives_dir, + accounts_package.slot, + &accounts_hash, + accounts_package.archive_format, + ), + ), + SnapshotType::IncrementalSnapshot(incremental_snapshot_base_slot) => { + snapshot_storages.retain(|storages| { + storages + .first() // storages are grouped by slot in the outer Vec, so all storages will have the same slot as the first + .map(|storage| storage.slot() > incremental_snapshot_base_slot) + .unwrap_or_default() + }); + assert!( + snapshot_storages.iter().all(|storage| storage + .iter() + .all(|entry| entry.slot() > incremental_snapshot_base_slot)), + "Incremental snapshot package must only contain storage entries where slot > incremental snapshot base slot (i.e. full snapshot slot)!" ); - snapshot_utils::build_incremental_snapshot_archive_path( - accounts_package.incremental_snapshot_archives_dir, - incremental_snapshot_base_slot, - accounts_package.slot, - &accounts_hash, - accounts_package.archive_format, - ) - } + ( + snapshot_type, + snapshot_utils::build_incremental_snapshot_archive_path( + accounts_package.incremental_snapshot_archives_dir, + incremental_snapshot_base_slot, + accounts_package.slot, + &accounts_hash, + accounts_package.archive_format, + ), + ) + } + }, + _ => panic!( + "The AccountsPackage must be of type Snapshot in order to make a SnapshotPackage!" + ), }; Self { @@ -182,7 +198,7 @@ impl SnapshotPackage { snapshot_links: accounts_package.snapshot_links, snapshot_storages, snapshot_version: accounts_package.snapshot_version, - snapshot_type: accounts_package.snapshot_type.unwrap(), + snapshot_type, } } } diff --git a/runtime/src/snapshot_utils.rs b/runtime/src/snapshot_utils.rs index 9e80f5c9d5..11e2afc264 100644 --- a/runtime/src/snapshot_utils.rs +++ b/runtime/src/snapshot_utils.rs @@ -17,7 +17,8 @@ use { FullSnapshotArchiveInfo, IncrementalSnapshotArchiveInfo, SnapshotArchiveInfoGetter, }, snapshot_package::{ - AccountsPackage, PendingAccountsPackage, SnapshotPackage, SnapshotType, + AccountsPackage, AccountsPackageType, PendingAccountsPackage, SnapshotPackage, + SnapshotType, }, snapshot_utils::snapshot_storage_rebuilder::SnapshotStorageRebuilder, status_cache, @@ -2099,7 +2100,7 @@ pub fn snapshot_bank( snapshot_version: SnapshotVersion, archive_format: ArchiveFormat, hash_for_testing: Option, - snapshot_type: Option, + accounts_package_type: AccountsPackageType, ) -> Result<()> { let snapshot_storages = get_snapshot_storages(root_bank); @@ -2114,6 +2115,7 @@ pub fn snapshot_bank( inc_new_counter_info!("add-snapshot-ms", add_snapshot_time.as_ms() as usize); let accounts_package = AccountsPackage::new( + accounts_package_type, root_bank, &bank_snapshot_info, bank_snapshots_dir, @@ -2124,7 +2126,6 @@ pub fn snapshot_bank( archive_format, snapshot_version, hash_for_testing, - snapshot_type, ) .expect("failed to hard link bank snapshot into a tmpdir"); @@ -2133,17 +2134,18 @@ pub fn snapshot_bank( { let mut pending_accounts_package = pending_accounts_package.lock().unwrap(); if can_submit_accounts_package(&accounts_package, pending_accounts_package.as_ref()) { + let package_type = accounts_package.package_type; let old_accounts_package = pending_accounts_package.replace(accounts_package); drop(pending_accounts_package); if let Some(old_accounts_package) = old_accounts_package { - debug!( + info!( "The pending AccountsPackage has been overwritten: \ - \nNew AccountsPackage slot: {}, snapshot type: {:?} \ - \nOld AccountsPackage slot: {}, snapshot type: {:?}", + \nNew AccountsPackage slot: {}, package type: {:?} \ + \nOld AccountsPackage slot: {}, package type: {:?}", root_bank.slot(), - snapshot_type, + package_type, old_accounts_package.slot, - old_accounts_package.snapshot_type, + old_accounts_package.package_type, ); } } @@ -2279,6 +2281,7 @@ pub fn package_and_archive_full_snapshot( ) -> Result { let slot_deltas = bank.status_cache.read().unwrap().root_slot_deltas(); let accounts_package = AccountsPackage::new( + AccountsPackageType::Snapshot(SnapshotType::FullSnapshot), bank, bank_snapshot_info, bank_snapshots_dir, @@ -2289,7 +2292,6 @@ pub fn package_and_archive_full_snapshot( archive_format, snapshot_version, None, - Some(SnapshotType::FullSnapshot), )?; crate::serde_snapshot::reserialize_bank_with_new_accounts_hash( @@ -2331,6 +2333,9 @@ pub fn package_and_archive_incremental_snapshot( ) -> Result { let slot_deltas = bank.status_cache.read().unwrap().root_slot_deltas(); let accounts_package = AccountsPackage::new( + AccountsPackageType::Snapshot(SnapshotType::IncrementalSnapshot( + incremental_snapshot_base_slot, + )), bank, bank_snapshot_info, bank_snapshots_dir, @@ -2341,9 +2346,6 @@ pub fn package_and_archive_incremental_snapshot( archive_format, snapshot_version, None, - Some(SnapshotType::IncrementalSnapshot( - incremental_snapshot_base_slot, - )), )?; crate::serde_snapshot::reserialize_bank_with_new_accounts_hash( @@ -2389,6 +2391,7 @@ pub fn should_take_incremental_snapshot( /// /// If there's no pending accounts package, then submit /// Otherwise, check if the pending accounts package can be overwritten +#[must_use] fn can_submit_accounts_package( accounts_package: &AccountsPackage, pending_accounts_package: Option<&AccountsPackage>, @@ -2402,23 +2405,39 @@ fn can_submit_accounts_package( /// Decide when it is appropriate to overwrite a pending accounts package /// -/// This is based on the values for `snapshot_type` in both the `accounts_package` and the -/// `pending_accounts_package`: -/// - if the new AccountsPackage is for a full snapshot, always overwrite -/// - if the new AccountsPackage is for an incremental snapshot, overwrite as long as there isn't a -/// pending full snapshot -/// - otherwise, only overwrite if the pending package's snapshot type is None +/// The priority of the package types is (from highest to lowest): +/// 1. Epoch Account Hash +/// 2. Full Snapshot +/// 3. Incremental Snapshot +/// 4. Accounts Hash Verifier +/// +/// New packages of higher priority types can overwrite pending packages of *equivalent or lower* +/// priority types. +#[must_use] fn can_overwrite_pending_accounts_package( accounts_package: &AccountsPackage, pending_accounts_package: &AccountsPackage, ) -> bool { - match accounts_package.snapshot_type { - Some(SnapshotType::FullSnapshot) => true, - Some(SnapshotType::IncrementalSnapshot(_)) => pending_accounts_package - .snapshot_type - .map(|snapshot_type| !snapshot_type.is_full_snapshot()) - .unwrap_or(true), - None => pending_accounts_package.snapshot_type.is_none(), + match ( + &pending_accounts_package.package_type, + &accounts_package.package_type, + ) { + (AccountsPackageType::EpochAccountsHash, AccountsPackageType::EpochAccountsHash) => { + panic!( + "Both pending and new accounts packages are of type EpochAccountsHash! \ + EAH calculations must complete before new ones are enqueued. \ + \npending accounts package slot: {} \ + \n new accounts package slot: {}", + pending_accounts_package.slot, accounts_package.slot, + ); + } + (_, AccountsPackageType::EpochAccountsHash) => true, + (AccountsPackageType::EpochAccountsHash, _) => false, + (_, AccountsPackageType::Snapshot(SnapshotType::FullSnapshot)) => true, + (AccountsPackageType::Snapshot(SnapshotType::FullSnapshot), _) => false, + (_, AccountsPackageType::Snapshot(SnapshotType::IncrementalSnapshot(_))) => true, + (AccountsPackageType::Snapshot(SnapshotType::IncrementalSnapshot(_)), _) => false, + _ => true, } } @@ -2426,7 +2445,10 @@ fn can_overwrite_pending_accounts_package( mod tests { use { super::*, - crate::{accounts_db::ACCOUNTS_DB_CONFIG_FOR_TESTING, status_cache::Status}, + crate::{ + accounts_db::ACCOUNTS_DB_CONFIG_FOR_TESTING, snapshot_package::AccountsPackageType, + status_cache::Status, + }, assert_matches::assert_matches, bincode::{deserialize_from, serialize_into}, solana_sdk::{ @@ -4012,91 +4034,162 @@ mod tests { assert_eq!(bank_fields.parent_slot, bank2.parent_slot()); } - /// All the permutations of `snapshot_type` for the new-and-old accounts packages: + /// Test `can_submit_accounts_packages()` with all permutations of `package_type` + /// for both the pending accounts package and the new accounts package. /// - /// new | old | - /// snapshot | snapshot | - /// type | type | result - /// ----------+----------+-------- - /// FSS | FSS | true - /// FSS | ISS | true - /// FSS | None | true - /// ISS | FSS | false - /// ISS | ISS | true - /// ISS | None | true - /// None | FSS | false - /// None | ISS | false - /// None | None | true + /// pending | new | + /// package | package | result + /// ---------+---------+-------- + /// 1. None | X | true + /// 2. AHV | AHV | true + /// 3. AHV | ISS | true + /// 4. AHV | FSS | true + /// 5. AHV | EAH | true + /// 6. ISS | AHV | false + /// 7. ISS | ISS | true + /// 8. ISS | FSS | true + /// 9. ISS | EAH | true + /// 10. FSS | AHV | false + /// 11. FSS | ISS | false + /// 12. FSS | FSS | true + /// 13. FSS | EAH | true + /// 14. EAH | AHV | false + /// 15. EAH | ISS | false + /// 16. EAH | FSS | false + /// 17. EAH | EAH | assert unreachable, so test separately #[test] fn test_can_submit_accounts_package() { - /// helper function to create an AccountsPackage that's good enough for this test - fn new_accounts_package_with(snapshot_type: Option) -> AccountsPackage { - AccountsPackage { - slot: Slot::default(), - block_height: Slot::default(), - slot_deltas: Vec::default(), - snapshot_links: TempDir::new().unwrap(), - snapshot_storages: SnapshotStorages::default(), - archive_format: ArchiveFormat::Tar, - snapshot_version: SnapshotVersion::default(), - full_snapshot_archives_dir: PathBuf::default(), - incremental_snapshot_archives_dir: PathBuf::default(), - expected_capitalization: u64::default(), - accounts_hash_for_testing: None, - cluster_type: solana_sdk::genesis_config::ClusterType::Development, - snapshot_type, - accounts: Arc::new(crate::accounts::Accounts::default_for_tests()), - epoch_schedule: solana_sdk::epoch_schedule::EpochSchedule::default(), - rent_collector: crate::rent_collector::RentCollector::default(), - } - } - - for (new_snapshot_type, old_snapshot_type, expected_result) in [ - ( - Some(SnapshotType::FullSnapshot), - Some(SnapshotType::FullSnapshot), - true, - ), - ( - Some(SnapshotType::FullSnapshot), - Some(SnapshotType::IncrementalSnapshot(0)), - true, - ), - (Some(SnapshotType::FullSnapshot), None, true), - ( - Some(SnapshotType::IncrementalSnapshot(0)), - Some(SnapshotType::FullSnapshot), - false, - ), - ( - Some(SnapshotType::IncrementalSnapshot(0)), - Some(SnapshotType::IncrementalSnapshot(0)), - true, - ), - (Some(SnapshotType::IncrementalSnapshot(0)), None, true), - (None, Some(SnapshotType::FullSnapshot), false), - (None, Some(SnapshotType::IncrementalSnapshot(0)), false), - (None, None, true), - ] { - let new_accounts_package = new_accounts_package_with(new_snapshot_type); - let old_accounts_package = new_accounts_package_with(old_snapshot_type); - - let actual_result = can_overwrite_pending_accounts_package( - &new_accounts_package, - &old_accounts_package, - ); - assert_eq!(expected_result, actual_result); - } - - // Also test when the pending package is None + // Test 1 { - let accounts_package = new_accounts_package_with(None); let pending_accounts_package = None; + let accounts_package = new_accounts_package(AccountsPackageType::AccountsHashVerifier); assert!(can_submit_accounts_package( &accounts_package, pending_accounts_package )); } + + for (pending_package_type, new_package_type, expected_result) in [ + // Tests 2-5, pending package is AHV + ( + AccountsPackageType::AccountsHashVerifier, + AccountsPackageType::AccountsHashVerifier, + true, + ), + ( + AccountsPackageType::AccountsHashVerifier, + AccountsPackageType::Snapshot(SnapshotType::IncrementalSnapshot(0)), + true, + ), + ( + AccountsPackageType::AccountsHashVerifier, + AccountsPackageType::Snapshot(SnapshotType::FullSnapshot), + true, + ), + ( + AccountsPackageType::AccountsHashVerifier, + AccountsPackageType::EpochAccountsHash, + true, + ), + // Tests 6-9, pending package is ISS + ( + AccountsPackageType::Snapshot(SnapshotType::IncrementalSnapshot(0)), + AccountsPackageType::AccountsHashVerifier, + false, + ), + ( + AccountsPackageType::Snapshot(SnapshotType::IncrementalSnapshot(0)), + AccountsPackageType::Snapshot(SnapshotType::IncrementalSnapshot(0)), + true, + ), + ( + AccountsPackageType::Snapshot(SnapshotType::IncrementalSnapshot(0)), + AccountsPackageType::Snapshot(SnapshotType::FullSnapshot), + true, + ), + ( + AccountsPackageType::Snapshot(SnapshotType::IncrementalSnapshot(0)), + AccountsPackageType::EpochAccountsHash, + true, + ), + // Tests 10-13, pending package is FSS + ( + AccountsPackageType::Snapshot(SnapshotType::FullSnapshot), + AccountsPackageType::AccountsHashVerifier, + false, + ), + ( + AccountsPackageType::Snapshot(SnapshotType::FullSnapshot), + AccountsPackageType::Snapshot(SnapshotType::IncrementalSnapshot(0)), + false, + ), + ( + AccountsPackageType::Snapshot(SnapshotType::FullSnapshot), + AccountsPackageType::Snapshot(SnapshotType::FullSnapshot), + true, + ), + ( + AccountsPackageType::Snapshot(SnapshotType::FullSnapshot), + AccountsPackageType::EpochAccountsHash, + true, + ), + // Tests 14-16, pending package is EAH + ( + AccountsPackageType::EpochAccountsHash, + AccountsPackageType::AccountsHashVerifier, + false, + ), + ( + AccountsPackageType::EpochAccountsHash, + AccountsPackageType::Snapshot(SnapshotType::IncrementalSnapshot(0)), + false, + ), + ( + AccountsPackageType::EpochAccountsHash, + AccountsPackageType::Snapshot(SnapshotType::FullSnapshot), + false, + ), + // tested separately: (AccountsPackageType::EpochAccountsHash, None, AccountsPackageType::EpochAccountsHash, None, X), + ] { + let pending_accounts_package = new_accounts_package(pending_package_type); + let accounts_package = new_accounts_package(new_package_type); + + let actual_result = + can_submit_accounts_package(&accounts_package, Some(&pending_accounts_package)); + assert_eq!(expected_result, actual_result); + } + } + + /// It should not be allowed to have a new accounts package intended for EAH when there is + /// already a pending EAH accounts package. + #[test] + #[should_panic] + fn test_can_submit_accounts_package_both_are_eah() { + let pending_accounts_package = new_accounts_package(AccountsPackageType::EpochAccountsHash); + let accounts_package = new_accounts_package(AccountsPackageType::EpochAccountsHash); + _ = can_submit_accounts_package(&accounts_package, Some(&pending_accounts_package)); + } + + /// helper function to create an AccountsPackage that's good enough for tests + fn new_accounts_package(package_type: AccountsPackageType) -> AccountsPackage { + AccountsPackage { + package_type, + slot: Slot::default(), + block_height: Slot::default(), + slot_deltas: Vec::default(), + snapshot_links: TempDir::new().unwrap(), + snapshot_storages: SnapshotStorages::default(), + archive_format: ArchiveFormat::Tar, + snapshot_version: SnapshotVersion::default(), + full_snapshot_archives_dir: PathBuf::default(), + incremental_snapshot_archives_dir: PathBuf::default(), + expected_capitalization: u64::default(), + accounts_hash_for_testing: None, + cluster_type: solana_sdk::genesis_config::ClusterType::Development, + accounts: Arc::new(crate::accounts::Accounts::default_for_tests()), + epoch_schedule: solana_sdk::epoch_schedule::EpochSchedule::default(), + rent_collector: crate::rent_collector::RentCollector::default(), + } } #[test]