From 88edd249154f1049089e1c307429700f66084d1e Mon Sep 17 00:00:00 2001 From: Stephen Akridge Date: Mon, 6 Jul 2020 21:23:07 -0700 Subject: [PATCH 01/11] Check bank capitalization --- accounts-bench/src/main.rs | 6 ++- runtime/benches/accounts.rs | 18 ++++++-- runtime/src/accounts.rs | 15 ++++++- runtime/src/accounts_db.rs | 90 ++++++++++++++++++++++++++----------- runtime/src/bank.rs | 39 ++++++++++++---- 5 files changed, 128 insertions(+), 40 deletions(-) diff --git a/accounts-bench/src/main.rs b/accounts-bench/src/main.rs index b88489c57f0210..2af61561e2b5d1 100644 --- a/accounts-bench/src/main.rs +++ b/accounts-bench/src/main.rs @@ -83,6 +83,7 @@ fn main() { ancestors.insert(i as u64, i - 1); accounts.add_root(i as u64); } + let cap_exempt = HashSet::new(); for x in 0..iterations { if clean { let mut time = Measure::start("clean"); @@ -96,7 +97,10 @@ fn main() { } else { let mut pubkeys: Vec = vec![]; let mut time = Measure::start("hash"); - let hash = accounts.accounts_db.update_accounts_hash(0, &ancestors); + let hash = accounts + .accounts_db + .update_accounts_hash(0, &ancestors, &cap_exempt) + .0; time.stop(); println!("hash: {} {}", hash, time); create_test_accounts(&accounts, &mut pubkeys, 1, 0); diff --git a/runtime/benches/accounts.rs b/runtime/benches/accounts.rs index 47cd7c8361cf99..daca6e6520dd49 100644 --- a/runtime/benches/accounts.rs +++ b/runtime/benches/accounts.rs @@ -74,10 +74,17 @@ fn test_accounts_hash_bank_hash(bencher: &mut Bencher) { &ClusterType::Development, ); let mut pubkeys: Vec = vec![]; - create_test_accounts(&accounts, &mut pubkeys, 60000, 0); + let num_accounts = 60_000; + let slot = 0; + create_test_accounts(&accounts, &mut pubkeys, num_accounts, slot); let ancestors = vec![(0, 0)].into_iter().collect(); - accounts.accounts_db.update_accounts_hash(0, &ancestors); - bencher.iter(|| assert!(accounts.verify_bank_hash(0, &ancestors))); + let cap_exempt = HashSet::new(); + let (_, total_lamports) = accounts + .accounts_db + .update_accounts_hash(0, &ancestors, &cap_exempt); + bencher.iter(|| { + assert!(accounts.verify_bank_hash_and_lamports(0, &ancestors, total_lamports, &cap_exempt)) + }); } #[bench] @@ -90,8 +97,11 @@ fn test_update_accounts_hash(bencher: &mut Bencher) { let mut pubkeys: Vec = vec![]; create_test_accounts(&accounts, &mut pubkeys, 50_000, 0); let ancestors = vec![(0, 0)].into_iter().collect(); + let cap_exempt = HashSet::new(); bencher.iter(|| { - accounts.accounts_db.update_accounts_hash(0, &ancestors); + accounts + .accounts_db + .update_accounts_hash(0, &ancestors, &cap_exempt); }); } diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index d696d6e831080a..28a29715bd06d4 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -429,8 +429,19 @@ impl Accounts { } #[must_use] - pub fn verify_bank_hash(&self, slot: Slot, ancestors: &Ancestors) -> bool { - if let Err(err) = self.accounts_db.verify_bank_hash(slot, ancestors) { + pub fn verify_bank_hash_and_lamports( + &self, + slot: Slot, + ancestors: &Ancestors, + total_lamports: u64, + capitalization_exempt: &HashSet, + ) -> bool { + if let Err(err) = self.accounts_db.verify_bank_hash_and_lamports( + slot, + ancestors, + total_lamports, + capitalization_exempt, + ) { warn!("verify_bank_hash failed: {:?}", err); false } else { diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index 6e34e8fb0c97a6..7aaa0061a52e4d 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -154,6 +154,7 @@ pub enum BankHashVerificationError { MismatchedAccountHash, MismatchedBankHash, MissingBankHash, + MismatchedTotalLamports, } /// Persistent storage structure holding the accounts @@ -1652,13 +1653,15 @@ impl AccountsDB { &self, ancestors: &Ancestors, check_hash: bool, - ) -> Result { + capitalization_exempt: &HashSet, + ) -> Result<(Hash, u64), BankHashVerificationError> { use BankHashVerificationError::*; let mut scan = Measure::start("scan"); let accounts_index = self.accounts_index.read().unwrap(); let storage = self.storage.read().unwrap(); let keys: Vec<_> = accounts_index.account_maps.keys().collect(); let mismatch_found = AtomicU64::new(0); + let total_lamports = AtomicU64::new(0); let hashes: Vec<_> = keys .par_iter() .filter_map(|pubkey| { @@ -1671,6 +1674,12 @@ impl AccountsDB { .and_then(|storage_map| storage_map.get(&account_info.store_id)) .and_then(|store| { let account = store.accounts.get_account(account_info.offset)?.0; + if !(solana_sdk::sysvar::check_id(&account.account_meta.owner) + || capitalization_exempt.contains(&pubkey)) + { + total_lamports + .fetch_add(account_info.lamports, Ordering::Relaxed); + } if check_hash { let hash = Self::hash_stored_account( @@ -1716,7 +1725,7 @@ impl AccountsDB { ("hash_accumulate", accumulate.as_us(), i64), ("hash_total", hash_total, i64), ); - Ok(accumulated_hash) + Ok((accumulated_hash, total_lamports.load(Ordering::Relaxed))) } pub fn get_accounts_hash(&self, slot: Slot) -> Hash { @@ -1725,22 +1734,40 @@ impl AccountsDB { bank_hash_info.snapshot_hash } - pub fn update_accounts_hash(&self, slot: Slot, ancestors: &Ancestors) -> Hash { - let hash = self.calculate_accounts_hash(ancestors, false).unwrap(); + pub fn update_accounts_hash( + &self, + slot: Slot, + ancestors: &Ancestors, + capitalization_exempt: &HashSet, + ) -> (Hash, u64) { + let (hash, total_lamports) = self + .calculate_accounts_hash(ancestors, false, capitalization_exempt) + .unwrap(); let mut bank_hashes = self.bank_hashes.write().unwrap(); let mut bank_hash_info = bank_hashes.get_mut(&slot).unwrap(); bank_hash_info.snapshot_hash = hash; - hash + (hash, total_lamports) } - pub fn verify_bank_hash( + pub fn verify_bank_hash_and_lamports( &self, slot: Slot, ancestors: &Ancestors, + total_lamports: u64, + capitalization_exempt: &HashSet, ) -> Result<(), BankHashVerificationError> { use BankHashVerificationError::*; - let calculated_hash = self.calculate_accounts_hash(ancestors, true)?; + let (calculated_hash, calculated_lamports) = + self.calculate_accounts_hash(ancestors, true, capitalization_exempt)?; + + if calculated_lamports != total_lamports { + warn!( + "Mismatched total lamports: {} calculated: {}", + total_lamports, calculated_lamports + ); + return Err(MismatchedTotalLamports); + } let bank_hashes = self.bank_hashes.read().unwrap(); if let Some(found_hash_info) = bank_hashes.get(&slot) { @@ -3025,8 +3052,8 @@ pub mod tests { let ancestors = linear_ancestors(latest_slot); assert_eq!( - daccounts.update_accounts_hash(latest_slot, &ancestors), - accounts.update_accounts_hash(latest_slot, &ancestors) + daccounts.update_accounts_hash(latest_slot, &ancestors, &HashSet::new()), + accounts.update_accounts_hash(latest_slot, &ancestors, &HashSet::new()) ); } @@ -3159,13 +3186,13 @@ pub mod tests { let ancestors = linear_ancestors(current_slot); info!("ancestors: {:?}", ancestors); - let hash = accounts.update_accounts_hash(current_slot, &ancestors); + let hash = accounts.update_accounts_hash(current_slot, &ancestors, &HashSet::new()); accounts.clean_accounts(); accounts.process_dead_slots(None); assert_eq!( - accounts.update_accounts_hash(current_slot, &ancestors), + accounts.update_accounts_hash(current_slot, &ancestors, &HashSet::new()), hash ); @@ -3286,7 +3313,7 @@ pub mod tests { accounts.add_root(current_slot); accounts.print_accounts_stats("pre_f"); - accounts.update_accounts_hash(4, &HashMap::default()); + accounts.update_accounts_hash(4, &HashMap::default(), &HashSet::new()); let accounts = f(accounts, current_slot); @@ -3297,7 +3324,9 @@ pub mod tests { assert_load_account(&accounts, current_slot, purged_pubkey2, 0); assert_load_account(&accounts, current_slot, dummy_pubkey, dummy_lamport); - accounts.verify_bank_hash(4, &HashMap::default()).unwrap(); + accounts + .verify_bank_hash_and_lamports(4, &HashMap::default(), 1222, &HashSet::new()) + .unwrap(); } #[test] @@ -3681,12 +3710,12 @@ pub mod tests { } #[test] - fn test_verify_bank_hash() { + fn test_verify_bank_hash1() { use BankHashVerificationError::*; solana_logger::setup(); let db = AccountsDB::new(Vec::new(), &ClusterType::Development); - let key = Pubkey::default(); + let key = Pubkey::new_rand(); let some_data_len = 0; let some_slot: Slot = 0; let account = Account::new(1, some_data_len, &key); @@ -3694,12 +3723,20 @@ pub mod tests { db.store(some_slot, &[(&key, &account)]); db.add_root(some_slot); - db.update_accounts_hash(some_slot, &ancestors); - assert_matches!(db.verify_bank_hash(some_slot, &ancestors), Ok(_)); + db.update_accounts_hash(some_slot, &ancestors, &HashSet::new()); + assert_matches!( + db.verify_bank_hash_and_lamports(some_slot, &ancestors, 1, &HashSet::new()), + Ok(_) + ); + + assert_matches!( + db.verify_bank_hash_and_lamports(some_slot, &ancestors, 10, &HashSet::new()), + Err(MismatchedTotalLamports) + ); db.bank_hashes.write().unwrap().remove(&some_slot).unwrap(); assert_matches!( - db.verify_bank_hash(some_slot, &ancestors), + db.verify_bank_hash_and_lamports(some_slot, &ancestors, 1, &HashSet::new()), Err(MissingBankHash) ); @@ -3714,7 +3751,7 @@ pub mod tests { .unwrap() .insert(some_slot, bank_hash_info); assert_matches!( - db.verify_bank_hash(some_slot, &ancestors), + db.verify_bank_hash_and_lamports(some_slot, &ancestors, 1, &HashSet::new()), Err(MismatchedBankHash) ); } @@ -3732,8 +3769,11 @@ pub mod tests { .unwrap() .insert(some_slot, BankHashInfo::default()); db.add_root(some_slot); - db.update_accounts_hash(some_slot, &ancestors); - assert_matches!(db.verify_bank_hash(some_slot, &ancestors), Ok(_)); + db.update_accounts_hash(some_slot, &ancestors, &HashSet::new()); + assert_matches!( + db.verify_bank_hash_and_lamports(some_slot, &ancestors, 0, &HashSet::new()), + Ok(_) + ); } #[test] @@ -3756,7 +3796,7 @@ pub mod tests { db.store_with_hashes(some_slot, accounts, &[some_hash]); db.add_root(some_slot); assert_matches!( - db.verify_bank_hash(some_slot, &ancestors), + db.verify_bank_hash_and_lamports(some_slot, &ancestors, 1, &HashSet::new()), Err(MismatchedAccountHash) ); } @@ -4174,14 +4214,14 @@ pub mod tests { ); let no_ancestors = HashMap::default(); - accounts.update_accounts_hash(current_slot, &no_ancestors); + accounts.update_accounts_hash(current_slot, &no_ancestors, &HashSet::new()); accounts - .verify_bank_hash(current_slot, &no_ancestors) + .verify_bank_hash_and_lamports(current_slot, &no_ancestors, 22300, &HashSet::new()) .unwrap(); let accounts = reconstruct_accounts_db_via_serialization(&accounts, current_slot); accounts - .verify_bank_hash(current_slot, &no_ancestors) + .verify_bank_hash_and_lamports(current_slot, &no_ancestors, 22300, &HashSet::new()) .unwrap(); // repeating should be no-op diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index df0d746db9d152..4eeadf7c7e8ada 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -458,6 +458,10 @@ pub struct Bank { // this is temporary field only to remove rewards_pool entirely pub rewards_pool_pubkeys: Arc>, + + pub builtin_programs: Arc>>, + + pub genesis_cap_exempt_ids: Arc>>, } impl Default for BlockhashQueue { @@ -577,6 +581,8 @@ impl Bank { parent.lazy_rent_collection.load(Ordering::Relaxed), ), rewards_pool_pubkeys: parent.rewards_pool_pubkeys.clone(), + builtin_programs: parent.builtin_programs.clone(), + genesis_cap_exempt_ids: parent.genesis_cap_exempt_ids.clone(), }; datapoint_info!( @@ -679,6 +685,8 @@ impl Bank { cluster_type: Some(genesis_config.cluster_type), lazy_rent_collection: new(), rewards_pool_pubkeys: new(), + genesis_cap_exempt_ids: new(), + builtin_programs: new(), }; bank.finish_init(genesis_config); @@ -1186,6 +1194,7 @@ impl Bank { panic!("{} repeated in genesis config", pubkey); } self.store_account(pubkey, account); + self.genesis_cap_exempt_ids.write().unwrap().insert(*pubkey); } // highest staked node is the first collector @@ -1258,8 +1267,11 @@ impl Bank { // Add a bogus executable native account, which will be loaded and ignored. let account = native_loader::create_loadable_account(name); self.store_account(&program_id, &account); + self.genesis_cap_exempt_ids + .write() + .unwrap() + .insert(*program_id); } - debug!("Added native program {} under {:?}", name, program_id); } pub fn set_cross_program_support(&mut self, is_supported: bool) { @@ -2848,9 +2860,14 @@ impl Bank { /// snapshot. #[must_use] fn verify_bank_hash(&self) -> bool { - self.rc - .accounts - .verify_bank_hash(self.slot(), &self.ancestors) + let mut capitalization_exempt = self.builtin_programs.read().unwrap().clone(); + capitalization_exempt.extend(self.genesis_cap_exempt_ids.read().unwrap().iter()); + self.rc.accounts.verify_bank_hash_and_lamports( + self.slot(), + &self.ancestors, + self.capitalization(), + &capitalization_exempt, + ) } pub fn get_snapshot_storages(&self) -> SnapshotStorages { @@ -2912,10 +2929,15 @@ impl Bank { } pub fn update_accounts_hash(&self) -> Hash { - self.rc - .accounts - .accounts_db - .update_accounts_hash(self.slot(), &self.ancestors) + let mut capitalization_exempt = self.builtin_programs.read().unwrap().clone(); + capitalization_exempt.extend(self.genesis_cap_exempt_ids.read().unwrap().iter()); + let (hash, total_lamports) = self.rc.accounts.accounts_db.update_accounts_hash( + self.slot(), + &self.ancestors, + &capitalization_exempt, + ); + assert_eq!(total_lamports, self.capitalization()); + hash } /// A snapshot bank should be purged of 0 lamport accounts which are not part of the hash @@ -3154,6 +3176,7 @@ impl Bank { debug!("Added builtin loader {} under {:?}", name, program_id); } } + self.builtin_programs.write().unwrap().insert(program_id); } pub fn compare_bank(&self, dbank: &Bank) { From 69b5120200350a5af88714b3564d5eadccf12feb Mon Sep 17 00:00:00 2001 From: Ryo Onodera Date: Mon, 31 Aug 2020 18:27:45 +0900 Subject: [PATCH 02/11] Simplify and unify capitalization calculation --- accounts-bench/src/main.rs | 6 +-- runtime/benches/accounts.rs | 14 ++----- runtime/src/accounts.rs | 20 +++++---- runtime/src/accounts_db.rs | 82 ++++++++++++++++++++----------------- runtime/src/bank.rs | 47 ++++----------------- 5 files changed, 68 insertions(+), 101 deletions(-) diff --git a/accounts-bench/src/main.rs b/accounts-bench/src/main.rs index 2af61561e2b5d1..e2a0d0a7e556dc 100644 --- a/accounts-bench/src/main.rs +++ b/accounts-bench/src/main.rs @@ -83,7 +83,6 @@ fn main() { ancestors.insert(i as u64, i - 1); accounts.add_root(i as u64); } - let cap_exempt = HashSet::new(); for x in 0..iterations { if clean { let mut time = Measure::start("clean"); @@ -97,10 +96,7 @@ fn main() { } else { let mut pubkeys: Vec = vec![]; let mut time = Measure::start("hash"); - let hash = accounts - .accounts_db - .update_accounts_hash(0, &ancestors, &cap_exempt) - .0; + let hash = accounts.accounts_db.update_accounts_hash(0, &ancestors).0; time.stop(); println!("hash: {} {}", hash, time); create_test_accounts(&accounts, &mut pubkeys, 1, 0); diff --git a/runtime/benches/accounts.rs b/runtime/benches/accounts.rs index daca6e6520dd49..5ba533a5bfbf61 100644 --- a/runtime/benches/accounts.rs +++ b/runtime/benches/accounts.rs @@ -78,13 +78,8 @@ fn test_accounts_hash_bank_hash(bencher: &mut Bencher) { let slot = 0; create_test_accounts(&accounts, &mut pubkeys, num_accounts, slot); let ancestors = vec![(0, 0)].into_iter().collect(); - let cap_exempt = HashSet::new(); - let (_, total_lamports) = accounts - .accounts_db - .update_accounts_hash(0, &ancestors, &cap_exempt); - bencher.iter(|| { - assert!(accounts.verify_bank_hash_and_lamports(0, &ancestors, total_lamports, &cap_exempt)) - }); + let (_, total_lamports) = accounts.accounts_db.update_accounts_hash(0, &ancestors); + bencher.iter(|| assert!(accounts.verify_bank_hash_and_lamports(0, &ancestors, total_lamports))); } #[bench] @@ -97,11 +92,8 @@ fn test_update_accounts_hash(bencher: &mut Bencher) { let mut pubkeys: Vec = vec![]; create_test_accounts(&accounts, &mut pubkeys, 50_000, 0); let ancestors = vec![(0, 0)].into_iter().collect(); - let cap_exempt = HashSet::new(); bencher.iter(|| { - accounts - .accounts_db - .update_accounts_hash(0, &ancestors, &cap_exempt); + accounts.accounts_db.update_accounts_hash(0, &ancestors); }); } diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 28a29715bd06d4..6d6d3b5f34d45b 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -428,20 +428,26 @@ impl Accounts { accounts_balances } + pub fn calculate_capitalization(&self, ancestors: &Ancestors) -> u64 { + self.load_by_program(ancestors, None) + .into_iter() + .map(|(_pubkey, account)| { + AccountsDB::account_balance_for_capitalization(account.lamports, &account.owner) + }) + .sum() + } + #[must_use] pub fn verify_bank_hash_and_lamports( &self, slot: Slot, ancestors: &Ancestors, total_lamports: u64, - capitalization_exempt: &HashSet, ) -> bool { - if let Err(err) = self.accounts_db.verify_bank_hash_and_lamports( - slot, - ancestors, - total_lamports, - capitalization_exempt, - ) { + if let Err(err) = + self.accounts_db + .verify_bank_hash_and_lamports(slot, ancestors, total_lamports) + { warn!("verify_bank_hash failed: {:?}", err); false } else { diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index 7aaa0061a52e4d..514637237d529c 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -154,7 +154,7 @@ pub enum BankHashVerificationError { MismatchedAccountHash, MismatchedBankHash, MissingBankHash, - MismatchedTotalLamports, + MismatchedTotalLamports(u64, u64), } /// Persistent storage structure holding the accounts @@ -1649,11 +1649,24 @@ impl AccountsDB { res } + pub fn account_balance_for_capitalization(lamports: u64, owner: &Pubkey) -> u64 { + let is_specially_retained = + solana_sdk::native_loader::check_id(owner) || solana_sdk::sysvar::check_id(owner); + + if is_specially_retained { + // specially retained accounts are ensured to exist by + // always having a balance of 1 lamports, which is + // outside the capitalization calculation. + lamports - 1 + } else { + lamports + } + } + fn calculate_accounts_hash( &self, ancestors: &Ancestors, check_hash: bool, - capitalization_exempt: &HashSet, ) -> Result<(Hash, u64), BankHashVerificationError> { use BankHashVerificationError::*; let mut scan = Measure::start("scan"); @@ -1674,12 +1687,13 @@ impl AccountsDB { .and_then(|storage_map| storage_map.get(&account_info.store_id)) .and_then(|store| { let account = store.accounts.get_account(account_info.offset)?.0; - if !(solana_sdk::sysvar::check_id(&account.account_meta.owner) - || capitalization_exempt.contains(&pubkey)) - { - total_lamports - .fetch_add(account_info.lamports, Ordering::Relaxed); - } + total_lamports.fetch_add( + Self::account_balance_for_capitalization( + account_info.lamports, + &account.account_meta.owner, + ), + Ordering::Relaxed, + ); if check_hash { let hash = Self::hash_stored_account( @@ -1734,15 +1748,8 @@ impl AccountsDB { bank_hash_info.snapshot_hash } - pub fn update_accounts_hash( - &self, - slot: Slot, - ancestors: &Ancestors, - capitalization_exempt: &HashSet, - ) -> (Hash, u64) { - let (hash, total_lamports) = self - .calculate_accounts_hash(ancestors, false, capitalization_exempt) - .unwrap(); + pub fn update_accounts_hash(&self, slot: Slot, ancestors: &Ancestors) -> (Hash, u64) { + let (hash, total_lamports) = self.calculate_accounts_hash(ancestors, false).unwrap(); let mut bank_hashes = self.bank_hashes.write().unwrap(); let mut bank_hash_info = bank_hashes.get_mut(&slot).unwrap(); bank_hash_info.snapshot_hash = hash; @@ -1754,19 +1761,18 @@ impl AccountsDB { slot: Slot, ancestors: &Ancestors, total_lamports: u64, - capitalization_exempt: &HashSet, ) -> Result<(), BankHashVerificationError> { use BankHashVerificationError::*; let (calculated_hash, calculated_lamports) = - self.calculate_accounts_hash(ancestors, true, capitalization_exempt)?; + self.calculate_accounts_hash(ancestors, true)?; if calculated_lamports != total_lamports { warn!( "Mismatched total lamports: {} calculated: {}", total_lamports, calculated_lamports ); - return Err(MismatchedTotalLamports); + return Err(MismatchedTotalLamports(calculated_lamports, total_lamports)); } let bank_hashes = self.bank_hashes.read().unwrap(); @@ -3052,8 +3058,8 @@ pub mod tests { let ancestors = linear_ancestors(latest_slot); assert_eq!( - daccounts.update_accounts_hash(latest_slot, &ancestors, &HashSet::new()), - accounts.update_accounts_hash(latest_slot, &ancestors, &HashSet::new()) + daccounts.update_accounts_hash(latest_slot, &ancestors), + accounts.update_accounts_hash(latest_slot, &ancestors) ); } @@ -3186,13 +3192,13 @@ pub mod tests { let ancestors = linear_ancestors(current_slot); info!("ancestors: {:?}", ancestors); - let hash = accounts.update_accounts_hash(current_slot, &ancestors, &HashSet::new()); + let hash = accounts.update_accounts_hash(current_slot, &ancestors); accounts.clean_accounts(); accounts.process_dead_slots(None); assert_eq!( - accounts.update_accounts_hash(current_slot, &ancestors, &HashSet::new()), + accounts.update_accounts_hash(current_slot, &ancestors), hash ); @@ -3313,7 +3319,7 @@ pub mod tests { accounts.add_root(current_slot); accounts.print_accounts_stats("pre_f"); - accounts.update_accounts_hash(4, &HashMap::default(), &HashSet::new()); + accounts.update_accounts_hash(4, &HashMap::default()); let accounts = f(accounts, current_slot); @@ -3325,7 +3331,7 @@ pub mod tests { assert_load_account(&accounts, current_slot, dummy_pubkey, dummy_lamport); accounts - .verify_bank_hash_and_lamports(4, &HashMap::default(), 1222, &HashSet::new()) + .verify_bank_hash_and_lamports(4, &HashMap::default(), 1222) .unwrap(); } @@ -3723,20 +3729,20 @@ pub mod tests { db.store(some_slot, &[(&key, &account)]); db.add_root(some_slot); - db.update_accounts_hash(some_slot, &ancestors, &HashSet::new()); + db.update_accounts_hash(some_slot, &ancestors); assert_matches!( - db.verify_bank_hash_and_lamports(some_slot, &ancestors, 1, &HashSet::new()), + db.verify_bank_hash_and_lamports(some_slot, &ancestors, 1), Ok(_) ); assert_matches!( - db.verify_bank_hash_and_lamports(some_slot, &ancestors, 10, &HashSet::new()), - Err(MismatchedTotalLamports) + db.verify_bank_hash_and_lamports(some_slot, &ancestors, 10), + Err(MismatchedTotalLamports(_, _)) ); db.bank_hashes.write().unwrap().remove(&some_slot).unwrap(); assert_matches!( - db.verify_bank_hash_and_lamports(some_slot, &ancestors, 1, &HashSet::new()), + db.verify_bank_hash_and_lamports(some_slot, &ancestors, 1), Err(MissingBankHash) ); @@ -3751,7 +3757,7 @@ pub mod tests { .unwrap() .insert(some_slot, bank_hash_info); assert_matches!( - db.verify_bank_hash_and_lamports(some_slot, &ancestors, 1, &HashSet::new()), + db.verify_bank_hash_and_lamports(some_slot, &ancestors, 1), Err(MismatchedBankHash) ); } @@ -3769,9 +3775,9 @@ pub mod tests { .unwrap() .insert(some_slot, BankHashInfo::default()); db.add_root(some_slot); - db.update_accounts_hash(some_slot, &ancestors, &HashSet::new()); + db.update_accounts_hash(some_slot, &ancestors); assert_matches!( - db.verify_bank_hash_and_lamports(some_slot, &ancestors, 0, &HashSet::new()), + db.verify_bank_hash_and_lamports(some_slot, &ancestors, 0), Ok(_) ); } @@ -3796,7 +3802,7 @@ pub mod tests { db.store_with_hashes(some_slot, accounts, &[some_hash]); db.add_root(some_slot); assert_matches!( - db.verify_bank_hash_and_lamports(some_slot, &ancestors, 1, &HashSet::new()), + db.verify_bank_hash_and_lamports(some_slot, &ancestors, 1), Err(MismatchedAccountHash) ); } @@ -4214,14 +4220,14 @@ pub mod tests { ); let no_ancestors = HashMap::default(); - accounts.update_accounts_hash(current_slot, &no_ancestors, &HashSet::new()); + accounts.update_accounts_hash(current_slot, &no_ancestors); accounts - .verify_bank_hash_and_lamports(current_slot, &no_ancestors, 22300, &HashSet::new()) + .verify_bank_hash_and_lamports(current_slot, &no_ancestors, 22300) .unwrap(); let accounts = reconstruct_accounts_db_via_serialization(&accounts, current_slot); accounts - .verify_bank_hash_and_lamports(current_slot, &no_ancestors, 22300, &HashSet::new()) + .verify_bank_hash_and_lamports(current_slot, &no_ancestors, 22300) .unwrap(); // repeating should be no-op diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 4eeadf7c7e8ada..443ec52e360192 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -458,10 +458,6 @@ pub struct Bank { // this is temporary field only to remove rewards_pool entirely pub rewards_pool_pubkeys: Arc>, - - pub builtin_programs: Arc>>, - - pub genesis_cap_exempt_ids: Arc>>, } impl Default for BlockhashQueue { @@ -581,8 +577,6 @@ impl Bank { parent.lazy_rent_collection.load(Ordering::Relaxed), ), rewards_pool_pubkeys: parent.rewards_pool_pubkeys.clone(), - builtin_programs: parent.builtin_programs.clone(), - genesis_cap_exempt_ids: parent.genesis_cap_exempt_ids.clone(), }; datapoint_info!( @@ -685,8 +679,6 @@ impl Bank { cluster_type: Some(genesis_config.cluster_type), lazy_rent_collection: new(), rewards_pool_pubkeys: new(), - genesis_cap_exempt_ids: new(), - builtin_programs: new(), }; bank.finish_init(genesis_config); @@ -1194,7 +1186,6 @@ impl Bank { panic!("{} repeated in genesis config", pubkey); } self.store_account(pubkey, account); - self.genesis_cap_exempt_ids.write().unwrap().insert(*pubkey); } // highest staked node is the first collector @@ -1267,11 +1258,8 @@ impl Bank { // Add a bogus executable native account, which will be loaded and ignored. let account = native_loader::create_loadable_account(name); self.store_account(&program_id, &account); - self.genesis_cap_exempt_ids - .write() - .unwrap() - .insert(*program_id); } + debug!("Added native program {} under {:?}", name, program_id); } pub fn set_cross_program_support(&mut self, is_supported: bool) { @@ -2860,13 +2848,10 @@ impl Bank { /// snapshot. #[must_use] fn verify_bank_hash(&self) -> bool { - let mut capitalization_exempt = self.builtin_programs.read().unwrap().clone(); - capitalization_exempt.extend(self.genesis_cap_exempt_ids.read().unwrap().iter()); self.rc.accounts.verify_bank_hash_and_lamports( self.slot(), &self.ancestors, self.capitalization(), - &capitalization_exempt, ) } @@ -2897,22 +2882,7 @@ impl Bank { } pub fn calculate_capitalization(&self) -> u64 { - self.get_program_accounts(None) - .into_iter() - .map(|(_pubkey, account)| { - let is_specially_retained = solana_sdk::native_loader::check_id(&account.owner) - || solana_sdk::sysvar::check_id(&account.owner); - - if is_specially_retained { - // specially retained accounts are ensured to exist by - // always having a balance of 1 lamports, which is - // outside the capitalization calculation. - account.lamports - 1 - } else { - account.lamports - } - }) - .sum() + self.rc.accounts.calculate_capitalization(&self.ancestors) } /// Forcibly overwrites current capitalization by actually recalculating accounts' balances. @@ -2929,13 +2899,11 @@ impl Bank { } pub fn update_accounts_hash(&self) -> Hash { - let mut capitalization_exempt = self.builtin_programs.read().unwrap().clone(); - capitalization_exempt.extend(self.genesis_cap_exempt_ids.read().unwrap().iter()); - let (hash, total_lamports) = self.rc.accounts.accounts_db.update_accounts_hash( - self.slot(), - &self.ancestors, - &capitalization_exempt, - ); + let (hash, total_lamports) = self + .rc + .accounts + .accounts_db + .update_accounts_hash(self.slot(), &self.ancestors); assert_eq!(total_lamports, self.capitalization()); hash } @@ -3176,7 +3144,6 @@ impl Bank { debug!("Added builtin loader {} under {:?}", name, program_id); } } - self.builtin_programs.write().unwrap().insert(program_id); } pub fn compare_bank(&self, dbank: &Bank) { From 3d4ae83ebfb2d7bcfdf79b82351488b424509911 Mon Sep 17 00:00:00 2001 From: Ryo Onodera Date: Tue, 1 Sep 2020 11:51:51 +0900 Subject: [PATCH 03/11] Improve and add tests --- runtime/src/accounts.rs | 6 +- runtime/src/accounts_db.rs | 116 ++++++++++++++++++++++++++++++++++--- 2 files changed, 112 insertions(+), 10 deletions(-) diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 6d6d3b5f34d45b..4fa38dcc5a7eb9 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -432,7 +432,11 @@ impl Accounts { self.load_by_program(ancestors, None) .into_iter() .map(|(_pubkey, account)| { - AccountsDB::account_balance_for_capitalization(account.lamports, &account.owner) + AccountsDB::account_balance_for_capitalization( + account.lamports, + &account.owner, + account.executable, + ) }) .sum() } diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index 514637237d529c..63ce13135e1e90 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -1649,9 +1649,13 @@ impl AccountsDB { res } - pub fn account_balance_for_capitalization(lamports: u64, owner: &Pubkey) -> u64 { - let is_specially_retained = - solana_sdk::native_loader::check_id(owner) || solana_sdk::sysvar::check_id(owner); + pub fn account_balance_for_capitalization( + lamports: u64, + owner: &Pubkey, + executable: bool, + ) -> u64 { + let is_specially_retained = (solana_sdk::native_loader::check_id(owner) && executable) + || solana_sdk::sysvar::check_id(owner); if is_specially_retained { // specially retained accounts are ensured to exist by @@ -1691,6 +1695,7 @@ impl AccountsDB { Self::account_balance_for_capitalization( account_info.lamports, &account.account_meta.owner, + account.account_meta.executable, ), Ordering::Relaxed, ); @@ -3716,7 +3721,7 @@ pub mod tests { } #[test] - fn test_verify_bank_hash1() { + fn test_verify_bank_hash() { use BankHashVerificationError::*; solana_logger::setup(); let db = AccountsDB::new(Vec::new(), &ClusterType::Development); @@ -3735,11 +3740,6 @@ pub mod tests { Ok(_) ); - assert_matches!( - db.verify_bank_hash_and_lamports(some_slot, &ancestors, 10), - Err(MismatchedTotalLamports(_, _)) - ); - db.bank_hashes.write().unwrap().remove(&some_slot).unwrap(); assert_matches!( db.verify_bank_hash_and_lamports(some_slot, &ancestors, 1), @@ -3762,6 +3762,46 @@ pub mod tests { ); } + #[test] + fn test_verify_bank_capitalization() { + use BankHashVerificationError::*; + solana_logger::setup(); + let db = AccountsDB::new(Vec::new()); + + let key = Pubkey::new_rand(); + 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); + db.update_accounts_hash(some_slot, &ancestors); + assert_matches!( + db.verify_bank_hash_and_lamports(some_slot, &ancestors, 1), + Ok(_) + ); + + let native_account_pubkey = Pubkey::new_rand(); + db.store( + some_slot, + &[( + &native_account_pubkey, + &solana_sdk::native_loader::create_loadable_account("foo"), + )], + ); + db.update_accounts_hash(some_slot, &ancestors); + assert_matches!( + db.verify_bank_hash_and_lamports(some_slot, &ancestors, 1), + Ok(_) + ); + + assert_matches!( + db.verify_bank_hash_and_lamports(some_slot, &ancestors, 10), + Err(MismatchedTotalLamports(expected, actual)) if expected == 1 && actual == 10 + ); + } + #[test] fn test_verify_bank_hash_no_account() { solana_logger::setup(); @@ -4414,4 +4454,62 @@ pub mod tests { shrink_thread.join().unwrap(); } } + + #[test] + fn test_account_balance_for_capitalization_normal() { + // system accounts + assert_eq!( + AccountsDB::account_balance_for_capitalization(10, &Pubkey::default(), false), + 10 + ); + // any random program data accounts + assert_eq!( + AccountsDB::account_balance_for_capitalization(10, &Pubkey::new_rand(), false), + 10 + ); + } + + #[test] + fn test_account_balance_for_capitalization_sysvar() { + use solana_sdk::sysvar::Sysvar; + + let normal_sysvar = solana_sdk::slot_history::SlotHistory::default().create_account(1); + assert_eq!( + AccountsDB::account_balance_for_capitalization( + normal_sysvar.lamports, + &normal_sysvar.owner, + normal_sysvar.executable + ), + 0 + ); + + // currently transactions can send any lamports to sysvars although this is not sensible. + assert_eq!( + AccountsDB::account_balance_for_capitalization(10, &solana_sdk::sysvar::id(), false), + 9 + ); + } + + #[test] + fn test_account_balance_for_capitalization_native_program() { + let normal_native_program = solana_sdk::native_loader::create_loadable_account("foo"); + assert_eq!( + AccountsDB::account_balance_for_capitalization( + normal_native_program.lamports, + &normal_native_program.owner, + normal_native_program.executable + ), + 0 + ); + + // test maliciously assigned bogus native loader account + assert_eq!( + AccountsDB::account_balance_for_capitalization( + 1, + &solana_sdk::native_loader::id(), + false + ), + 1 + ) + } } From 5fd884681a0cb72f2a57d16e63069d96030da19c Mon Sep 17 00:00:00 2001 From: Ryo Onodera Date: Tue, 1 Sep 2020 13:01:18 +0900 Subject: [PATCH 04/11] Avoid overflow and inhibit automatic restart --- ledger/src/blockstore_processor.rs | 6 +++ runtime/src/accounts.rs | 2 +- runtime/src/accounts_db.rs | 68 ++++++++++++++++++++---------- runtime/src/bank.rs | 4 ++ sdk/src/genesis_config.rs | 8 ++-- 5 files changed, 61 insertions(+), 27 deletions(-) diff --git a/ledger/src/blockstore_processor.rs b/ledger/src/blockstore_processor.rs index 4005edd2b56bcd..73ce294246c21a 100644 --- a/ledger/src/blockstore_processor.rs +++ b/ledger/src/blockstore_processor.rs @@ -293,6 +293,9 @@ pub enum BlockstoreProcessorError { #[error("invalid hard fork")] InvalidHardFork(Slot), + + #[error("invalid root bank with bad account hash/capitalization at {0}")] + InvalidRootBank(Slot), } /// Callback for accessing bank state while processing the blockstore @@ -480,6 +483,9 @@ fn do_process_blockstore_from_root( }, ); assert!(bank_forks.active_banks().is_empty()); + if !bank_forks.root_bank().verify_bank() { + return Err(BlockstoreProcessorError::InvalidRootBank(root)); + } Ok((bank_forks, leader_schedule_cache)) } diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 4fa38dcc5a7eb9..9ffcc175288d91 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -438,7 +438,7 @@ impl Accounts { account.executable, ) }) - .sum() + .fold(0, |a, b| a.checked_add(b).unwrap()) } #[must_use] diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index 63ce13135e1e90..22a9e623f38135 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -1607,8 +1607,11 @@ impl AccountsDB { ); } - pub fn compute_merkle_root(hashes: Vec<(Pubkey, Hash)>, fanout: usize) -> Hash { - let hashes: Vec<_> = hashes.into_iter().map(|(_pubkey, hash)| hash).collect(); + pub fn compute_merkle_root(hashes: Vec<(Pubkey, Hash, u64)>, fanout: usize) -> Hash { + let hashes: Vec<_> = hashes + .into_iter() + .map(|(_pubkey, hash, _lamports)| hash) + .collect(); let mut hashes: Vec<_> = hashes.chunks(fanout).map(|x| x.to_vec()).collect(); while hashes.len() > 1 { let mut time = Measure::start("time"); @@ -1633,20 +1636,44 @@ impl AccountsDB { hasher.result() } - fn accumulate_account_hashes(mut hashes: Vec<(Pubkey, Hash)>) -> Hash { - let mut sort = Measure::start("sort"); + fn accumulate_account_hashes(hashes: Vec<(Pubkey, Hash, u64)>) -> Hash { + let (hash, ..) = Self::do_accumulate_account_hashes_and_capitalization(hashes, false); + hash + } + + fn accumulate_account_hashes_and_capitalization( + hashes: Vec<(Pubkey, Hash, u64)>, + ) -> (Hash, u64) { + let (hash, cap) = Self::do_accumulate_account_hashes_and_capitalization(hashes, true); + (hash, cap.unwrap()) + } + + fn do_accumulate_account_hashes_and_capitalization( + mut hashes: Vec<(Pubkey, Hash, u64)>, + calculate_cap: bool, + ) -> (Hash, Option) { + let mut sort_time = Measure::start("sort"); hashes.par_sort_by(|a, b| a.0.cmp(&b.0)); - sort.stop(); - let mut hash_time = Measure::start("hash"); + sort_time.stop(); - let fanout = 16; + let mut sum_time = Measure::start("cap"); + let cap = if calculate_cap { + Some(hashes.iter().fold(0, |acuum: u64, (_, _, lamports)| { + acuum.checked_add(*lamports).unwrap() + })) + } else { + None + }; + sum_time.stop(); + let mut hash_time = Measure::start("hash"); + let fanout = 16; let res = Self::compute_merkle_root(hashes, fanout); - hash_time.stop(); - debug!("{} {}", sort, hash_time); - res + debug!("{} {} {}", sort_time, hash_time, sum_time); + + (res, cap) } pub fn account_balance_for_capitalization( @@ -1678,7 +1705,6 @@ impl AccountsDB { let storage = self.storage.read().unwrap(); let keys: Vec<_> = accounts_index.account_maps.keys().collect(); let mismatch_found = AtomicU64::new(0); - let total_lamports = AtomicU64::new(0); let hashes: Vec<_> = keys .par_iter() .filter_map(|pubkey| { @@ -1691,13 +1717,10 @@ impl AccountsDB { .and_then(|storage_map| storage_map.get(&account_info.store_id)) .and_then(|store| { let account = store.accounts.get_account(account_info.offset)?.0; - total_lamports.fetch_add( - Self::account_balance_for_capitalization( - account_info.lamports, - &account.account_meta.owner, - account.account_meta.executable, - ), - Ordering::Relaxed, + let balance = Self::account_balance_for_capitalization( + account_info.lamports, + &account.account_meta.owner, + account.account_meta.executable, ); if check_hash { @@ -1714,7 +1737,7 @@ impl AccountsDB { } } - Some((**pubkey, *account.hash)) + Some((**pubkey, *account.hash, balance)) }) } else { None @@ -1736,7 +1759,8 @@ impl AccountsDB { let hash_total = hashes.len(); let mut accumulate = Measure::start("accumulate"); - let accumulated_hash = Self::accumulate_account_hashes(hashes); + let (accumulated_hash, total_lamports) = + Self::accumulate_account_hashes_and_capitalization(hashes); accumulate.stop(); datapoint_info!( "update_accounts_hash", @@ -1744,7 +1768,7 @@ impl AccountsDB { ("hash_accumulate", accumulate.as_us(), i64), ("hash_total", hash_total, i64), ); - Ok((accumulated_hash, total_lamports.load(Ordering::Relaxed))) + Ok((accumulated_hash, total_lamports)) } pub fn get_accounts_hash(&self, slot: Slot) -> Hash { @@ -1819,7 +1843,7 @@ impl AccountsDB { let mut accumulate = Measure::start("accumulate"); let hashes: Vec<_> = account_maps .into_iter() - .map(|(pubkey, (_, hash))| (pubkey, hash)) + .map(|(pubkey, (_, hash))| (pubkey, hash, 0)) .collect(); let ret = Self::accumulate_account_hashes(hashes); accumulate.stop(); diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 443ec52e360192..ae19f16b497f4c 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -2913,6 +2913,10 @@ impl Bank { pub fn verify_snapshot_bank(&self) -> bool { self.clean_accounts(); self.shrink_all_slots(); + self.verify_bank() + } + + pub fn verify_bank(&self) -> bool { // Order and short-circuiting is significant; verify_hash requires a valid bank hash self.verify_bank_hash() && self.verify_hash() } diff --git a/sdk/src/genesis_config.rs b/sdk/src/genesis_config.rs index 9478cac8c36ea4..3953028ce11cfd 100644 --- a/sdk/src/genesis_config.rs +++ b/sdk/src/genesis_config.rs @@ -202,10 +202,6 @@ impl GenesisConfig { self.native_instruction_processors.push((name, program_id)); } - pub fn add_rewards_pool(&mut self, pubkey: Pubkey, account: Account) { - self.rewards_pools.insert(pubkey, account); - } - pub fn hashes_per_tick(&self) -> Option { self.poh_config.hashes_per_tick } @@ -244,6 +240,8 @@ impl fmt::Display for GenesisConfig { {:?}\n\ {:?}\n\ Capitalization: {} SOL in {} accounts\n\ + Native instruction processors: {:#?}\n\ + Rewards pool: {:#?}\n\ ", Utc.timestamp(self.creation_time, 0).to_rfc3339(), self.cluster_type, @@ -272,6 +270,8 @@ impl fmt::Display for GenesisConfig { .sum::() ), self.accounts.len(), + self.native_instruction_processors, + self.rewards_pools, ) } } From e421e51637b26defc7bdfcb7396a24045f10a1e6 Mon Sep 17 00:00:00 2001 From: Ryo Onodera Date: Wed, 2 Sep 2020 01:39:08 +0900 Subject: [PATCH 05/11] Fix test --- ledger-tool/src/main.rs | 13 ++----------- ledger/src/blockstore_processor.rs | 12 ++++++++---- runtime/src/bank.rs | 18 ++++++++++++++---- 3 files changed, 24 insertions(+), 19 deletions(-) diff --git a/ledger-tool/src/main.rs b/ledger-tool/src/main.rs index 3ad267ec04125a..3ceaad642e7e22 100644 --- a/ledger-tool/src/main.rs +++ b/ledger-tool/src/main.rs @@ -44,7 +44,7 @@ use solana_vote_program::{ }; use std::{ collections::{BTreeMap, BTreeSet, HashMap, HashSet}, - convert::{TryFrom, TryInto}, + convert::TryInto, ffi::OsStr, fs::{self, File}, io::{self, stdout, BufRead, BufReader, Write}, @@ -708,16 +708,7 @@ fn open_genesis_config_by(ledger_path: &Path, matches: &ArgMatches<'_>) -> Genes } fn assert_capitalization(bank: &Bank) { - let calculated_capitalization = bank.calculate_capitalization(); - assert_eq!( - bank.capitalization(), - calculated_capitalization, - "Capitalization mismatch!?: +/-{}", - Sol(u64::try_from( - (i128::from(calculated_capitalization) - i128::from(bank.capitalization())).abs() - ) - .unwrap()), - ); + assert!(bank.calculate_and_verify_capitalization()); } #[allow(clippy::cognitive_complexity)] diff --git a/ledger/src/blockstore_processor.rs b/ledger/src/blockstore_processor.rs index 73ce294246c21a..b04696886b238d 100644 --- a/ledger/src/blockstore_processor.rs +++ b/ledger/src/blockstore_processor.rs @@ -294,8 +294,8 @@ pub enum BlockstoreProcessorError { #[error("invalid hard fork")] InvalidHardFork(Slot), - #[error("invalid root bank with bad account hash/capitalization at {0}")] - InvalidRootBank(Slot), + #[error("root bank with mismatched capitalization at {0}")] + RootBankWithMismatchedCapitalization(Slot), } /// Callback for accessing bank state while processing the blockstore @@ -483,8 +483,12 @@ fn do_process_blockstore_from_root( }, ); assert!(bank_forks.active_banks().is_empty()); - if !bank_forks.root_bank().verify_bank() { - return Err(BlockstoreProcessorError::InvalidRootBank(root)); + + // We might be promptly restarted after bad capitalization was detected while creating newer snapshot. + // In that case, we're most likely restored from the last good snapshot and replayed up to this root. + // So again check here for the bad capitalization to avoid to continue until the next snapshot creation. + if !bank_forks.root_bank().calculate_and_verify_capitalization() { + return Err(BlockstoreProcessorError::RootBankWithMismatchedCapitalization(root)); } Ok((bank_forks, leader_schedule_cache)) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index ae19f16b497f4c..5c4869d8d1c7a9 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -2885,6 +2885,20 @@ impl Bank { self.rc.accounts.calculate_capitalization(&self.ancestors) } + pub fn calculate_and_verify_capitalization(&self) -> bool { + let calculated = self.calculate_capitalization(); + let expected = self.capitalization(); + if calculated == expected { + true + } else { + warn!( + "Capitalization mismatch: calculated: {} != expected: {}", + calculated, expected + ); + false + } + } + /// Forcibly overwrites current capitalization by actually recalculating accounts' balances. /// This should only be used for developing purposes. pub fn set_capitalization(&self) -> u64 { @@ -2913,10 +2927,6 @@ impl Bank { pub fn verify_snapshot_bank(&self) -> bool { self.clean_accounts(); self.shrink_all_slots(); - self.verify_bank() - } - - pub fn verify_bank(&self) -> bool { // Order and short-circuiting is significant; verify_hash requires a valid bank hash self.verify_bank_hash() && self.verify_hash() } From 1d8714c7d9468d30e1e4eda38cb399976f13999d Mon Sep 17 00:00:00 2001 From: Ryo Onodera Date: Sun, 6 Sep 2020 14:49:44 +0900 Subject: [PATCH 06/11] Tweak checked sum for cap. and add tests --- Cargo.lock | 1 + accounts-bench/Cargo.toml | 1 + accounts-bench/src/main.rs | 12 +++++++----- runtime/src/accounts.rs | 22 ++++++++++++---------- runtime/src/accounts_db.rs | 32 +++++++++++++++++++++++++++++--- 5 files changed, 50 insertions(+), 18 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1ff2317994243d..ad7eeef6067e2f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3272,6 +3272,7 @@ dependencies = [ "solana-measure", "solana-runtime", "solana-sdk 1.4.0", + "solana-version", ] [[package]] diff --git a/accounts-bench/Cargo.toml b/accounts-bench/Cargo.toml index 4cf0408acfbbe2..b4f46845b30195 100644 --- a/accounts-bench/Cargo.toml +++ b/accounts-bench/Cargo.toml @@ -14,6 +14,7 @@ solana-logger = { path = "../logger", version = "1.4.0" } solana-runtime = { path = "../runtime", version = "1.4.0" } solana-measure = { path = "../measure", version = "1.4.0" } solana-sdk = { path = "../sdk", version = "1.4.0" } +solana-version = { path = "../version", version = "1.4.0" } rand = "0.7.0" clap = "2.33.1" crossbeam-channel = "0.4" diff --git a/accounts-bench/src/main.rs b/accounts-bench/src/main.rs index e2a0d0a7e556dc..30ded428254e78 100644 --- a/accounts-bench/src/main.rs +++ b/accounts-bench/src/main.rs @@ -1,4 +1,4 @@ -use clap::{value_t, App, Arg}; +use clap::{crate_description, crate_name, value_t, App, Arg}; use rayon::prelude::*; use solana_measure::measure::Measure; use solana_runtime::{ @@ -6,15 +6,16 @@ use solana_runtime::{ accounts_index::Ancestors, }; use solana_sdk::{genesis_config::ClusterType, pubkey::Pubkey}; +use std::env; use std::fs; use std::path::PathBuf; fn main() { solana_logger::setup(); - let matches = App::new("crate") - .about("about") - .version("version") + let matches = App::new(crate_name!()) + .about(crate_description!()) + .version(solana_version::version!()) .arg( Arg::with_name("num_slots") .long("num_slots") @@ -50,7 +51,8 @@ fn main() { let clean = matches.is_present("clean"); println!("clean: {:?}", clean); - let path = PathBuf::from("farf/accounts-bench"); + let path = PathBuf::from(env::var("FARF_DIR").unwrap_or_else(|_| "farf".to_owned())) + .join("accounts-bench"); if fs::remove_dir_all(path.clone()).is_err() { println!("Warning: Couldn't remove {:?}", path); } diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 9ffcc175288d91..21a96dc258f0f7 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -429,16 +429,18 @@ impl Accounts { } pub fn calculate_capitalization(&self, ancestors: &Ancestors) -> u64 { - self.load_by_program(ancestors, None) - .into_iter() - .map(|(_pubkey, account)| { - AccountsDB::account_balance_for_capitalization( - account.lamports, - &account.owner, - account.executable, - ) - }) - .fold(0, |a, b| a.checked_add(b).unwrap()) + let balances = + self.load_by_program(ancestors, None) + .into_iter() + .map(|(_pubkey, account)| { + AccountsDB::account_balance_for_capitalization( + account.lamports, + &account.owner, + account.executable, + ) + }); + + AccountsDB::checked_sum_for_capitalization(balances) } #[must_use] diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index 22a9e623f38135..269837a2b7674b 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -40,6 +40,7 @@ use solana_sdk::{ use std::convert::TryFrom; use std::{ collections::{HashMap, HashSet}, + convert::TryInto, io::{Error as IOError, Result as IOResult}, iter::FromIterator, ops::RangeBounds, @@ -1658,9 +1659,9 @@ impl AccountsDB { let mut sum_time = Measure::start("cap"); let cap = if calculate_cap { - Some(hashes.iter().fold(0, |acuum: u64, (_, _, lamports)| { - acuum.checked_add(*lamports).unwrap() - })) + Some(Self::checked_sum_for_capitalization( + hashes.iter().map(|(_, _, lamports)| *lamports), + )) } else { None }; @@ -1676,6 +1677,14 @@ impl AccountsDB { (res, cap) } + pub fn checked_sum_for_capitalization>(balances: T) -> u64 { + balances + .map(|b| b as u128) + .sum::() + .try_into() + .expect("overflow is detected while summing capitalization") + } + pub fn account_balance_for_capitalization( lamports: u64, owner: &Pubkey, @@ -4536,4 +4545,21 @@ pub mod tests { 1 ) } + + #[test] + fn test_checked_sum_for_capitalization_normal() { + assert_eq!( + AccountsDB::checked_sum_for_capitalization(vec![1, 2].into_iter()), + 3 + ); + } + + #[test] + #[should_panic(expected = "overflow is detected while summing capitalization")] + fn test_checked_sum_for_capitalization_overflow() { + assert_eq!( + AccountsDB::checked_sum_for_capitalization(vec![1, u64::max_value()].into_iter()), + 3 + ); + } } From df155134ef074687e1844dd314f891a6fcd6b1ff Mon Sep 17 00:00:00 2001 From: Ryo Onodera Date: Mon, 7 Sep 2020 18:32:15 +0900 Subject: [PATCH 07/11] Fix broken build after merge conflicts.. --- runtime/src/accounts_db.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index 269837a2b7674b..4b348cf03b60a9 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -3799,7 +3799,7 @@ pub mod tests { fn test_verify_bank_capitalization() { use BankHashVerificationError::*; solana_logger::setup(); - let db = AccountsDB::new(Vec::new()); + let db = AccountsDB::new(Vec::new(), &OperatingMode::Development); let key = Pubkey::new_rand(); let some_data_len = 0; From 4ba5a8c5b86393ef5a63d808ddd278fb3bcdcfc9 Mon Sep 17 00:00:00 2001 From: Ryo Onodera Date: Wed, 9 Sep 2020 00:43:32 +0900 Subject: [PATCH 08/11] Rename to ClusterType --- runtime/src/accounts_db.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index 4b348cf03b60a9..1adea81b74eae4 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -3799,7 +3799,7 @@ pub mod tests { fn test_verify_bank_capitalization() { use BankHashVerificationError::*; solana_logger::setup(); - let db = AccountsDB::new(Vec::new(), &OperatingMode::Development); + let db = AccountsDB::new(Vec::new(), &ClusterType::Development); let key = Pubkey::new_rand(); let some_data_len = 0; From 21d6864ccf395ff86c510df2823277078d6dc20c Mon Sep 17 00:00:00 2001 From: Ryo Onodera Date: Fri, 11 Sep 2020 17:51:02 +0900 Subject: [PATCH 09/11] Rename confusing method --- core/src/non_circulating_supply.rs | 2 +- core/src/rpc.rs | 2 +- ledger-tool/src/main.rs | 4 ++-- runtime/src/accounts.rs | 24 ++++++++++++------------ runtime/src/bank.rs | 16 ++++++++-------- 5 files changed, 24 insertions(+), 24 deletions(-) diff --git a/core/src/non_circulating_supply.rs b/core/src/non_circulating_supply.rs index 68477dd7cf01e9..cff6dea5ab653a 100644 --- a/core/src/non_circulating_supply.rs +++ b/core/src/non_circulating_supply.rs @@ -18,7 +18,7 @@ pub fn calculate_non_circulating_supply(bank: &Arc) -> NonCirculatingSuppl let withdraw_authority_list = withdraw_authority(); let clock = bank.clock(); - let stake_accounts = bank.get_program_accounts(Some(&solana_stake_program::id())); + let stake_accounts = bank.get_program_accounts(&solana_stake_program::id()); for (pubkey, account) in stake_accounts.iter() { let stake_account = StakeState::from(&account).unwrap_or_default(); match stake_account { diff --git a/core/src/rpc.rs b/core/src/rpc.rs index c62cd533d95b4e..d696aa4efdf93e 100644 --- a/core/src/rpc.rs +++ b/core/src/rpc.rs @@ -1302,7 +1302,7 @@ fn get_filtered_program_accounts( program_id: &Pubkey, filters: Vec, ) -> impl Iterator { - bank.get_program_accounts(Some(&program_id)) + bank.get_program_accounts(&program_id) .into_iter() .filter(move |(_, account)| { filters.iter().all(|filter_type| match filter_type { diff --git a/ledger-tool/src/main.rs b/ledger-tool/src/main.rs index 3ceaad642e7e22..edcd681792a841 100644 --- a/ledger-tool/src/main.rs +++ b/ledger-tool/src/main.rs @@ -1686,7 +1686,7 @@ fn main() { if remove_stake_accounts { for (address, mut account) in bank - .get_program_accounts(Some(&solana_stake_program::id())) + .get_program_accounts(&solana_stake_program::id()) .into_iter() { account.lamports = 0; @@ -1714,7 +1714,7 @@ fn main() { // Delete existing vote accounts for (address, mut account) in bank - .get_program_accounts(Some(&solana_vote_program::id())) + .get_program_accounts(&solana_vote_program::id()) .into_iter() { account.lamports = 0; diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 21a96dc258f0f7..7f4226517fe37d 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -429,16 +429,16 @@ impl Accounts { } pub fn calculate_capitalization(&self, ancestors: &Ancestors) -> u64 { - let balances = - self.load_by_program(ancestors, None) - .into_iter() - .map(|(_pubkey, account)| { - AccountsDB::account_balance_for_capitalization( - account.lamports, - &account.owner, - account.executable, - ) - }); + let balances = self + .load_all(ancestors) + .into_iter() + .map(|(_pubkey, account, _slot)| { + AccountsDB::account_balance_for_capitalization( + account.lamports, + &account.owner, + account.executable, + ) + }); AccountsDB::checked_sum_for_capitalization(balances) } @@ -483,13 +483,13 @@ impl Accounts { pub fn load_by_program( &self, ancestors: &Ancestors, - program_id: Option<&Pubkey>, + program_id: &Pubkey, ) -> Vec<(Pubkey, Account)> { self.accounts_db.scan_accounts( ancestors, |collector: &mut Vec<(Pubkey, Account)>, some_account_tuple| { Self::load_while_filtering(collector, some_account_tuple, |account| { - program_id.is_none() || Some(&account.owner) == program_id + account.owner == *program_id }) }, ) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 5c4869d8d1c7a9..2e8f0314dc4ba7 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -2714,7 +2714,7 @@ impl Bank { .map(|(acc, _slot)| acc) } - pub fn get_program_accounts(&self, program_id: Option<&Pubkey>) -> Vec<(Pubkey, Account)> { + pub fn get_program_accounts(&self, program_id: &Pubkey) -> Vec<(Pubkey, Account)> { self.rc .accounts .load_by_program(&self.ancestors, program_id) @@ -6839,17 +6839,17 @@ mod tests { let parent = Arc::new(Bank::new(&genesis_config)); parent.lazy_rent_collection.store(true, Ordering::Relaxed); - let genesis_accounts: Vec<_> = parent.get_program_accounts(None); + let genesis_accounts: Vec<_> = parent.get_all_accounts_with_modified_slots(); assert!( genesis_accounts .iter() - .any(|(pubkey, _)| *pubkey == mint_keypair.pubkey()), + .any(|(pubkey, _, _)| *pubkey == mint_keypair.pubkey()), "mint pubkey not found" ); assert!( genesis_accounts .iter() - .any(|(pubkey, _)| solana_sdk::sysvar::is_sysvar_id(pubkey)), + .any(|(pubkey, _, _)| solana_sdk::sysvar::is_sysvar_id(pubkey)), "no sysvars found" ); @@ -6867,11 +6867,11 @@ mod tests { let bank1 = Arc::new(new_from_parent(&bank0)); bank1.squash(); assert_eq!( - bank0.get_program_accounts(Some(&program_id)), + bank0.get_program_accounts(&program_id), vec![(pubkey0, account0.clone())] ); assert_eq!( - bank1.get_program_accounts(Some(&program_id)), + bank1.get_program_accounts(&program_id), vec![(pubkey0, account0)] ); assert_eq!( @@ -6890,8 +6890,8 @@ mod tests { let bank3 = Arc::new(new_from_parent(&bank2)); bank3.squash(); - assert_eq!(bank1.get_program_accounts(Some(&program_id)).len(), 2); - assert_eq!(bank3.get_program_accounts(Some(&program_id)).len(), 2); + assert_eq!(bank1.get_program_accounts(&program_id).len(), 2); + assert_eq!(bank3.get_program_accounts(&program_id).len(), 2); } #[test] From 22c1f2c9ee90410e8ffef6960dcee15de28258a6 Mon Sep 17 00:00:00 2001 From: Ryo Onodera Date: Fri, 11 Sep 2020 18:37:11 +0900 Subject: [PATCH 10/11] Clarify comment --- runtime/src/accounts_db.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index 1adea81b74eae4..5cbccdaf340bd8 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -1694,9 +1694,9 @@ impl AccountsDB { || solana_sdk::sysvar::check_id(owner); if is_specially_retained { - // specially retained accounts are ensured to exist by - // always having a balance of 1 lamports, which is - // outside the capitalization calculation. + // specially retained accounts always have an initial 1 lamport + // balance, but could be modified by transfers which increase + // the balance but don't affect the capitalization. lamports - 1 } else { lamports From 95cb371ecd6ba8dfb0527ee4e828081c1d0e937f Mon Sep 17 00:00:00 2001 From: Ryo Onodera Date: Fri, 11 Sep 2020 18:37:26 +0900 Subject: [PATCH 11/11] Verify cap. in rent and inflation tests --- runtime/src/bank.rs | 52 +++++++++++++++++++++++++++++++++++++-------- 1 file changed, 43 insertions(+), 9 deletions(-) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 2e8f0314dc4ba7..8252f90908447b 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -2600,6 +2600,28 @@ impl Bank { } } + #[cfg(test)] + fn add_account_and_update_capitalization(&self, pubkey: &Pubkey, new_account: &Account) { + if let Some(old_account) = self.get_account(&pubkey) { + if new_account.lamports > old_account.lamports { + self.capitalization.fetch_add( + new_account.lamports - old_account.lamports, + Ordering::Relaxed, + ); + } else { + self.capitalization.fetch_sub( + old_account.lamports - new_account.lamports, + Ordering::Relaxed, + ); + } + } else { + self.capitalization + .fetch_add(new_account.lamports, Ordering::Relaxed); + } + + self.store_account(pubkey, new_account); + } + pub fn withdraw(&self, pubkey: &Pubkey, lamports: u64) -> Result<()> { match self.get_account(pubkey) { Some(mut account) => { @@ -3879,6 +3901,8 @@ mod tests { #[test] fn test_rent_distribution() { + solana_logger::setup(); + let bootstrap_validator_pubkey = Pubkey::new_rand(); let bootstrap_validator_stake_lamports = 30; let mut genesis_config = create_genesis_config_with_leader( @@ -4014,11 +4038,11 @@ mod tests { let payer = Keypair::new(); let payer_account = Account::new(400, 0, &system_program::id()); - bank.store_account(&payer.pubkey(), &payer_account); + bank.add_account_and_update_capitalization(&payer.pubkey(), &payer_account); let payee = Keypair::new(); let payee_account = Account::new(70, 1, &system_program::id()); - bank.store_account(&payee.pubkey(), &payee_account); + bank.add_account_and_update_capitalization(&payee.pubkey(), &payee_account); let bootstrap_validator_initial_balance = bank.get_balance(&bootstrap_validator_pubkey); @@ -4099,6 +4123,8 @@ mod tests { previous_capitalization - current_capitalization, burned_portion ); + bank.freeze(); + assert!(bank.calculate_and_verify_capitalization()); } #[test] @@ -5047,6 +5073,8 @@ mod tests { #[test] fn test_bank_update_rewards() { + solana_logger::setup(); + // create a bank that ticks really slowly... let bank = Arc::new(Bank::new(&GenesisConfig { accounts: (0..42) @@ -5083,7 +5111,7 @@ mod tests { crate::stakes::tests::create_staked_node_accounts(1_0000); // set up accounts - bank.store_account(&stake_id, &stake_account); + bank.add_account_and_update_capitalization(&stake_id, &stake_account); // generate some rewards let mut vote_state = Some(VoteState::from(&vote_account).unwrap()); @@ -5093,7 +5121,7 @@ mod tests { } let versioned = VoteStateVersions::Current(Box::new(vote_state.take().unwrap())); VoteState::to(&versioned, &mut vote_account).unwrap(); - bank.store_account(&vote_id, &vote_account); + bank.add_account_and_update_capitalization(&vote_id, &vote_account); match versioned { VoteStateVersions::Current(v) => { vote_state = Some(*v); @@ -5101,7 +5129,7 @@ mod tests { _ => panic!("Has to be of type Current"), }; } - bank.store_account(&vote_id, &vote_account); + bank.add_account_and_update_capitalization(&vote_id, &vote_account); let validator_points: u128 = bank .stake_delegation_accounts() @@ -5156,6 +5184,8 @@ mod tests { (rewards.validator_point_value * validator_points as f64) as i64 )]) ); + bank1.freeze(); + assert!(bank1.calculate_and_verify_capitalization()); } fn do_test_bank_update_rewards_determinism() -> u64 { @@ -5197,8 +5227,8 @@ mod tests { let (stake_id2, stake_account2) = crate::stakes::tests::create_stake_account(456, &vote_id); // set up accounts - bank.store_account(&stake_id1, &stake_account1); - bank.store_account(&stake_id2, &stake_account2); + bank.add_account_and_update_capitalization(&stake_id1, &stake_account1); + bank.add_account_and_update_capitalization(&stake_id2, &stake_account2); // generate some rewards let mut vote_state = Some(VoteState::from(&vote_account).unwrap()); @@ -5208,7 +5238,7 @@ mod tests { } let versioned = VoteStateVersions::Current(Box::new(vote_state.take().unwrap())); VoteState::to(&versioned, &mut vote_account).unwrap(); - bank.store_account(&vote_id, &vote_account); + bank.add_account_and_update_capitalization(&vote_id, &vote_account); match versioned { VoteStateVersions::Current(v) => { vote_state = Some(*v); @@ -5216,7 +5246,7 @@ mod tests { _ => panic!("Has to be of type Current"), }; } - bank.store_account(&vote_id, &vote_account); + bank.add_account_and_update_capitalization(&vote_id, &vote_account); // put a child bank in epoch 1, which calls update_rewards()... let bank1 = Bank::new_from_parent( @@ -5227,11 +5257,15 @@ mod tests { // verify that there's inflation assert_ne!(bank1.capitalization(), bank.capitalization()); + bank1.freeze(); + assert!(bank1.calculate_and_verify_capitalization()); bank1.capitalization() } #[test] fn test_bank_update_rewards_determinism() { + solana_logger::setup(); + // The same reward should be distributed given same credits let expected_capitalization = do_test_bank_update_rewards_determinism(); // Repeat somewhat large number of iterations to expose possible different behavior