From f558b9b6bf88c1fc56dcf93a6c12231cb8eef66d Mon Sep 17 00:00:00 2001 From: "Jeff Washington (jwash)" <75863576+jeffwashington@users.noreply.github.com> Date: Tue, 15 Jun 2021 11:52:12 -0500 Subject: [PATCH] verify bank hash on startup with ledger tool option (#17939) --- core/tests/snapshots.rs | 2 ++ ledger/src/bank_forks_utils.rs | 1 + runtime/benches/accounts.rs | 10 ++++++++- runtime/src/accounts.rs | 11 ++++++---- runtime/src/accounts_db.rs | 40 +++++++++++++++++++--------------- runtime/src/bank.rs | 31 +++++++++++++------------- runtime/src/snapshot_utils.rs | 3 ++- 7 files changed, 59 insertions(+), 39 deletions(-) diff --git a/core/tests/snapshots.rs b/core/tests/snapshots.rs index f569d6b286..05b554bc3d 100644 --- a/core/tests/snapshots.rs +++ b/core/tests/snapshots.rs @@ -146,6 +146,7 @@ mod tests { let old_last_bank = old_bank_forks.get(old_last_slot).unwrap(); + let check_hash_calculation = false; let (deserialized_bank, _timing) = snapshot_utils::bank_from_archive( &account_paths, &[], @@ -167,6 +168,7 @@ mod tests { false, None, accounts_db::AccountShrinkThreshold::default(), + check_hash_calculation, ) .unwrap(); diff --git a/ledger/src/bank_forks_utils.rs b/ledger/src/bank_forks_utils.rs index 629319f164..2b272a911a 100644 --- a/ledger/src/bank_forks_utils.rs +++ b/ledger/src/bank_forks_utils.rs @@ -143,6 +143,7 @@ fn load_from_snapshot( process_options.accounts_db_caching_enabled, process_options.limit_load_slot_count_from_snapshot, process_options.shrink_ratio, + process_options.accounts_db_test_hash_calculation, ) .expect("Load from snapshot failed"); if let Some(shrink_paths) = shrink_paths { diff --git a/runtime/benches/accounts.rs b/runtime/benches/accounts.rs index 864789b381..08ad50df4e 100644 --- a/runtime/benches/accounts.rs +++ b/runtime/benches/accounts.rs @@ -114,7 +114,15 @@ fn test_accounts_hash_bank_hash(bencher: &mut Bencher) { create_test_accounts(&accounts, &mut pubkeys, num_accounts, slot); let ancestors = Ancestors::from(vec![0]); let (_, total_lamports) = accounts.accounts_db.update_accounts_hash(0, &ancestors); - bencher.iter(|| assert!(accounts.verify_bank_hash_and_lamports(0, &ancestors, total_lamports))); + let test_hash_calculation = false; + bencher.iter(|| { + assert!(accounts.verify_bank_hash_and_lamports( + 0, + &ancestors, + total_lamports, + test_hash_calculation + )) + }); } #[bench] diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index b6fc3d1fe3..5b1ab29fe5 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -655,11 +655,14 @@ impl Accounts { slot: Slot, ancestors: &Ancestors, total_lamports: u64, + test_hash_calculation: bool, ) -> bool { - if let Err(err) = - self.accounts_db - .verify_bank_hash_and_lamports(slot, ancestors, total_lamports) - { + if let Err(err) = self.accounts_db.verify_bank_hash_and_lamports( + slot, + ancestors, + total_lamports, + test_hash_calculation, + ) { warn!("verify_bank_hash failed: {:?}", err); false } else { diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index fe2f071b1a..a3504f7ff5 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -4866,19 +4866,23 @@ impl AccountsDb { slot: Slot, ancestors: &Ancestors, total_lamports: u64, + test_hash_calculation: bool, ) -> Result<(), BankHashVerificationError> { use BankHashVerificationError::*; let use_index = true; let check_hash = true; let can_cached_slot_be_unflushed = false; - let (calculated_hash, calculated_lamports) = self.calculate_accounts_hash_helper( - use_index, - slot, - ancestors, - check_hash, - can_cached_slot_be_unflushed, - )?; + let (calculated_hash, calculated_lamports) = self + .calculate_accounts_hash_helper_with_verify( + use_index, + test_hash_calculation, + slot, + ancestors, + None, + can_cached_slot_be_unflushed, + check_hash, + )?; if calculated_lamports != total_lamports { warn!( @@ -8215,7 +8219,7 @@ pub mod tests { assert_load_account(&accounts, current_slot, dummy_pubkey, dummy_lamport); accounts - .verify_bank_hash_and_lamports(4, &Ancestors::default(), 1222) + .verify_bank_hash_and_lamports(4, &Ancestors::default(), 1222, true) .unwrap(); } @@ -8690,13 +8694,13 @@ pub mod tests { db.add_root(some_slot); db.update_accounts_hash_test(some_slot, &ancestors); assert_matches!( - db.verify_bank_hash_and_lamports(some_slot, &ancestors, 1), + db.verify_bank_hash_and_lamports(some_slot, &ancestors, 1, true), Ok(_) ); db.bank_hashes.write().unwrap().remove(&some_slot).unwrap(); assert_matches!( - db.verify_bank_hash_and_lamports(some_slot, &ancestors, 1), + db.verify_bank_hash_and_lamports(some_slot, &ancestors, 1, true), Err(MissingBankHash) ); @@ -8711,7 +8715,7 @@ pub mod tests { .unwrap() .insert(some_slot, bank_hash_info); assert_matches!( - db.verify_bank_hash_and_lamports(some_slot, &ancestors, 1), + db.verify_bank_hash_and_lamports(some_slot, &ancestors, 1, true), Err(MismatchedBankHash) ); } @@ -8732,7 +8736,7 @@ pub mod tests { db.add_root(some_slot); db.update_accounts_hash_test(some_slot, &ancestors); assert_matches!( - db.verify_bank_hash_and_lamports(some_slot, &ancestors, 1), + db.verify_bank_hash_and_lamports(some_slot, &ancestors, 1, true), Ok(_) ); @@ -8746,12 +8750,12 @@ pub mod tests { ); db.update_accounts_hash_test(some_slot, &ancestors); assert_matches!( - db.verify_bank_hash_and_lamports(some_slot, &ancestors, 2), + db.verify_bank_hash_and_lamports(some_slot, &ancestors, 2, true), Ok(_) ); assert_matches!( - db.verify_bank_hash_and_lamports(some_slot, &ancestors, 10), + db.verify_bank_hash_and_lamports(some_slot, &ancestors, 10, true), Err(MismatchedTotalLamports(expected, actual)) if expected == 2 && actual == 10 ); } @@ -8771,7 +8775,7 @@ pub mod tests { db.add_root(some_slot); db.update_accounts_hash_test(some_slot, &ancestors); assert_matches!( - db.verify_bank_hash_and_lamports(some_slot, &ancestors, 0), + db.verify_bank_hash_and_lamports(some_slot, &ancestors, 0, true), Ok(_) ); } @@ -8801,7 +8805,7 @@ pub mod tests { db.store_accounts_unfrozen(some_slot, accounts, Some(&[&some_hash]), false); db.add_root(some_slot); assert_matches!( - db.verify_bank_hash_and_lamports(some_slot, &ancestors, 1), + db.verify_bank_hash_and_lamports(some_slot, &ancestors, 1, true), Err(MismatchedAccountHash) ); } @@ -9370,12 +9374,12 @@ pub mod tests { let no_ancestors = Ancestors::default(); accounts.update_accounts_hash(current_slot, &no_ancestors); accounts - .verify_bank_hash_and_lamports(current_slot, &no_ancestors, 22300) + .verify_bank_hash_and_lamports(current_slot, &no_ancestors, 22300, true) .unwrap(); let accounts = reconstruct_accounts_db_via_serialization(&accounts, current_slot); accounts - .verify_bank_hash_and_lamports(current_slot, &no_ancestors, 22300) + .verify_bank_hash_and_lamports(current_slot, &no_ancestors, 22300, true) .unwrap(); // repeating should be no-op diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 0e5f05c62f..3c9611678c 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -4561,11 +4561,12 @@ impl Bank { /// Recalculate the hash_internal_state from the account stores. Would be used to verify a /// snapshot. #[must_use] - fn verify_bank_hash(&self) -> bool { + fn verify_bank_hash(&self, test_hash_calculation: bool) -> bool { self.rc.accounts.verify_bank_hash_and_lamports( self.slot(), &self.ancestors, self.capitalization(), + test_hash_calculation, ) } @@ -4675,7 +4676,7 @@ impl Bank { /// A snapshot bank should be purged of 0 lamport accounts which are not part of the hash /// calculation and could shield other real accounts. - pub fn verify_snapshot_bank(&self) -> bool { + pub fn verify_snapshot_bank(&self, test_hash_calculation: bool) -> bool { info!("cleaning.."); let mut clean_time = Measure::start("clean"); if self.slot() > 0 { @@ -4692,7 +4693,7 @@ impl Bank { info!("verify_bank_hash.."); let mut verify_time = Measure::start("verify_bank_hash"); - let mut verify = self.verify_bank_hash(); + let mut verify = self.verify_bank_hash(test_hash_calculation); verify_time.stop(); info!("verify_hash.."); @@ -7514,17 +7515,17 @@ pub(crate) mod tests { assert_eq!(bank0.get_account(&keypair.pubkey()).unwrap().lamports(), 10); assert_eq!(bank1.get_account(&keypair.pubkey()), None); - assert!(bank0.verify_bank_hash()); + assert!(bank0.verify_bank_hash(true)); // Squash and then verify hash_internal value bank0.freeze(); bank0.squash(); - assert!(bank0.verify_bank_hash()); + assert!(bank0.verify_bank_hash(true)); bank1.freeze(); bank1.squash(); bank1.update_accounts_hash(); - assert!(bank1.verify_bank_hash()); + assert!(bank1.verify_bank_hash(true)); // keypair should have 0 tokens on both forks assert_eq!(bank0.get_account(&keypair.pubkey()), None); @@ -7532,7 +7533,7 @@ pub(crate) mod tests { bank1.force_flush_accounts_cache(); bank1.clean_accounts(false, false); - assert!(bank1.verify_bank_hash()); + assert!(bank1.verify_bank_hash(true)); } #[test] @@ -8338,7 +8339,7 @@ pub(crate) mod tests { info!("transfer 2 {}", pubkey2); bank2.transfer(10, &mint_keypair, &pubkey2).unwrap(); bank2.update_accounts_hash(); - assert!(bank2.verify_bank_hash()); + assert!(bank2.verify_bank_hash(true)); } #[test] @@ -8362,19 +8363,19 @@ pub(crate) mod tests { // Checkpointing should never modify the checkpoint's state once frozen let bank0_state = bank0.hash_internal_state(); bank2.update_accounts_hash(); - assert!(bank2.verify_bank_hash()); + assert!(bank2.verify_bank_hash(true)); let bank3 = Bank::new_from_parent(&bank0, &solana_sdk::pubkey::new_rand(), 2); assert_eq!(bank0_state, bank0.hash_internal_state()); - assert!(bank2.verify_bank_hash()); + assert!(bank2.verify_bank_hash(true)); bank3.update_accounts_hash(); - assert!(bank3.verify_bank_hash()); + assert!(bank3.verify_bank_hash(true)); let pubkey2 = solana_sdk::pubkey::new_rand(); info!("transfer 2 {}", pubkey2); bank2.transfer(10, &mint_keypair, &pubkey2).unwrap(); bank2.update_accounts_hash(); - assert!(bank2.verify_bank_hash()); - assert!(bank3.verify_bank_hash()); + assert!(bank2.verify_bank_hash(true)); + assert!(bank3.verify_bank_hash(true)); } #[test] @@ -8394,11 +8395,11 @@ pub(crate) mod tests { bank.transfer(1_000, &mint_keypair, &pubkey).unwrap(); bank.freeze(); bank.update_accounts_hash(); - assert!(bank.verify_snapshot_bank()); + assert!(bank.verify_snapshot_bank(true)); // tamper the bank after freeze! bank.increment_signature_count(1); - assert!(!bank.verify_snapshot_bank()); + assert!(!bank.verify_snapshot_bank(true)); } // Test that two bank forks with the same accounts should not hash to the same value. diff --git a/runtime/src/snapshot_utils.rs b/runtime/src/snapshot_utils.rs index 25dc35a045..46f764624c 100644 --- a/runtime/src/snapshot_utils.rs +++ b/runtime/src/snapshot_utils.rs @@ -613,6 +613,7 @@ pub fn bank_from_archive>( accounts_db_caching_enabled: bool, limit_load_slot_count_from_snapshot: Option, shrink_ratio: AccountShrinkThreshold, + test_hash_calculation: bool, ) -> Result<(Bank, BankFromArchiveTimings)> { let unpack_dir = tempfile::Builder::new() .prefix(TMP_SNAPSHOT_PREFIX) @@ -651,7 +652,7 @@ pub fn bank_from_archive>( measure.stop(); let mut verify = Measure::start("verify"); - if !bank.verify_snapshot_bank() { + if !bank.verify_snapshot_bank(test_hash_calculation) { panic!("Snapshot bank for slot {} failed to verify", bank.slot()); } verify.stop();