Check account hashes in snapshot (#7559)

* Check for incorrect hash value

* Finish up checking for incorrect hash value

* Fix comment a bit

Co-authored-by: sakridge <sakridge@gmail.com>

runtime/benches/accounts.rs

@@ -59,12 +59,12 @@ fn test_accounts_squash(bencher: &mut Bencher) {
 }
 
 #[bench]
-fn test_accounts_hash_internal_state(bencher: &mut Bencher) {
+fn test_accounts_hash_bank_hash(bencher: &mut Bencher) {
     let accounts = Accounts::new(vec![PathBuf::from("bench_accounts_hash_internal")]);
     let mut pubkeys: Vec<Pubkey> = vec![];
     create_test_accounts(&accounts, &mut pubkeys, 60000, 0);
     let ancestors = vec![(0, 0)].into_iter().collect();
     bencher.iter(|| {
-        accounts.verify_hash_internal_state(0, &ancestors);
+        accounts.verify_bank_hash(0, &ancestors);
     });
 }

runtime/src/accounts.rs

@@ -345,8 +345,8 @@ impl Accounts {
         })
     }
 
-    pub fn verify_hash_internal_state(&self, slot: Slot, ancestors: &HashMap<Slot, usize>) -> bool {
+    pub fn verify_bank_hash(&self, slot: Slot, ancestors: &HashMap<Slot, usize>) -> bool {
-        self.accounts_db.verify_hash_internal_state(slot, ancestors)
+        self.accounts_db.verify_bank_hash(slot, ancestors).is_ok()
     }
 
     pub fn load_by_program(
@@ -476,7 +476,7 @@ impl Accounts {
         }
     }
 
-    pub fn hash_internal_state(&self, slot_id: Slot) -> BankHash {
+    pub fn bank_hash_at(&self, slot_id: Slot) -> BankHash {
         let slot_hashes = self.accounts_db.slot_hashes.read().unwrap();
         *slot_hashes
             .get(&slot_id)
@@ -1164,17 +1164,17 @@ mod tests {
 
     #[test]
     #[should_panic]
-    fn test_accounts_empty_hash_internal_state() {
+    fn test_accounts_empty_bank_hash() {
         let accounts = Accounts::new(Vec::new());
-        accounts.hash_internal_state(0);
+        accounts.bank_hash_at(0);
     }
 
     #[test]
     #[should_panic]
-    fn test_accounts_empty_account_hash_internal_state() {
+    fn test_accounts_empty_account_bank_hash() {
         let accounts = Accounts::new(Vec::new());
         accounts.store_slow(0, &Pubkey::default(), &Account::new(1, 0, &sysvar::id()));
-        accounts.hash_internal_state(0);
+        accounts.bank_hash_at(0);
     }
 
     fn check_accounts(accounts: &Accounts, pubkeys: &Vec<Pubkey>, num: usize) {
@@ -1221,10 +1221,7 @@ mod tests {
             .accounts_from_stream(&mut reader, &daccounts_paths, copied_accounts.path())
             .is_ok());
         check_accounts(&daccounts, &pubkeys, 100);
-        assert_eq!(
+        assert_eq!(accounts.bank_hash_at(0), daccounts.bank_hash_at(0));
-            accounts.hash_internal_state(0),
-            daccounts.hash_internal_state(0)
-        );
     }
 
     #[test]

runtime/src/accounts_db.rs

@@ -175,6 +175,13 @@ pub enum AccountStorageStatus {
     Candidate = 2,
 }
 
+#[derive(Debug)]
+pub enum BankHashVerificatonError {
+    MismatchedAccountHash,
+    MismatchedBankHash,
+    MissingBankHash,
+}
+
 /// Persistent storage structure holding the accounts
 #[derive(Debug, Serialize, Deserialize)]
 pub struct AccountStorageEntry {
@@ -354,13 +361,11 @@ pub struct AccountsDB {
     /// Keeps tracks of index into AppendVec on a per slot basis
     pub accounts_index: RwLock<AccountsIndex<AccountInfo>>,
 
-    /// Account storage
     pub storage: RwLock<AccountStorage>,
 
     /// distribute the accounts across storage lists
     pub next_id: AtomicUsize,
 
-    /// write version
     write_version: AtomicUsize,
 
     /// Set of storage paths to pick from
@@ -379,7 +384,6 @@ pub struct AccountsDB {
     /// the accounts
     min_num_stores: usize,
 
-    /// slot to BankHash and a status flag to indicate if the hash has been initialized or not
     pub slot_hashes: RwLock<HashMap<Slot, BankHash>>,
 }
 
@@ -969,28 +973,49 @@ impl AccountsDB {
         datapoint_info!("accounts_db-stores", ("total_count", total_count, i64));
     }
 
-    pub fn verify_hash_internal_state(&self, slot: Slot, ancestors: &HashMap<Slot, usize>) -> bool {
+    pub fn verify_bank_hash(
-        let mut hash_state = BankHash::default();
+        &self,
-        let hashes: Vec<_> = self.scan_accounts(
+        slot: Slot,
+        ancestors: &HashMap<Slot, usize>,
+    ) -> Result<(), BankHashVerificatonError> {
+        use BankHashVerificatonError::*;
+
+        let (hashes, mismatch_found) = self.scan_accounts(
             ancestors,
-            |collector: &mut Vec<BankHash>, option: Option<(&Pubkey, Account, Slot)>| {
+            |(collector, mismatch_found): &mut (Vec<BankHash>, bool),
+             option: Option<(&Pubkey, Account, Slot)>| {
                 if let Some((pubkey, account, slot)) = option {
                     if !sysvar::check_id(&account.owner) {
-                        let hash = BankHash::from_hash(&Self::hash_account(slot, &account, pubkey));
+                        let hash = Self::hash_account(slot, &account, pubkey);
+                        if hash != account.hash {
+                            *mismatch_found = true;
+                        }
+                        if *mismatch_found {
+                            return;
+                        }
+                        let hash = BankHash::from_hash(&hash);
                         debug!("xoring..{} key: {}", hash, pubkey);
                         collector.push(hash);
                     }
                 }
             },
         );
+        if mismatch_found {
+            return Err(MismatchedAccountHash);
+        }
+        let mut calculated_hash = BankHash::default();
         for hash in hashes {
-            hash_state.xor(hash);
+            calculated_hash.xor(hash);
         }
         let slot_hashes = self.slot_hashes.read().unwrap();
-        if let Some(state) = slot_hashes.get(&slot) {
+        if let Some(found_hash) = slot_hashes.get(&slot) {
-            hash_state == *state
+            if calculated_hash == *found_hash {
+                Ok(())
             } else {
-            false
+                Err(MismatchedBankHash)
+            }
+        } else {
+            Err(BankHashVerificatonError::MissingBankHash)
         }
     }
 
@@ -1120,9 +1145,12 @@ impl AccountsDB {
     /// Store the account update.
     pub fn store(&self, slot_id: Slot, accounts: &[(&Pubkey, &Account)]) {
         let hashes = self.hash_accounts(slot_id, accounts);
+        self.store_with_hashes(slot_id, accounts, &hashes);
+    }
 
+    fn store_with_hashes(&self, slot_id: Slot, accounts: &[(&Pubkey, &Account)], hashes: &[Hash]) {
         let mut store_accounts = Measure::start("store::store_accounts");
-        let infos = self.store_accounts(slot_id, accounts, &hashes);
+        let infos = self.store_accounts(slot_id, accounts, hashes);
         store_accounts.stop();
 
         let mut update_index = Measure::start("store::update_index");
@@ -1234,9 +1262,11 @@ pub mod tests {
     // TODO: all the bank tests are bank specific, issue: 2194
     use super::*;
     use crate::append_vec::AccountMeta;
+    use assert_matches::assert_matches;
     use bincode::serialize_into;
     use rand::{thread_rng, Rng};
     use solana_sdk::account::Account;
+    use solana_sdk::hash::HASH_BYTES;
     use std::fs;
     use std::str::FromStr;
     use tempfile::TempDir;
@@ -2195,4 +2225,78 @@ pub mod tests {
             "Account-based hashing must be consistent with StoredAccount-based one."
         );
     }
+
+    #[test]
+    fn test_verify_bank_hash() {
+        use BankHashVerificatonError::*;
+        solana_logger::setup();
+        let db = AccountsDB::new(Vec::new());
+
+        let key = Pubkey::default();
+        let some_data_len = 0;
+        let some_slot: Slot = 0;
+        let account = Account::new(1, some_data_len, &key);
+        let ancestors = vec![(some_slot, 0)].into_iter().collect();
+
+        db.store(some_slot, &[(&key, &account)]);
+        db.add_root(some_slot);
+        assert_matches!(db.verify_bank_hash(some_slot, &ancestors), Ok(_));
+
+        db.slot_hashes.write().unwrap().remove(&some_slot).unwrap();
+        assert_matches!(
+            db.verify_bank_hash(some_slot, &ancestors),
+            Err(MissingBankHash)
+        );
+
+        let some_bank_hash = BankHash::from_hash(&Hash::new(&[0xca; HASH_BYTES]));
+        db.slot_hashes
+            .write()
+            .unwrap()
+            .insert(some_slot, some_bank_hash);
+        assert_matches!(
+            db.verify_bank_hash(some_slot, &ancestors),
+            Err(MismatchedBankHash)
+        );
+    }
+
+    #[test]
+    fn test_verify_bank_hash_no_account() {
+        solana_logger::setup();
+        let db = AccountsDB::new(Vec::new());
+
+        let some_slot: Slot = 0;
+        let ancestors = vec![(some_slot, 0)].into_iter().collect();
+
+        db.slot_hashes
+            .write()
+            .unwrap()
+            .insert(some_slot, BankHash::default());
+        db.add_root(some_slot);
+        assert_matches!(db.verify_bank_hash(some_slot, &ancestors), Ok(_));
+    }
+
+    #[test]
+    fn test_verify_bank_hash_bad_account_hash() {
+        use BankHashVerificatonError::*;
+        solana_logger::setup();
+        let db = AccountsDB::new(Vec::new());
+
+        let key = Pubkey::default();
+        let some_data_len = 0;
+        let some_slot: Slot = 0;
+        let account = Account::new(1, some_data_len, &key);
+        let ancestors = vec![(some_slot, 0)].into_iter().collect();
+
+        let accounts = &[(&key, &account)];
+        // update AccountsDB's bank hash but discard real account hashes
+        db.hash_accounts(some_slot, accounts);
+        // provide bogus account hashes
+        let some_hash = Hash::new(&[0xca; HASH_BYTES]);
+        db.store_with_hashes(some_slot, accounts, &vec![some_hash]);
+        db.add_root(some_slot);
+        assert_matches!(
+            db.verify_bank_hash(some_slot, &ancestors),
+            Err(MismatchedAccountHash)
+        );
+    }
 }

runtime/src/bank.rs

@@ -1595,7 +1595,7 @@ impl Bank {
     ///  of the delta of the ledger since the last vote and up to now
     fn hash_internal_state(&self) -> Hash {
         // If there are no accounts, return the hash of the previous state and the latest blockhash
-        let accounts_delta_hash = self.rc.accounts.hash_internal_state(self.slot());
+        let accounts_delta_hash = self.rc.accounts.bank_hash_at(self.slot());
         let mut signature_count_buf = [0u8; 8];
         LittleEndian::write_u64(&mut signature_count_buf[..], self.signature_count() as u64);
         hashv(&[
@@ -1611,7 +1611,7 @@ impl Bank {
     pub fn verify_hash_internal_state(&self) -> bool {
         self.rc
             .accounts
-            .verify_hash_internal_state(self.slot(), &self.ancestors)
+            .verify_bank_hash(self.slot(), &self.ancestors)
     }
 
     /// A snapshot bank should be purged of 0 lamport accounts which are not part of the hash
@@ -1620,7 +1620,7 @@ impl Bank {
         self.purge_zero_lamport_accounts();
         self.rc
             .accounts
-            .verify_hash_internal_state(self.slot(), &self.ancestors)
+            .verify_bank_hash(self.slot(), &self.ancestors)
     }
 
     /// Return the number of hashes per tick
@@ -1806,8 +1806,8 @@ impl Bank {
         let dsc = dbank.src.status_cache.read().unwrap();
         assert_eq!(*sc, *dsc);
         assert_eq!(
-            self.rc.accounts.hash_internal_state(self.slot),
+            self.rc.accounts.bank_hash_at(self.slot),
-            dbank.rc.accounts.hash_internal_state(dbank.slot)
+            dbank.rc.accounts.bank_hash_at(dbank.slot)
         );
     }
 

sdk/src/hash.rs

@@ -7,9 +7,10 @@ use std::fmt;
 use std::mem;
 use std::str::FromStr;
 
+pub const HASH_BYTES: usize = 32;
 #[derive(Serialize, Deserialize, Clone, Copy, Default, Eq, PartialEq, Ord, PartialOrd, Hash)]
 #[repr(transparent)]
-pub struct Hash([u8; 32]);
+pub struct Hash([u8; HASH_BYTES]);
 
 #[derive(Clone, Default)]
 pub struct Hasher {
@@ -28,7 +29,7 @@ impl Hasher {
     pub fn result(self) -> Hash {
         // At the time of this writing, the sha2 library is stuck on an old version
         // of generic_array (0.9.0). Decouple ourselves with a clone to our version.
-        Hash(<[u8; 32]>::try_from(self.hasher.result().as_slice()).unwrap())
+        Hash(<[u8; HASH_BYTES]>::try_from(self.hasher.result().as_slice()).unwrap())
     }
 }
 
@@ -73,7 +74,7 @@ impl FromStr for Hash {
 
 impl Hash {
     pub fn new(hash_slice: &[u8]) -> Self {
-        Hash(<[u8; 32]>::try_from(hash_slice).unwrap())
+        Hash(<[u8; HASH_BYTES]>::try_from(hash_slice).unwrap())
     }
 }