Skip to content

Removes OldStoragesPolicy #6492

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
brooksprumo merged 1 commit into from
Jun 11, 2025
Merged

Removes OldStoragesPolicy #6492

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 2 additions & 52 deletions accounts-db/src/accounts_db.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2438,28 +2438,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_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 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
Expand Down Expand Up @@ -2571,16 +2554,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
Expand Down Expand Up @@ -2689,7 +2663,6 @@ impl AccountsDb {
max_clean_root_inclusive: Option<Slot>,
is_startup: bool,
epoch_schedule: &EpochSchedule,
old_storages_policy: OldStoragesPolicy,
) {
if self.exhaustively_verify_refcounts {
//at startup use all cores to verify refcounts
Expand Down Expand Up @@ -2722,7 +2695,6 @@ impl AccountsDb {
is_startup,
&mut key_timings,
epoch_schedule,
old_storages_policy,
);
measure_construct_candidates.stop();
drop(active_guard);
Expand Down Expand Up @@ -4318,15 +4290,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);
}
};

Expand Down Expand Up @@ -8826,20 +8790,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 {
Expand Down
93 changes: 14 additions & 79 deletions accounts-db/src/accounts_db/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1709,23 +1709,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);

Expand Down Expand Up @@ -3194,12 +3184,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!(
Expand Down Expand Up @@ -4404,12 +4389,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
{
Expand All @@ -4428,12 +4408,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);
Expand Down Expand Up @@ -4478,7 +4453,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.
Expand Down Expand Up @@ -4510,12 +4485,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());
Expand Down Expand Up @@ -4556,12 +4526,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
{
Expand Down Expand Up @@ -4595,12 +4560,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);
Expand Down Expand Up @@ -5414,30 +5374,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);
}
);
Expand Down Expand Up @@ -7169,12 +7114,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_storages(..=slot);
let storages = SortedStorages::new(&storages);
accounts_db.calculate_accounts_hash(
Expand Down Expand Up @@ -7240,12 +7180,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_storages(full_accounts_hash_slot + 1..=slot);
let storages = SortedStorages::new(&storages);
accounts_db.calculate_incremental_accounts_hash(
Expand Down
1 change: 0 additions & 1 deletion runtime/src/accounts_background_service.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -678,7 +678,6 @@ impl AccountsBackgroundService {
Some(max_clean_slot_inclusive),
false,
bank.epoch_schedule(),
bank.clean_accounts_old_storages_policy(),
);
last_cleaned_slot = max_clean_slot_inclusive;
previous_clean_time = Instant::now();
Expand Down
14 changes: 1 addition & 13 deletions runtime/src/bank.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,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,
Expand Down Expand Up @@ -6048,7 +6047,6 @@ impl Bank {
Some(latest_full_snapshot_slot),
true,
self.epoch_schedule(),
self.clean_accounts_old_storages_policy(),
);
info!("Cleaning... Done.");
} else {
Expand Down Expand Up @@ -6395,7 +6393,6 @@ impl Bank {
Some(highest_slot_to_clean),
false,
self.epoch_schedule(),
self.clean_accounts_old_storages_policy(),
);
}

Expand Down Expand Up @@ -6433,15 +6430,6 @@ impl Bank {
can_skip_rewrites || test_skip_rewrites_but_include_in_bank_hash
}

/// Returns how clean_accounts() should handle old storages
pub fn clean_accounts_old_storages_policy(&self) -> OldStoragesPolicy {
if self.are_ancient_storages_enabled() {
Copy link
Copy Markdown

@HaoranYi HaoranYi Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are_ancient_storages_enabled is the next to clean up?

Copy link
Copy Markdown
Author

@brooksprumo brooksprumo Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once rent collection is gone. It's too intertwined otherwise, imo. I've tried 3+ separate times and have given up each time.

Copy link
Copy Markdown

@HaoranYi HaoranYi Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😃 ok.

OldStoragesPolicy::Leave
} else {
OldStoragesPolicy::Clean
}
}

pub fn read_cost_tracker(&self) -> LockResult<RwLockReadGuard<CostTracker>> {
self.cost_tracker.read()
}
Expand Down
Loading