From 98bc69460608b9bcf549e9e5c75687955d0e08dc Mon Sep 17 00:00:00 2001 From: "Jeff Washington (jwash)" <75863576+jeffwashington@users.noreply.github.com> Date: Thu, 26 Aug 2021 14:32:43 -0500 Subject: [PATCH] hash calculation adds really old slots to dirty_stores (#19434) --- accounts-bench/src/main.rs | 1 + runtime/src/accounts.rs | 1 + runtime/src/accounts_background_service.rs | 3 +- runtime/src/accounts_db.rs | 53 +++++++++++++++++----- runtime/src/bank.rs | 4 +- 5 files changed, 49 insertions(+), 13 deletions(-) diff --git a/accounts-bench/src/main.rs b/accounts-bench/src/main.rs index ac4394e3bd..6a0f2d4fd0 100644 --- a/accounts-bench/src/main.rs +++ b/accounts-bench/src/main.rs @@ -122,6 +122,7 @@ fn main() { &ancestors, None, false, + None, ); time_store.stop(); if results != results_store { diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 9d608d7953..8267240ef9 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -670,6 +670,7 @@ impl Accounts { ancestors, None, can_cached_slot_be_unflushed, + None, ) .1 } diff --git a/runtime/src/accounts_background_service.rs b/runtime/src/accounts_background_service.rs index 7c1e61416b..7b592a3e77 100644 --- a/runtime/src/accounts_background_service.rs +++ b/runtime/src/accounts_background_service.rs @@ -108,7 +108,7 @@ impl SnapshotRequestHandler { let previous_hash = if test_hash_calculation { // We have to use the index version here. // We cannot calculate the non-index way because cache has not been flushed and stores don't match reality. - snapshot_root_bank.update_accounts_hash_with_index_option(true, false) + snapshot_root_bank.update_accounts_hash_with_index_option(true, false, None) } else { Hash::default() }; @@ -146,6 +146,7 @@ impl SnapshotRequestHandler { let this_hash = snapshot_root_bank.update_accounts_hash_with_index_option( use_index_hash_calculation, test_hash_calculation, + Some(snapshot_root_bank.epoch_schedule().slots_per_epoch), ); let hash_for_testing = if test_hash_calculation { assert_eq!(previous_hash, this_hash); diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index 700cbbb7ac..9b9218d051 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -4698,11 +4698,11 @@ impl AccountsDb { } pub fn update_accounts_hash(&self, slot: Slot, ancestors: &Ancestors) -> (Hash, u64) { - self.update_accounts_hash_with_index_option(true, false, slot, ancestors, None, false) + self.update_accounts_hash_with_index_option(true, false, slot, ancestors, None, false, None) } pub fn update_accounts_hash_test(&self, slot: Slot, ancestors: &Ancestors) -> (Hash, u64) { - self.update_accounts_hash_with_index_option(true, true, slot, ancestors, None, false) + self.update_accounts_hash_with_index_option(true, true, slot, ancestors, None, false, None) } fn scan_multiple_account_storages_one_slot( @@ -4822,6 +4822,25 @@ impl AccountsDb { .collect() } + // storages are sorted by slot and have range info. + // if we know slots_per_epoch, then 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: Option) { + if let Some(slots_per_epoch) = slots_per_epoch { + let max = storages.range().end; + 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 in storages.range().start..in_epoch_range_start { + if let Some(storages) = storages.get(slot) { + storages.iter().for_each(|store| { + self.dirty_stores + .insert((slot, store.id.load(Ordering::Relaxed)), store.clone()); + }); + } + } + } + } + fn calculate_accounts_hash_helper( &self, use_index: bool, @@ -4829,6 +4848,7 @@ impl AccountsDb { ancestors: &Ancestors, check_hash: bool, can_cached_slot_be_unflushed: bool, + slots_per_epoch: Option, ) -> Result<(Hash, u64), BankHashVerificationError> { if !use_index { let accounts_cache_and_ancestors = if can_cached_slot_be_unflushed { @@ -4848,6 +4868,8 @@ impl AccountsDb { min_root, Some(slot), ); + + self.mark_old_slots_as_dirty(&storages, slots_per_epoch); sort_time.stop(); let timings = HashStats { @@ -4877,6 +4899,7 @@ impl AccountsDb { expected_capitalization: Option, can_cached_slot_be_unflushed: bool, check_hash: bool, + slots_per_epoch: Option, ) -> Result<(Hash, u64), BankHashVerificationError> { let (hash, total_lamports) = self.calculate_accounts_hash_helper( use_index, @@ -4884,6 +4907,7 @@ impl AccountsDb { ancestors, check_hash, can_cached_slot_be_unflushed, + slots_per_epoch, )?; if debug_verify { // calculate the other way (store or non-store) and verify results match. @@ -4893,6 +4917,7 @@ impl AccountsDb { ancestors, check_hash, can_cached_slot_be_unflushed, + None, )?; let success = hash == hash_other @@ -4911,6 +4936,7 @@ impl AccountsDb { ancestors: &Ancestors, expected_capitalization: Option, can_cached_slot_be_unflushed: bool, + slots_per_epoch: Option, ) -> (Hash, u64) { let check_hash = false; let (hash, total_lamports) = self @@ -4922,6 +4948,7 @@ impl AccountsDb { expected_capitalization, can_cached_slot_be_unflushed, check_hash, + slots_per_epoch, ) .unwrap(); // unwrap here will never fail since check_hash = false let mut bank_hashes = self.bank_hashes.write().unwrap(); @@ -5125,6 +5152,7 @@ impl AccountsDb { None, can_cached_slot_be_unflushed, check_hash, + None, )?; if calculated_lamports != total_lamports { @@ -9031,12 +9059,13 @@ pub mod tests { ); db.add_root(some_slot); let check_hash = true; - assert!(db - .calculate_accounts_hash_helper(false, some_slot, &ancestors, check_hash, false) - .is_err()); - assert!(db - .calculate_accounts_hash_helper(true, some_slot, &ancestors, check_hash, false) - .is_err()); + for use_index in [true, false] { + assert!(db + .calculate_accounts_hash_helper( + use_index, some_slot, &ancestors, check_hash, false, None + ) + .is_err()); + } } #[test] @@ -9055,9 +9084,11 @@ pub mod tests { db.add_root(some_slot); let check_hash = true; assert_eq!( - db.calculate_accounts_hash_helper(false, some_slot, &ancestors, check_hash, false) - .unwrap(), - db.calculate_accounts_hash_helper(true, some_slot, &ancestors, check_hash, false) + db.calculate_accounts_hash_helper( + false, some_slot, &ancestors, check_hash, false, None + ) + .unwrap(), + db.calculate_accounts_hash_helper(true, some_slot, &ancestors, check_hash, false, None) .unwrap(), ); } diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index bc1171921f..d21018878e 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -5011,6 +5011,7 @@ impl Bank { &self, use_index: bool, debug_verify: bool, + slots_per_epoch: Option, ) -> Hash { let (hash, total_lamports) = self .rc @@ -5023,6 +5024,7 @@ impl Bank { &self.ancestors, Some(self.capitalization()), false, + slots_per_epoch, ); if total_lamports != self.capitalization() { datapoint_info!( @@ -5043,7 +5045,7 @@ impl Bank { } pub fn update_accounts_hash(&self) -> Hash { - self.update_accounts_hash_with_index_option(true, false) + self.update_accounts_hash_with_index_option(true, false, None) } /// A snapshot bank should be purged of 0 lamport accounts which are not part of the hash