Populates cache for accounts lt hash in Bank::new_from_parent() (sola…

…na-labs#3581)

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<Pubkey> {
+        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

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<AHashMap<Pubkey, InitialStateOfAccount>>,
+    ///
+    /// Note: The initial state must be strictly from an ancestor,
+    /// and not an intermediate state within this slot.
+    cache_for_accounts_lt_hash: RwLock<AHashMap<Pubkey, AccountsLtHashCacheValue>>,
 
     /// 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,
             },
         );
 

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,25 +274,39 @@ 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,
     /// The account was initially alive
     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());
+    }
 }

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<u64>,
 }
 
 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<usize>,
     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<i64>
+        ),
+        (
+            "populate_cache_for_accounts_lt_hash_us",
+            timings.populate_cache_for_accounts_lt_hash_us,
+            Option<i64>
+        ),
     );
 }