From 683ca1b8f6922d4126f801d4ff3641885e938801 Mon Sep 17 00:00:00 2001 From: Brooks Date: Mon, 11 Nov 2024 15:11:30 -0500 Subject: [PATCH] Populates cache for accounts lt hash in Bank::new_from_parent() (#3581) --- accounts-db/src/accounts_db.rs | 15 +++++ runtime/src/bank.rs | 38 ++++++++++- runtime/src/bank/accounts_lt_hash.rs | 94 +++++++++++++++++++++++----- runtime/src/bank/metrics.rs | 12 ++++ 4 files changed, 140 insertions(+), 19 deletions(-) diff --git a/accounts-db/src/accounts_db.rs b/accounts-db/src/accounts_db.rs index 35eb2efdb97416..8eecff5cb872fe 100644 --- a/accounts-db/src/accounts_db.rs +++ b/accounts-db/src/accounts_db.rs @@ -7428,6 +7428,21 @@ impl AccountsDb { Ok(()) } + /// Returns all of the accounts' pubkeys for a given slot + pub fn get_pubkeys_for_slot(&self, slot: Slot) -> Vec { + let scan_result = self.scan_account_storage( + slot, + |loaded_account| Some(*loaded_account.pubkey()), + |accum: &DashSet<_>, loaded_account, _data| { + accum.insert(*loaded_account.pubkey()); + }, + ScanAccountStorageData::NoData, + ); + match scan_result { + ScanStorageResult::Cached(cached_result) => cached_result, + ScanStorageResult::Stored(stored_result) => stored_result.into_iter().collect(), + } + } /// helper to return /// 1. pubkey, hash pairs for the slot /// 2. us spent scanning diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 864dd759c73681..1242d125b475ae 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -59,7 +59,7 @@ use { transaction_batch::{OwnedOrBorrowed, TransactionBatch}, verify_precompiles::verify_precompiles, }, - accounts_lt_hash::InitialStateOfAccount, + accounts_lt_hash::CacheValue as AccountsLtHashCacheValue, ahash::AHashMap, byteorder::{ByteOrder, LittleEndian}, dashmap::{DashMap, DashSet}, @@ -933,7 +933,10 @@ pub struct Bank { /// /// The accounts lt hash needs both the initial and final state of each /// account that was modified in this slot. Cache the initial state here. - cache_for_accounts_lt_hash: RwLock>, + /// + /// Note: The initial state must be strictly from an ancestor, + /// and not an intermediate state within this slot. + cache_for_accounts_lt_hash: RwLock>, /// The unique identifier for the corresponding block for this bank. /// None for banks that have not yet completed replay or for leader banks as we cannot populate block_id @@ -1381,12 +1384,40 @@ impl Bank { let (_, fill_sysvar_cache_time_us) = measure_us!(new .transaction_processor .fill_missing_sysvar_cache_entries(&new)); - time.stop(); + let (num_accounts_modified_this_slot, populate_cache_for_accounts_lt_hash_us) = new + .is_accounts_lt_hash_enabled() + .then(|| { + measure_us!({ + // The cache for accounts lt hash needs to be made aware of accounts modified + // before transaction processing begins. Otherwise we may calculate the wrong + // accounts lt hash due to having the wrong initial state of the account. The + // lt hash cache's initial state must always be from an ancestor, and cannot be + // an intermediate state within this Bank's slot. If the lt hash cache has the + // wrong initial account state, we'll mix out the wrong lt hash value, and thus + // have the wrong overall accounts lt hash, and diverge. + let accounts_modified_this_slot = + new.rc.accounts.accounts_db.get_pubkeys_for_slot(slot); + let num_accounts_modified_this_slot = accounts_modified_this_slot.len(); + let cache_for_accounts_lt_hash = + new.cache_for_accounts_lt_hash.get_mut().unwrap(); + cache_for_accounts_lt_hash.reserve(num_accounts_modified_this_slot); + for pubkey in accounts_modified_this_slot { + cache_for_accounts_lt_hash + .entry(pubkey) + .or_insert(AccountsLtHashCacheValue::BankNew); + } + num_accounts_modified_this_slot + }) + }) + .unzip(); + + time.stop(); report_new_bank_metrics( slot, parent.slot(), new.block_height, + num_accounts_modified_this_slot, NewBankTimings { bank_rc_creation_time_us, total_elapsed_time_us: time.as_us(), @@ -1406,6 +1437,7 @@ impl Bank { cache_preparation_time_us, update_sysvars_time_us, fill_sysvar_cache_time_us, + populate_cache_for_accounts_lt_hash_us, }, ); diff --git a/runtime/src/bank/accounts_lt_hash.rs b/runtime/src/bank/accounts_lt_hash.rs index 814106533e972b..dc97b12690fbed 100644 --- a/runtime/src/bank/accounts_lt_hash.rs +++ b/runtime/src/bank/accounts_lt_hash.rs @@ -122,14 +122,17 @@ impl Bank { // load the initial state of the account let (initial_state_of_account, measure_load) = meas_dur!({ match cache_for_accounts_lt_hash.get(pubkey) { - Some(initial_state_of_account) => initial_state_of_account.clone(), - None => { + Some(CacheValue::InspectAccount(initial_state_of_account)) => { + initial_state_of_account.clone() + } + Some(CacheValue::BankNew) | None => { accum.1.num_cache_misses += 1; // If the initial state of the account is not in the accounts - // lt hash cache, it is likely this account was stored - // *outside* of transaction processing (e.g. as part of rent - // collection). Do not populate the read cache, as this - // account likely will not be accessed again soon. + // lt hash cache, or is explicitly unknown, then it is likely + // this account was stored *outside* of transaction processing + // (e.g. as part of rent collection, or creating a new bank). + // Do not populate the read cache, as this account likely will + // not be accessed again soon. let account_slot = self .rc .accounts @@ -271,18 +274,21 @@ impl Bank { .write() .unwrap() .entry(*address) - .or_insert_with(|| match account_state { - AccountState::Dead => InitialStateOfAccount::Dead, - AccountState::Alive(account) => { - InitialStateOfAccount::Alive((*account).clone()) - } + .or_insert_with(|| { + let initial_state_of_account = match account_state { + AccountState::Dead => InitialStateOfAccount::Dead, + AccountState::Alive(account) => { + InitialStateOfAccount::Alive((*account).clone()) + } + }; + CacheValue::InspectAccount(initial_state_of_account) }); } } } /// The initial state of an account prior to being modified in this slot/transaction -#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq)] pub enum InitialStateOfAccount { /// The account was initiall dead Dead, @@ -290,6 +296,17 @@ pub enum InitialStateOfAccount { Alive(AccountSharedData), } +/// The value type for the accounts lt hash cache +#[derive(Debug, Clone, PartialEq)] +pub enum CacheValue { + /// The value was inserted by `inspect_account()`. + /// This means we will have the initial state of the account. + InspectAccount(InitialStateOfAccount), + /// The value was inserted by `Bank::new()`. + /// This means we will *not* have the initial state of the account. + BankNew, +} + #[cfg(test)] mod tests { use { @@ -607,7 +624,7 @@ mod tests { let mut account = AccountSharedData::new(initial_lamports, 0, &Pubkey::default()); bank.inspect_account_for_accounts_lt_hash(&address, &AccountState::Alive(&account), true); assert_eq!(bank.cache_for_accounts_lt_hash.read().unwrap().len(), 2); - if let InitialStateOfAccount::Alive(cached_account) = bank + if let CacheValue::InspectAccount(InitialStateOfAccount::Alive(cached_account)) = bank .cache_for_accounts_lt_hash .read() .unwrap() @@ -624,7 +641,7 @@ mod tests { account.set_lamports(updated_lamports); bank.inspect_account_for_accounts_lt_hash(&address, &AccountState::Alive(&account), true); assert_eq!(bank.cache_for_accounts_lt_hash.read().unwrap().len(), 2); - if let InitialStateOfAccount::Alive(cached_account) = bank + if let CacheValue::InspectAccount(InitialStateOfAccount::Alive(cached_account)) = bank .cache_for_accounts_lt_hash .read() .unwrap() @@ -648,7 +665,9 @@ mod tests { .get(&address) .unwrap() { - InitialStateOfAccount::Dead => { /* this is expected, nothing to do here*/ } + CacheValue::InspectAccount(InitialStateOfAccount::Dead) => { + // this is expected, nothing to do here + } _ => panic!("wrong initial state for account"), }; @@ -665,7 +684,9 @@ mod tests { .get(&address) .unwrap() { - InitialStateOfAccount::Dead => { /* this is expected, nothing to do here*/ } + CacheValue::InspectAccount(InitialStateOfAccount::Dead) => { + // this is expected, nothing to do here + } _ => panic!("wrong initial state for account"), }; } @@ -912,4 +933,45 @@ mod tests { roundtrip_bank.wait_for_initial_accounts_hash_verification_completed_for_tests(); assert_eq!(roundtrip_bank, *bank); } + + /// Ensure that accounts written in Bank::new() are added to the accounts lt hash cache. + #[test_case(Features::None; "no features")] + #[test_case(Features::All; "all features")] + fn test_accounts_lt_hash_cache_values_from_bank_new(features: Features) { + let (genesis_config, _mint_keypair) = genesis_config_with(features); + let (mut bank, bank_forks) = Bank::new_with_bank_forks_for_tests(&genesis_config); + bank.rc + .accounts + .accounts_db + .set_is_experimental_accumulator_hash_enabled(true); + + // ensure the accounts lt hash is enabled, otherwise this test doesn't actually do anything... + assert!(bank.is_accounts_lt_hash_enabled()); + + let slot = bank.slot() + 1; + bank = new_bank_from_parent_with_bank_forks(&bank_forks, bank, &Pubkey::default(), slot); + + // These are the two accounts *currently* added to the bank during Bank::new(). + // More accounts could be added later, so if the test fails, inspect the actual cache + // accounts and update the expected cache accounts as necessary. + let expected_cache = &[ + ( + Pubkey::from_str_const("SysvarC1ock11111111111111111111111111111111"), + CacheValue::BankNew, + ), + ( + Pubkey::from_str_const("SysvarS1otHashes111111111111111111111111111"), + CacheValue::BankNew, + ), + ]; + let mut actual_cache: Vec<_> = bank + .cache_for_accounts_lt_hash + .read() + .unwrap() + .iter() + .map(|(k, v)| (*k, v.clone())) + .collect(); + actual_cache.sort_unstable_by(|a, b| a.0.cmp(&b.0)); + assert_eq!(expected_cache, actual_cache.as_slice()); + } } diff --git a/runtime/src/bank/metrics.rs b/runtime/src/bank/metrics.rs index 227713d8a5d701..44dd4f25b836d4 100644 --- a/runtime/src/bank/metrics.rs +++ b/runtime/src/bank/metrics.rs @@ -46,6 +46,7 @@ pub(crate) struct NewBankTimings { pub(crate) cache_preparation_time_us: u64, pub(crate) update_sysvars_time_us: u64, pub(crate) fill_sysvar_cache_time_us: u64, + pub(crate) populate_cache_for_accounts_lt_hash_us: Option, } pub(crate) fn report_new_epoch_metrics( @@ -115,6 +116,7 @@ pub(crate) fn report_new_bank_metrics( slot: Slot, parent_slot: Slot, block_height: u64, + num_accounts_modified_this_slot: Option, timings: NewBankTimings, ) { datapoint_info!( @@ -164,6 +166,16 @@ pub(crate) fn report_new_bank_metrics( timings.fill_sysvar_cache_time_us, i64 ), + ( + "num_accounts_modified_this_slot", + num_accounts_modified_this_slot, + Option + ), + ( + "populate_cache_for_accounts_lt_hash_us", + timings.populate_cache_for_accounts_lt_hash_us, + Option + ), ); }