diff --git a/accounts-db/src/accounts_db.rs b/accounts-db/src/accounts_db.rs index 80e6120fa689b6..e8d13bd271a1cf 100644 --- a/accounts-db/src/accounts_db.rs +++ b/accounts-db/src/accounts_db.rs @@ -2541,28 +2541,11 @@ impl AccountsDb { is_startup: bool, timings: &mut CleanKeyTimings, epoch_schedule: &EpochSchedule, - old_storages_policy: OldStoragesPolicy, ) -> CleaningCandidates { let oldest_non_ancient_slot = self.get_oldest_non_ancient_slot(epoch_schedule); let mut dirty_store_processing_time = Measure::start("dirty_store_processing"); - let max_root_inclusive = self.accounts_index.max_root_inclusive(); - let max_slot_inclusive = max_clean_root_inclusive.unwrap_or(max_root_inclusive); - - if old_storages_policy == OldStoragesPolicy::Clean { - let slot_one_epoch_old = - max_root_inclusive.saturating_sub(epoch_schedule.slots_per_epoch); - // do nothing special for these 100 old storages that will likely get cleaned up shortly - let acceptable_straggler_slot_count = 100; - let old_slot_cutoff = - slot_one_epoch_old.saturating_sub(acceptable_straggler_slot_count); - let (old_storages, old_slots) = self.get_snapshot_storages(..old_slot_cutoff); - let num_old_storages = old_storages.len(); - for (old_slot, old_storage) in std::iter::zip(old_slots, old_storages) { - self.dirty_stores.entry(old_slot).or_insert(old_storage); - } - info!("Marked {num_old_storages} old storages as dirty"); - } - + let max_slot_inclusive = + max_clean_root_inclusive.unwrap_or_else(|| self.accounts_index.max_root_inclusive()); let mut dirty_stores = Vec::with_capacity(self.dirty_stores.len()); // find the oldest dirty slot // we'll add logging if that append vec cannot be marked dead @@ -2674,16 +2657,7 @@ impl AccountsDb { /// Call clean_accounts() with the common parameters that tests/benches use. pub fn clean_accounts_for_tests(&self) { - self.clean_accounts( - None, - false, - &EpochSchedule::default(), - if self.ancient_append_vec_offset.is_some() { - OldStoragesPolicy::Leave - } else { - OldStoragesPolicy::Clean - }, - ) + self.clean_accounts(None, false, &EpochSchedule::default()) } /// called with cli argument to verify refcounts are correct on all accounts @@ -2790,7 +2764,6 @@ impl AccountsDb { max_clean_root_inclusive: Option, is_startup: bool, epoch_schedule: &EpochSchedule, - old_storages_policy: OldStoragesPolicy, ) { if self.exhaustively_verify_refcounts { self.exhaustively_verify_refcounts(max_clean_root_inclusive); @@ -2812,7 +2785,6 @@ impl AccountsDb { is_startup, &mut key_timings, epoch_schedule, - old_storages_policy, ); let num_candidates = Self::count_pubkeys(&candidates); @@ -4737,15 +4709,7 @@ impl AccountsDb { let maybe_clean = || { if self.dirty_stores.len() > DIRTY_STORES_CLEANING_THRESHOLD { let latest_full_snapshot_slot = self.latest_full_snapshot_slot(); - self.clean_accounts( - latest_full_snapshot_slot, - is_startup, - epoch_schedule, - // Leave any old storages alone for now. Once the validator is running - // normal, calls to clean_accounts() will have the correct policy based - // on if ancient storages are enabled or not. - OldStoragesPolicy::Leave, - ); + self.clean_accounts(latest_full_snapshot_slot, is_startup, epoch_schedule); } }; @@ -6919,6 +6883,40 @@ impl AccountsDb { true } + /// storages are sorted by slot and have range info. + /// add all stores older than slots_per_epoch to dirty_stores so clean visits these slots + fn mark_old_slots_as_dirty( + &self, + storages: &SortedStorages, + slots_per_epoch: Slot, + stats: &mut crate::accounts_hash::HashStats, + ) { + // Nothing to do if ancient append vecs are enabled. + // Ancient slots will be visited by the ancient append vec code and dealt with correctly. + // we expect these ancient append vecs to be old and keeping accounts + // We can expect the normal processes will keep them cleaned. + // If we included them here then ALL accounts in ALL ancient append vecs will be visited by clean each time. + if self.ancient_append_vec_offset.is_some() { + return; + } + + let mut mark_time = Measure::start("mark_time"); + let mut num_dirty_slots: usize = 0; + let max = storages.max_slot_inclusive(); + let acceptable_straggler_slot_count = 100; // do nothing special for these old stores which will likely get cleaned up shortly + let sub = slots_per_epoch + acceptable_straggler_slot_count; + let in_epoch_range_start = max.saturating_sub(sub); + for (slot, storage) in storages.iter_range(&(..in_epoch_range_start)) { + if let Some(storage) = storage { + self.dirty_stores.insert(slot, storage.clone()); + num_dirty_slots += 1; + } + } + mark_time.stop(); + stats.mark_time_us = mark_time.as_us(); + stats.num_dirty_slots = num_dirty_slots; + } + pub fn calculate_accounts_hash_from( &self, data_source: CalcAccountsHashDataSource, @@ -7259,6 +7257,8 @@ impl AccountsDb { let storages_start_slot = storages.range().start; stats.oldest_root = storages_start_slot; + self.mark_old_slots_as_dirty(storages, config.epoch_schedule.slots_per_epoch, &mut stats); + let slot = storages.max_slot_inclusive(); let use_bg_thread_pool = config.use_bg_thread_pool; let accounts_hash_cache_path = self.accounts_hash_cache_path.clone(); @@ -9354,20 +9354,6 @@ pub(crate) enum UpdateIndexThreadSelection { PoolWithThreshold, } -/// How should old storages be handled in clean_accounts()? -#[derive(Debug, Copy, Clone, Eq, PartialEq)] -pub enum OldStoragesPolicy { - /// Clean all old storages, even if they were not explictly marked as dirty. - /// - /// This is the default behavior when not skipping rewrites. - Clean, - /// Leave all old storages. - /// - /// When skipping rewrites, we intentionally will have ancient storages. - /// Do not clean them up automatically in clean_accounts(). - Leave, -} - // These functions/fields are only usable from a dev context (i.e. tests and benches) #[cfg(feature = "dev-context-only-utils")] impl AccountStorageEntry { diff --git a/accounts-db/src/accounts_db/tests.rs b/accounts-db/src/accounts_db/tests.rs index 1cf6a5a3d02bb2..9b9ae6c66a9266 100644 --- a/accounts-db/src/accounts_db/tests.rs +++ b/accounts-db/src/accounts_db/tests.rs @@ -1953,23 +1953,13 @@ fn test_clean_max_slot_zero_lamport_account() { // updates in later slots in slot 1 assert_eq!(accounts.alive_account_count_in_slot(0), 1); assert_eq!(accounts.alive_account_count_in_slot(1), 1); - accounts.clean_accounts( - Some(0), - false, - &EpochSchedule::default(), - OldStoragesPolicy::Leave, - ); + accounts.clean_accounts(Some(0), false, &EpochSchedule::default()); assert_eq!(accounts.alive_account_count_in_slot(0), 1); assert_eq!(accounts.alive_account_count_in_slot(1), 1); assert!(accounts.accounts_index.contains_with(&pubkey, None, None)); // Now the account can be cleaned up - accounts.clean_accounts( - Some(1), - false, - &EpochSchedule::default(), - OldStoragesPolicy::Leave, - ); + accounts.clean_accounts(Some(1), false, &EpochSchedule::default()); assert_eq!(accounts.alive_account_count_in_slot(0), 0); assert_eq!(accounts.alive_account_count_in_slot(1), 0); @@ -3498,12 +3488,7 @@ fn test_zero_lamport_new_root_not_cleaned() { db.add_root_and_flush_write_cache(1); // Only clean zero lamport accounts up to slot 0 - db.clean_accounts( - Some(0), - false, - &EpochSchedule::default(), - OldStoragesPolicy::Leave, - ); + db.clean_accounts(Some(0), false, &EpochSchedule::default()); // Should still be able to find zero lamport account in slot 1 assert_eq!( @@ -4716,12 +4701,7 @@ fn run_test_shrink_unref(do_intra_cache_clean: bool) { db.calculate_accounts_delta_hash(1); // Clean to remove outdated entry from slot 0 - db.clean_accounts( - Some(1), - false, - &EpochSchedule::default(), - OldStoragesPolicy::Leave, - ); + db.clean_accounts(Some(1), false, &EpochSchedule::default()); // Shrink Slot 0 { @@ -4740,12 +4720,7 @@ fn run_test_shrink_unref(do_intra_cache_clean: bool) { // Should be one store before clean for slot 0 db.get_and_assert_single_storage(0); db.calculate_accounts_delta_hash(2); - db.clean_accounts( - Some(2), - false, - &EpochSchedule::default(), - OldStoragesPolicy::Leave, - ); + db.clean_accounts(Some(2), false, &EpochSchedule::default()); // No stores should exist for slot 0 after clean assert_no_storages_at_slot(&db, 0); @@ -4790,7 +4765,7 @@ fn test_clean_drop_dead_zero_lamport_single_ref_accounts() { accounts_db.calculate_accounts_delta_hash(1); // run clean - accounts_db.clean_accounts(Some(1), false, &epoch_schedule, OldStoragesPolicy::Leave); + accounts_db.clean_accounts(Some(1), false, &epoch_schedule); // After clean, both slot0 and slot1 should be marked dead and dropped // from the store map. @@ -4822,12 +4797,7 @@ fn test_clean_drop_dead_storage_handle_zero_lamport_single_ref_accounts() { // Clean should mark slot 0 dead and drop it. During the dropping, it // will find that slot 1 has a single ref zero accounts and mark it. - db.clean_accounts( - Some(1), - false, - &EpochSchedule::default(), - OldStoragesPolicy::Leave, - ); + db.clean_accounts(Some(1), false, &EpochSchedule::default()); // Assert that after clean, slot 0 is dropped. assert!(db.storage.get_slot_storage_entry(0).is_none()); @@ -4868,12 +4838,7 @@ fn test_shrink_unref_handle_zero_lamport_single_ref_accounts() { db.calculate_accounts_delta_hash(1); // Clean to remove outdated entry from slot 0 - db.clean_accounts( - Some(1), - false, - &EpochSchedule::default(), - OldStoragesPolicy::Leave, - ); + db.clean_accounts(Some(1), false, &EpochSchedule::default()); // Shrink Slot 0 { @@ -4907,12 +4872,7 @@ fn test_shrink_unref_handle_zero_lamport_single_ref_accounts() { db.get_and_assert_single_storage(0); db.get_and_assert_single_storage(1); db.calculate_accounts_delta_hash(2); - db.clean_accounts( - Some(2), - false, - &EpochSchedule::default(), - OldStoragesPolicy::Leave, - ); + db.clean_accounts(Some(2), false, &EpochSchedule::default()); // No stores should exist for slot 0 after clean assert_no_storages_at_slot(&db, 0); @@ -5730,30 +5690,15 @@ define_accounts_db_test!( assert_eq!(accounts_db.ref_count_for_pubkey(&pubkey), 3); accounts_db.set_latest_full_snapshot_slot(slot2); - accounts_db.clean_accounts( - Some(slot2), - false, - &EpochSchedule::default(), - OldStoragesPolicy::Leave, - ); + accounts_db.clean_accounts(Some(slot2), false, &EpochSchedule::default()); assert_eq!(accounts_db.ref_count_for_pubkey(&pubkey), 2); accounts_db.set_latest_full_snapshot_slot(slot2); - accounts_db.clean_accounts( - None, - false, - &EpochSchedule::default(), - OldStoragesPolicy::Leave, - ); + accounts_db.clean_accounts(None, false, &EpochSchedule::default()); assert_eq!(accounts_db.ref_count_for_pubkey(&pubkey), 1); accounts_db.set_latest_full_snapshot_slot(slot3); - accounts_db.clean_accounts( - None, - false, - &EpochSchedule::default(), - OldStoragesPolicy::Leave, - ); + accounts_db.clean_accounts(None, false, &EpochSchedule::default()); assert_eq!(accounts_db.ref_count_for_pubkey(&pubkey), 0); } ); @@ -8062,12 +8007,7 @@ define_accounts_db_test!(test_calculate_incremental_accounts_hash, |accounts_db| // calculate the full accounts hash let full_accounts_hash = { - accounts_db.clean_accounts( - Some(slot - 1), - false, - &EpochSchedule::default(), - OldStoragesPolicy::Leave, - ); + accounts_db.clean_accounts(Some(slot - 1), false, &EpochSchedule::default()); let (storages, _) = accounts_db.get_snapshot_storages(..=slot); let storages = SortedStorages::new(&storages); accounts_db.calculate_accounts_hash( @@ -8133,12 +8073,7 @@ define_accounts_db_test!(test_calculate_incremental_accounts_hash, |accounts_db| // calculate the incremental accounts hash let incremental_accounts_hash = { accounts_db.set_latest_full_snapshot_slot(full_accounts_hash_slot); - accounts_db.clean_accounts( - Some(slot - 1), - false, - &EpochSchedule::default(), - OldStoragesPolicy::Leave, - ); + accounts_db.clean_accounts(Some(slot - 1), false, &EpochSchedule::default()); let (storages, _) = accounts_db.get_snapshot_storages(full_accounts_hash_slot + 1..=slot); let storages = SortedStorages::new(&storages); accounts_db.calculate_incremental_accounts_hash( diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index ba80fbd5d26210..08de68eed6934f 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -75,8 +75,7 @@ use { accounts::{AccountAddressFilter, Accounts, PubkeyAccountSlot}, accounts_db::{ AccountStorageEntry, AccountsDb, AccountsDbConfig, CalcAccountsHashDataSource, - DuplicatesLtHash, OldStoragesPolicy, PubkeyHashAccount, - VerifyAccountsHashAndLamportsConfig, + DuplicatesLtHash, PubkeyHashAccount, VerifyAccountsHashAndLamportsConfig, }, accounts_hash::{ AccountHash, AccountsHash, AccountsLtHash, CalcAccountsHashConfig, HashStats, @@ -6280,7 +6279,7 @@ impl Bank { let should_clean = force_clean || (!skip_shrink && self.slot() > 0); if should_clean { info!("Cleaning..."); - // We cannot clean past the latest full snapshot's slot because we are about to + // We cannot clean past the last full snapshot's slot because we are about to // perform an accounts hash calculation *up to that slot*. If we cleaned *past* // that slot, then accounts could be removed from older storages, which would // change the accounts hash. @@ -6288,7 +6287,6 @@ impl Bank { Some(latest_full_snapshot_slot), true, self.epoch_schedule(), - self.clean_accounts_old_storages_policy(), ); info!("Cleaning... Done."); } else { @@ -6596,7 +6594,6 @@ impl Bank { Some(highest_slot_to_clean), false, self.epoch_schedule(), - self.clean_accounts_old_storages_policy(), ); } @@ -6612,10 +6609,16 @@ impl Bank { } pub(crate) fn shrink_ancient_slots(&self) { + let can_skip_rewrites = self.bank_hash_skips_rent_rewrites(); + let test_skip_rewrites_but_include_in_bank_hash = self + .rc + .accounts + .accounts_db + .test_skip_rewrites_but_include_in_bank_hash; // Invoke ancient slot shrinking only when the validator is // explicitly configured to do so. This condition may be // removed when the skip rewrites feature is enabled. - if self.are_ancient_storages_enabled() { + if can_skip_rewrites || test_skip_rewrites_but_include_in_bank_hash { self.rc .accounts .accounts_db @@ -6623,26 +6626,6 @@ impl Bank { } } - /// Returns if ancient storages are enabled or not - pub fn are_ancient_storages_enabled(&self) -> bool { - let can_skip_rewrites = self.bank_hash_skips_rent_rewrites(); - let test_skip_rewrites_but_include_in_bank_hash = self - .rc - .accounts - .accounts_db - .test_skip_rewrites_but_include_in_bank_hash; - can_skip_rewrites || test_skip_rewrites_but_include_in_bank_hash - } - - /// Returns how clean_accounts() should handle old storages - fn clean_accounts_old_storages_policy(&self) -> OldStoragesPolicy { - if self.are_ancient_storages_enabled() { - OldStoragesPolicy::Leave - } else { - OldStoragesPolicy::Clean - } - } - pub fn read_cost_tracker(&self) -> LockResult> { self.cost_tracker.read() }