From 3249e4a123f06436ca52a7380e352dccc89517bf Mon Sep 17 00:00:00 2001 From: Brooks Date: Thu, 2 Feb 2023 16:50:44 -0500 Subject: [PATCH] Removes {get,set}_bank_hash_info_from_snapshot() (#30087) --- runtime/src/accounts.rs | 6 +- runtime/src/accounts_db.rs | 208 ++++++++++++++++------------ runtime/src/serde_snapshot.rs | 21 ++- runtime/src/serde_snapshot/newer.rs | 24 +++- runtime/src/serde_snapshot/tests.rs | 39 +++++- 5 files changed, 200 insertions(+), 98 deletions(-) diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index ecea8ad477..79d0814983 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -2379,15 +2379,15 @@ mod tests { } #[test] - fn test_accounts_empty_bank_hash() { + fn test_accounts_empty_bank_hash_stats() { let accounts = Accounts::new_with_config_for_tests( Vec::new(), &ClusterType::Development, AccountSecondaryIndexes::default(), AccountShrinkThreshold::default(), ); - assert!(accounts.accounts_db.get_bank_hash_info(0).is_some()); - assert!(accounts.accounts_db.get_bank_hash_info(1).is_none()); + assert!(accounts.accounts_db.get_bank_hash_stats(0).is_some()); + assert!(accounts.accounts_db.get_bank_hash_stats(1).is_none()); } #[test] diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index b899efe15b..0d91017df4 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -7244,6 +7244,14 @@ impl AccountsDb { .insert(slot, accounts_hash) } + pub fn set_accounts_hash_from_snapshot( + &self, + slot: Slot, + accounts_hash: AccountsHash, + ) -> Option { + self.set_accounts_hash(slot, accounts_hash) + } + /// Get the accounts hash for `slot` in the `accounts_hashes` map pub fn get_accounts_hash(&self, slot: Slot) -> Option { self.accounts_hashes.lock().unwrap().get(&slot).cloned() @@ -7647,6 +7655,14 @@ impl AccountsDb { .insert(slot, accounts_delta_hash) } + pub fn set_accounts_delta_hash_from_snapshot( + &self, + slot: Slot, + accounts_delta_hash: AccountsDeltaHash, + ) -> Option { + self.set_accounts_delta_hash(slot, accounts_delta_hash) + } + /// Get the accounts delta hash for `slot` in the `accounts_delta_hashes` map pub fn get_accounts_delta_hash(&self, slot: Slot) -> Option { self.accounts_delta_hashes @@ -7656,18 +7672,23 @@ impl AccountsDb { .cloned() } - /// Set the bank hash stats for `slot` in the `bank_hash_stats` map + /// When reconstructing AccountsDb from a snapshot, insert the `bank_hash_stats` into the + /// internal bank hash stats map. /// - /// returns the previous bank hash stats for `slot` - fn set_bank_hash_stats( + /// This fn is only called when loading from a snapshot, which means AccountsDb is new and its + /// bank hash stats map is unpopulated. Except for slot 0. + /// + /// Slot 0 is a special case. When a new AccountsDb is created--like when loading from a + /// snapshot--the bank hash stats map is populated with a default entry at slot 0. Remove the + /// default entry at slot 0, and then insert the new value at `slot`. + pub fn update_bank_hash_stats_from_snapshot( &self, slot: Slot, - bank_hash_stats: BankHashStats, + stats: BankHashStats, ) -> Option { - self.bank_hash_stats - .lock() - .unwrap() - .insert(slot, bank_hash_stats) + let mut bank_hash_stats = self.bank_hash_stats.lock().unwrap(); + bank_hash_stats.remove(&0); + bank_hash_stats.insert(slot, stats) } /// Get the bank hash stats for `slot` in the `bank_hash_stats` map @@ -7675,60 +7696,6 @@ impl AccountsDb { self.bank_hash_stats.lock().unwrap().get(&slot).cloned() } - /// Set the "bank hash info" for `slot` - /// - /// Internally this sets the accounts delta hash, the accounts hash, and the bank hash stats - /// from `bank_hash_info` for `slot` in their respective maps. - /// - /// returns the previous accounts delta hash, accounts hash, and bank hash stats for `slot` - fn set_bank_hash_info( - &self, - slot: Slot, - bank_hash_info: BankHashInfo, - ) -> ( - Option, - Option, - Option, - ) { - let BankHashInfo { - accounts_delta_hash, - accounts_hash, - stats, - } = bank_hash_info; - let old_accounts_delta_hash = self.set_accounts_delta_hash(slot, accounts_delta_hash); - let old_accounts_hash = self.set_accounts_hash(slot, accounts_hash); - let old_stats = self.set_bank_hash_stats(slot, stats); - (old_accounts_delta_hash, old_accounts_hash, old_stats) - } - - /// When reconstructing AccountsDb from a snapshot, insert the `bank_hash_info` into the - /// internal bank hash info maps. - /// - /// This fn is only called when loading from a snapshot, which means AccountsDb is new and its - /// bank hash info maps are unpopulated. Therefore, a bank hash info must not already exist at - /// `slot` [^1]. - /// - /// [^1] Slot 0 is a special case, however. When a new AccountsDb is created--like when - /// loading from a snapshot--the bank hash stats map is populated with a default entry at slot 0. - /// It is valid to have a snapshot at slot 0, so it must be handled accordingly. - pub fn set_bank_hash_info_from_snapshot(&self, slot: Slot, bank_hash_info: BankHashInfo) { - let (old_accounts_delta_hash, old_accounts_hash, old_stats) = - self.set_bank_hash_info(slot, bank_hash_info); - - assert!( - old_accounts_delta_hash.is_none(), - "There should not already be an AccountsDeltaHash at slot {slot}: {old_accounts_delta_hash:?}", - ); - assert!( - old_accounts_hash.is_none(), - "There should not already be an AccountsHash at slot {slot}: {old_accounts_hash:?}", - ); - assert!( - old_stats.is_none() || (slot == 0 && old_stats == Some(BankHashStats::default())), - "There should not already be a BankHashStats at slot {slot}: {old_stats:?}", - ); - } - /// Remove "bank hash info" for `slot` /// /// This fn removes the accounts delta hash, accounts hash, and bank hash stats for `slot` from @@ -7753,33 +7720,6 @@ impl AccountsDb { } } - /// Get the "bank hash info" for `slot` - /// - /// Internally this gets the accounts delta hash, the accounts hash, and the bank hash stats - /// for `slot` from their respective maps. - /// - /// Only called by tests or serde_snapshot when serializing accounts db fields - pub fn get_bank_hash_info(&self, slot: Slot) -> Option { - let Some(stats) = self.get_bank_hash_stats(slot) else { - return None; - }; - - // If there is a bank hash stats at this slot, then we'll return a `Some` regardless. Use - // default values for accounts hash and accounts delta hash if not found. - let accounts_hash = self - .get_accounts_hash(slot) - .unwrap_or_else(|| AccountsHash(Hash::default())); - let accounts_delta_hash = self - .get_accounts_delta_hash(slot) - .unwrap_or_else(|| AccountsDeltaHash(Hash::default())); - - Some(BankHashInfo { - accounts_hash, - accounts_delta_hash, - stats, - }) - } - fn update_index<'a, T: ReadableAccount + Sync>( &self, infos: Vec, @@ -9507,6 +9447,71 @@ pub mod tests { pub fn set_accounts_hash_for_tests(&self, slot: Slot, accounts_hash: AccountsHash) { self.set_accounts_hash(slot, accounts_hash); } + + /// Set the bank hash stats for `slot` in the `bank_hash_stats` map + /// + /// returns the previous bank hash stats for `slot` + fn set_bank_hash_stats( + &self, + slot: Slot, + bank_hash_stats: BankHashStats, + ) -> Option { + self.bank_hash_stats + .lock() + .unwrap() + .insert(slot, bank_hash_stats) + } + + /// Set the "bank hash info" for `slot` + /// + /// Internally this sets the accounts delta hash, the accounts hash, and the bank hash stats + /// from `bank_hash_info` for `slot` in their respective maps. + /// + /// returns the previous accounts delta hash, accounts hash, and bank hash stats for `slot` + fn set_bank_hash_info( + &self, + slot: Slot, + bank_hash_info: BankHashInfo, + ) -> ( + Option, + Option, + Option, + ) { + let BankHashInfo { + accounts_delta_hash, + accounts_hash, + stats, + } = bank_hash_info; + let old_accounts_delta_hash = self.set_accounts_delta_hash(slot, accounts_delta_hash); + let old_accounts_hash = self.set_accounts_hash(slot, accounts_hash); + let old_stats = self.set_bank_hash_stats(slot, stats); + (old_accounts_delta_hash, old_accounts_hash, old_stats) + } + + /// Get the "bank hash info" for `slot` + /// + /// Internally this gets the accounts delta hash, the accounts hash, and the bank hash stats + /// for `slot` from their respective maps. + fn get_bank_hash_info(&self, slot: Slot) -> Option { + let Some(stats) = self.get_bank_hash_stats(slot) else { + return None; + }; + + // If there is a bank hash stats at this slot, then we'll return a `Some` regardless. Use + // default values for accounts hash and accounts delta hash if not found. + let accounts_hash = self + .get_accounts_hash(slot) + .unwrap_or_else(|| AccountsHash(Hash::default())); + let accounts_delta_hash = self + .get_accounts_delta_hash(slot) + .unwrap_or_else(|| AccountsDeltaHash(Hash::default())); + + Some(BankHashInfo { + accounts_hash, + accounts_delta_hash, + stats, + }) + } } /// This impl exists until this feature is activated: @@ -10886,6 +10891,9 @@ pub mod tests { db.store_for_tests(new_root, &[(&key2, &account0)]); db.add_root_and_flush_write_cache(new_root); + db.calculate_accounts_delta_hash(new_root); + db.update_accounts_hash_for_tests(new_root, &linear_ancestors(new_root), false, false); + // Simulate reconstruction from snapshot let db = reconstruct_accounts_db_via_serialization(&db, new_root); @@ -12006,6 +12014,14 @@ pub mod tests { accounts.clean_accounts_for_tests(); accounts.print_accounts_stats("accounts_post_purge"); + + accounts.calculate_accounts_delta_hash(current_slot); + accounts.update_accounts_hash_for_tests( + current_slot, + &linear_ancestors(current_slot), + false, + false, + ); let accounts = reconstruct_accounts_db_via_serialization(&accounts, current_slot); accounts.print_accounts_stats("reconstructed"); @@ -12056,6 +12072,7 @@ pub mod tests { accounts.add_root_and_flush_write_cache(current_slot); accounts.print_accounts_stats("pre_f"); + accounts.calculate_accounts_delta_hash(current_slot); accounts.update_accounts_hash_for_tests(4, &Ancestors::default(), false, false); let accounts = f(accounts, current_slot); @@ -12854,6 +12871,13 @@ pub mod tests { accounts.add_root_and_flush_write_cache(current_slot); accounts.print_count_and_status("before reconstruct"); + accounts.calculate_accounts_delta_hash(current_slot); + accounts.update_accounts_hash_for_tests( + current_slot, + &linear_ancestors(current_slot), + false, + false, + ); let accounts = reconstruct_accounts_db_via_serialization(&accounts, current_slot); accounts.print_count_and_status("before purge zero"); accounts.clean_accounts_for_tests(); @@ -13087,6 +13111,12 @@ pub mod tests { // So, prevent that from happening by introducing refcount ((current_slot - 1)..=current_slot).for_each(|slot| accounts.flush_root_write_cache(slot)); accounts.clean_accounts_for_tests(); + accounts.update_accounts_hash_for_tests( + current_slot, + &linear_ancestors(current_slot), + false, + false, + ); let accounts = reconstruct_accounts_db_via_serialization(&accounts, current_slot); accounts.clean_accounts_for_tests(); diff --git a/runtime/src/serde_snapshot.rs b/runtime/src/serde_snapshot.rs index ef5cf7c556..f498b190b3 100644 --- a/runtime/src/serde_snapshot.rs +++ b/runtime/src/serde_snapshot.rs @@ -718,7 +718,26 @@ where ); // Process deserialized data, set necessary fields in self - accounts_db.set_bank_hash_info_from_snapshot(snapshot_slot, snapshot_bank_hash_info); + let old_accounts_delta_hash = accounts_db.set_accounts_delta_hash_from_snapshot( + snapshot_slot, + snapshot_bank_hash_info.accounts_delta_hash, + ); + assert!( + old_accounts_delta_hash.is_none(), + "There should not already be an AccountsDeltaHash at slot {snapshot_slot}: {old_accounts_delta_hash:?}", + ); + let old_accounts_hash = accounts_db + .set_accounts_hash_from_snapshot(snapshot_slot, snapshot_bank_hash_info.accounts_hash); + assert!( + old_accounts_hash.is_none(), + "There should not already be an AccountsHash at slot {snapshot_slot}: {old_accounts_hash:?}", + ); + let old_stats = accounts_db + .update_bank_hash_stats_from_snapshot(snapshot_slot, snapshot_bank_hash_info.stats); + assert!( + old_stats.is_none(), + "There should not already be a BankHashStats at slot {snapshot_slot}: {old_stats:?}", + ); accounts_db.storage.initialize(storage); accounts_db .next_id diff --git a/runtime/src/serde_snapshot/newer.rs b/runtime/src/serde_snapshot/newer.rs index aa673d8882..5e45c3e423 100644 --- a/runtime/src/serde_snapshot/newer.rs +++ b/runtime/src/serde_snapshot/newer.rs @@ -5,6 +5,7 @@ use { *, }, crate::{ + accounts_db::BankHashInfo, accounts_hash::AccountsHash, ancestors::AncestorsForSerialization, stakes::{serde_stakes_enum_compat, StakesEnum}, @@ -270,10 +271,27 @@ impl<'a> TypeContext<'a> for Context { ) })); let slot = serializable_db.slot; - let bank_hash_info = serializable_db + let accounts_delta_hash = serializable_db .accounts_db - .get_bank_hash_info(serializable_db.slot) - .unwrap_or_else(|| panic!("No bank_hashes entry for slot {}", serializable_db.slot)); + .get_accounts_delta_hash(slot) + .unwrap_or_else(|| panic!("Missing accounts delta hash entry for slot {slot}")); + // NOTE: The accounts hash is calculated in AHV, which is *after* a bank snapshot is taken + // (and serialized here). Thus it is expected that an accounts hash is *not* found for + // this slot, and a placeholder value will be used instead. The real accounts hash will be + // set by `reserialize_bank_with_new_accounts_hash` from AHV. + let accounts_hash = serializable_db + .accounts_db + .get_accounts_hash(slot) + .unwrap_or_default(); + let stats = serializable_db + .accounts_db + .get_bank_hash_stats(slot) + .unwrap_or_else(|| panic!("Missing bank hash stats entry for slot {slot}")); + let bank_hash_info = BankHashInfo { + accounts_delta_hash, + accounts_hash, + stats, + }; let historical_roots = serializable_db .accounts_db diff --git a/runtime/src/serde_snapshot/tests.rs b/runtime/src/serde_snapshot/tests.rs index fd09b7a180..d1920c069e 100644 --- a/runtime/src/serde_snapshot/tests.rs +++ b/runtime/src/serde_snapshot/tests.rs @@ -8,7 +8,7 @@ use { accounts_db::{ get_temp_accounts_paths, test_utils::create_test_accounts, AccountShrinkThreshold, }, - accounts_hash::AccountsHash, + accounts_hash::{AccountsDeltaHash, AccountsHash}, append_vec::AppendVec, bank::{Bank, BankTestConfig}, epoch_accounts_hash, @@ -25,6 +25,7 @@ use { clock::Slot, feature_set::{self, disable_fee_calculator}, genesis_config::{create_genesis_config, ClusterType}, + hash::Hash, pubkey::Pubkey, signature::{Keypair, Signer}, }, @@ -181,6 +182,11 @@ fn test_accounts_serialize_style(serde_style: SerdeStyle) { create_test_accounts(&accounts, &mut pubkeys, 100, slot); check_accounts(&accounts, &pubkeys, 100); accounts.add_root(slot); + let accounts_delta_hash = accounts.accounts_db.calculate_accounts_delta_hash(slot); + let accounts_hash = AccountsHash(Hash::new_unique()); + accounts + .accounts_db + .set_accounts_hash_from_snapshot(slot, accounts_hash); let mut writer = Cursor::new(vec![]); accountsdb_to_stream( @@ -211,9 +217,10 @@ fn test_accounts_serialize_style(serde_style: SerdeStyle) { .unwrap(), ); check_accounts(&daccounts, &pubkeys, 100); - let accounts_delta_hash = accounts.accounts_db.calculate_accounts_delta_hash(slot); let daccounts_delta_hash = daccounts.accounts_db.calculate_accounts_delta_hash(slot); assert_eq!(accounts_delta_hash, daccounts_delta_hash); + let daccounts_hash = daccounts.accounts_db.get_accounts_hash(slot); + assert_eq!(Some(accounts_hash), daccounts_hash); } fn test_bank_serialize_style( @@ -510,6 +517,15 @@ fn test_extra_fields_eof() { let mut bank = Bank::new_from_parent(&bank0, &Pubkey::default(), 1); add_root_and_flush_write_cache(&bank0); + bank.rc + .accounts + .accounts_db + .set_accounts_delta_hash_from_snapshot(bank.slot(), AccountsDeltaHash(Hash::new_unique())); + bank.rc + .accounts + .accounts_db + .set_accounts_hash_from_snapshot(bank.slot(), AccountsHash(Hash::new_unique())); + // Set extra fields bank.fee_rate_governor.lamports_per_signature = 7000; @@ -637,6 +653,14 @@ fn test_blank_extra_fields() { bank0.squash(); let mut bank = Bank::new_from_parent(&bank0, &Pubkey::default(), 1); add_root_and_flush_write_cache(&bank0); + bank.rc + .accounts + .accounts_db + .set_accounts_delta_hash_from_snapshot(bank.slot(), AccountsDeltaHash(Hash::new_unique())); + bank.rc + .accounts + .accounts_db + .set_accounts_hash_from_snapshot(bank.slot(), AccountsHash(Hash::new_unique())); // Set extra fields bank.fee_rate_governor.lamports_per_signature = 7000; @@ -705,6 +729,17 @@ mod test_bank_serialize { where S: serde::Serializer, { + bank.rc + .accounts + .accounts_db + .set_accounts_delta_hash_from_snapshot( + bank.slot(), + AccountsDeltaHash(Hash::new_unique()), + ); + bank.rc + .accounts + .accounts_db + .set_accounts_hash_from_snapshot(bank.slot(), AccountsHash(Hash::new_unique())); let snapshot_storages = bank .rc .accounts