From 510d195620edded54e8a8289f5b1c74fcf8a24f0 Mon Sep 17 00:00:00 2001 From: Brooks Prumo Date: Fri, 19 Aug 2022 18:15:04 -0400 Subject: [PATCH] Simplify `Bank::clean_accounts()` by removing params (#27254) --- runtime/src/accounts_background_service.rs | 8 ++----- runtime/src/bank.rs | 24 ++++++++++++--------- runtime/src/snapshot_utils.rs | 6 +++--- runtime/src/system_instruction_processor.rs | 5 +++-- 4 files changed, 22 insertions(+), 21 deletions(-) diff --git a/runtime/src/accounts_background_service.rs b/runtime/src/accounts_background_service.rs index c38203ab821e96..8d21fed9c7c939 100644 --- a/runtime/src/accounts_background_service.rs +++ b/runtime/src/accounts_background_service.rs @@ -254,11 +254,7 @@ impl SnapshotRequestHandler { }; let mut clean_time = Measure::start("clean_time"); - // Don't clean the slot we're snapshotting because it may have zero-lamport - // accounts that were included in the bank delta hash when the bank was frozen, - // and if we clean them here, the newly created snapshot's hash may not match - // the frozen hash. - snapshot_root_bank.clean_accounts(true, false, *last_full_snapshot_slot); + snapshot_root_bank.clean_accounts(*last_full_snapshot_slot); clean_time.stop(); if accounts_db_caching_enabled { @@ -564,7 +560,7 @@ impl AccountsBackgroundService { // slots >= bank.slot() bank.force_flush_accounts_cache(); } - bank.clean_accounts(true, false, last_full_snapshot_slot); + bank.clean_accounts(last_full_snapshot_slot); last_cleaned_block_height = bank.block_height(); } } diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 7e5aeef16465cd..a6bbb8044b26ef 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -7159,7 +7159,7 @@ impl Bank { let mut clean_time = Measure::start("clean"); if !accounts_db_skip_shrink && self.slot() > 0 { info!("cleaning.."); - self.clean_accounts(true, true, Some(last_full_snapshot_slot)); + self._clean_accounts(true, true, Some(last_full_snapshot_slot)); } clean_time.stop(); @@ -7443,12 +7443,7 @@ impl Bank { debug!("Added precompiled program {:?}", program_id); } - pub fn clean_accounts( - &self, - skip_last: bool, - is_startup: bool, - last_full_snapshot_slot: Option, - ) { + pub(crate) fn clean_accounts(&self, last_full_snapshot_slot: Option) { // Don't clean the slot we're snapshotting because it may have zero-lamport // accounts that were included in the bank delta hash when the bank was frozen, // and if we clean them here, any newly created snapshot's hash for this bank @@ -7456,10 +7451,19 @@ impl Bank { // // So when we're snapshotting, set `skip_last` to true so the highest slot to clean is // lowered by one. - let highest_slot_to_clean = skip_last.then(|| self.slot().saturating_sub(1)); + self._clean_accounts(true, false, last_full_snapshot_slot) + } + + fn _clean_accounts( + &self, + skip_last: bool, + is_startup: bool, + last_full_snapshot_slot: Option, + ) { + let max_clean_root = skip_last.then(|| self.slot().saturating_sub(1)); self.rc.accounts.accounts_db.clean_accounts( - highest_slot_to_clean, + max_clean_root, is_startup, last_full_snapshot_slot, ); @@ -16260,7 +16264,7 @@ pub(crate) mod tests { current_bank.squash(); if current_bank.slot() % 2 == 0 { current_bank.force_flush_accounts_cache(); - current_bank.clean_accounts(true, false, None); + current_bank.clean_accounts(None); } prev_bank = current_bank.clone(); current_bank = Arc::new(Bank::new_from_parent( diff --git a/runtime/src/snapshot_utils.rs b/runtime/src/snapshot_utils.rs index 93cdbc0f33fc0c..4717bb1ab4f356 100644 --- a/runtime/src/snapshot_utils.rs +++ b/runtime/src/snapshot_utils.rs @@ -2078,7 +2078,7 @@ pub fn bank_to_full_snapshot_archive( assert!(bank.is_complete()); bank.squash(); // Bank may not be a root bank.force_flush_accounts_cache(); - bank.clean_accounts(true, false, Some(bank.slot())); + bank.clean_accounts(Some(bank.slot())); bank.update_accounts_hash(); bank.rehash(); // Bank accounts may have been manually modified by the caller @@ -2125,7 +2125,7 @@ pub fn bank_to_incremental_snapshot_archive( assert!(bank.slot() > full_snapshot_slot); bank.squash(); // Bank may not be a root bank.force_flush_accounts_cache(); - bank.clean_accounts(true, false, Some(full_snapshot_slot)); + bank.clean_accounts(Some(full_snapshot_slot)); bank.update_accounts_hash(); bank.rehash(); // Bank accounts may have been manually modified by the caller @@ -3771,7 +3771,7 @@ mod tests { // Ensure account1 has been cleaned/purged from everywhere bank4.squash(); - bank4.clean_accounts(true, false, Some(full_snapshot_slot)); + bank4.clean_accounts(Some(full_snapshot_slot)); assert!( bank4.get_account_modified_slot(&key1.pubkey()).is_none(), "Ensure Account1 has been cleaned and purged from AccountsDb" diff --git a/runtime/src/system_instruction_processor.rs b/runtime/src/system_instruction_processor.rs index 67f1f931147cef..c0b588c25c42c4 100644 --- a/runtime/src/system_instruction_processor.rs +++ b/runtime/src/system_instruction_processor.rs @@ -1626,6 +1626,7 @@ mod tests { .unwrap(); // super fun time; callback chooses to .clean_accounts(None) or not + let bank = Arc::new(Bank::new_from_parent(&bank, &collector, bank.slot() + 1)); callback(&*bank); // create a normal account at the same pubkey as the zero-lamports account @@ -1651,9 +1652,9 @@ mod tests { bank.squash(); bank.force_flush_accounts_cache(); // do clean and assert that it actually did its job + assert_eq!(4, bank.get_snapshot_storages(None).len()); + bank.clean_accounts(None); assert_eq!(3, bank.get_snapshot_storages(None).len()); - bank.clean_accounts(false, false, None); - assert_eq!(2, bank.get_snapshot_storages(None).len()); }); }