bank hash details supports skipped rewrites (#1648)

runtime/src/bank.rs

@@ -73,7 +73,7 @@ use {
         accounts::{AccountAddressFilter, Accounts, PubkeyAccountSlot},
         accounts_db::{
             AccountShrinkThreshold, AccountStorageEntry, AccountsDb, AccountsDbConfig,
-            CalcAccountsHashDataSource, VerifyAccountsHashAndLamportsConfig,
+            CalcAccountsHashDataSource, PubkeyHashAccount, VerifyAccountsHashAndLamportsConfig,
         },
         accounts_hash::{
             AccountHash, AccountsHash, CalcAccountsHashConfig, HashStats, IncrementalAccountsHash,
@@ -4254,20 +4254,65 @@ impl Bank {
     fn calculate_skipped_rewrites(&self) -> HashMap<Pubkey, AccountHash> {
         // The returned skipped rewrites may include accounts that were actually *not* skipped!
         // (This is safe, as per the fn's documentation above.)
-        HashMap::from_iter(
-            self.rent_collection_partitions()
-                .into_iter()
-                .map(accounts_partition::pubkey_range_from_partition)
-                .flat_map(|pubkey_range| {
-                    self.rc
-                        .accounts
-                        .load_to_collect_rent_eagerly(&self.ancestors, pubkey_range)
-                })
-                .map(|(pubkey, account, _slot)| {
-                    let account_hash = AccountsDb::hash_account(&account, &pubkey);
-                    (pubkey, account_hash)
-                }),
-        )
+        self.get_accounts_for_skipped_rewrites()
+            .map(|(pubkey, account_hash, _account)| (pubkey, account_hash))
+            .collect()
+    }
+
+    /// Loads accounts that were selected for rent collection this slot.
+    /// After loading the accounts, also calculate and return the account hashes.
+    /// This is used when dealing with skipped rewrites.
+    fn get_accounts_for_skipped_rewrites(
+        &self,
+    ) -> impl Iterator<Item = (Pubkey, AccountHash, AccountSharedData)> + '_ {
+        self.rent_collection_partitions()
+            .into_iter()
+            .map(accounts_partition::pubkey_range_from_partition)
+            .flat_map(|pubkey_range| {
+                self.rc
+                    .accounts
+                    .load_to_collect_rent_eagerly(&self.ancestors, pubkey_range)
+            })
+            .map(|(pubkey, account, _slot)| {
+                let account_hash = AccountsDb::hash_account(&account, &pubkey);
+                (pubkey, account_hash, account)
+            })
+    }
+
+    /// Returns the accounts, sorted by pubkey, that were part of accounts delta hash calculation
+    /// This is used when writing a bank hash details file.
+    pub(crate) fn get_accounts_for_bank_hash_details(&self) -> Vec<PubkeyHashAccount> {
+        let accounts_db = &self.rc.accounts.accounts_db;
+
+        let mut accounts_written_this_slot =
+            accounts_db.get_pubkey_hash_account_for_slot(self.slot());
+
+        // If we are skipping rewrites but also include them in the accounts delta hash, then we
+        // need to go load those accounts and add them to the list of accounts written this slot.
+        if !self.bank_hash_skips_rent_rewrites()
+            && accounts_db.test_skip_rewrites_but_include_in_bank_hash
+        {
+            let pubkeys_written_this_slot: HashSet<_> = accounts_written_this_slot
+                .iter()
+                .map(|pubkey_hash_account| pubkey_hash_account.pubkey)
+                .collect();
+
+            let rent_collection_accounts = self.get_accounts_for_skipped_rewrites();
+            for (pubkey, hash, account) in rent_collection_accounts {
+                if !pubkeys_written_this_slot.contains(&pubkey) {
+                    accounts_written_this_slot.push(PubkeyHashAccount {
+                        pubkey,
+                        hash,
+                        account,
+                    });
+                }
+            }
+        }
+
+        // Sort the accounts by pubkey to match the order of the accounts delta hash.
+        // This also makes comparison of files from different nodes deterministic.
+        accounts_written_this_slot.sort_unstable_by_key(|account| account.pubkey);
+        accounts_written_this_slot
     }
 
     fn collect_rent_eagerly(&self) {

runtime/src/bank/bank_hash_details.rs

@@ -136,14 +136,8 @@ impl TryFrom<&Bank> for BankHashSlotDetails {
             .accounts_db
             .get_accounts_delta_hash(slot)
             .unwrap();
-        let mut accounts = bank
-            .rc
-            .accounts
-            .accounts_db
-            .get_pubkey_hash_account_for_slot(slot);
-        // get_pubkey_hash_account_for_slot() returns an arbitrary ordering;
-        // sort by pubkey to match the ordering used for accounts delta hash
-        accounts.sort_by_key(|account| account.pubkey);
+
+        let accounts = bank.get_accounts_for_bank_hash_details();
 
         Ok(Self::new(
             slot,

runtime/src/bank/tests.rs

@@ -27,6 +27,7 @@ use {
     solana_accounts_db::{
         accounts::AccountAddressFilter,
         accounts_db::{AccountShrinkThreshold, DEFAULT_ACCOUNTS_SHRINK_RATIO},
+        accounts_hash::{AccountsDeltaHash, AccountsHasher},
         accounts_index::{
             AccountIndex, AccountSecondaryIndexes, IndexKey, ScanConfig, ScanError, ITER_BATCH_SIZE,
         },
@@ -12833,6 +12834,75 @@ fn test_rebuild_skipped_rewrites() {
     assert_eq!(snapshot_skipped_rewrites, actual_skipped_rewrites);
 }
 
+/// Test that getting accounts for BankHashDetails works with skipped rewrites
+#[test_case(true; "skip rewrites")]
+#[test_case(false; "do rewrites")]
+fn test_get_accounts_for_bank_hash_details(skip_rewrites: bool) {
+    let genesis_config = GenesisConfig::default();
+    let accounts_db_config = AccountsDbConfig {
+        test_skip_rewrites_but_include_in_bank_hash: skip_rewrites,
+        ..ACCOUNTS_DB_CONFIG_FOR_TESTING
+    };
+    let bank = Arc::new(Bank::new_with_paths(
+        &genesis_config,
+        Arc::new(RuntimeConfig::default()),
+        Vec::default(),
+        None,
+        None,
+        AccountSecondaryIndexes::default(),
+        AccountShrinkThreshold::default(),
+        false,
+        Some(accounts_db_config.clone()),
+        None,
+        Some(Pubkey::new_unique()),
+        Arc::new(AtomicBool::new(false)),
+    ));
+    // This test is only meaningful while the bank hash contains rewrites.
+    // Once this feature is enabled, it may be possible to remove this test entirely.
+    assert!(!bank.bank_hash_skips_rent_rewrites());
+
+    // Store an account *in this bank* that will be checked for rent collection *in the next bank*
+    let pubkey = {
+        let rent_collection_partition = bank
+            .variable_cycle_partitions_between_slots(bank.slot(), bank.slot() + 1)
+            .last()
+            .copied()
+            .unwrap();
+        let pubkey_range =
+            accounts_partition::pubkey_range_from_partition(rent_collection_partition);
+        *pubkey_range.end()
+    };
+    let mut account = AccountSharedData::new(123_456_789, 0, &Pubkey::default());
+    // The account's rent epoch must be set to EXEMPT
+    // in order for its rewrite to be skipped by rent collection.
+    account.set_rent_epoch(RENT_EXEMPT_RENT_EPOCH);
+    bank.store_account_and_update_capitalization(&pubkey, &account);
+
+    // Create a new bank that will do rent collection on the account stored in the previous slot
+    let bank = Bank::new_from_parent(bank.clone(), &Pubkey::new_unique(), bank.slot() + 1);
+
+    // Freeze the bank to do rent collection and calculate the accounts delta hash
+    bank.freeze();
+
+    // Ensure that the accounts returned by `get_accounts_for_bank_hash_details()` produces the
+    // same AccountsDeltaHash as the actual value stored in the Bank.
+    let calculated_accounts_delta_hash = {
+        let accounts = bank.get_accounts_for_bank_hash_details();
+        let hashes = accounts
+            .into_iter()
+            .map(|account| (account.pubkey, account.hash))
+            .collect();
+        AccountsDeltaHash(AccountsHasher::accumulate_account_hashes(hashes))
+    };
+    let actual_accounts_delta_hash = bank
+        .rc
+        .accounts
+        .accounts_db
+        .get_accounts_delta_hash(bank.slot())
+        .unwrap();
+    assert_eq!(calculated_accounts_delta_hash, actual_accounts_delta_hash);
+}
+
 /// Test that simulations report the compute units of failed transactions
 #[test]
 fn test_failed_simulation_compute_units() {