From 5b9189a40c73286b561b108beab0144e4f6cc8bb Mon Sep 17 00:00:00 2001 From: Carl Lin Date: Mon, 24 May 2021 18:55:11 -0700 Subject: [PATCH 01/12] Add safe scan --- Cargo.lock | 1 + client/Cargo.toml | 1 + client/src/rpc_custom_error.rs | 42 ++++-- ledger-tool/src/main.rs | 3 + rpc/src/rpc.rs | 52 +++---- runtime/benches/accounts.rs | 4 +- runtime/src/accounts.rs | 203 +++++++++++++++++--------- runtime/src/accounts_db.rs | 111 +++++++++----- runtime/src/accounts_index.rs | 96 ++++++++++-- runtime/src/ancestors.rs | 4 + runtime/src/bank.rs | 89 ++++++----- runtime/src/non_circulating_supply.rs | 5 +- sdk/program/src/clock.rs | 4 + 13 files changed, 427 insertions(+), 188 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 83fc19c1202..ac964156941 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4476,6 +4476,7 @@ dependencies = [ "solana-faucet", "solana-logger 1.8.0", "solana-net-utils", + "solana-runtime", "solana-sdk", "solana-transaction-status", "solana-version", diff --git a/client/Cargo.toml b/client/Cargo.toml index 1ea43fb8377..b7a5b7da057 100644 --- a/client/Cargo.toml +++ b/client/Cargo.toml @@ -28,6 +28,7 @@ solana-account-decoder = { path = "../account-decoder", version = "=1.8.0" } solana-clap-utils = { path = "../clap-utils", version = "=1.8.0" } solana-faucet = { path = "../faucet", version = "=1.8.0" } solana-net-utils = { path = "../net-utils", version = "=1.8.0" } +solana-runtime = { path = "../runtime", version = "=1.8.0" } solana-sdk = { path = "../sdk", version = "=1.8.0" } solana-transaction-status = { path = "../transaction-status", version = "=1.8.0" } solana-version = { path = "../version", version = "=1.8.0" } diff --git a/client/src/rpc_custom_error.rs b/client/src/rpc_custom_error.rs index 400947dcd6e..cd2c5b070c2 100644 --- a/client/src/rpc_custom_error.rs +++ b/client/src/rpc_custom_error.rs @@ -1,5 +1,7 @@ //! Implementation defined RPC server errors +use solana_runtime::accounts_index::ScanError; +use thiserror::Error; use { crate::rpc_response::RpcSimulateTransactionResult, jsonrpc_core::{Error, ErrorCode}, @@ -17,35 +19,40 @@ pub const JSON_RPC_SERVER_ERROR_NO_SNAPSHOT: i64 = -32008; pub const JSON_RPC_SERVER_ERROR_LONG_TERM_STORAGE_SLOT_SKIPPED: i64 = -32009; pub const JSON_RPC_SERVER_ERROR_KEY_EXCLUDED_FROM_SECONDARY_INDEX: i64 = -32010; pub const JSON_RPC_SERVER_ERROR_TRANSACTION_HISTORY_NOT_AVAILABLE: i64 = -32011; +pub const JSON_RPC_REMOVED_SLOT: i64 = -32012; +#[derive(Error, Debug)] pub enum RpcCustomError { + #[error("BlockCleanedUp")] BlockCleanedUp { slot: Slot, first_available_block: Slot, }, + #[error("SendTransactionPreflightFailure")] SendTransactionPreflightFailure { message: String, result: RpcSimulateTransactionResult, }, + #[error("TransactionSignatureVerificationFailure")] TransactionSignatureVerificationFailure, - BlockNotAvailable { - slot: Slot, - }, - NodeUnhealthy { - num_slots_behind: Option, - }, + #[error("BlockNotAvailable")] + BlockNotAvailable { slot: Slot }, + #[error("NodeUnhealthy")] + NodeUnhealthy { num_slots_behind: Option }, + #[error("TransactionPrecompileVerificationFailure")] TransactionPrecompileVerificationFailure(solana_sdk::transaction::TransactionError), - SlotSkipped { - slot: Slot, - }, + #[error("SlotSkipped")] + SlotSkipped { slot: Slot }, + #[error("NoSnapshot")] NoSnapshot, - LongTermStorageSlotSkipped { - slot: Slot, - }, - KeyExcludedFromSecondaryIndex { - index_key: String, - }, + #[error("LongTermStorageSlotSkipped")] + LongTermStorageSlotSkipped { slot: Slot }, + #[error("KeyExcludedFromSecondaryIndex")] + KeyExcludedFromSecondaryIndex { index_key: String }, + #[error("TransactionHistoryNotAvailable")] TransactionHistoryNotAvailable, + #[error("ScanError")] + ScanError(#[from] ScanError), } #[derive(Debug, Serialize, Deserialize)] @@ -141,6 +148,11 @@ impl From for Error { message: "Transaction history is not available from this node".to_string(), data: None, }, + RpcCustomError::ScanError(scan_err) => Self { + code: ErrorCode::ServerError(JSON_RPC_REMOVED_SLOT), + message: scan_err.to_string(), + data: None, + }, } } } diff --git a/ledger-tool/src/main.rs b/ledger-tool/src/main.rs index 1084e690471..00bfb8795f9 100644 --- a/ledger-tool/src/main.rs +++ b/ledger-tool/src/main.rs @@ -2040,6 +2040,7 @@ fn main() { if remove_stake_accounts { for (address, mut account) in bank .get_program_accounts(&solana_stake_program::id()) + .unwrap() .into_iter() { account.set_lamports(0); @@ -2073,6 +2074,7 @@ fn main() { // Delete existing vote accounts for (address, mut account) in bank .get_program_accounts(&solana_vote_program::id()) + .unwrap() .into_iter() { account.set_lamports(0); @@ -2234,6 +2236,7 @@ fn main() { let accounts: BTreeMap<_, _> = bank .get_all_accounts_with_modified_slots() + .unwrap() .into_iter() .filter(|(pubkey, _account, _slot)| { include_sysvars || !solana_sdk::sysvar::is_sysvar_id(pubkey) diff --git a/rpc/src/rpc.rs b/rpc/src/rpc.rs index ed9822944f7..14769526f10 100644 --- a/rpc/src/rpc.rs +++ b/rpc/src/rpc.rs @@ -92,6 +92,8 @@ use { tokio::runtime::Runtime, }; +type RpcCustomResult = std::result::Result; + pub const MAX_REQUEST_PAYLOAD_SIZE: usize = 50 * (1 << 10); // 50kB pub const PERFORMANCE_SAMPLES_LIMIT: usize = 720; @@ -705,15 +707,15 @@ impl JsonRpcRequestProcessor { fn get_largest_accounts( &self, config: Option, - ) -> RpcResponse> { + ) -> RpcCustomResult>> { let config = config.unwrap_or_default(); let bank = self.bank(config.commitment); if let Some((slot, accounts)) = self.get_cached_largest_accounts(&config.filter) { - Response { + Ok(Response { context: RpcResponseContext { slot }, value: accounts, - } + }) } else { let (addresses, address_filter) = if let Some(filter) = config.clone().filter { let non_circulating_supply = calculate_non_circulating_supply(&bank); @@ -727,7 +729,7 @@ impl JsonRpcRequestProcessor { (HashSet::new(), AccountAddressFilter::Exclude) }; let accounts = bank - .get_largest_accounts(NUM_LARGEST_ACCOUNTS, &addresses, address_filter) + .get_largest_accounts(NUM_LARGEST_ACCOUNTS, &addresses, address_filter)? .into_iter() .map(|(address, lamports)| RpcAccountBalance { address: address.to_string(), @@ -736,7 +738,7 @@ impl JsonRpcRequestProcessor { .collect::>(); self.set_cached_largest_accounts(&config.filter, bank.slot(), &accounts); - new_response(&bank, accounts) + Ok(new_response(&bank, accounts)) } } @@ -1738,7 +1740,7 @@ impl JsonRpcRequestProcessor { bank: &Arc, program_id: &Pubkey, filters: Vec, - ) -> Result> { + ) -> RpcCustomResult> { let filter_closure = |account: &AccountSharedData| { filters.iter().all(|filter_type| match filter_type { RpcFilterType::DataSize(size) => account.data().len() as u64 == *size, @@ -1753,21 +1755,21 @@ impl JsonRpcRequestProcessor { if !self.config.account_indexes.include_key(program_id) { return Err(RpcCustomError::KeyExcludedFromSecondaryIndex { index_key: program_id.to_string(), - } - .into()); + }); } - Ok( - bank.get_filtered_indexed_accounts(&IndexKey::ProgramId(*program_id), |account| { + Ok(bank.get_filtered_indexed_accounts( + &IndexKey::ProgramId(*program_id), + |account| { // The program-id account index checks for Account owner on inclusion. However, due // to the current AccountsDb implementation, an account may remain in storage as a // zero-lamport AccountSharedData::Default() after being wiped and reinitialized in later // updates. We include the redundant filters here to avoid returning these // accounts. account.owner() == program_id && filter_closure(account) - }), - ) + }, + )?) } else { - Ok(bank.get_filtered_program_accounts(program_id, filter_closure)) + Ok(bank.get_filtered_program_accounts(program_id, filter_closure)?) } } @@ -1777,7 +1779,7 @@ impl JsonRpcRequestProcessor { bank: &Arc, owner_key: &Pubkey, mut filters: Vec, - ) -> Result> { + ) -> RpcCustomResult> { // The by-owner accounts index checks for Token Account state and Owner address on // inclusion. However, due to the current AccountsDb implementation, an account may remain // in storage as a zero-lamport AccountSharedData::Default() after being wiped and reinitialized in @@ -1802,8 +1804,7 @@ impl JsonRpcRequestProcessor { if !self.config.account_indexes.include_key(owner_key) { return Err(RpcCustomError::KeyExcludedFromSecondaryIndex { index_key: owner_key.to_string(), - } - .into()); + }); } Ok(bank.get_filtered_indexed_accounts( &IndexKey::SplTokenOwner(*owner_key), @@ -1814,7 +1815,7 @@ impl JsonRpcRequestProcessor { RpcFilterType::Memcmp(compare) => compare.bytes_match(&account.data()), }) }, - )) + )?) } else { self.get_filtered_program_accounts(bank, &spl_token_id_v2_0(), filters) } @@ -1826,7 +1827,7 @@ impl JsonRpcRequestProcessor { bank: &Arc, mint_key: &Pubkey, mut filters: Vec, - ) -> Result> { + ) -> RpcCustomResult> { // The by-mint accounts index checks for Token Account state and Mint address on inclusion. // However, due to the current AccountsDb implementation, an account may remain in storage // as be zero-lamport AccountSharedData::Default() after being wiped and reinitialized in later @@ -1850,18 +1851,18 @@ impl JsonRpcRequestProcessor { if !self.config.account_indexes.include_key(mint_key) { return Err(RpcCustomError::KeyExcludedFromSecondaryIndex { index_key: mint_key.to_string(), - } - .into()); + }); } - Ok( - bank.get_filtered_indexed_accounts(&IndexKey::SplTokenMint(*mint_key), |account| { + Ok(bank.get_filtered_indexed_accounts( + &IndexKey::SplTokenMint(*mint_key), + |account| { account.owner() == &spl_token_id_v2_0() && filters.iter().all(|filter_type| match filter_type { RpcFilterType::DataSize(size) => account.data().len() as u64 == *size, RpcFilterType::Memcmp(compare) => compare.bytes_match(&account.data()), }) - }), - ) + }, + )?) } else { self.get_filtered_program_accounts(bank, &spl_token_id_v2_0(), filters) } @@ -2872,7 +2873,8 @@ pub mod rpc_full { config: Option, ) -> Result>> { debug!("get_largest_accounts rpc request received"); - Ok(meta.get_largest_accounts(config)) + let res = meta.get_largest_accounts(config)?; + Ok(res) } fn get_supply( diff --git a/runtime/benches/accounts.rs b/runtime/benches/accounts.rs index f4e3383777b..7d925b8d6f1 100644 --- a/runtime/benches/accounts.rs +++ b/runtime/benches/accounts.rs @@ -388,10 +388,12 @@ fn bench_load_largest_accounts(b: &mut Bencher) { let account = AccountSharedData::new(lamports, 0, &Pubkey::default()); accounts.store_slow_uncached(0, &pubkey, &account); } - let ancestors = Ancestors::from(vec![0]); + let ancestors = Ancestors::from(vec![(0, 0)]); + let slot_id = 0; b.iter(|| { accounts.load_largest_accounts( &ancestors, + slot_id, 20, &HashSet::new(), AccountAddressFilter::Exclude, diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 1767a75374f..ce03bc2f6a6 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -3,7 +3,7 @@ use crate::{ AccountShrinkThreshold, AccountsDb, BankHashInfo, ErrorCounters, LoadHint, LoadedAccount, ScanStorageResult, }, - accounts_index::{AccountSecondaryIndexes, IndexKey}, + accounts_index::{AccountSecondaryIndexes, IndexKey, ScanResult}, ancestors::Ancestors, bank::{ NonceRollbackFull, NonceRollbackInfo, RentDebits, TransactionCheckResult, @@ -23,7 +23,7 @@ use solana_sdk::{ account::{Account, AccountSharedData, ReadableAccount, WritableAccount}, account_utils::StateMut, bpf_loader_upgradeable::{self, UpgradeableLoaderState}, - clock::{Slot, INITIAL_RENT_EPOCH}, + clock::{Slot, SlotId, INITIAL_RENT_EPOCH}, feature_set::{self, FeatureSet}, fee_calculator::{FeeCalculator, FeeConfig}, genesis_config::ClusterType, @@ -585,15 +585,17 @@ impl Accounts { pub fn load_largest_accounts( &self, ancestors: &Ancestors, + slot_id: SlotId, num: usize, filter_by_address: &HashSet, filter: AccountAddressFilter, - ) -> Vec<(Pubkey, u64)> { + ) -> ScanResult> { if num == 0 { - return vec![]; + return Ok(vec![]); } let account_balances = self.accounts_db.scan_accounts( ancestors, + slot_id, |collector: &mut BinaryHeap>, option| { if let Some((pubkey, account, _slot)) = option { if account.lamports() == 0 { @@ -619,12 +621,12 @@ impl Accounts { collector.push(Reverse((account.lamports(), *pubkey))); } }, - ); - account_balances + )?; + Ok(account_balances .into_sorted_vec() .into_iter() .map(|Reverse((balance, pubkey))| (pubkey, balance)) - .collect() + .collect()) } pub fn calculate_capitalization(&self, ancestors: &Ancestors) -> u64 { @@ -683,10 +685,12 @@ impl Accounts { pub fn load_by_program( &self, ancestors: &Ancestors, + slot_id: SlotId, program_id: &Pubkey, - ) -> Vec<(Pubkey, AccountSharedData)> { + ) -> ScanResult> { self.accounts_db.scan_accounts( ancestors, + slot_id, |collector: &mut Vec<(Pubkey, AccountSharedData)>, some_account_tuple| { Self::load_while_filtering(collector, some_account_tuple, |account| { account.owner() == program_id @@ -698,11 +702,13 @@ impl Accounts { pub fn load_by_program_with_filter bool>( &self, ancestors: &Ancestors, + slot_id: SlotId, program_id: &Pubkey, filter: F, - ) -> Vec<(Pubkey, AccountSharedData)> { + ) -> ScanResult> { self.accounts_db.scan_accounts( ancestors, + slot_id, |collector: &mut Vec<(Pubkey, AccountSharedData)>, some_account_tuple| { Self::load_while_filtering(collector, some_account_tuple, |account| { account.owner() == program_id && filter(account) @@ -714,12 +720,14 @@ impl Accounts { pub fn load_by_index_key_with_filter bool>( &self, ancestors: &Ancestors, + slot_id: SlotId, index_key: &IndexKey, filter: F, - ) -> Vec<(Pubkey, AccountSharedData)> { + ) -> ScanResult> { self.accounts_db .index_scan_accounts( ancestors, + slot_id, *index_key, |collector: &mut Vec<(Pubkey, AccountSharedData)>, some_account_tuple| { Self::load_while_filtering(collector, some_account_tuple, |account| { @@ -727,16 +735,21 @@ impl Accounts { }) }, ) - .0 + .map(|result| result.0) } pub fn account_indexes_include_key(&self, key: &Pubkey) -> bool { self.accounts_db.account_indexes.include_key(key) } - pub fn load_all(&self, ancestors: &Ancestors) -> Vec<(Pubkey, AccountSharedData, Slot)> { + pub fn load_all( + &self, + ancestors: &Ancestors, + slot_id: SlotId, + ) -> ScanResult> { self.accounts_db.scan_accounts( ancestors, + slot_id, |collector: &mut Vec<(Pubkey, AccountSharedData, Slot)>, some_account_tuple| { if let Some((pubkey, account, slot)) = some_account_tuple .filter(|(_, account, _)| Self::is_loadable(account.lamports())) @@ -2576,108 +2589,160 @@ mod tests { let all_pubkeys: HashSet<_> = vec![pubkey0, pubkey1, pubkey2].into_iter().collect(); // num == 0 should always return empty set + let slot_id = 0; assert_eq!( - accounts.load_largest_accounts( - &ancestors, - 0, - &HashSet::new(), - AccountAddressFilter::Exclude - ), + accounts + .load_largest_accounts( + &ancestors, + slot_id, + 0, + &HashSet::new(), + AccountAddressFilter::Exclude + ) + .unwrap(), vec![] ); assert_eq!( - accounts.load_largest_accounts( - &ancestors, - 0, - &all_pubkeys, - AccountAddressFilter::Include - ), + accounts + .load_largest_accounts( + &ancestors, + slot_id, + 0, + &all_pubkeys, + AccountAddressFilter::Include + ) + .unwrap(), vec![] ); // list should be sorted by balance, then pubkey, descending assert!(pubkey1 > pubkey0); assert_eq!( - accounts.load_largest_accounts( - &ancestors, - 1, - &HashSet::new(), - AccountAddressFilter::Exclude - ), + accounts + .load_largest_accounts( + &ancestors, + slot_id, + 1, + &HashSet::new(), + AccountAddressFilter::Exclude + ) + .unwrap(), vec![(pubkey1, 42)] ); assert_eq!( - accounts.load_largest_accounts( - &ancestors, - 2, - &HashSet::new(), - AccountAddressFilter::Exclude - ), + accounts + .load_largest_accounts( + &ancestors, + slot_id, + 2, + &HashSet::new(), + AccountAddressFilter::Exclude + ) + .unwrap(), vec![(pubkey1, 42), (pubkey0, 42)] ); assert_eq!( - accounts.load_largest_accounts( - &ancestors, - 3, - &HashSet::new(), - AccountAddressFilter::Exclude - ), + accounts + .load_largest_accounts( + &ancestors, + slot_id, + 3, + &HashSet::new(), + AccountAddressFilter::Exclude + ) + .unwrap(), vec![(pubkey1, 42), (pubkey0, 42), (pubkey2, 41)] ); // larger num should not affect results assert_eq!( - accounts.load_largest_accounts( - &ancestors, - 6, - &HashSet::new(), - AccountAddressFilter::Exclude - ), + accounts + .load_largest_accounts( + &ancestors, + slot_id, + 6, + &HashSet::new(), + AccountAddressFilter::Exclude + ) + .unwrap(), vec![(pubkey1, 42), (pubkey0, 42), (pubkey2, 41)] ); // AccountAddressFilter::Exclude should exclude entry let exclude1: HashSet<_> = vec![pubkey1].into_iter().collect(); assert_eq!( - accounts.load_largest_accounts(&ancestors, 1, &exclude1, AccountAddressFilter::Exclude), + accounts + .load_largest_accounts( + &ancestors, + slot_id, + 1, + &exclude1, + AccountAddressFilter::Exclude + ) + .unwrap(), vec![(pubkey0, 42)] ); assert_eq!( - accounts.load_largest_accounts(&ancestors, 2, &exclude1, AccountAddressFilter::Exclude), + accounts + .load_largest_accounts( + &ancestors, + slot_id, + 2, + &exclude1, + AccountAddressFilter::Exclude + ) + .unwrap(), vec![(pubkey0, 42), (pubkey2, 41)] ); assert_eq!( - accounts.load_largest_accounts(&ancestors, 3, &exclude1, AccountAddressFilter::Exclude), + accounts + .load_largest_accounts( + &ancestors, + slot_id, + 3, + &exclude1, + AccountAddressFilter::Exclude + ) + .unwrap(), vec![(pubkey0, 42), (pubkey2, 41)] ); // AccountAddressFilter::Include should limit entries let include1_2: HashSet<_> = vec![pubkey1, pubkey2].into_iter().collect(); assert_eq!( - accounts.load_largest_accounts( - &ancestors, - 1, - &include1_2, - AccountAddressFilter::Include - ), + accounts + .load_largest_accounts( + &ancestors, + slot_id, + 1, + &include1_2, + AccountAddressFilter::Include + ) + .unwrap(), vec![(pubkey1, 42)] ); assert_eq!( - accounts.load_largest_accounts( - &ancestors, - 2, - &include1_2, - AccountAddressFilter::Include - ), + accounts + .load_largest_accounts( + &ancestors, + slot_id, + 2, + &include1_2, + AccountAddressFilter::Include + ) + .unwrap(), vec![(pubkey1, 42), (pubkey2, 41)] ); assert_eq!( - accounts.load_largest_accounts( - &ancestors, - 3, - &include1_2, - AccountAddressFilter::Include - ), + accounts + .load_largest_accounts( + &ancestors, + slot_id, + 3, + &include1_2, + AccountAddressFilter::Include + ) + .unwrap(), vec![(pubkey1, 42), (pubkey2, 41)] ); } diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index 8b61a418508..7d540a86bed 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -24,7 +24,7 @@ use crate::{ accounts_hash::{AccountsHash, CalculateHashIntermediate, HashStats, PreviousPass}, accounts_index::{ AccountIndexGetResult, AccountSecondaryIndexes, AccountsIndex, AccountsIndexRootsStats, - IndexKey, IsCached, SlotList, SlotSlice, ZeroLamport, + IndexKey, IsCached, ScanResult, SlotList, SlotSlice, ZeroLamport, }, ancestors::Ancestors, append_vec::{AppendVec, StoredAccountMeta, StoredMeta, StoredMetaWriteVersion}, @@ -48,7 +48,7 @@ use solana_measure::measure::Measure; use solana_rayon_threadlimit::get_thread_count; use solana_sdk::{ account::{AccountSharedData, ReadableAccount}, - clock::{Epoch, Slot}, + clock::{Epoch, Slot, SlotId}, genesis_config::ClusterType, hash::{Hash, Hasher}, pubkey::Pubkey, @@ -2420,21 +2420,29 @@ impl AccountsDb { } } - pub fn scan_accounts(&self, ancestors: &Ancestors, scan_func: F) -> A + pub fn scan_accounts( + &self, + ancestors: &Ancestors, + slot_id: SlotId, + scan_func: F, + ) -> ScanResult where F: Fn(&mut A, Option<(&Pubkey, AccountSharedData, Slot)>), A: Default, { let mut collector = A::default(); + + // This can error out if the slots being scanned over are aborted self.accounts_index - .scan_accounts(ancestors, |pubkey, (account_info, slot)| { + .scan_accounts(ancestors, slot_id, |pubkey, (account_info, slot)| { let account_slot = self .get_account_accessor(slot, pubkey, account_info.store_id, account_info.offset) .get_loaded_account() .map(|loaded_account| (pubkey, loaded_account.take_account(), slot)); scan_func(&mut collector, account_slot) - }); - collector + })?; + + Ok(collector) } pub fn unchecked_scan_accounts( @@ -2504,9 +2512,10 @@ impl AccountsDb { pub fn index_scan_accounts( &self, ancestors: &Ancestors, + slot_id: SlotId, index_key: IndexKey, scan_func: F, - ) -> (A, bool) + ) -> ScanResult<(A, bool)> where F: Fn(&mut A, Option<(&Pubkey, AccountSharedData, Slot)>), A: Default, @@ -2519,12 +2528,14 @@ impl AccountsDb { if !self.account_indexes.include_key(key) { // the requested key was not indexed in the secondary index, so do a normal scan let used_index = false; - return (self.scan_accounts(ancestors, scan_func), used_index); + let scan_result = self.scan_accounts(ancestors, slot_id, scan_func)?; + return Ok((scan_result, used_index)); } let mut collector = A::default(); self.accounts_index.index_scan_accounts( ancestors, + slot_id, index_key, |pubkey, (account_info, slot)| { let account_slot = self @@ -2533,9 +2544,9 @@ impl AccountsDb { .map(|loaded_account| (pubkey, loaded_account.take_account(), slot)); scan_func(&mut collector, account_slot) }, - ); + )?; let used_index = true; - (collector, used_index) + Ok((collector, used_index)) } /// Scan a specific slot through all the account storage in parallel @@ -3630,6 +3641,16 @@ impl AccountsDb { } } + // Before purging, let all relevant ongoing scans know that their scans may be corrupt + { + let mut scan_slots_locked = self.accounts_index.tip_of_scanned_forks.lock().unwrap(); + for remove_slot in remove_slots.iter() { + if let Some(removed_scan_slot) = scan_slots_locked.get_mut(remove_slot) { + removed_scan_slot.mark_removed(); + } + } + } + let remove_unrooted_purge_stats = PurgeStats::default(); self.purge_slots_from_cache_and_store(remove_slots.iter(), &remove_unrooted_purge_stats); remove_unrooted_purge_stats.report("remove_unrooted_slots_purge_slots_stats", Some(0)); @@ -7515,11 +7536,13 @@ pub mod tests { // Secondary index should still find both pubkeys let mut found_accounts = HashSet::new(); let index_key = IndexKey::SplTokenMint(mint_key); + let slot_id = 0; accounts .accounts_index - .index_scan_accounts(&Ancestors::default(), index_key, |key, _| { + .index_scan_accounts(&Ancestors::default(), slot_id, index_key, |key, _| { found_accounts.insert(*key); - }); + }) + .unwrap(); assert_eq!(found_accounts.len(), 2); assert!(found_accounts.contains(&pubkey1)); assert!(found_accounts.contains(&pubkey2)); @@ -7530,13 +7553,16 @@ pub mod tests { keys: [mint_key].iter().cloned().collect::>(), }); // Secondary index can't be used - do normal scan: should still find both pubkeys - let found_accounts = accounts.index_scan_accounts( - &Ancestors::default(), - index_key, - |collection: &mut HashSet, account| { - collection.insert(*account.unwrap().0); - }, - ); + let found_accounts = accounts + .index_scan_accounts( + &Ancestors::default(), + slot_id, + index_key, + |collection: &mut HashSet, account| { + collection.insert(*account.unwrap().0); + }, + ) + .unwrap(); assert!(!found_accounts.1); assert_eq!(found_accounts.0.len(), 2); assert!(found_accounts.0.contains(&pubkey1)); @@ -7545,13 +7571,16 @@ pub mod tests { accounts.account_indexes.keys = None; // Secondary index can now be used since it isn't marked as excluded - let found_accounts = accounts.index_scan_accounts( - &Ancestors::default(), - index_key, - |collection: &mut HashSet, account| { - collection.insert(*account.unwrap().0); - }, - ); + let found_accounts = accounts + .index_scan_accounts( + &Ancestors::default(), + slot_id, + index_key, + |collection: &mut HashSet, account| { + collection.insert(*account.unwrap().0); + }, + ) + .unwrap(); assert!(found_accounts.1); assert_eq!(found_accounts.0.len(), 2); assert!(found_accounts.0.contains(&pubkey1)); @@ -7576,11 +7605,15 @@ pub mod tests { // Secondary index should have purged `pubkey1` as well let mut found_accounts = vec![]; - accounts.accounts_index.index_scan_accounts( - &Ancestors::default(), - IndexKey::SplTokenMint(mint_key), - |key, _| found_accounts.push(*key), - ); + accounts + .accounts_index + .index_scan_accounts( + &Ancestors::default(), + slot_id, + IndexKey::SplTokenMint(mint_key), + |key, _| found_accounts.push(*key), + ) + .unwrap(); assert_eq!(found_accounts, vec![pubkey2]); } @@ -10054,6 +10087,7 @@ pub mod tests { fn setup_scan( db: Arc, scan_ancestors: Arc, + slot_id: SlotId, stall_key: Pubkey, ) -> ScanTracker { let exit = Arc::new(AtomicBool::new(false)); @@ -10066,6 +10100,7 @@ pub mod tests { .spawn(move || { db.scan_accounts( &scan_ancestors, + slot_id, |_collector: &mut Vec<(Pubkey, AccountSharedData)>, maybe_account| { ready_.store(true, Ordering::Relaxed); if let Some((pubkey, _, _)) = maybe_account { @@ -10080,7 +10115,8 @@ pub mod tests { } } }, - ); + ) + .unwrap(); }) .unwrap(); @@ -10126,7 +10162,8 @@ pub mod tests { let max_scan_root = 0; db.add_root(max_scan_root); let scan_ancestors: Arc = Arc::new(vec![(0, 1), (1, 1)].into_iter().collect()); - let scan_tracker = setup_scan(db.clone(), scan_ancestors.clone(), account_key2); + let slot_id = 0; + let scan_tracker = setup_scan(db.clone(), scan_ancestors.clone(), slot_id, account_key2); // Add a new root 2 let new_root = 2; @@ -10291,7 +10328,13 @@ pub mod tests { accounts_db.add_root(*slot as Slot); if Some(*slot) == scan_slot { let ancestors = Arc::new(vec![(stall_slot, 1), (*slot, 1)].into_iter().collect()); - scan_tracker = Some(setup_scan(accounts_db.clone(), ancestors, scan_stall_key)); + let slot_id = 0; + scan_tracker = Some(setup_scan( + accounts_db.clone(), + ancestors, + slot_id, + scan_stall_key, + )); assert_eq!( accounts_db.accounts_index.min_ongoing_scan_root().unwrap(), *slot diff --git a/runtime/src/accounts_index.rs b/runtime/src/accounts_index.rs index 0f4773a6da2..60c643e96f4 100644 --- a/runtime/src/accounts_index.rs +++ b/runtime/src/accounts_index.rs @@ -10,13 +10,13 @@ use log::*; use ouroboros::self_referencing; use solana_measure::measure::Measure; use solana_sdk::{ - clock::Slot, + clock::{Slot, SlotId}, pubkey::{Pubkey, PUBKEY_BYTES}, }; use std::{ collections::{ btree_map::{self, BTreeMap}, - HashSet, + HashMap, HashSet, }, ops::{ Bound, @@ -25,15 +25,16 @@ use std::{ }, sync::{ atomic::{AtomicU64, Ordering}, - Arc, RwLock, RwLockReadGuard, RwLockWriteGuard, + Arc, Mutex, RwLock, RwLockReadGuard, RwLockWriteGuard, }, }; +use thiserror::Error; pub const ITER_BATCH_SIZE: usize = 1000; +pub type ScanResult = Result; pub type SlotList = Vec<(Slot, T)>; pub type SlotSlice<'s, T> = &'s [(Slot, T)]; - pub type RefCount = u64; pub type AccountMap = BTreeMap; @@ -55,6 +56,12 @@ impl IsCached for u64 { } } +#[derive(Error, Debug)] +pub enum ScanError { + #[error("Node detected it replayed bad version of slot {slot:?} with id {slot_id:?}, thus the scan on said slot was aborted")] + SlotRemoved { slot: Slot, slot_id: SlotId }, +} + enum ScanTypes> { Unindexed(Option), Indexed(IndexKey), @@ -458,6 +465,10 @@ impl RollingBitField { std::mem::swap(&mut n, self); } + pub fn max(&self) -> u64 { + self.max + } + pub fn get_all(&self) -> Vec { let mut all = Vec::with_capacity(self.count); self.excess.iter().for_each(|slot| all.push(*slot)); @@ -584,6 +595,22 @@ type MapType = AccountMap>; type AccountMapsWriteLock<'a, T> = RwLockWriteGuard<'a, AccountMap>>; type AccountMapsReadLock<'a, T> = RwLockReadGuard<'a, AccountMap>>; +#[derive(Debug, Default)] +pub struct ScanSlotTracker { + is_removed: bool, + ref_count: u64, +} + +impl ScanSlotTracker { + pub fn is_removed(&self) -> bool { + self.is_removed + } + + pub fn mark_removed(&mut self) { + self.is_removed = true; + } +} + #[derive(Debug)] pub struct AccountsIndex { pub account_maps: RwLock>, @@ -592,6 +619,13 @@ pub struct AccountsIndex { spl_token_owner_index: SecondaryIndex, roots_tracker: RwLock, ongoing_scan_roots: RwLock>, + // Each scan has some latest slot `S` that is the tip of the fork the scan + // is iterating over. The unique id of that slot `S` is recorded here (note we don't use + // `S` as the id because there can be more than one version of a slot `S`). If a fork + // is abandoned, all of the slots on that fork up to `S` will be removed via + // `AccountsDb::remove_unrooted_slots()`. When the scan finishes, it'll realize that the + // results of the scan may have been corrupted by `remove_unrooted_slots` and abort its results. + pub tip_of_scanned_forks: Mutex>, zero_lamport_pubkeys: DashSet, } @@ -610,6 +644,7 @@ impl Default for AccountsIndex { ), roots_tracker: RwLock::::default(), ongoing_scan_roots: RwLock::>::default(), + tip_of_scanned_forks: Mutex::>::default(), zero_lamport_pubkeys: DashSet::::default(), } } @@ -627,9 +662,11 @@ impl AccountsIndex { &self, metric_name: &'static str, ancestors: &Ancestors, + scan_slot_id: SlotId, func: F, scan_type: ScanTypes, - ) where + ) -> Result<(), ScanError> + where F: FnMut(&Pubkey, (&T, Slot)), R: RangeBounds, { @@ -652,6 +689,12 @@ impl AccountsIndex { max_root }; + { + let mut w_tip_of_scanned_forks = self.tip_of_scanned_forks.lock().unwrap(); + let tracker = w_tip_of_scanned_forks.entry(scan_slot_id).or_default(); + tracker.ref_count += 1; + } + // First we show that for any bank `B` that is a descendant of // the current `max_root`, it must be true that and `B.ancestors.contains(max_root)`, // regardless of the pattern of `squash()` behavior, where `ancestors` is the set @@ -809,6 +852,28 @@ impl AccountsIndex { ongoing_scan_roots.remove(&max_root); } } + + let is_scan_aborted = { + let mut w_tip_of_scanned_forks = self.tip_of_scanned_forks.lock().unwrap(); + let tracker = w_tip_of_scanned_forks + .get_mut(&scan_slot_id) + .expect("Cannot have removed this scan_slot_id while scan was stil in progress"); + let is_scan_aborted = tracker.is_removed(); + tracker.ref_count -= 1; + if tracker.ref_count == 0 { + w_tip_of_scanned_forks.remove(&scan_slot_id); + } + is_scan_aborted + }; + + if is_scan_aborted { + Err(ScanError::SlotRemoved { + slot: ancestors.max(), + slot_id: scan_slot_id, + }) + } else { + Ok(()) + } } fn do_unchecked_scan_accounts( @@ -1008,7 +1073,12 @@ impl AccountsIndex { } /// call func with every pubkey and index visible from a given set of ancestors - pub(crate) fn scan_accounts(&self, ancestors: &Ancestors, func: F) + pub(crate) fn scan_accounts( + &self, + ancestors: &Ancestors, + scan_slot_id: SlotId, + func: F, + ) -> Result<(), ScanError> where F: FnMut(&Pubkey, (&T, Slot)), { @@ -1016,9 +1086,10 @@ impl AccountsIndex { self.do_checked_scan_accounts( "", ancestors, + scan_slot_id, func, ScanTypes::Unindexed(None::>), - ); + ) } pub(crate) fn unchecked_scan_accounts( @@ -1048,7 +1119,13 @@ impl AccountsIndex { } /// call func with every pubkey and index visible from a given set of ancestors - pub(crate) fn index_scan_accounts(&self, ancestors: &Ancestors, index_key: IndexKey, func: F) + pub(crate) fn index_scan_accounts( + &self, + ancestors: &Ancestors, + scan_slot_id: SlotId, + index_key: IndexKey, + func: F, + ) -> Result<(), ScanError> where F: FnMut(&Pubkey, (&T, Slot)), { @@ -1056,9 +1133,10 @@ impl AccountsIndex { self.do_checked_scan_accounts( "", ancestors, + scan_slot_id, func, ScanTypes::>::Indexed(index_key), - ); + ) } pub fn get_rooted_entries(&self, slice: SlotSlice, max: Option) -> SlotList { diff --git a/runtime/src/ancestors.rs b/runtime/src/ancestors.rs index 048e07cdb41..2fb891539b7 100644 --- a/runtime/src/ancestors.rs +++ b/runtime/src/ancestors.rs @@ -75,6 +75,10 @@ impl Ancestors { pub fn is_empty(&self) -> bool { self.len() == 0 } + + pub fn max(&self) -> Slot { + self.ancestors.max() + } } #[cfg(test)] pub mod tests { diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index c144b0f6c52..3e8e7ef6d2a 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -39,7 +39,7 @@ use crate::{ TransactionLoadResult, TransactionLoaders, }, accounts_db::{AccountShrinkThreshold, ErrorCounters, SnapshotStorages}, - accounts_index::{AccountSecondaryIndexes, IndexKey}, + accounts_index::{AccountSecondaryIndexes, IndexKey, ScanResult}, ancestors::{Ancestors, AncestorsForSerialization}, blockhash_queue::BlockhashQueue, builtins::{self, ActivationType}, @@ -4335,38 +4335,43 @@ impl Bank { .map(|(acc, _slot)| acc) } - pub fn get_program_accounts(&self, program_id: &Pubkey) -> Vec<(Pubkey, AccountSharedData)> { + pub fn get_program_accounts( + &self, + program_id: &Pubkey, + ) -> ScanResult> { self.rc .accounts - .load_by_program(&self.ancestors, program_id) + .load_by_program(&self.ancestors, 0, program_id) } pub fn get_filtered_program_accounts bool>( &self, program_id: &Pubkey, filter: F, - ) -> Vec<(Pubkey, AccountSharedData)> { + ) -> ScanResult> { self.rc .accounts - .load_by_program_with_filter(&self.ancestors, program_id, filter) + .load_by_program_with_filter(&self.ancestors, 0, program_id, filter) } pub fn get_filtered_indexed_accounts bool>( &self, index_key: &IndexKey, filter: F, - ) -> Vec<(Pubkey, AccountSharedData)> { + ) -> ScanResult> { self.rc .accounts - .load_by_index_key_with_filter(&self.ancestors, index_key, filter) + .load_by_index_key_with_filter(&self.ancestors, 0, index_key, filter) } pub fn account_indexes_include_key(&self, key: &Pubkey) -> bool { self.rc.accounts.account_indexes_include_key(key) } - pub fn get_all_accounts_with_modified_slots(&self) -> Vec<(Pubkey, AccountSharedData, Slot)> { - self.rc.accounts.load_all(&self.ancestors) + pub fn get_all_accounts_with_modified_slots( + &self, + ) -> ScanResult> { + self.rc.accounts.load_all(&self.ancestors, 0) } pub fn get_program_accounts_modified_since_parent( @@ -4421,10 +4426,10 @@ impl Bank { num: usize, filter_by_address: &HashSet, filter: AccountAddressFilter, - ) -> Vec<(Pubkey, u64)> { + ) -> ScanResult> { self.rc .accounts - .load_largest_accounts(&self.ancestors, num, filter_by_address, filter) + .load_largest_accounts(&self.ancestors, 0, num, filter_by_address, filter) } pub fn transaction_count(&self) -> u64 { @@ -9137,7 +9142,7 @@ pub(crate) mod tests { let parent = Arc::new(Bank::new(&genesis_config)); parent.restore_old_behavior_for_fragile_tests(); - let genesis_accounts: Vec<_> = parent.get_all_accounts_with_modified_slots(); + let genesis_accounts: Vec<_> = parent.get_all_accounts_with_modified_slots().unwrap(); assert!( genesis_accounts .iter() @@ -9165,11 +9170,11 @@ pub(crate) mod tests { let bank1 = Arc::new(new_from_parent(&bank0)); bank1.squash(); assert_eq!( - bank0.get_program_accounts(&program_id), + bank0.get_program_accounts(&program_id).unwrap(), vec![(pubkey0, account0.clone())] ); assert_eq!( - bank1.get_program_accounts(&program_id), + bank1.get_program_accounts(&program_id).unwrap(), vec![(pubkey0, account0)] ); assert_eq!( @@ -9188,8 +9193,8 @@ pub(crate) mod tests { let bank3 = Arc::new(new_from_parent(&bank2)); bank3.squash(); - assert_eq!(bank1.get_program_accounts(&program_id).len(), 2); - assert_eq!(bank3.get_program_accounts(&program_id).len(), 2); + assert_eq!(bank1.get_program_accounts(&program_id).unwrap().len(), 2); + assert_eq!(bank3.get_program_accounts(&program_id).unwrap().len(), 2); } #[test] @@ -9209,8 +9214,9 @@ pub(crate) mod tests { let account = AccountSharedData::new(1, 0, &program_id); bank.store_account(&address, &account); - let indexed_accounts = - bank.get_filtered_indexed_accounts(&IndexKey::ProgramId(program_id), |_| true); + let indexed_accounts = bank + .get_filtered_indexed_accounts(&IndexKey::ProgramId(program_id), |_| true) + .unwrap(); assert_eq!(indexed_accounts.len(), 1); assert_eq!(indexed_accounts[0], (address, account)); @@ -9221,12 +9227,14 @@ pub(crate) mod tests { let new_account = AccountSharedData::new(1, 0, &another_program_id); let bank = Arc::new(new_from_parent(&bank)); bank.store_account(&address, &new_account); - let indexed_accounts = - bank.get_filtered_indexed_accounts(&IndexKey::ProgramId(program_id), |_| true); + let indexed_accounts = bank + .get_filtered_indexed_accounts(&IndexKey::ProgramId(program_id), |_| true) + .unwrap(); assert_eq!(indexed_accounts.len(), 1); assert_eq!(indexed_accounts[0], (address, new_account.clone())); - let indexed_accounts = - bank.get_filtered_indexed_accounts(&IndexKey::ProgramId(another_program_id), |_| true); + let indexed_accounts = bank + .get_filtered_indexed_accounts(&IndexKey::ProgramId(another_program_id), |_| true) + .unwrap(); assert_eq!(indexed_accounts.len(), 1); assert_eq!(indexed_accounts[0], (address, new_account.clone())); @@ -9234,12 +9242,14 @@ pub(crate) mod tests { let indexed_accounts = bank .get_filtered_indexed_accounts(&IndexKey::ProgramId(program_id), |account| { account.owner() == &program_id - }); + }) + .unwrap(); assert!(indexed_accounts.is_empty()); let indexed_accounts = bank .get_filtered_indexed_accounts(&IndexKey::ProgramId(another_program_id), |account| { account.owner() == &another_program_id - }); + }) + .unwrap(); assert_eq!(indexed_accounts.len(), 1); assert_eq!(indexed_accounts[0], (address, new_account)); } @@ -11954,7 +11964,7 @@ pub(crate) mod tests { if let Ok(bank_to_scan) = bank_to_scan_receiver.recv_timeout(Duration::from_millis(10)) { - let accounts = bank_to_scan.get_program_accounts(&program_id); + let accounts = bank_to_scan.get_program_accounts(&program_id).unwrap(); // Should never see empty accounts because no slot ever deleted // any of the original accounts, and the scan should reflect the // account state at some frozen slot `X` (no partial updates). @@ -12656,21 +12666,25 @@ pub(crate) mod tests { // Return only one largest account assert_eq!( - bank.get_largest_accounts(1, &pubkeys_hashset, AccountAddressFilter::Include), + bank.get_largest_accounts(1, &pubkeys_hashset, AccountAddressFilter::Include) + .unwrap(), vec![(pubkeys[4], sol_to_lamports(5.0))] ); assert_eq!( - bank.get_largest_accounts(1, &HashSet::new(), AccountAddressFilter::Exclude), + bank.get_largest_accounts(1, &HashSet::new(), AccountAddressFilter::Exclude) + .unwrap(), vec![(pubkeys[4], sol_to_lamports(5.0))] ); assert_eq!( - bank.get_largest_accounts(1, &exclude4, AccountAddressFilter::Exclude), + bank.get_largest_accounts(1, &exclude4, AccountAddressFilter::Exclude) + .unwrap(), vec![(pubkeys[3], sol_to_lamports(4.0))] ); // Return all added accounts - let results = - bank.get_largest_accounts(10, &pubkeys_hashset, AccountAddressFilter::Include); + let results = bank + .get_largest_accounts(10, &pubkeys_hashset, AccountAddressFilter::Include) + .unwrap(); assert_eq!(results.len(), sorted_accounts.len()); for pubkey_balance in sorted_accounts.iter() { assert!(results.contains(pubkey_balance)); @@ -12680,7 +12694,9 @@ pub(crate) mod tests { assert_eq!(sorted_results, results); let expected_accounts = sorted_accounts[1..].to_vec(); - let results = bank.get_largest_accounts(10, &exclude4, AccountAddressFilter::Exclude); + let results = bank + .get_largest_accounts(10, &exclude4, AccountAddressFilter::Exclude) + .unwrap(); // results include 5 Bank builtins assert_eq!(results.len(), 10); for pubkey_balance in expected_accounts.iter() { @@ -12692,14 +12708,18 @@ pub(crate) mod tests { // Return 3 added accounts let expected_accounts = sorted_accounts[0..4].to_vec(); - let results = bank.get_largest_accounts(4, &pubkeys_hashset, AccountAddressFilter::Include); + let results = bank + .get_largest_accounts(4, &pubkeys_hashset, AccountAddressFilter::Include) + .unwrap(); assert_eq!(results.len(), expected_accounts.len()); for pubkey_balance in expected_accounts.iter() { assert!(results.contains(pubkey_balance)); } let expected_accounts = expected_accounts[1..4].to_vec(); - let results = bank.get_largest_accounts(3, &exclude4, AccountAddressFilter::Exclude); + let results = bank + .get_largest_accounts(3, &exclude4, AccountAddressFilter::Exclude) + .unwrap(); assert_eq!(results.len(), expected_accounts.len()); for pubkey_balance in expected_accounts.iter() { assert!(results.contains(pubkey_balance)); @@ -12711,7 +12731,8 @@ pub(crate) mod tests { .cloned() .collect(); assert_eq!( - bank.get_largest_accounts(2, &exclude, AccountAddressFilter::Exclude), + bank.get_largest_accounts(2, &exclude, AccountAddressFilter::Exclude) + .unwrap(), vec![pubkeys_balances[3], pubkeys_balances[1]] ); } diff --git a/runtime/src/non_circulating_supply.rs b/runtime/src/non_circulating_supply.rs index fee9a314d2f..a31c4effc3d 100644 --- a/runtime/src/non_circulating_supply.rs +++ b/runtime/src/non_circulating_supply.rs @@ -41,7 +41,10 @@ pub fn calculate_non_circulating_supply(bank: &Arc) -> NonCirculatingSuppl ) } else { bank.get_program_accounts(&solana_stake_program::id()) - }; + } + // TODO: Figure out how to properly handle errors here + .unwrap_or_default(); + for (pubkey, account) in stake_accounts.iter() { let stake_account = StakeState::from(account).unwrap_or_default(); match stake_account { diff --git a/sdk/program/src/clock.rs b/sdk/program/src/clock.rs index 51f45fc23a9..4aa1807eb84 100644 --- a/sdk/program/src/clock.rs +++ b/sdk/program/src/clock.rs @@ -77,6 +77,10 @@ pub const MAX_TRANSACTION_FORWARDING_DELAY: usize = 6; /// is some some number of Ticks long. pub type Slot = u64; +/// Uniquely distinguishes every version of a slot, even if the +/// slot number is the same, i.e. duplicate slots +pub type SlotId = u64; + /// Epoch is a unit of time a given leader schedule is honored, /// some number of Slots. pub type Epoch = u64; From 8b7a4147bbc0d3b52dd6dc2e0a9904a252854a04 Mon Sep 17 00:00:00 2001 From: Carl Lin Date: Mon, 24 May 2021 22:53:17 -0700 Subject: [PATCH 02/12] Add unique bank ids to distinguish banks for same slot --- rpc/src/rpc.rs | 18 ++++++----- rpc/src/rpc_service.rs | 1 + runtime/src/bank.rs | 43 +++++++++++++++++++-------- runtime/src/non_circulating_supply.rs | 22 +++++++------- 4 files changed, 52 insertions(+), 32 deletions(-) diff --git a/rpc/src/rpc.rs b/rpc/src/rpc.rs index 14769526f10..3c2ef6706b1 100644 --- a/rpc/src/rpc.rs +++ b/rpc/src/rpc.rs @@ -718,7 +718,7 @@ impl JsonRpcRequestProcessor { }) } else { let (addresses, address_filter) = if let Some(filter) = config.clone().filter { - let non_circulating_supply = calculate_non_circulating_supply(&bank); + let non_circulating_supply = calculate_non_circulating_supply(&bank)?; let addresses = non_circulating_supply.accounts.into_iter().collect(); let address_filter = match filter { RpcLargestAccountsFilter::Circulating => AccountAddressFilter::Exclude, @@ -742,11 +742,14 @@ impl JsonRpcRequestProcessor { } } - fn get_supply(&self, commitment: Option) -> RpcResponse { + fn get_supply( + &self, + commitment: Option, + ) -> RpcCustomResult> { let bank = self.bank(commitment); - let non_circulating_supply = calculate_non_circulating_supply(&bank); + let non_circulating_supply = calculate_non_circulating_supply(&bank)?; let total_supply = bank.capitalization(); - new_response( + Ok(new_response( &bank, RpcSupply { total: total_supply, @@ -758,7 +761,7 @@ impl JsonRpcRequestProcessor { .map(|pubkey| pubkey.to_string()) .collect(), }, - ) + )) } fn get_vote_accounts( @@ -2873,8 +2876,7 @@ pub mod rpc_full { config: Option, ) -> Result>> { debug!("get_largest_accounts rpc request received"); - let res = meta.get_largest_accounts(config)?; - Ok(res) + Ok(meta.get_largest_accounts(config)?) } fn get_supply( @@ -2883,7 +2885,7 @@ pub mod rpc_full { commitment: Option, ) -> Result> { debug!("get_supply rpc request received"); - Ok(meta.get_supply(commitment)) + Ok(meta.get_supply(commitment)?) } fn request_airdrop( diff --git a/rpc/src/rpc_service.rs b/rpc/src/rpc_service.rs index 0d8adf819f0..d911043214f 100644 --- a/rpc/src/rpc_service.rs +++ b/rpc/src/rpc_service.rs @@ -246,6 +246,7 @@ fn process_rest(bank_forks: &Arc>, path: &str) -> Option, } #[cfg(RUSTC_WITH_SPECIALIZATION)] @@ -440,6 +442,7 @@ impl BankRc { accounts: Arc::new(accounts), parent: RwLock::new(None), slot, + slot_id_generator: Arc::new(AtomicU64::new(0)), } } @@ -909,6 +912,8 @@ pub struct Bank { /// Bank slot (i.e. block) slot: Slot, + slot_id: SlotId, + /// Bank epoch epoch: Epoch, @@ -1131,6 +1136,7 @@ impl Bank { )), parent: RwLock::new(Some(parent.clone())), slot, + slot_id_generator: parent.rc.slot_id_generator.clone(), }; let src = StatusCacheRc { status_cache: parent.src.status_cache.clone(), @@ -1139,10 +1145,12 @@ impl Bank { let fee_rate_governor = FeeRateGovernor::new_derived(&parent.fee_rate_governor, parent.signature_count()); + let slot_id = rc.slot_id_generator.fetch_add(1, Relaxed) + 1; let mut new = Bank { rc, src, slot, + slot_id, epoch, blockhash_queue: RwLock::new(parent.blockhash_queue.read().unwrap().clone()), @@ -1322,6 +1330,7 @@ impl Bank { slots_per_year: fields.slots_per_year, unused: genesis_config.unused, slot: fields.slot, + slot_id: 0, epoch: fields.epoch, block_height: fields.block_height, collector_id: fields.collector_id, @@ -4341,7 +4350,7 @@ impl Bank { ) -> ScanResult> { self.rc .accounts - .load_by_program(&self.ancestors, 0, program_id) + .load_by_program(&self.ancestors, self.slot_id, program_id) } pub fn get_filtered_program_accounts bool>( @@ -4349,9 +4358,12 @@ impl Bank { program_id: &Pubkey, filter: F, ) -> ScanResult> { - self.rc - .accounts - .load_by_program_with_filter(&self.ancestors, 0, program_id, filter) + self.rc.accounts.load_by_program_with_filter( + &self.ancestors, + self.slot_id, + program_id, + filter, + ) } pub fn get_filtered_indexed_accounts bool>( @@ -4359,9 +4371,12 @@ impl Bank { index_key: &IndexKey, filter: F, ) -> ScanResult> { - self.rc - .accounts - .load_by_index_key_with_filter(&self.ancestors, 0, index_key, filter) + self.rc.accounts.load_by_index_key_with_filter( + &self.ancestors, + self.slot_id, + index_key, + filter, + ) } pub fn account_indexes_include_key(&self, key: &Pubkey) -> bool { @@ -4371,7 +4386,7 @@ impl Bank { pub fn get_all_accounts_with_modified_slots( &self, ) -> ScanResult> { - self.rc.accounts.load_all(&self.ancestors, 0) + self.rc.accounts.load_all(&self.ancestors, self.slot_id) } pub fn get_program_accounts_modified_since_parent( @@ -4427,9 +4442,13 @@ impl Bank { filter_by_address: &HashSet, filter: AccountAddressFilter, ) -> ScanResult> { - self.rc - .accounts - .load_largest_accounts(&self.ancestors, 0, num, filter_by_address, filter) + self.rc.accounts.load_largest_accounts( + &self.ancestors, + self.slot_id, + num, + filter_by_address, + filter, + ) } pub fn transaction_count(&self) -> u64 { diff --git a/runtime/src/non_circulating_supply.rs b/runtime/src/non_circulating_supply.rs index a31c4effc3d..76668ac811b 100644 --- a/runtime/src/non_circulating_supply.rs +++ b/runtime/src/non_circulating_supply.rs @@ -1,6 +1,6 @@ use { crate::{ - accounts_index::{AccountIndex, IndexKey}, + accounts_index::{AccountIndex, IndexKey, ScanResult}, bank::Bank, }, log::*, @@ -14,7 +14,7 @@ pub struct NonCirculatingSupply { pub accounts: Vec, } -pub fn calculate_non_circulating_supply(bank: &Arc) -> NonCirculatingSupply { +pub fn calculate_non_circulating_supply(bank: &Arc) -> ScanResult { debug!("Updating Bank supply, epoch: {}", bank.epoch()); let mut non_circulating_accounts_set: HashSet = HashSet::new(); @@ -38,12 +38,10 @@ pub fn calculate_non_circulating_supply(bank: &Arc) -> NonCirculatingSuppl // zero-lamport Account::Default() after being wiped and reinitialized in later // updates. We include the redundant filter here to avoid returning these accounts. |account| account.owner() == &solana_stake_program::id(), - ) + )? } else { - bank.get_program_accounts(&solana_stake_program::id()) - } - // TODO: Figure out how to properly handle errors here - .unwrap_or_default(); + bank.get_program_accounts(&solana_stake_program::id())? + }; for (pubkey, account) in stake_accounts.iter() { let stake_account = StakeState::from(account).unwrap_or_default(); @@ -71,10 +69,10 @@ pub fn calculate_non_circulating_supply(bank: &Arc) -> NonCirculatingSuppl .map(|pubkey| bank.get_balance(&pubkey)) .sum(); - NonCirculatingSupply { + Ok(NonCirculatingSupply { lamports, accounts: non_circulating_accounts_set.into_iter().collect(), - } + }) } // Mainnet-beta accounts that should be considered non-circulating @@ -259,7 +257,7 @@ mod tests { + sysvar_and_native_program_delta, ); - let non_circulating_supply = calculate_non_circulating_supply(&bank); + let non_circulating_supply = calculate_non_circulating_supply(&bank).unwrap(); assert_eq!( non_circulating_supply.lamports, (num_non_circulating_accounts + num_stake_accounts) * balance @@ -277,7 +275,7 @@ mod tests { &AccountSharedData::new(new_balance, 0, &Pubkey::default()), ); } - let non_circulating_supply = calculate_non_circulating_supply(&bank); + let non_circulating_supply = calculate_non_circulating_supply(&bank).unwrap(); assert_eq!( non_circulating_supply.lamports, (num_non_circulating_accounts * new_balance) + (num_stake_accounts * balance) @@ -292,7 +290,7 @@ mod tests { bank = Arc::new(new_from_parent(&bank)); } assert_eq!(bank.epoch(), 1); - let non_circulating_supply = calculate_non_circulating_supply(&bank); + let non_circulating_supply = calculate_non_circulating_supply(&bank).unwrap(); assert_eq!( non_circulating_supply.lamports, num_non_circulating_accounts * new_balance From 5d5fca007e9fa972bc13a58a029b1c12029d54eb Mon Sep 17 00:00:00 2001 From: Carl Lin Date: Tue, 25 May 2021 17:22:22 -0700 Subject: [PATCH 03/12] Account for scans starting after remove_unrooted_slots() --- core/src/repair_service.rs | 9 +- runtime/src/accounts.rs | 4 +- runtime/src/accounts_background_service.rs | 4 +- runtime/src/accounts_db.rs | 108 ++++++++++++--------- runtime/src/accounts_index.rs | 47 ++++----- runtime/src/bank.rs | 56 ++++++++++- 6 files changed, 151 insertions(+), 77 deletions(-) diff --git a/core/src/repair_service.rs b/core/src/repair_service.rs index ceb0b2c65f0..5ad494fb9c4 100644 --- a/core/src/repair_service.rs +++ b/core/src/repair_service.rs @@ -559,7 +559,7 @@ impl RepairService { #[allow(dead_code)] fn process_new_duplicate_slots( - new_duplicate_slots: &[Slot], + new_duplicate_slots: &[(Slot, SlotId)], duplicate_slot_repair_statuses: &mut HashMap, cluster_slots: &ClusterSlots, root_bank: &Bank, @@ -568,7 +568,7 @@ impl RepairService { duplicate_slots_reset_sender: &DuplicateSlotsResetSender, repair_validators: &Option>, ) { - for slot in new_duplicate_slots { + for (slot, slot_id) in new_duplicate_slots { warn!( "Cluster confirmed slot: {}, dumping our current version and repairing", slot @@ -577,7 +577,7 @@ impl RepairService { root_bank.clear_slot_signatures(*slot); // Clear the accounts for this slot - root_bank.remove_unrooted_slots(&[*slot]); + root_bank.remove_unrooted_slots(&[(*slot, *slot_id)]); // Clear the slot-related data in blockstore. This will: // 1) Clear old shreds allowing new ones to be inserted @@ -1139,6 +1139,7 @@ mod test { ); let bank0 = Arc::new(Bank::new(&genesis_config)); let bank9 = Bank::new_from_parent(&bank0, &Pubkey::default(), duplicate_slot); + let duplicate_slot_id = bank9.slot_id(); let old_balance = bank9.get_balance(&keypairs.node_keypair.pubkey()); bank9 .transfer(10_000, &mint_keypair, &keypairs.node_keypair.pubkey()) @@ -1156,7 +1157,7 @@ mod test { assert!(bank9.get_signature_status(&vote_tx.signatures[0]).is_some()); RepairService::process_new_duplicate_slots( - &[duplicate_slot], + &[(duplicate_slot, duplicate_slot_id)], &mut duplicate_slot_repair_statuses, &cluster_slots, &bank9, diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index ce03bc2f6a6..34796c9b63e 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -940,8 +940,8 @@ impl Accounts { /// Purge a slot if it is not a root /// Root slots cannot be purged /// `is_from_abs` is true if the caller is the AccountsBackgroundService - pub fn purge_slot(&self, slot: Slot, is_from_abs: bool) { - self.accounts_db.purge_slot(slot, is_from_abs); + pub fn purge_slot(&self, slot: Slot, slot_id: SlotId, is_from_abs: bool) { + self.accounts_db.purge_slot(slot, slot_id, is_from_abs); } /// Add a slot to root. Root slots cannot be purged diff --git a/runtime/src/accounts_background_service.rs b/runtime/src/accounts_background_service.rs index 277e25f9180..6a95e26a4f4 100644 --- a/runtime/src/accounts_background_service.rs +++ b/runtime/src/accounts_background_service.rs @@ -268,7 +268,9 @@ impl AbsRequestHandler { let mut count = 0; for pruned_slot in self.pruned_banks_receiver.try_iter() { count += 1; - bank.rc.accounts.purge_slot(pruned_slot, is_from_abs); + bank.rc + .accounts + .purge_slot(pruned_slot, bank.slot_id(), is_from_abs); } count diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index 7d540a86bed..b6c3ec23a89 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -3296,13 +3296,11 @@ impl AccountsDb { } /// `is_from_abs` is true if the caller is the AccountsBackgroundService - pub fn purge_slot(&self, slot: Slot, is_from_abs: bool) { + pub fn purge_slot(&self, slot: Slot, slot_id: SlotId, is_from_abs: bool) { if self.is_bank_drop_callback_enabled.load(Ordering::SeqCst) && !is_from_abs { panic!("bad drop callpath detected; Bank::drop() must run serially with other logic in ABS like clean_accounts()") } - let mut slots = HashSet::new(); - slots.insert(slot); - self.purge_slots(&slots); + self.purge_slots(std::iter::once(&(slot, slot_id))); } fn recycle_slot_stores( @@ -3335,15 +3333,18 @@ impl AccountsDb { /// Purges every slot in `removed_slots` from both the cache and storage. This includes /// entries in the accounts index, cache entries, and any backing storage entries. + /// + /// This should only be called after the `Bank::drop()` runs in bank.rs, See BANK_DROP_SAFETY + /// comment below for why fn purge_slots_from_cache_and_store<'a>( - &'a self, - removed_slots: impl Iterator, + &self, + removed_slots: impl Iterator, purge_stats: &PurgeStats, ) { let mut remove_cache_elapsed_across_slots = 0; let mut num_cached_slots_removed = 0; let mut total_removed_cached_bytes = 0; - for remove_slot in removed_slots { + for (remove_slot, remove_slot_id) in removed_slots { // This function is only currently safe with respect to `flush_slot_cache()` because // both functions run serially in AccountsBackgroundService. let mut remove_cache_elapsed = Measure::start("remove_cache_elapsed"); @@ -3361,6 +3362,19 @@ impl AccountsDb { // It should not be possible that a slot is neither in the cache or storage. Even in // a slot with all ticks, `Bank::new_from_parent()` immediately stores some sysvars // on bank creation. + + // BANK_DROP_SAFETY: Because this function only runs once the bank is dropped, + // we know that there are no longer any ongoing scans on this bank, because scans require + // and hold a reference to the bank at the tip of the fork they're scanning. Hence it's + // safe to remove this slot_id from the `removed_slot_ids` list at this point. + // + // This could be folded into handle_reclaims() -> accounts_index.clean_dead_slot(), but + // that requires plumbing slot id's through the regular store/clean path as well. + self.accounts_index + .removed_slot_ids + .lock() + .unwrap() + .remove(&remove_slot_id); } purge_stats @@ -3542,39 +3556,33 @@ impl AccountsDb { } #[allow(clippy::needless_collect)] - fn purge_slots(&self, slots: &HashSet) { + fn purge_slots<'a>(&self, slots: impl Iterator) { // `add_root()` should be called first let mut safety_checks_elapsed = Measure::start("safety_checks_elapsed"); - let non_roots: Vec<&Slot> = slots - .iter() - .filter(|slot| !self.accounts_index.is_root(**slot)) - .collect(); + let non_roots = slots + // Only safe to check when there are duplciate versions of a slot + // because ReplayStage will not make new roots before dumping the + // duplicate slots first. Thus we will not be in a case where we + // root slot `S`, then try to dump some other version of slot `S`, the + // dumping has to finish first + // + // Also note roots are never removed via `remove_unrooted_slot()`, so + // it's safe to filter them out here as they won't need deletion from + // self.accounts_index.removed_slot_ids in `purge_slots_from_cache_and_store()`. + .filter(|(slot, _)| !self.accounts_index.is_root(*slot)); safety_checks_elapsed.stop(); self.external_purge_slots_stats .safety_checks_elapsed .fetch_add(safety_checks_elapsed.as_us(), Ordering::Relaxed); - self.purge_slots_from_cache_and_store( - non_roots.into_iter(), - &self.external_purge_slots_stats, - ); + self.purge_slots_from_cache_and_store(non_roots, &self.external_purge_slots_stats); self.external_purge_slots_stats .report("external_purge_slots_stats", Some(1000)); } - // TODO: This is currently unsafe with scan because it can remove a slot in the middle - /// Remove the set of slots in `remove_slots` from both the cache and storage. This requires - /// we know the contents of the slot are either: - /// - /// 1) Completely in the cache - /// 2) Have been completely flushed from the cache - /// - /// in order to guarantee that when this function returns, the contents of the slot have - /// been completely and not only partially removed. Thus synchronization with `flush_slot_cache()` - /// through `self.remove_unrooted_slots_synchronization` is necessary. - pub fn remove_unrooted_slots(&self, remove_slots: &[Slot]) { + pub fn remove_unrooted_slots(&self, remove_slots: &[(Slot, SlotId)]) { let rooted_slots = self .accounts_index - .get_rooted_from_list(remove_slots.iter()); + .get_rooted_from_list(remove_slots.iter().map(|(slot, _)| slot)); assert!( rooted_slots.is_empty(), "Trying to remove accounts for rooted slots {:?}", @@ -3594,7 +3602,7 @@ impl AccountsDb { // we want to remove in this function let mut remaining_contended_flush_slots: Vec = remove_slots .iter() - .filter(|remove_slot| { + .filter_map(|(remove_slot, _)| { let is_being_flushed = currently_contended_slots.contains(remove_slot); if !is_being_flushed { // Reserve the slots that we want to purge that aren't currently @@ -3605,10 +3613,10 @@ impl AccountsDb { // before another version of the same slot can be replayed. This means // multiple threads should not call `remove_unrooted_slots()` simultaneously // with the same slot. - currently_contended_slots.insert(**remove_slot); + currently_contended_slots.insert(*remove_slot); } // If the cache is currently flushing this slot, add it to the list - is_being_flushed + Some(remove_slot).filter(|_| is_being_flushed) }) .cloned() .collect(); @@ -3641,13 +3649,13 @@ impl AccountsDb { } } - // Before purging, let all relevant ongoing scans know that their scans may be corrupt + // Mark down these slots are about to be purged so that new attempts to scan these + // banks fail, and any ongoing scans over these slots will detect that they should abort + // their results { - let mut scan_slots_locked = self.accounts_index.tip_of_scanned_forks.lock().unwrap(); - for remove_slot in remove_slots.iter() { - if let Some(removed_scan_slot) = scan_slots_locked.get_mut(remove_slot) { - removed_scan_slot.mark_removed(); - } + let mut locked_removed_slot_ids = self.accounts_index.removed_slot_ids.lock().unwrap(); + for (_, remove_slot_id) in remove_slots.iter() { + locked_removed_slot_ids.insert(*remove_slot_id); } } @@ -3656,8 +3664,8 @@ impl AccountsDb { remove_unrooted_purge_stats.report("remove_unrooted_slots_purge_slots_stats", Some(0)); let mut currently_contended_slots = slots_under_contention.lock().unwrap(); - for slot in remove_slots { - assert!(currently_contended_slots.remove(slot)); + for (remove_slot, _) in remove_slots { + assert!(currently_contended_slots.remove(remove_slot)); } } @@ -6853,6 +6861,7 @@ pub mod tests { fn run_test_remove_unrooted_slot(is_cached: bool) { let unrooted_slot = 9; + let unrooted_slot_id = 9; let mut db = AccountsDb::new(Vec::new(), &ClusterType::Development); db.caching_enabled = true; let key = Pubkey::default(); @@ -6874,7 +6883,7 @@ pub mod tests { assert_load_account(&db, unrooted_slot, key, 1); // Purge the slot - db.remove_unrooted_slots(&[unrooted_slot]); + db.remove_unrooted_slots(&[(unrooted_slot, unrooted_slot_id)]); assert!(db.load_without_fixed_root(&ancestors, &key).is_none()); assert!(db.bank_hashes.read().unwrap().get(&unrooted_slot).is_none()); assert!(db.accounts_cache.slot_cache(unrooted_slot).is_none()); @@ -6905,13 +6914,14 @@ pub mod tests { fn test_remove_unrooted_slot_snapshot() { solana_logger::setup(); let unrooted_slot = 9; + let unrooted_slot_id = 9; let db = AccountsDb::new(Vec::new(), &ClusterType::Development); let key = solana_sdk::pubkey::new_rand(); let account0 = AccountSharedData::new(1, 0, &key); db.store_uncached(unrooted_slot, &[(&key, &account0)]); // Purge the slot - db.remove_unrooted_slots(&[unrooted_slot]); + db.remove_unrooted_slots(&[(unrooted_slot, unrooted_slot_id)]); // Add a new root let key2 = solana_sdk::pubkey::new_rand(); @@ -10223,7 +10233,8 @@ pub mod tests { assert_eq!(account.0.lamports(), slot1_account.lamports()); // Simulate dropping the bank, which finally removes the slot from the cache - db.purge_slot(1, false); + let slot_id = 1; + db.purge_slot(1, slot_id, false); assert!(db .do_load( &scan_ancestors, @@ -11167,7 +11178,12 @@ pub mod tests { account.set_lamports(lamports); // Pick random 50% of the slots to pass to `remove_unrooted_slots()` - let mut all_slots: Vec = (0..num_cached_slots).collect(); + let mut all_slots: Vec<(Slot, SlotId)> = (0..num_cached_slots) + .map(|slot| { + let slot_id = slot + 1; + (slot, slot_id) + }) + .collect(); all_slots.shuffle(&mut rand::thread_rng()); let slots_to_dump = &all_slots[0..num_cached_slots as usize / 2]; let slots_to_keep = &all_slots[num_cached_slots as usize / 2..]; @@ -11200,7 +11216,7 @@ pub mod tests { // Check that all the slots in `slots_to_dump` were completely removed from the // cache, storage, and index - for slot in slots_to_dump { + for (slot, _) in slots_to_dump { assert!(db.storage.get_slot_storage_entries(*slot).is_none()); assert!(db.accounts_cache.slot_cache(*slot).is_none()); let account_in_slot = slot_to_pubkey_map[slot]; @@ -11213,7 +11229,7 @@ pub mod tests { // Wait for flush to finish before starting next trial flush_done_receiver.recv().unwrap(); - for slot in slots_to_keep { + for (slot, slot_id) in slots_to_keep { let account_in_slot = slot_to_pubkey_map[slot]; assert!(db .load( @@ -11224,7 +11240,7 @@ pub mod tests { .is_some()); // Clear for next iteration so that `assert!(self.storage.get_slot_stores(purged_slot).is_none());` // in `purge_slot_pubkeys()` doesn't trigger - db.remove_unrooted_slots(&[*slot]); + db.remove_unrooted_slots(&[(*slot, *slot_id)]); } } diff --git a/runtime/src/accounts_index.rs b/runtime/src/accounts_index.rs index 60c643e96f4..beb75764ca5 100644 --- a/runtime/src/accounts_index.rs +++ b/runtime/src/accounts_index.rs @@ -16,7 +16,7 @@ use solana_sdk::{ use std::{ collections::{ btree_map::{self, BTreeMap}, - HashMap, HashSet, + HashSet, }, ops::{ Bound, @@ -625,7 +625,11 @@ pub struct AccountsIndex { // is abandoned, all of the slots on that fork up to `S` will be removed via // `AccountsDb::remove_unrooted_slots()`. When the scan finishes, it'll realize that the // results of the scan may have been corrupted by `remove_unrooted_slots` and abort its results. - pub tip_of_scanned_forks: Mutex>, + // + // `removed_slot_ids` tracks all the slot ids that were removed via `remove_unrooted_slots()` so any attempted scans + // on any of these slots fails. This is safe to purge once the associated Bank is dropped and + // scanning the fork with that Bank at the tip is no longer possible. + pub removed_slot_ids: Mutex>, zero_lamport_pubkeys: DashSet, } @@ -644,7 +648,7 @@ impl Default for AccountsIndex { ), roots_tracker: RwLock::::default(), ongoing_scan_roots: RwLock::>::default(), - tip_of_scanned_forks: Mutex::>::default(), + removed_slot_ids: Mutex::>::default(), zero_lamport_pubkeys: DashSet::::default(), } } @@ -670,6 +674,16 @@ impl AccountsIndex { F: FnMut(&Pubkey, (&T, Slot)), R: RangeBounds, { + { + let locked_removed_slot_ids = self.removed_slot_ids.lock().unwrap(); + if locked_removed_slot_ids.contains(&scan_slot_id) { + return Err(ScanError::SlotRemoved { + slot: ancestors.max(), + slot_id: scan_slot_id, + }); + } + } + let max_root = { let mut w_ongoing_scan_roots = self // This lock is also grabbed by clean_accounts(), so clean @@ -689,12 +703,6 @@ impl AccountsIndex { max_root }; - { - let mut w_tip_of_scanned_forks = self.tip_of_scanned_forks.lock().unwrap(); - let tracker = w_tip_of_scanned_forks.entry(scan_slot_id).or_default(); - tracker.ref_count += 1; - } - // First we show that for any bank `B` that is a descendant of // the current `max_root`, it must be true that and `B.ancestors.contains(max_root)`, // regardless of the pattern of `squash()` behavior, where `ancestors` is the set @@ -853,20 +861,15 @@ impl AccountsIndex { } } - let is_scan_aborted = { - let mut w_tip_of_scanned_forks = self.tip_of_scanned_forks.lock().unwrap(); - let tracker = w_tip_of_scanned_forks - .get_mut(&scan_slot_id) - .expect("Cannot have removed this scan_slot_id while scan was stil in progress"); - let is_scan_aborted = tracker.is_removed(); - tracker.ref_count -= 1; - if tracker.ref_count == 0 { - w_tip_of_scanned_forks.remove(&scan_slot_id); - } - is_scan_aborted - }; + // If the fork with tip at bank `scan_slot_id` was removed durin our scan, then the scan + // may have been corrupted, so abort the results. + let was_scan_corrupted = self + .removed_slot_ids + .lock() + .unwrap() + .contains(&scan_slot_id); - if is_scan_aborted { + if was_scan_corrupted { Err(ScanError::SlotRemoved { slot: ancestors.max(), slot_id: scan_slot_id, diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 43753378940..f4fe6bfc668 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -1450,6 +1450,10 @@ impl Bank { self.slot } + pub fn slot_id(&self) -> SlotId { + self.slot_id + } + pub fn epoch(&self) -> Epoch { self.epoch } @@ -2667,7 +2671,7 @@ impl Bank { } } - pub fn remove_unrooted_slots(&self, slots: &[Slot]) { + pub fn remove_unrooted_slots(&self, slots: &[(Slot, SlotId)]) { self.rc.accounts.accounts_db.remove_unrooted_slots(slots) } @@ -5282,7 +5286,9 @@ impl Drop for Bank { // 1. Tests // 2. At startup when replaying blockstore and there's no // AccountsBackgroundService to perform cleanups yet. - self.rc.accounts.purge_slot(self.slot(), false); + self.rc + .accounts + .purge_slot(self.slot(), self.slot_id(), false); } } } @@ -12176,6 +12182,52 @@ pub(crate) mod tests { } } + #[test] + fn test_remove_unrooted_scan_consistency() { + for accounts_db_caching_enabled in &[false, true] { + test_store_scan_consistency( + *accounts_db_caching_enabled, + |bank0, bank_to_scan_sender, pubkeys_to_modify, program_id, starting_lamports| { + loop { + let mut bank_at_fork_tip = bank0.clone(); + let num_banks_on_fork = 100; + let mut slots_to_remove = Vec::with_capacity(num_banks_on_fork); + for _ in 0..num_banks_on_fork { + bank_at_fork_tip = Arc::new(Bank::new_from_parent( + &bank_at_fork_tip, + &solana_sdk::pubkey::new_rand(), + bank_at_fork_tip.slot() + 1, + )); + let lamports_this_round = + bank_at_fork_tip.slot() + starting_lamports + 1; + let account = + AccountSharedData::new(lamports_this_round, 0, &program_id); + for key in pubkeys_to_modify.iter() { + bank_at_fork_tip.store_account(key, &account); + } + bank_at_fork_tip.freeze(); + slots_to_remove + .push((bank_at_fork_tip.slot(), bank_at_fork_tip.slot_id())); + } + // Send the previous bank to the scan thread to perform the scan. + // + // The capacity of the channel is 1 so that this thread will wait for the scan to + // finish before starting the next iteration, allowing the scan to stay in sync with + // these updates such that every scan will see these slot being removed + if bank_to_scan_sender.send(bank_at_fork_tip.clone()).is_err() { + // Channel was disconnected, exit + return; + } + + // Remove account state for all banks on the fork + bank_at_fork_tip.remove_unrooted_slots(&slots_to_remove); + } + }, + None, + ); + } + } + #[test] fn test_stake_rewrite() { let GenesisConfigInfo { genesis_config, .. } = From e6e807219cbd1968186e1fb86176d025faefdb56 Mon Sep 17 00:00:00 2001 From: Carl Lin Date: Wed, 26 May 2021 02:47:08 -0700 Subject: [PATCH 04/12] Add tests and fixup --- core/src/replay_stage.rs | 6 -- runtime/src/accounts_db.rs | 48 ++++++------ runtime/src/accounts_index.rs | 6 +- runtime/src/ancestors.rs | 4 +- runtime/src/bank.rs | 143 ++++++++++++++++++++++++---------- 5 files changed, 135 insertions(+), 72 deletions(-) diff --git a/core/src/replay_stage.rs b/core/src/replay_stage.rs index 136db5dfc1f..0ae35ee4f23 100644 --- a/core/src/replay_stage.rs +++ b/core/src/replay_stage.rs @@ -856,12 +856,6 @@ impl ReplayStage { // Clear the duplicate banks from BankForks { let mut w_bank_forks = bank_forks.write().unwrap(); - // Purging should have already been taken care of by logic - // in repair_service, so make sure drop implementation doesn't - // run - if let Some(b) = w_bank_forks.get(*d) { - b.skip_drop.store(true, Ordering::Relaxed) - } w_bank_forks.remove(*d); } } diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index b6c3ec23a89..b482b87952c 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -3295,12 +3295,29 @@ impl AccountsDb { SendDroppedBankCallback::new(pruned_banks_sender) } + /// This should only be called after the `Bank::drop()` runs in bank.rs, See BANK_DROP_SAFETY + /// comment below for why /// `is_from_abs` is true if the caller is the AccountsBackgroundService pub fn purge_slot(&self, slot: Slot, slot_id: SlotId, is_from_abs: bool) { if self.is_bank_drop_callback_enabled.load(Ordering::SeqCst) && !is_from_abs { panic!("bad drop callpath detected; Bank::drop() must run serially with other logic in ABS like clean_accounts()") } - self.purge_slots(std::iter::once(&(slot, slot_id))); + // BANK_DROP_SAFETY: Because this function only runs once the bank is dropped, + // we know that there are no longer any ongoing scans on this bank, because scans require + // and hold a reference to the bank at the tip of the fork they're scanning. Hence it's + // safe to remove this slot_id from the `removed_slot_ids` list at this point. + if self + .accounts_index + .removed_slot_ids + .lock() + .unwrap() + .remove(&slot_id) + { + // If this slot was already cleaned up, no need to do any further cleans + return; + } + + self.purge_slots(std::iter::once(&slot)); } fn recycle_slot_stores( @@ -3333,18 +3350,15 @@ impl AccountsDb { /// Purges every slot in `removed_slots` from both the cache and storage. This includes /// entries in the accounts index, cache entries, and any backing storage entries. - /// - /// This should only be called after the `Bank::drop()` runs in bank.rs, See BANK_DROP_SAFETY - /// comment below for why fn purge_slots_from_cache_and_store<'a>( &self, - removed_slots: impl Iterator, + removed_slots: impl Iterator, purge_stats: &PurgeStats, ) { let mut remove_cache_elapsed_across_slots = 0; let mut num_cached_slots_removed = 0; let mut total_removed_cached_bytes = 0; - for (remove_slot, remove_slot_id) in removed_slots { + for remove_slot in removed_slots { // This function is only currently safe with respect to `flush_slot_cache()` because // both functions run serially in AccountsBackgroundService. let mut remove_cache_elapsed = Measure::start("remove_cache_elapsed"); @@ -3362,19 +3376,6 @@ impl AccountsDb { // It should not be possible that a slot is neither in the cache or storage. Even in // a slot with all ticks, `Bank::new_from_parent()` immediately stores some sysvars // on bank creation. - - // BANK_DROP_SAFETY: Because this function only runs once the bank is dropped, - // we know that there are no longer any ongoing scans on this bank, because scans require - // and hold a reference to the bank at the tip of the fork they're scanning. Hence it's - // safe to remove this slot_id from the `removed_slot_ids` list at this point. - // - // This could be folded into handle_reclaims() -> accounts_index.clean_dead_slot(), but - // that requires plumbing slot id's through the regular store/clean path as well. - self.accounts_index - .removed_slot_ids - .lock() - .unwrap() - .remove(&remove_slot_id); } purge_stats @@ -3556,7 +3557,7 @@ impl AccountsDb { } #[allow(clippy::needless_collect)] - fn purge_slots<'a>(&self, slots: impl Iterator) { + fn purge_slots<'a>(&self, slots: impl Iterator) { // `add_root()` should be called first let mut safety_checks_elapsed = Measure::start("safety_checks_elapsed"); let non_roots = slots @@ -3569,7 +3570,7 @@ impl AccountsDb { // Also note roots are never removed via `remove_unrooted_slot()`, so // it's safe to filter them out here as they won't need deletion from // self.accounts_index.removed_slot_ids in `purge_slots_from_cache_and_store()`. - .filter(|(slot, _)| !self.accounts_index.is_root(*slot)); + .filter(|slot| !self.accounts_index.is_root(**slot)); safety_checks_elapsed.stop(); self.external_purge_slots_stats .safety_checks_elapsed @@ -3660,7 +3661,10 @@ impl AccountsDb { } let remove_unrooted_purge_stats = PurgeStats::default(); - self.purge_slots_from_cache_and_store(remove_slots.iter(), &remove_unrooted_purge_stats); + self.purge_slots_from_cache_and_store( + remove_slots.iter().map(|(slot, _)| slot), + &remove_unrooted_purge_stats, + ); remove_unrooted_purge_stats.report("remove_unrooted_slots_purge_slots_stats", Some(0)); let mut currently_contended_slots = slots_under_contention.lock().unwrap(); diff --git a/runtime/src/accounts_index.rs b/runtime/src/accounts_index.rs index beb75764ca5..d531f36a7ee 100644 --- a/runtime/src/accounts_index.rs +++ b/runtime/src/accounts_index.rs @@ -56,7 +56,7 @@ impl IsCached for u64 { } } -#[derive(Error, Debug)] +#[derive(Error, Debug, PartialEq)] pub enum ScanError { #[error("Node detected it replayed bad version of slot {slot:?} with id {slot_id:?}, thus the scan on said slot was aborted")] SlotRemoved { slot: Slot, slot_id: SlotId }, @@ -678,7 +678,7 @@ impl AccountsIndex { let locked_removed_slot_ids = self.removed_slot_ids.lock().unwrap(); if locked_removed_slot_ids.contains(&scan_slot_id) { return Err(ScanError::SlotRemoved { - slot: ancestors.max(), + slot: ancestors.max_slot(), slot_id: scan_slot_id, }); } @@ -871,7 +871,7 @@ impl AccountsIndex { if was_scan_corrupted { Err(ScanError::SlotRemoved { - slot: ancestors.max(), + slot: ancestors.max_slot(), slot_id: scan_slot_id, }) } else { diff --git a/runtime/src/ancestors.rs b/runtime/src/ancestors.rs index 2fb891539b7..0f6bafba853 100644 --- a/runtime/src/ancestors.rs +++ b/runtime/src/ancestors.rs @@ -76,8 +76,8 @@ impl Ancestors { self.len() == 0 } - pub fn max(&self) -> Slot { - self.ancestors.max() + pub fn max_slot(&self) -> Slot { + self.ancestors.max() - 1 } } #[cfg(test)] diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index f4fe6bfc668..02d86142aad 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -970,8 +970,6 @@ pub struct Bank { /// Protocol-level rewards that were distributed by this bank pub rewards: RwLock>, - pub skip_drop: AtomicBool, - pub cluster_type: Option, pub lazy_rent_collection: AtomicBool, @@ -1192,7 +1190,6 @@ impl Bank { hard_forks: parent.hard_forks.clone(), last_vote_sync: AtomicU64::new(parent.last_vote_sync.load(Relaxed)), rewards: RwLock::new(vec![]), - skip_drop: AtomicBool::new(false), cluster_type: parent.cluster_type, lazy_rent_collection: AtomicBool::new(parent.lazy_rent_collection.load(Relaxed)), no_stake_rewrite: AtomicBool::new(parent.no_stake_rewrite.load(Relaxed)), @@ -1350,7 +1347,6 @@ impl Bank { feature_builtins: new(), last_vote_sync: new(), rewards: new(), - skip_drop: new(), cluster_type: Some(genesis_config.cluster_type), lazy_rent_collection: new(), no_stake_rewrite: new(), @@ -5275,10 +5271,6 @@ impl Bank { impl Drop for Bank { fn drop(&mut self) { - if self.skip_drop.load(Relaxed) { - return; - } - if let Some(drop_callback) = self.drop_callback.read().unwrap().0.as_ref() { drop_callback.callback(self); } else { @@ -5329,7 +5321,9 @@ pub(crate) mod tests { use crate::{ accounts_background_service::{AbsRequestHandler, SendDroppedBankCallback}, accounts_db::DEFAULT_ACCOUNTS_SHRINK_RATIO, - accounts_index::{AccountIndex, AccountMap, AccountSecondaryIndexes, ITER_BATCH_SIZE}, + accounts_index::{ + AccountIndex, AccountMap, AccountSecondaryIndexes, ScanError, ITER_BATCH_SIZE, + }, ancestors::Ancestors, genesis_utils::{ activate_all_features, bootstrap_validator_stake_lamports, @@ -11922,10 +11916,17 @@ pub(crate) mod tests { assert!(!debug.is_empty()); } + enum AcceptableScanResults { + DroppedSlotError, + NoFailure, + Both, + } + fn test_store_scan_consistency( accounts_db_caching_enabled: bool, update_f: F, drop_callback: Option>, + acceptable_scan_results: AcceptableScanResults, ) where F: Fn(Arc, crossbeam_channel::Sender>, Arc>, Pubkey, u64) + std::marker::Send, @@ -11989,7 +11990,24 @@ pub(crate) mod tests { if let Ok(bank_to_scan) = bank_to_scan_receiver.recv_timeout(Duration::from_millis(10)) { - let accounts = bank_to_scan.get_program_accounts(&program_id).unwrap(); + let accounts_result = bank_to_scan.get_program_accounts(&program_id); + let accounts = { + match (&acceptable_scan_results, accounts_result.is_err()) { + (AcceptableScanResults::DroppedSlotError, _) + | (AcceptableScanResults::Both, true) => { + assert_eq!( + accounts_result, + Err(ScanError::SlotRemoved { + slot: bank_to_scan.slot(), + slot_id: bank_to_scan.slot_id() + }) + ); + continue; + } + (AcceptableScanResults::NoFailure, _) + | (AcceptableScanResults::Both, false) => accounts_result.unwrap(), + } + }; // Should never see empty accounts because no slot ever deleted // any of the original accounts, and the scan should reflect the // account state at some frozen slot `X` (no partial updates). @@ -12134,6 +12152,7 @@ pub(crate) mod tests { Some(Box::new(SendDroppedBankCallback::new( pruned_banks_sender.clone(), ))), + AcceptableScanResults::NoFailure, ) } } @@ -12178,52 +12197,98 @@ pub(crate) mod tests { } }, None, + AcceptableScanResults::NoFailure, ); } } + fn setup_banks_on_fork_to_remove( + bank0: Arc, + pubkeys_to_modify: Arc>, + program_id: &Pubkey, + starting_lamports: u64, + ) -> (Arc, Vec<(Slot, SlotId)>) { + let mut bank_at_fork_tip = bank0; + let num_banks_on_fork = 100; + let mut slots_to_remove = Vec::with_capacity(num_banks_on_fork); + for _ in 0..num_banks_on_fork { + bank_at_fork_tip = Arc::new(Bank::new_from_parent( + &bank_at_fork_tip, + &solana_sdk::pubkey::new_rand(), + bank_at_fork_tip.slot() + 1, + )); + let lamports_this_round = bank_at_fork_tip.slot() + starting_lamports + 1; + let account = AccountSharedData::new(lamports_this_round, 0, program_id); + for key in pubkeys_to_modify.iter() { + bank_at_fork_tip.store_account(key, &account); + } + bank_at_fork_tip.freeze(); + slots_to_remove.push((bank_at_fork_tip.slot(), bank_at_fork_tip.slot_id())); + } + + (bank_at_fork_tip, slots_to_remove) + } + #[test] - fn test_remove_unrooted_scan_consistency() { + fn test_remove_unrooted_scan_consistency_remove_before_scan() { for accounts_db_caching_enabled in &[false, true] { test_store_scan_consistency( *accounts_db_caching_enabled, |bank0, bank_to_scan_sender, pubkeys_to_modify, program_id, starting_lamports| { loop { - let mut bank_at_fork_tip = bank0.clone(); - let num_banks_on_fork = 100; - let mut slots_to_remove = Vec::with_capacity(num_banks_on_fork); - for _ in 0..num_banks_on_fork { - bank_at_fork_tip = Arc::new(Bank::new_from_parent( - &bank_at_fork_tip, - &solana_sdk::pubkey::new_rand(), - bank_at_fork_tip.slot() + 1, - )); - let lamports_this_round = - bank_at_fork_tip.slot() + starting_lamports + 1; - let account = - AccountSharedData::new(lamports_this_round, 0, &program_id); - for key in pubkeys_to_modify.iter() { - bank_at_fork_tip.store_account(key, &account); - } - bank_at_fork_tip.freeze(); - slots_to_remove - .push((bank_at_fork_tip.slot(), bank_at_fork_tip.slot_id())); - } - // Send the previous bank to the scan thread to perform the scan. - // - // The capacity of the channel is 1 so that this thread will wait for the scan to - // finish before starting the next iteration, allowing the scan to stay in sync with - // these updates such that every scan will see these slot being removed + let (bank_at_fork_tip, slots_on_fork) = setup_banks_on_fork_to_remove( + bank0.clone(), + pubkeys_to_modify.clone(), + &program_id, + starting_lamports, + ); + // Test removing the slot before the scan starts, should error every time + bank_at_fork_tip.remove_unrooted_slots(&slots_on_fork); if bank_to_scan_sender.send(bank_at_fork_tip.clone()).is_err() { - // Channel was disconnected, exit return; } + } + }, + None, + // Test removing the slot before the scan starts, should error every time + AcceptableScanResults::DroppedSlotError, + ); + } + } - // Remove account state for all banks on the fork - bank_at_fork_tip.remove_unrooted_slots(&slots_to_remove); + #[test] + fn test_remove_unrooted_scan_consistency_remove_and_recreate_same_slot_before_scan() { + for accounts_db_caching_enabled in &[false, true] { + test_store_scan_consistency( + *accounts_db_caching_enabled, + |bank0, bank_to_scan_sender, pubkeys_to_modify, program_id, starting_lamports| { + let mut prev_bank = bank0.clone(); + loop { + let (bank_at_fork_tip, slots_on_fork) = setup_banks_on_fork_to_remove( + bank0.clone(), + pubkeys_to_modify.clone(), + &program_id, + starting_lamports, + ); + // Remove the fork. Then we'll recreate the slots and only after we've + // recreated the slots, do we send this old bank for scanning. + bank_at_fork_tip.remove_unrooted_slots(&slots_on_fork); + // Skip scanning bank 0 on first iteration of loop, since those accounts + // aren't being removed + if prev_bank.slot() != 0 + // Now after we've recreated the slots removed in the previous loop + // iteration, send the previous bank, should fail even though the + // same slots were recreated + && bank_to_scan_sender.send(prev_bank.clone()).is_err() + { + continue; + } + prev_bank = bank_at_fork_tip; } }, None, + // Test removing the slot before the scan starts, should error every time + AcceptableScanResults::DroppedSlotError, ); } } From f5d360310f6081721d667e2556599c074ec62e04 Mon Sep 17 00:00:00 2001 From: Carl Lin Date: Wed, 26 May 2021 20:26:58 -0700 Subject: [PATCH 05/12] temp --- runtime/src/accounts_index.rs | 12 ++-- runtime/src/bank.rs | 121 +++++++++++++++++++++++----------- 2 files changed, 89 insertions(+), 44 deletions(-) diff --git a/runtime/src/accounts_index.rs b/runtime/src/accounts_index.rs index d531f36a7ee..8cafdc07c9e 100644 --- a/runtime/src/accounts_index.rs +++ b/runtime/src/accounts_index.rs @@ -674,7 +674,7 @@ impl AccountsIndex { F: FnMut(&Pubkey, (&T, Slot)), R: RangeBounds, { - { + /*{ let locked_removed_slot_ids = self.removed_slot_ids.lock().unwrap(); if locked_removed_slot_ids.contains(&scan_slot_id) { return Err(ScanError::SlotRemoved { @@ -682,7 +682,7 @@ impl AccountsIndex { slot_id: scan_slot_id, }); } - } + }*/ let max_root = { let mut w_ongoing_scan_roots = self @@ -863,7 +863,7 @@ impl AccountsIndex { // If the fork with tip at bank `scan_slot_id` was removed durin our scan, then the scan // may have been corrupted, so abort the results. - let was_scan_corrupted = self + /*let was_scan_corrupted = self .removed_slot_ids .lock() .unwrap() @@ -876,7 +876,8 @@ impl AccountsIndex { }) } else { Ok(()) - } + }*/ + Ok(()) } fn do_unchecked_scan_accounts( @@ -915,6 +916,7 @@ impl AccountsIndex { let mut read_lock_elapsed = 0; let mut iterator_elapsed = 0; let mut iterator_timer = Measure::start("iterator_elapsed"); + let mut num_found = 0; for pubkey_list in self.iter(range) { iterator_timer.stop(); iterator_elapsed += iterator_timer.as_us(); @@ -930,6 +932,7 @@ impl AccountsIndex { latest_slot_elapsed += latest_slot_timer.as_us(); let mut load_account_timer = Measure::start("load_account"); func(&pubkey, (&list_r[index].1, list_r[index].0)); + num_found += 1; load_account_timer.stop(); load_account_elapsed += load_account_timer.as_us(); } @@ -937,6 +940,7 @@ impl AccountsIndex { iterator_timer = Measure::start("iterator_elapsed"); } + println!("Scan found {} keys", num_found); total_elapsed_timer.stop(); if !metric_name.is_empty() { datapoint_info!( diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 02d86142aad..c24a85e6e31 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -11916,6 +11916,7 @@ pub(crate) mod tests { assert!(!debug.is_empty()); } + #[derive(Debug)] enum AcceptableScanResults { DroppedSlotError, NoFailure, @@ -11928,8 +11929,14 @@ pub(crate) mod tests { drop_callback: Option>, acceptable_scan_results: AcceptableScanResults, ) where - F: Fn(Arc, crossbeam_channel::Sender>, Arc>, Pubkey, u64) - + std::marker::Send, + F: Fn( + Arc, + crossbeam_channel::Sender>, + crossbeam_channel::Receiver, + Arc>, + Pubkey, + u64, + ) + std::marker::Send, { // Set up initial bank let mut genesis_config = create_genesis_config_with_leader( @@ -11981,6 +11988,11 @@ pub(crate) mod tests { crossbeam_channel::Sender>, crossbeam_channel::Receiver>, ) = bounded(1); + + let (scan_finished_sender, scan_finished_receiver): ( + crossbeam_channel::Sender, + crossbeam_channel::Receiver, + ) = unbounded(); let scan_thread = Builder::new() .name("scan".to_string()) .spawn(move || loop { @@ -11990,46 +12002,51 @@ pub(crate) mod tests { if let Ok(bank_to_scan) = bank_to_scan_receiver.recv_timeout(Duration::from_millis(10)) { + println!("scanning program accounts"); let accounts_result = bank_to_scan.get_program_accounts(&program_id); - let accounts = { - match (&acceptable_scan_results, accounts_result.is_err()) { - (AcceptableScanResults::DroppedSlotError, _) - | (AcceptableScanResults::Both, true) => { - assert_eq!( - accounts_result, - Err(ScanError::SlotRemoved { - slot: bank_to_scan.slot(), - slot_id: bank_to_scan.slot_id() - }) - ); - continue; - } - (AcceptableScanResults::NoFailure, _) - | (AcceptableScanResults::Both, false) => accounts_result.unwrap(), + let _ = scan_finished_sender.send(bank_to_scan.slot_id()); + println!("results: {:?} {:?}", acceptable_scan_results, accounts_result.is_err()); + match (&acceptable_scan_results, accounts_result.is_err()) { + (AcceptableScanResults::DroppedSlotError, _) + | (AcceptableScanResults::Both, true) => { + assert_eq!( + accounts_result, + Err(ScanError::SlotRemoved { + slot: bank_to_scan.slot(), + slot_id: bank_to_scan.slot_id() + }) + ); } - }; + (AcceptableScanResults::NoFailure, _) + | (AcceptableScanResults::Both, false) => { + assert!(accounts_result.is_ok()) + } + } + // Should never see empty accounts because no slot ever deleted // any of the original accounts, and the scan should reflect the // account state at some frozen slot `X` (no partial updates). - assert!(!accounts.is_empty()); - let mut expected_lamports = None; - let mut target_accounts_found = HashSet::new(); - for (pubkey, account) in accounts { - let account_balance = account.lamports(); - if pubkeys_to_modify_.contains(&pubkey) { - target_accounts_found.insert(pubkey); - if let Some(expected_lamports) = expected_lamports { - assert_eq!(account_balance, expected_lamports); - } else { - // All pubkeys in the specified set should have the same balance - expected_lamports = Some(account_balance); + if let Ok(accounts) = accounts_result { + assert!(!accounts.is_empty()); + let mut expected_lamports = None; + let mut target_accounts_found = HashSet::new(); + for (pubkey, account) in accounts { + let account_balance = account.lamports(); + if pubkeys_to_modify_.contains(&pubkey) { + target_accounts_found.insert(pubkey); + if let Some(expected_lamports) = expected_lamports { + assert_eq!(account_balance, expected_lamports); + } else { + // All pubkeys in the specified set should have the same balance + expected_lamports = Some(account_balance); + } } } - } - // Should've found all the accounts, i.e. no partial cleans should - // be detected - assert_eq!(target_accounts_found.len(), total_pubkeys_to_modify); + // Should've found all the accounts, i.e. no partial cleans should + // be detected + assert_eq!(target_accounts_found.len(), total_pubkeys_to_modify); + } } }) .unwrap(); @@ -12042,6 +12059,7 @@ pub(crate) mod tests { update_f( bank0, bank_to_scan_sender, + scan_finished_receiver, pubkeys_to_modify, program_id, starting_lamports, @@ -12068,6 +12086,7 @@ pub(crate) mod tests { *accounts_db_caching_enabled, move |bank0, bank_to_scan_sender, + _scan_finished_receiver, pubkeys_to_modify, program_id, starting_lamports| { @@ -12162,7 +12181,12 @@ pub(crate) mod tests { for accounts_db_caching_enabled in &[false, true] { test_store_scan_consistency( *accounts_db_caching_enabled, - |bank0, bank_to_scan_sender, pubkeys_to_modify, program_id, starting_lamports| { + |bank0, + bank_to_scan_sender, + _scan_finished_receiver, + pubkeys_to_modify, + program_id, + starting_lamports| { let mut current_bank = bank0.clone(); let mut prev_bank = bank0; loop { @@ -12209,7 +12233,7 @@ pub(crate) mod tests { starting_lamports: u64, ) -> (Arc, Vec<(Slot, SlotId)>) { let mut bank_at_fork_tip = bank0; - let num_banks_on_fork = 100; + let num_banks_on_fork = 10; let mut slots_to_remove = Vec::with_capacity(num_banks_on_fork); for _ in 0..num_banks_on_fork { bank_at_fork_tip = Arc::new(Bank::new_from_parent( @@ -12234,7 +12258,12 @@ pub(crate) mod tests { for accounts_db_caching_enabled in &[false, true] { test_store_scan_consistency( *accounts_db_caching_enabled, - |bank0, bank_to_scan_sender, pubkeys_to_modify, program_id, starting_lamports| { + |bank0, + bank_to_scan_sender, + _scan_finished_receiver, + pubkeys_to_modify, + program_id, + starting_lamports| { loop { let (bank_at_fork_tip, slots_on_fork) = setup_banks_on_fork_to_remove( bank0.clone(), @@ -12261,7 +12290,12 @@ pub(crate) mod tests { for accounts_db_caching_enabled in &[false, true] { test_store_scan_consistency( *accounts_db_caching_enabled, - |bank0, bank_to_scan_sender, pubkeys_to_modify, program_id, starting_lamports| { + |bank0, + bank_to_scan_sender, + scan_finished_receiver, + pubkeys_to_modify, + program_id, + starting_lamports| { let mut prev_bank = bank0.clone(); loop { let (bank_at_fork_tip, slots_on_fork) = setup_banks_on_fork_to_remove( @@ -12272,7 +12306,6 @@ pub(crate) mod tests { ); // Remove the fork. Then we'll recreate the slots and only after we've // recreated the slots, do we send this old bank for scanning. - bank_at_fork_tip.remove_unrooted_slots(&slots_on_fork); // Skip scanning bank 0 on first iteration of loop, since those accounts // aren't being removed if prev_bank.slot() != 0 @@ -12281,8 +12314,16 @@ pub(crate) mod tests { // same slots were recreated && bank_to_scan_sender.send(prev_bank.clone()).is_err() { - continue; + return; } + + let finished_scan_slot_id = scan_finished_receiver.recv(); + if finished_scan_slot_id.is_err() { + return; + } + // Wait for scan to finish before starting next iteration + assert_eq!(finished_scan_slot_id.unwrap(), prev_bank.slot_id()); + bank_at_fork_tip.remove_unrooted_slots(&slots_on_fork); prev_bank = bank_at_fork_tip; } }, From 86fec15e40c8317a7f868c19351751370bd63c02 Mon Sep 17 00:00:00 2001 From: Carl Lin Date: Thu, 27 May 2021 02:08:39 -0700 Subject: [PATCH 06/12] Fix bad ordering in purge_slots_from_cache_and_store() --- runtime/src/accounts_db.rs | 47 ++++- runtime/src/accounts_index.rs | 10 +- runtime/src/bank.rs | 318 ++++++++++++++++++++++++---------- 3 files changed, 279 insertions(+), 96 deletions(-) diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index b482b87952c..1ad1845324c 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -274,6 +274,33 @@ impl<'a> LoadedAccountAccessor<'a> { } } + fn get_loaded_account_logged(&mut self, pubkey: &Pubkey, slot: Slot) -> Option { + match self { + LoadedAccountAccessor::Cached(cached_account) => { + let i = cached_account.take(); + if i.is_none() { + println!("Failed to get {} {}", pubkey, slot); + } + let cached_account: (Pubkey, Cow<'a, CachedAccount>) = i.expect( + "Cache flushed/purged should be handled before trying to fetch account", + ); + Some(LoadedAccount::Cached(cached_account)) + } + LoadedAccountAccessor::Stored(maybe_storage_entry) => { + // storage entry may not be present if slot was cleaned up in + // between reading the accounts index and calling this function to + // get account meta from the storage entry here + maybe_storage_entry + .as_ref() + .and_then(|(storage_entry, offset)| { + storage_entry + .get_stored_account_meta(*offset) + .map(LoadedAccount::Stored) + }) + } + } + } + fn get_loaded_account(&mut self) -> Option { match self { LoadedAccountAccessor::Cached(cached_account) => { @@ -1583,6 +1610,7 @@ impl AccountsDb { self.accounts_index .handle_dead_keys(&dead_keys, &self.account_indexes); + println!("purging keys: {:?}", dead_keys); reclaims } @@ -2437,7 +2465,7 @@ impl AccountsDb { .scan_accounts(ancestors, slot_id, |pubkey, (account_info, slot)| { let account_slot = self .get_account_accessor(slot, pubkey, account_info.store_id, account_info.offset) - .get_loaded_account() + .get_loaded_account_logged(&pubkey, slot) .map(|loaded_account| (pubkey, loaded_account.take_account(), slot)); scan_func(&mut collector, account_slot) })?; @@ -3296,7 +3324,7 @@ impl AccountsDb { } /// This should only be called after the `Bank::drop()` runs in bank.rs, See BANK_DROP_SAFETY - /// comment below for why + /// comment below for why /// `is_from_abs` is true if the caller is the AccountsBackgroundService pub fn purge_slot(&self, slot: Slot, slot_id: SlotId, is_from_abs: bool) { if self.is_bank_drop_callback_enabled.load(Ordering::SeqCst) && !is_from_abs { @@ -3314,6 +3342,7 @@ impl AccountsDb { .remove(&slot_id) { // If this slot was already cleaned up, no need to do any further cleans + println!("purge_slot() slot_id: {}", slot_id); return; } @@ -3362,7 +3391,11 @@ impl AccountsDb { // This function is only currently safe with respect to `flush_slot_cache()` because // both functions run serially in AccountsBackgroundService. let mut remove_cache_elapsed = Measure::start("remove_cache_elapsed"); - if let Some(slot_cache) = self.accounts_cache.remove_slot(*remove_slot) { + // Note: we cannot remove this slot from the slot cache until we've removed its + // entries from the accounts index first. This is because `scan_accounts()` relies on + // holding the index lock, finding the index entry, and then looking up the entry + // in the cache. If it fails to find that entry, it will panic in `get_loaded_account()` + if let Some(slot_cache) = self.accounts_cache.slot_cache(*remove_slot) { // If the slot is still in the cache, remove the backing storages for // the slot and from the Accounts Index num_cached_slots_removed += 1; @@ -3370,6 +3403,8 @@ impl AccountsDb { self.purge_slot_cache(*remove_slot, slot_cache); remove_cache_elapsed.stop(); remove_cache_elapsed_across_slots += remove_cache_elapsed.as_us(); + // Nobody else shoud have removed the slot cache entry yet + assert!(self.accounts_cache.remove_slot(*remove_slot).is_some()); } else { self.purge_slot_storage(*remove_slot, purge_stats); } @@ -3655,7 +3690,11 @@ impl AccountsDb { // their results { let mut locked_removed_slot_ids = self.accounts_index.removed_slot_ids.lock().unwrap(); - for (_, remove_slot_id) in remove_slots.iter() { + for (slot, remove_slot_id) in remove_slots.iter() { + println!( + "remove_unrooted_slots slot: {}, id: {}", + slot, remove_slot_id + ); locked_removed_slot_ids.insert(*remove_slot_id); } } diff --git a/runtime/src/accounts_index.rs b/runtime/src/accounts_index.rs index 8cafdc07c9e..b4b4fd2cb74 100644 --- a/runtime/src/accounts_index.rs +++ b/runtime/src/accounts_index.rs @@ -674,7 +674,7 @@ impl AccountsIndex { F: FnMut(&Pubkey, (&T, Slot)), R: RangeBounds, { - /*{ + { let locked_removed_slot_ids = self.removed_slot_ids.lock().unwrap(); if locked_removed_slot_ids.contains(&scan_slot_id) { return Err(ScanError::SlotRemoved { @@ -682,7 +682,7 @@ impl AccountsIndex { slot_id: scan_slot_id, }); } - }*/ + } let max_root = { let mut w_ongoing_scan_roots = self @@ -863,21 +863,21 @@ impl AccountsIndex { // If the fork with tip at bank `scan_slot_id` was removed durin our scan, then the scan // may have been corrupted, so abort the results. - /*let was_scan_corrupted = self + let was_scan_corrupted = self .removed_slot_ids .lock() .unwrap() .contains(&scan_slot_id); if was_scan_corrupted { + println!("Scan was corrupted"); Err(ScanError::SlotRemoved { slot: ancestors.max_slot(), slot_id: scan_slot_id, }) } else { Ok(()) - }*/ - Ok(()) + } } fn do_unchecked_scan_accounts( diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index c24a85e6e31..2289c378ed4 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -11995,57 +11995,69 @@ pub(crate) mod tests { ) = unbounded(); let scan_thread = Builder::new() .name("scan".to_string()) - .spawn(move || loop { - if exit_.load(Relaxed) { - return; - } - if let Ok(bank_to_scan) = - bank_to_scan_receiver.recv_timeout(Duration::from_millis(10)) - { - println!("scanning program accounts"); - let accounts_result = bank_to_scan.get_program_accounts(&program_id); - let _ = scan_finished_sender.send(bank_to_scan.slot_id()); - println!("results: {:?} {:?}", acceptable_scan_results, accounts_result.is_err()); - match (&acceptable_scan_results, accounts_result.is_err()) { - (AcceptableScanResults::DroppedSlotError, _) - | (AcceptableScanResults::Both, true) => { - assert_eq!( - accounts_result, - Err(ScanError::SlotRemoved { - slot: bank_to_scan.slot(), - slot_id: bank_to_scan.slot_id() - }) - ); - } - (AcceptableScanResults::NoFailure, _) - | (AcceptableScanResults::Both, false) => { - assert!(accounts_result.is_ok()) - } + .spawn(move || { + let mut num_banks_scanned = 0; + loop { + println!("starting scan loop"); + if exit_.load(Relaxed) { + println!("scan exiting"); + return num_banks_scanned; } + if let Ok(bank_to_scan) = + bank_to_scan_receiver.recv_timeout(Duration::from_millis(10)) + { + println!("scanning program accounts"); + let accounts_result = bank_to_scan.get_program_accounts(&program_id); + let _ = scan_finished_sender.send(bank_to_scan.slot_id()); + num_banks_scanned += 1; + println!( + "results: {:?} {:?} {}", + acceptable_scan_results, + accounts_result.is_err(), + num_banks_scanned + ); + match (&acceptable_scan_results, accounts_result.is_err()) { + (AcceptableScanResults::DroppedSlotError, _) + | (AcceptableScanResults::Both, true) => { + assert_eq!( + accounts_result, + Err(ScanError::SlotRemoved { + slot: bank_to_scan.slot(), + slot_id: bank_to_scan.slot_id() + }) + ); + } + (AcceptableScanResults::NoFailure, _) + | (AcceptableScanResults::Both, false) => { + assert!(accounts_result.is_ok()) + } + } - // Should never see empty accounts because no slot ever deleted - // any of the original accounts, and the scan should reflect the - // account state at some frozen slot `X` (no partial updates). - if let Ok(accounts) = accounts_result { - assert!(!accounts.is_empty()); - let mut expected_lamports = None; - let mut target_accounts_found = HashSet::new(); - for (pubkey, account) in accounts { - let account_balance = account.lamports(); - if pubkeys_to_modify_.contains(&pubkey) { - target_accounts_found.insert(pubkey); - if let Some(expected_lamports) = expected_lamports { - assert_eq!(account_balance, expected_lamports); - } else { - // All pubkeys in the specified set should have the same balance - expected_lamports = Some(account_balance); + // Should never see empty accounts because no slot ever deleted + // any of the original accounts, and the scan should reflect the + // account state at some frozen slot `X` (no partial updates). + if let Ok(accounts) = accounts_result { + println!("Scan found {} accounts", accounts.len()); + assert!(!accounts.is_empty()); + let mut expected_lamports = None; + let mut target_accounts_found = HashSet::new(); + for (pubkey, account) in accounts { + let account_balance = account.lamports(); + if pubkeys_to_modify_.contains(&pubkey) { + target_accounts_found.insert(pubkey); + if let Some(expected_lamports) = expected_lamports { + assert_eq!(account_balance, expected_lamports); + } else { + // All pubkeys in the specified set should have the same balance + expected_lamports = Some(account_balance); + } } } - } - // Should've found all the accounts, i.e. no partial cleans should - // be detected - assert_eq!(target_accounts_found.len(), total_pubkeys_to_modify); + // Should've found all the accounts, i.e. no partial cleans should + // be detected + assert_eq!(target_accounts_found.len(), total_pubkeys_to_modify); + } } } }) @@ -12069,8 +12081,11 @@ pub(crate) mod tests { // Let threads run for a while, check the scans didn't see any mixed slots std::thread::sleep(Duration::new(5, 0)); + println!("storing exit"); exit.store(true, Relaxed); - scan_thread.join().unwrap(); + let min_expected_number_of_scans = 5; + let num_banks_scanned = scan_thread.join().unwrap(); + assert!(num_banks_scanned > min_expected_number_of_scans); update_thread.join().unwrap(); } @@ -12231,51 +12246,99 @@ pub(crate) mod tests { pubkeys_to_modify: Arc>, program_id: &Pubkey, starting_lamports: u64, - ) -> (Arc, Vec<(Slot, SlotId)>) { + num_banks_on_fork: usize, + step_size: usize, + ) -> (Arc, Vec<(Slot, SlotId)>, Ancestors) { + // Need at least 2 keys to create inconsistency in account balances when deleting + // slots + assert!(pubkeys_to_modify.len() > 1); + + // Tracks the bank at the tip of the to be created fork let mut bank_at_fork_tip = bank0; - let num_banks_on_fork = 10; - let mut slots_to_remove = Vec::with_capacity(num_banks_on_fork); - for _ in 0..num_banks_on_fork { - bank_at_fork_tip = Arc::new(Bank::new_from_parent( - &bank_at_fork_tip, - &solana_sdk::pubkey::new_rand(), - bank_at_fork_tip.slot() + 1, - )); - let lamports_this_round = bank_at_fork_tip.slot() + starting_lamports + 1; - let account = AccountSharedData::new(lamports_this_round, 0, program_id); - for key in pubkeys_to_modify.iter() { - bank_at_fork_tip.store_account(key, &account); + + // All the slots on the fork except slot 0 + let mut slots_on_fork = Vec::with_capacity(num_banks_on_fork); + + // All accounts in each set of `step_size` slots will have the same account balances. + // The account balances of the accounts changes every `step_size` banks. Thus if you + // delete any one of the latest `step_size` slots, then you will see varying account + // balances when loading the accounts. + assert!(num_banks_on_fork >= 2); + assert!(step_size >= 2); + let pubkeys_to_modify: Vec = pubkeys_to_modify.iter().cloned().collect(); + let pubkeys_to_modify_per_slot = (pubkeys_to_modify.len() / step_size).max(1); + for _ in (0..num_banks_on_fork).step_by(step_size) { + let mut lamports_this_round = 0; + for i in 0..step_size { + bank_at_fork_tip = Arc::new(Bank::new_from_parent( + &bank_at_fork_tip, + &solana_sdk::pubkey::new_rand(), + bank_at_fork_tip.slot() + 1, + )); + if lamports_this_round == 0 { + lamports_this_round = bank_at_fork_tip.slot_id() + starting_lamports + 1; + } + let pubkey_to_modify_starting_index = i * pubkeys_to_modify_per_slot; + let account = AccountSharedData::new(lamports_this_round, 0, program_id); + for pubkey_index_to_modify in pubkey_to_modify_starting_index + ..pubkey_to_modify_starting_index + pubkeys_to_modify_per_slot + { + let key = pubkeys_to_modify[pubkey_index_to_modify % pubkeys_to_modify.len()]; + bank_at_fork_tip.store_account(&key, &account); + } + bank_at_fork_tip.freeze(); + slots_on_fork.push((bank_at_fork_tip.slot(), bank_at_fork_tip.slot_id())); } - bank_at_fork_tip.freeze(); - slots_to_remove.push((bank_at_fork_tip.slot(), bank_at_fork_tip.slot_id())); } - (bank_at_fork_tip, slots_to_remove) + let ancestors: Vec<(Slot, usize)> = slots_on_fork.iter().map(|(s, _)| (*s, 0)).collect(); + let ancestors = Ancestors::from(ancestors); + + (bank_at_fork_tip, slots_on_fork, ancestors) } #[test] - fn test_remove_unrooted_scan_consistency_remove_before_scan() { + fn test_remove_unrooted_before_scan() { for accounts_db_caching_enabled in &[false, true] { test_store_scan_consistency( *accounts_db_caching_enabled, |bank0, bank_to_scan_sender, - _scan_finished_receiver, + scan_finished_receiver, pubkeys_to_modify, program_id, starting_lamports| { loop { - let (bank_at_fork_tip, slots_on_fork) = setup_banks_on_fork_to_remove( - bank0.clone(), - pubkeys_to_modify.clone(), - &program_id, - starting_lamports, - ); - // Test removing the slot before the scan starts, should error every time + let (bank_at_fork_tip, slots_on_fork, ancestors) = + setup_banks_on_fork_to_remove( + bank0.clone(), + pubkeys_to_modify.clone(), + &program_id, + starting_lamports, + 10, + 2, + ); + // Test removing the slot before the scan starts, should cause + // SlotRemoved error every time + for k in pubkeys_to_modify.iter() { + assert!(bank_at_fork_tip.load_slow(&ancestors, k).is_some()); + } bank_at_fork_tip.remove_unrooted_slots(&slots_on_fork); + + // Accounts on this fork should not be found after removal + for k in pubkeys_to_modify.iter() { + assert!(bank_at_fork_tip.load_slow(&ancestors, k).is_none()); + } if bank_to_scan_sender.send(bank_at_fork_tip.clone()).is_err() { return; } + + // Wait for scan to finish before starting next iteration + let finished_scan_slot_id = scan_finished_receiver.recv(); + if finished_scan_slot_id.is_err() { + return; + } + assert_eq!(finished_scan_slot_id.unwrap(), bank_at_fork_tip.slot_id()); } }, None, @@ -12286,7 +12349,7 @@ pub(crate) mod tests { } #[test] - fn test_remove_unrooted_scan_consistency_remove_and_recreate_same_slot_before_scan() { + fn test_remove_unrooted_scan_then_recreate_same_slot_before_scan() { for accounts_db_caching_enabled in &[false, true] { test_store_scan_consistency( *accounts_db_caching_enabled, @@ -12298,38 +12361,119 @@ pub(crate) mod tests { starting_lamports| { let mut prev_bank = bank0.clone(); loop { - let (bank_at_fork_tip, slots_on_fork) = setup_banks_on_fork_to_remove( - bank0.clone(), - pubkeys_to_modify.clone(), - &program_id, - starting_lamports, - ); + let start = Instant::now(); + let (bank_at_fork_tip, slots_on_fork, ancestors) = + setup_banks_on_fork_to_remove( + bank0.clone(), + pubkeys_to_modify.clone(), + &program_id, + starting_lamports, + 10, + 2, + ); + println!("setting up banks elapsed: {}", start.elapsed().as_millis()); // Remove the fork. Then we'll recreate the slots and only after we've // recreated the slots, do we send this old bank for scanning. // Skip scanning bank 0 on first iteration of loop, since those accounts // aren't being removed - if prev_bank.slot() != 0 + if prev_bank.slot() != 0 { + println!( + "sending bank with slot: {:?}, elapsed: {}", + prev_bank.slot(), + start.elapsed().as_millis() + ); + // Although we dumped the slots last iteration via `remove_unrooted_slots()`, + // we've recreated those slots this iteration, so they should be findable + // again + for k in pubkeys_to_modify.iter() { + assert!(bank_at_fork_tip.load_slow(&ancestors, k).is_some()); + } + // Now after we've recreated the slots removed in the previous loop // iteration, send the previous bank, should fail even though the // same slots were recreated - && bank_to_scan_sender.send(prev_bank.clone()).is_err() - { + if let Err(e) = bank_to_scan_sender.send(prev_bank.clone()) { + println!("returning because of error: {:?}", e); + return; + } + + let finished_scan_slot_id = scan_finished_receiver.recv(); + if finished_scan_slot_id.is_err() { + return; + } + // Wait for scan to finish before starting next iteration + assert_eq!(finished_scan_slot_id.unwrap(), prev_bank.slot_id()); + } + bank_at_fork_tip.remove_unrooted_slots(&slots_on_fork); + prev_bank = bank_at_fork_tip; + } + }, + None, + // Test removing the slot before the scan starts, should error every time + AcceptableScanResults::DroppedSlotError, + ); + } + } + + #[test] + fn test_remove_unrooted_scan_interleaved_with_remove_unrooted_slots() { + for accounts_db_caching_enabled in &[false, true] { + test_store_scan_consistency( + *accounts_db_caching_enabled, + |bank0, + bank_to_scan_sender, + scan_finished_receiver, + pubkeys_to_modify, + program_id, + starting_lamports| { + loop { + let step_size = 2; + let (bank_at_fork_tip, slots_on_fork, ancestors) = + setup_banks_on_fork_to_remove( + bank0.clone(), + pubkeys_to_modify.clone(), + &program_id, + starting_lamports, + 10, + step_size, + ); + // Although we dumped the slots last iteration via `remove_unrooted_slots()`, + // we've recreated those slots this iteration, so they should be findable + // again + for k in pubkeys_to_modify.iter() { + assert!(bank_at_fork_tip.load_slow(&ancestors, k).is_some()); + } + + // Now after we've recreated the slots removed in the previous loop + // iteration, send the previous bank, should fail even though the + // same slots were recreated + if let Err(e) = bank_to_scan_sender.send(bank_at_fork_tip.clone()) { + println!("returning because of error: {:?}", e); return; } + // Remove 1 < `step_size` of the *latest* slots while the scan is happening. + // This should create inconsistency between the account balances of accounts + // stored in that slot, and the accounts stored in earlier slots + let slot_to_remove = slots_on_fork.last().unwrap().clone(); + bank_at_fork_tip.remove_unrooted_slots(&[slot_to_remove]); + + // Wait for scan to finish before starting next iteration let finished_scan_slot_id = scan_finished_receiver.recv(); if finished_scan_slot_id.is_err() { return; } - // Wait for scan to finish before starting next iteration - assert_eq!(finished_scan_slot_id.unwrap(), prev_bank.slot_id()); - bank_at_fork_tip.remove_unrooted_slots(&slots_on_fork); - prev_bank = bank_at_fork_tip; + assert_eq!(finished_scan_slot_id.unwrap(), bank_at_fork_tip.slot_id()); + + // Remove the rest of the slots before the next iteration + for (slot, slot_id) in slots_on_fork { + bank_at_fork_tip.remove_unrooted_slots(&[(slot, slot_id)]); + } } }, None, // Test removing the slot before the scan starts, should error every time - AcceptableScanResults::DroppedSlotError, + AcceptableScanResults::Both, ); } } From b47e12bbd5a397d4cc6d4d69fbdcd7800ffb65c9 Mon Sep 17 00:00:00 2001 From: Carl Lin Date: Thu, 27 May 2021 15:49:22 -0700 Subject: [PATCH 07/12] Cleanup --- runtime/src/accounts_db.rs | 39 +++-------------------------------- runtime/src/accounts_index.rs | 4 ---- runtime/src/bank.rs | 26 +++++++---------------- 3 files changed, 11 insertions(+), 58 deletions(-) diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index 1ad1845324c..0320f6da73a 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -274,33 +274,6 @@ impl<'a> LoadedAccountAccessor<'a> { } } - fn get_loaded_account_logged(&mut self, pubkey: &Pubkey, slot: Slot) -> Option { - match self { - LoadedAccountAccessor::Cached(cached_account) => { - let i = cached_account.take(); - if i.is_none() { - println!("Failed to get {} {}", pubkey, slot); - } - let cached_account: (Pubkey, Cow<'a, CachedAccount>) = i.expect( - "Cache flushed/purged should be handled before trying to fetch account", - ); - Some(LoadedAccount::Cached(cached_account)) - } - LoadedAccountAccessor::Stored(maybe_storage_entry) => { - // storage entry may not be present if slot was cleaned up in - // between reading the accounts index and calling this function to - // get account meta from the storage entry here - maybe_storage_entry - .as_ref() - .and_then(|(storage_entry, offset)| { - storage_entry - .get_stored_account_meta(*offset) - .map(LoadedAccount::Stored) - }) - } - } - } - fn get_loaded_account(&mut self) -> Option { match self { LoadedAccountAccessor::Cached(cached_account) => { @@ -1610,7 +1583,6 @@ impl AccountsDb { self.accounts_index .handle_dead_keys(&dead_keys, &self.account_indexes); - println!("purging keys: {:?}", dead_keys); reclaims } @@ -2465,7 +2437,7 @@ impl AccountsDb { .scan_accounts(ancestors, slot_id, |pubkey, (account_info, slot)| { let account_slot = self .get_account_accessor(slot, pubkey, account_info.store_id, account_info.offset) - .get_loaded_account_logged(&pubkey, slot) + .get_loaded_account() .map(|loaded_account| (pubkey, loaded_account.take_account(), slot)); scan_func(&mut collector, account_slot) })?; @@ -3324,7 +3296,7 @@ impl AccountsDb { } /// This should only be called after the `Bank::drop()` runs in bank.rs, See BANK_DROP_SAFETY - /// comment below for why + /// comment below for more explanation. /// `is_from_abs` is true if the caller is the AccountsBackgroundService pub fn purge_slot(&self, slot: Slot, slot_id: SlotId, is_from_abs: bool) { if self.is_bank_drop_callback_enabled.load(Ordering::SeqCst) && !is_from_abs { @@ -3342,7 +3314,6 @@ impl AccountsDb { .remove(&slot_id) { // If this slot was already cleaned up, no need to do any further cleans - println!("purge_slot() slot_id: {}", slot_id); return; } @@ -3690,11 +3661,7 @@ impl AccountsDb { // their results { let mut locked_removed_slot_ids = self.accounts_index.removed_slot_ids.lock().unwrap(); - for (slot, remove_slot_id) in remove_slots.iter() { - println!( - "remove_unrooted_slots slot: {}, id: {}", - slot, remove_slot_id - ); + for (_slot, remove_slot_id) in remove_slots.iter() { locked_removed_slot_ids.insert(*remove_slot_id); } } diff --git a/runtime/src/accounts_index.rs b/runtime/src/accounts_index.rs index b4b4fd2cb74..d531f36a7ee 100644 --- a/runtime/src/accounts_index.rs +++ b/runtime/src/accounts_index.rs @@ -870,7 +870,6 @@ impl AccountsIndex { .contains(&scan_slot_id); if was_scan_corrupted { - println!("Scan was corrupted"); Err(ScanError::SlotRemoved { slot: ancestors.max_slot(), slot_id: scan_slot_id, @@ -916,7 +915,6 @@ impl AccountsIndex { let mut read_lock_elapsed = 0; let mut iterator_elapsed = 0; let mut iterator_timer = Measure::start("iterator_elapsed"); - let mut num_found = 0; for pubkey_list in self.iter(range) { iterator_timer.stop(); iterator_elapsed += iterator_timer.as_us(); @@ -932,7 +930,6 @@ impl AccountsIndex { latest_slot_elapsed += latest_slot_timer.as_us(); let mut load_account_timer = Measure::start("load_account"); func(&pubkey, (&list_r[index].1, list_r[index].0)); - num_found += 1; load_account_timer.stop(); load_account_elapsed += load_account_timer.as_us(); } @@ -940,7 +937,6 @@ impl AccountsIndex { iterator_timer = Measure::start("iterator_elapsed"); } - println!("Scan found {} keys", num_found); total_elapsed_timer.stop(); if !metric_name.is_empty() { datapoint_info!( diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 2289c378ed4..deeec2a0acc 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -11998,24 +11998,18 @@ pub(crate) mod tests { .spawn(move || { let mut num_banks_scanned = 0; loop { - println!("starting scan loop"); + info!("starting scan iteration"); if exit_.load(Relaxed) { - println!("scan exiting"); + info!("scan exiting"); return num_banks_scanned; } if let Ok(bank_to_scan) = bank_to_scan_receiver.recv_timeout(Duration::from_millis(10)) { - println!("scanning program accounts"); + info!("scanning program accounts for slot {}", bank_to_scan.slot()); let accounts_result = bank_to_scan.get_program_accounts(&program_id); let _ = scan_finished_sender.send(bank_to_scan.slot_id()); num_banks_scanned += 1; - println!( - "results: {:?} {:?} {}", - acceptable_scan_results, - accounts_result.is_err(), - num_banks_scanned - ); match (&acceptable_scan_results, accounts_result.is_err()) { (AcceptableScanResults::DroppedSlotError, _) | (AcceptableScanResults::Both, true) => { @@ -12037,7 +12031,6 @@ pub(crate) mod tests { // any of the original accounts, and the scan should reflect the // account state at some frozen slot `X` (no partial updates). if let Ok(accounts) = accounts_result { - println!("Scan found {} accounts", accounts.len()); assert!(!accounts.is_empty()); let mut expected_lamports = None; let mut target_accounts_found = HashSet::new(); @@ -12081,7 +12074,6 @@ pub(crate) mod tests { // Let threads run for a while, check the scans didn't see any mixed slots std::thread::sleep(Duration::new(5, 0)); - println!("storing exit"); exit.store(true, Relaxed); let min_expected_number_of_scans = 5; let num_banks_scanned = scan_thread.join().unwrap(); @@ -12371,13 +12363,13 @@ pub(crate) mod tests { 10, 2, ); - println!("setting up banks elapsed: {}", start.elapsed().as_millis()); + info!("setting up banks elapsed: {}", start.elapsed().as_millis()); // Remove the fork. Then we'll recreate the slots and only after we've // recreated the slots, do we send this old bank for scanning. // Skip scanning bank 0 on first iteration of loop, since those accounts // aren't being removed if prev_bank.slot() != 0 { - println!( + info!( "sending bank with slot: {:?}, elapsed: {}", prev_bank.slot(), start.elapsed().as_millis() @@ -12392,8 +12384,7 @@ pub(crate) mod tests { // Now after we've recreated the slots removed in the previous loop // iteration, send the previous bank, should fail even though the // same slots were recreated - if let Err(e) = bank_to_scan_sender.send(prev_bank.clone()) { - println!("returning because of error: {:?}", e); + if bank_to_scan_sender.send(prev_bank.clone()).is_err() { return; } @@ -12447,15 +12438,14 @@ pub(crate) mod tests { // Now after we've recreated the slots removed in the previous loop // iteration, send the previous bank, should fail even though the // same slots were recreated - if let Err(e) = bank_to_scan_sender.send(bank_at_fork_tip.clone()) { - println!("returning because of error: {:?}", e); + if bank_to_scan_sender.send(bank_at_fork_tip.clone()).is_err() { return; } // Remove 1 < `step_size` of the *latest* slots while the scan is happening. // This should create inconsistency between the account balances of accounts // stored in that slot, and the accounts stored in earlier slots - let slot_to_remove = slots_on_fork.last().unwrap().clone(); + let slot_to_remove = *slots_on_fork.last().unwrap(); bank_at_fork_tip.remove_unrooted_slots(&[slot_to_remove]); // Wait for scan to finish before starting next iteration From 49c3ccf676213ad411b844830529a7129c5ece7e Mon Sep 17 00:00:00 2001 From: Carl Lin Date: Wed, 2 Jun 2021 00:50:01 -0700 Subject: [PATCH 08/12] Use proper bank id --- core/src/repair_service.rs | 7 ++++++- programs/bpf/Cargo.lock | 1 + runtime/benches/accounts.rs | 10 ++++++++-- runtime/src/accounts_background_service.rs | 17 ++++++++++------- runtime/src/bank.rs | 1 + 5 files changed, 26 insertions(+), 10 deletions(-) diff --git a/core/src/repair_service.rs b/core/src/repair_service.rs index 5ad494fb9c4..2f5636933ff 100644 --- a/core/src/repair_service.rs +++ b/core/src/repair_service.rs @@ -17,7 +17,12 @@ use solana_ledger::{ }; use solana_measure::measure::Measure; use solana_runtime::{bank::Bank, bank_forks::BankForks, contains::Contains}; -use solana_sdk::{clock::Slot, epoch_schedule::EpochSchedule, pubkey::Pubkey, timing::timestamp}; +use solana_sdk::{ + clock::{Slot, SlotId}, + epoch_schedule::EpochSchedule, + pubkey::Pubkey, + timing::timestamp, +}; use std::{ collections::{HashMap, HashSet}, iter::Iterator, diff --git a/programs/bpf/Cargo.lock b/programs/bpf/Cargo.lock index b979dbf876a..c897893ba46 100644 --- a/programs/bpf/Cargo.lock +++ b/programs/bpf/Cargo.lock @@ -3170,6 +3170,7 @@ dependencies = [ "solana-clap-utils", "solana-faucet", "solana-net-utils", + "solana-runtime", "solana-sdk", "solana-transaction-status", "solana-version", diff --git a/runtime/benches/accounts.rs b/runtime/benches/accounts.rs index 7d925b8d6f1..c5c6e43fab8 100644 --- a/runtime/benches/accounts.rs +++ b/runtime/benches/accounts.rs @@ -260,7 +260,13 @@ fn bench_concurrent_read_write(bencher: &mut Bencher) { fn bench_concurrent_scan_write(bencher: &mut Bencher) { store_accounts_with_possible_contention("concurrent_scan_write", bencher, |accounts, _| loop { test::black_box( - accounts.load_by_program(&Ancestors::default(), AccountSharedData::default().owner()), + accounts + .load_by_program( + &Ancestors::default(), + 0, + AccountSharedData::default().owner(), + ) + .unwrap(), ); }) } @@ -388,7 +394,7 @@ fn bench_load_largest_accounts(b: &mut Bencher) { let account = AccountSharedData::new(lamports, 0, &Pubkey::default()); accounts.store_slow_uncached(0, &pubkey, &account); } - let ancestors = Ancestors::from(vec![(0, 0)]); + let ancestors = Ancestors::from(vec![0]); let slot_id = 0; b.iter(|| { accounts.load_largest_accounts( diff --git a/runtime/src/accounts_background_service.rs b/runtime/src/accounts_background_service.rs index 6a95e26a4f4..fe804fc49f1 100644 --- a/runtime/src/accounts_background_service.rs +++ b/runtime/src/accounts_background_service.rs @@ -12,7 +12,10 @@ use crossbeam_channel::{Receiver, SendError, Sender}; use log::*; use rand::{thread_rng, Rng}; use solana_measure::measure::Measure; -use solana_sdk::{clock::Slot, hash::Hash}; +use solana_sdk::{ + clock::{Slot, SlotId}, + hash::Hash, +}; use std::{ boxed::Box, fmt::{Debug, Formatter}, @@ -39,8 +42,8 @@ const RECYCLE_STORE_EXPIRATION_INTERVAL_SECS: u64 = crate::accounts_db::EXPIRATI pub type SnapshotRequestSender = Sender; pub type SnapshotRequestReceiver = Receiver; -pub type DroppedSlotsSender = Sender; -pub type DroppedSlotsReceiver = Receiver; +pub type DroppedSlotsSender = Sender<(Slot, SlotId)>; +pub type DroppedSlotsReceiver = Receiver<(Slot, SlotId)>; #[derive(Clone)] pub struct SendDroppedBankCallback { @@ -49,7 +52,7 @@ pub struct SendDroppedBankCallback { impl DropCallback for SendDroppedBankCallback { fn callback(&self, bank: &Bank) { - if let Err(e) = self.sender.send(bank.slot()) { + if let Err(e) = self.sender.send((bank.slot(), bank.slot_id())) { warn!("Error sending dropped banks: {:?}", e); } } @@ -266,11 +269,11 @@ impl AbsRequestHandler { /// `is_from_abs` is true if the caller is the AccountsBackgroundService pub fn handle_pruned_banks(&self, bank: &Bank, is_from_abs: bool) -> usize { let mut count = 0; - for pruned_slot in self.pruned_banks_receiver.try_iter() { + for (pruned_slot, pruned_slot_id) in self.pruned_banks_receiver.try_iter() { count += 1; bank.rc .accounts - .purge_slot(pruned_slot, bank.slot_id(), is_from_abs); + .purge_slot(pruned_slot, pruned_slot_id, is_from_abs); } count @@ -446,7 +449,7 @@ mod test { &AccountSharedData::new(264, 0, &Pubkey::default()), ); assert!(bank0.get_account(&account_key).is_some()); - pruned_banks_sender.send(0).unwrap(); + pruned_banks_sender.send((0, 0)).unwrap(); assert!(!bank0.rc.accounts.scan_slot(0, |_| Some(())).is_empty()); diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index deeec2a0acc..28a069151c0 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -432,6 +432,7 @@ impl AbiExample for BankRc { // AbiExample for Accounts is specially implemented to contain a storage example accounts: AbiExample::example(), slot: AbiExample::example(), + slot_id_generator: Arc::new(AtomicU64::new(0)), } } } From 911767b7689dd5480d4eb299a57a8c59d3079f87 Mon Sep 17 00:00:00 2001 From: Carl Lin Date: Fri, 4 Jun 2021 13:49:46 -0700 Subject: [PATCH 09/12] Enforce scan minimum in tests --- runtime/src/bank.rs | 127 ++++++++++++++++++++++++-------------------- 1 file changed, 68 insertions(+), 59 deletions(-) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 28a069151c0..815cb90fd20 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -11982,7 +11982,6 @@ pub(crate) mod tests { // Thread that runs scan and constantly checks for // consistency let pubkeys_to_modify_ = pubkeys_to_modify.clone(); - let exit_ = exit.clone(); // Channel over which the bank to scan is sent let (bank_to_scan_sender, bank_to_scan_receiver): ( @@ -11994,68 +11993,72 @@ pub(crate) mod tests { crossbeam_channel::Sender, crossbeam_channel::Receiver, ) = unbounded(); - let scan_thread = Builder::new() - .name("scan".to_string()) - .spawn(move || { - let mut num_banks_scanned = 0; - loop { - info!("starting scan iteration"); - if exit_.load(Relaxed) { - info!("scan exiting"); - return num_banks_scanned; - } - if let Ok(bank_to_scan) = - bank_to_scan_receiver.recv_timeout(Duration::from_millis(10)) - { - info!("scanning program accounts for slot {}", bank_to_scan.slot()); - let accounts_result = bank_to_scan.get_program_accounts(&program_id); - let _ = scan_finished_sender.send(bank_to_scan.slot_id()); - num_banks_scanned += 1; - match (&acceptable_scan_results, accounts_result.is_err()) { - (AcceptableScanResults::DroppedSlotError, _) - | (AcceptableScanResults::Both, true) => { - assert_eq!( - accounts_result, - Err(ScanError::SlotRemoved { - slot: bank_to_scan.slot(), - slot_id: bank_to_scan.slot_id() - }) - ); - } - (AcceptableScanResults::NoFailure, _) - | (AcceptableScanResults::Both, false) => { - assert!(accounts_result.is_ok()) - } + let num_banks_scanned = Arc::new(AtomicU64::new(0)); + let scan_thread = { + let exit = exit.clone(); + let num_banks_scanned = num_banks_scanned.clone(); + Builder::new() + .name("scan".to_string()) + .spawn(move || { + loop { + info!("starting scan iteration"); + if exit.load(Relaxed) { + info!("scan exiting"); + return; } + if let Ok(bank_to_scan) = + bank_to_scan_receiver.recv_timeout(Duration::from_millis(10)) + { + info!("scanning program accounts for slot {}", bank_to_scan.slot()); + let accounts_result = bank_to_scan.get_program_accounts(&program_id); + let _ = scan_finished_sender.send(bank_to_scan.slot_id()); + num_banks_scanned.fetch_add(1, Relaxed); + match (&acceptable_scan_results, accounts_result.is_err()) { + (AcceptableScanResults::DroppedSlotError, _) + | (AcceptableScanResults::Both, true) => { + assert_eq!( + accounts_result, + Err(ScanError::SlotRemoved { + slot: bank_to_scan.slot(), + slot_id: bank_to_scan.slot_id() + }) + ); + } + (AcceptableScanResults::NoFailure, _) + | (AcceptableScanResults::Both, false) => { + assert!(accounts_result.is_ok()) + } + } - // Should never see empty accounts because no slot ever deleted - // any of the original accounts, and the scan should reflect the - // account state at some frozen slot `X` (no partial updates). - if let Ok(accounts) = accounts_result { - assert!(!accounts.is_empty()); - let mut expected_lamports = None; - let mut target_accounts_found = HashSet::new(); - for (pubkey, account) in accounts { - let account_balance = account.lamports(); - if pubkeys_to_modify_.contains(&pubkey) { - target_accounts_found.insert(pubkey); - if let Some(expected_lamports) = expected_lamports { - assert_eq!(account_balance, expected_lamports); - } else { - // All pubkeys in the specified set should have the same balance - expected_lamports = Some(account_balance); + // Should never see empty accounts because no slot ever deleted + // any of the original accounts, and the scan should reflect the + // account state at some frozen slot `X` (no partial updates). + if let Ok(accounts) = accounts_result { + assert!(!accounts.is_empty()); + let mut expected_lamports = None; + let mut target_accounts_found = HashSet::new(); + for (pubkey, account) in accounts { + let account_balance = account.lamports(); + if pubkeys_to_modify_.contains(&pubkey) { + target_accounts_found.insert(pubkey); + if let Some(expected_lamports) = expected_lamports { + assert_eq!(account_balance, expected_lamports); + } else { + // All pubkeys in the specified set should have the same balance + expected_lamports = Some(account_balance); + } } } - } - // Should've found all the accounts, i.e. no partial cleans should - // be detected - assert_eq!(target_accounts_found.len(), total_pubkeys_to_modify); + // Should've found all the accounts, i.e. no partial cleans should + // be detected + assert_eq!(target_accounts_found.len(), total_pubkeys_to_modify); + } } } - } - }) - .unwrap(); + }) + .unwrap() + }; // Thread that constantly updates the accounts, sets // roots, and cleans @@ -12074,11 +12077,17 @@ pub(crate) mod tests { .unwrap(); // Let threads run for a while, check the scans didn't see any mixed slots + let min_expected_number_of_scans = 5; std::thread::sleep(Duration::new(5, 0)); + loop { + if num_banks_scanned.load(Relaxed) > min_expected_number_of_scans { + break; + } else { + std::thread::sleep(Duration::from_millis(100)); + } + } exit.store(true, Relaxed); - let min_expected_number_of_scans = 5; - let num_banks_scanned = scan_thread.join().unwrap(); - assert!(num_banks_scanned > min_expected_number_of_scans); + scan_thread.join().unwrap(); update_thread.join().unwrap(); } From ca71047ca86ea88f5deac78ecd8f71cbd3e7519b Mon Sep 17 00:00:00 2001 From: Carl Lin Date: Fri, 4 Jun 2021 21:35:31 -0700 Subject: [PATCH 10/12] Rename SlotId to BankId --- core/src/repair_service.rs | 12 ++-- runtime/benches/accounts.rs | 4 +- runtime/src/accounts.rs | 52 +++++++-------- runtime/src/accounts_background_service.rs | 12 ++-- runtime/src/accounts_db.rs | 76 +++++++++++----------- runtime/src/accounts_index.rs | 36 +++++----- runtime/src/bank.rs | 74 ++++++++++----------- sdk/program/src/clock.rs | 2 +- 8 files changed, 134 insertions(+), 134 deletions(-) diff --git a/core/src/repair_service.rs b/core/src/repair_service.rs index 2f5636933ff..bbfaf6d9f4f 100644 --- a/core/src/repair_service.rs +++ b/core/src/repair_service.rs @@ -18,7 +18,7 @@ use solana_ledger::{ use solana_measure::measure::Measure; use solana_runtime::{bank::Bank, bank_forks::BankForks, contains::Contains}; use solana_sdk::{ - clock::{Slot, SlotId}, + clock::{BankId, Slot}, epoch_schedule::EpochSchedule, pubkey::Pubkey, timing::timestamp, @@ -564,7 +564,7 @@ impl RepairService { #[allow(dead_code)] fn process_new_duplicate_slots( - new_duplicate_slots: &[(Slot, SlotId)], + new_duplicate_slots: &[(Slot, BankId)], duplicate_slot_repair_statuses: &mut HashMap, cluster_slots: &ClusterSlots, root_bank: &Bank, @@ -573,7 +573,7 @@ impl RepairService { duplicate_slots_reset_sender: &DuplicateSlotsResetSender, repair_validators: &Option>, ) { - for (slot, slot_id) in new_duplicate_slots { + for (slot, bank_id) in new_duplicate_slots { warn!( "Cluster confirmed slot: {}, dumping our current version and repairing", slot @@ -582,7 +582,7 @@ impl RepairService { root_bank.clear_slot_signatures(*slot); // Clear the accounts for this slot - root_bank.remove_unrooted_slots(&[(*slot, *slot_id)]); + root_bank.remove_unrooted_slots(&[(*slot, *bank_id)]); // Clear the slot-related data in blockstore. This will: // 1) Clear old shreds allowing new ones to be inserted @@ -1144,7 +1144,7 @@ mod test { ); let bank0 = Arc::new(Bank::new(&genesis_config)); let bank9 = Bank::new_from_parent(&bank0, &Pubkey::default(), duplicate_slot); - let duplicate_slot_id = bank9.slot_id(); + let duplicate_bank_id = bank9.bank_id(); let old_balance = bank9.get_balance(&keypairs.node_keypair.pubkey()); bank9 .transfer(10_000, &mint_keypair, &keypairs.node_keypair.pubkey()) @@ -1162,7 +1162,7 @@ mod test { assert!(bank9.get_signature_status(&vote_tx.signatures[0]).is_some()); RepairService::process_new_duplicate_slots( - &[(duplicate_slot, duplicate_slot_id)], + &[(duplicate_slot, duplicate_bank_id)], &mut duplicate_slot_repair_statuses, &cluster_slots, &bank9, diff --git a/runtime/benches/accounts.rs b/runtime/benches/accounts.rs index c5c6e43fab8..864789b3819 100644 --- a/runtime/benches/accounts.rs +++ b/runtime/benches/accounts.rs @@ -395,11 +395,11 @@ fn bench_load_largest_accounts(b: &mut Bencher) { accounts.store_slow_uncached(0, &pubkey, &account); } let ancestors = Ancestors::from(vec![0]); - let slot_id = 0; + let bank_id = 0; b.iter(|| { accounts.load_largest_accounts( &ancestors, - slot_id, + bank_id, 20, &HashSet::new(), AccountAddressFilter::Exclude, diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 34796c9b63e..c04378494f1 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -23,7 +23,7 @@ use solana_sdk::{ account::{Account, AccountSharedData, ReadableAccount, WritableAccount}, account_utils::StateMut, bpf_loader_upgradeable::{self, UpgradeableLoaderState}, - clock::{Slot, SlotId, INITIAL_RENT_EPOCH}, + clock::{BankId, Slot, INITIAL_RENT_EPOCH}, feature_set::{self, FeatureSet}, fee_calculator::{FeeCalculator, FeeConfig}, genesis_config::ClusterType, @@ -585,7 +585,7 @@ impl Accounts { pub fn load_largest_accounts( &self, ancestors: &Ancestors, - slot_id: SlotId, + bank_id: BankId, num: usize, filter_by_address: &HashSet, filter: AccountAddressFilter, @@ -595,7 +595,7 @@ impl Accounts { } let account_balances = self.accounts_db.scan_accounts( ancestors, - slot_id, + bank_id, |collector: &mut BinaryHeap>, option| { if let Some((pubkey, account, _slot)) = option { if account.lamports() == 0 { @@ -685,12 +685,12 @@ impl Accounts { pub fn load_by_program( &self, ancestors: &Ancestors, - slot_id: SlotId, + bank_id: BankId, program_id: &Pubkey, ) -> ScanResult> { self.accounts_db.scan_accounts( ancestors, - slot_id, + bank_id, |collector: &mut Vec<(Pubkey, AccountSharedData)>, some_account_tuple| { Self::load_while_filtering(collector, some_account_tuple, |account| { account.owner() == program_id @@ -702,13 +702,13 @@ impl Accounts { pub fn load_by_program_with_filter bool>( &self, ancestors: &Ancestors, - slot_id: SlotId, + bank_id: BankId, program_id: &Pubkey, filter: F, ) -> ScanResult> { self.accounts_db.scan_accounts( ancestors, - slot_id, + bank_id, |collector: &mut Vec<(Pubkey, AccountSharedData)>, some_account_tuple| { Self::load_while_filtering(collector, some_account_tuple, |account| { account.owner() == program_id && filter(account) @@ -720,14 +720,14 @@ impl Accounts { pub fn load_by_index_key_with_filter bool>( &self, ancestors: &Ancestors, - slot_id: SlotId, + bank_id: BankId, index_key: &IndexKey, filter: F, ) -> ScanResult> { self.accounts_db .index_scan_accounts( ancestors, - slot_id, + bank_id, *index_key, |collector: &mut Vec<(Pubkey, AccountSharedData)>, some_account_tuple| { Self::load_while_filtering(collector, some_account_tuple, |account| { @@ -745,11 +745,11 @@ impl Accounts { pub fn load_all( &self, ancestors: &Ancestors, - slot_id: SlotId, + bank_id: BankId, ) -> ScanResult> { self.accounts_db.scan_accounts( ancestors, - slot_id, + bank_id, |collector: &mut Vec<(Pubkey, AccountSharedData, Slot)>, some_account_tuple| { if let Some((pubkey, account, slot)) = some_account_tuple .filter(|(_, account, _)| Self::is_loadable(account.lamports())) @@ -940,8 +940,8 @@ impl Accounts { /// Purge a slot if it is not a root /// Root slots cannot be purged /// `is_from_abs` is true if the caller is the AccountsBackgroundService - pub fn purge_slot(&self, slot: Slot, slot_id: SlotId, is_from_abs: bool) { - self.accounts_db.purge_slot(slot, slot_id, is_from_abs); + pub fn purge_slot(&self, slot: Slot, bank_id: BankId, is_from_abs: bool) { + self.accounts_db.purge_slot(slot, bank_id, is_from_abs); } /// Add a slot to root. Root slots cannot be purged @@ -2589,12 +2589,12 @@ mod tests { let all_pubkeys: HashSet<_> = vec![pubkey0, pubkey1, pubkey2].into_iter().collect(); // num == 0 should always return empty set - let slot_id = 0; + let bank_id = 0; assert_eq!( accounts .load_largest_accounts( &ancestors, - slot_id, + bank_id, 0, &HashSet::new(), AccountAddressFilter::Exclude @@ -2606,7 +2606,7 @@ mod tests { accounts .load_largest_accounts( &ancestors, - slot_id, + bank_id, 0, &all_pubkeys, AccountAddressFilter::Include @@ -2621,7 +2621,7 @@ mod tests { accounts .load_largest_accounts( &ancestors, - slot_id, + bank_id, 1, &HashSet::new(), AccountAddressFilter::Exclude @@ -2633,7 +2633,7 @@ mod tests { accounts .load_largest_accounts( &ancestors, - slot_id, + bank_id, 2, &HashSet::new(), AccountAddressFilter::Exclude @@ -2645,7 +2645,7 @@ mod tests { accounts .load_largest_accounts( &ancestors, - slot_id, + bank_id, 3, &HashSet::new(), AccountAddressFilter::Exclude @@ -2659,7 +2659,7 @@ mod tests { accounts .load_largest_accounts( &ancestors, - slot_id, + bank_id, 6, &HashSet::new(), AccountAddressFilter::Exclude @@ -2674,7 +2674,7 @@ mod tests { accounts .load_largest_accounts( &ancestors, - slot_id, + bank_id, 1, &exclude1, AccountAddressFilter::Exclude @@ -2686,7 +2686,7 @@ mod tests { accounts .load_largest_accounts( &ancestors, - slot_id, + bank_id, 2, &exclude1, AccountAddressFilter::Exclude @@ -2698,7 +2698,7 @@ mod tests { accounts .load_largest_accounts( &ancestors, - slot_id, + bank_id, 3, &exclude1, AccountAddressFilter::Exclude @@ -2713,7 +2713,7 @@ mod tests { accounts .load_largest_accounts( &ancestors, - slot_id, + bank_id, 1, &include1_2, AccountAddressFilter::Include @@ -2725,7 +2725,7 @@ mod tests { accounts .load_largest_accounts( &ancestors, - slot_id, + bank_id, 2, &include1_2, AccountAddressFilter::Include @@ -2737,7 +2737,7 @@ mod tests { accounts .load_largest_accounts( &ancestors, - slot_id, + bank_id, 3, &include1_2, AccountAddressFilter::Include diff --git a/runtime/src/accounts_background_service.rs b/runtime/src/accounts_background_service.rs index fe804fc49f1..a0100b2cb58 100644 --- a/runtime/src/accounts_background_service.rs +++ b/runtime/src/accounts_background_service.rs @@ -13,7 +13,7 @@ use log::*; use rand::{thread_rng, Rng}; use solana_measure::measure::Measure; use solana_sdk::{ - clock::{Slot, SlotId}, + clock::{BankId, Slot}, hash::Hash, }; use std::{ @@ -42,8 +42,8 @@ const RECYCLE_STORE_EXPIRATION_INTERVAL_SECS: u64 = crate::accounts_db::EXPIRATI pub type SnapshotRequestSender = Sender; pub type SnapshotRequestReceiver = Receiver; -pub type DroppedSlotsSender = Sender<(Slot, SlotId)>; -pub type DroppedSlotsReceiver = Receiver<(Slot, SlotId)>; +pub type DroppedSlotsSender = Sender<(Slot, BankId)>; +pub type DroppedSlotsReceiver = Receiver<(Slot, BankId)>; #[derive(Clone)] pub struct SendDroppedBankCallback { @@ -52,7 +52,7 @@ pub struct SendDroppedBankCallback { impl DropCallback for SendDroppedBankCallback { fn callback(&self, bank: &Bank) { - if let Err(e) = self.sender.send((bank.slot(), bank.slot_id())) { + if let Err(e) = self.sender.send((bank.slot(), bank.bank_id())) { warn!("Error sending dropped banks: {:?}", e); } } @@ -269,11 +269,11 @@ impl AbsRequestHandler { /// `is_from_abs` is true if the caller is the AccountsBackgroundService pub fn handle_pruned_banks(&self, bank: &Bank, is_from_abs: bool) -> usize { let mut count = 0; - for (pruned_slot, pruned_slot_id) in self.pruned_banks_receiver.try_iter() { + for (pruned_slot, pruned_bank_id) in self.pruned_banks_receiver.try_iter() { count += 1; bank.rc .accounts - .purge_slot(pruned_slot, pruned_slot_id, is_from_abs); + .purge_slot(pruned_slot, pruned_bank_id, is_from_abs); } count diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index 0320f6da73a..ce749290420 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -48,7 +48,7 @@ use solana_measure::measure::Measure; use solana_rayon_threadlimit::get_thread_count; use solana_sdk::{ account::{AccountSharedData, ReadableAccount}, - clock::{Epoch, Slot, SlotId}, + clock::{BankId, Epoch, Slot}, genesis_config::ClusterType, hash::{Hash, Hasher}, pubkey::Pubkey, @@ -1506,7 +1506,7 @@ impl AccountsDb { }; if no_delete { let mut pending_store_ids: HashSet = HashSet::new(); - for (_slot_id, account_info) in account_infos { + for (_bank_id, account_info) in account_infos { if !already_counted.contains(&account_info.store_id) { pending_store_ids.insert(account_info.store_id); } @@ -2423,7 +2423,7 @@ impl AccountsDb { pub fn scan_accounts( &self, ancestors: &Ancestors, - slot_id: SlotId, + bank_id: BankId, scan_func: F, ) -> ScanResult where @@ -2434,7 +2434,7 @@ impl AccountsDb { // This can error out if the slots being scanned over are aborted self.accounts_index - .scan_accounts(ancestors, slot_id, |pubkey, (account_info, slot)| { + .scan_accounts(ancestors, bank_id, |pubkey, (account_info, slot)| { let account_slot = self .get_account_accessor(slot, pubkey, account_info.store_id, account_info.offset) .get_loaded_account() @@ -2512,7 +2512,7 @@ impl AccountsDb { pub fn index_scan_accounts( &self, ancestors: &Ancestors, - slot_id: SlotId, + bank_id: BankId, index_key: IndexKey, scan_func: F, ) -> ScanResult<(A, bool)> @@ -2528,14 +2528,14 @@ impl AccountsDb { if !self.account_indexes.include_key(key) { // the requested key was not indexed in the secondary index, so do a normal scan let used_index = false; - let scan_result = self.scan_accounts(ancestors, slot_id, scan_func)?; + let scan_result = self.scan_accounts(ancestors, bank_id, scan_func)?; return Ok((scan_result, used_index)); } let mut collector = A::default(); self.accounts_index.index_scan_accounts( ancestors, - slot_id, + bank_id, index_key, |pubkey, (account_info, slot)| { let account_slot = self @@ -3298,20 +3298,20 @@ impl AccountsDb { /// This should only be called after the `Bank::drop()` runs in bank.rs, See BANK_DROP_SAFETY /// comment below for more explanation. /// `is_from_abs` is true if the caller is the AccountsBackgroundService - pub fn purge_slot(&self, slot: Slot, slot_id: SlotId, is_from_abs: bool) { + pub fn purge_slot(&self, slot: Slot, bank_id: BankId, is_from_abs: bool) { if self.is_bank_drop_callback_enabled.load(Ordering::SeqCst) && !is_from_abs { panic!("bad drop callpath detected; Bank::drop() must run serially with other logic in ABS like clean_accounts()") } // BANK_DROP_SAFETY: Because this function only runs once the bank is dropped, // we know that there are no longer any ongoing scans on this bank, because scans require // and hold a reference to the bank at the tip of the fork they're scanning. Hence it's - // safe to remove this slot_id from the `removed_slot_ids` list at this point. + // safe to remove this bank_id from the `removed_bank_ids` list at this point. if self .accounts_index - .removed_slot_ids + .removed_bank_ids .lock() .unwrap() - .remove(&slot_id) + .remove(&bank_id) { // If this slot was already cleaned up, no need to do any further cleans return; @@ -3575,7 +3575,7 @@ impl AccountsDb { // // Also note roots are never removed via `remove_unrooted_slot()`, so // it's safe to filter them out here as they won't need deletion from - // self.accounts_index.removed_slot_ids in `purge_slots_from_cache_and_store()`. + // self.accounts_index.removed_bank_ids in `purge_slots_from_cache_and_store()`. .filter(|slot| !self.accounts_index.is_root(**slot)); safety_checks_elapsed.stop(); self.external_purge_slots_stats @@ -3586,7 +3586,7 @@ impl AccountsDb { .report("external_purge_slots_stats", Some(1000)); } - pub fn remove_unrooted_slots(&self, remove_slots: &[(Slot, SlotId)]) { + pub fn remove_unrooted_slots(&self, remove_slots: &[(Slot, BankId)]) { let rooted_slots = self .accounts_index .get_rooted_from_list(remove_slots.iter().map(|(slot, _)| slot)); @@ -3660,9 +3660,9 @@ impl AccountsDb { // banks fail, and any ongoing scans over these slots will detect that they should abort // their results { - let mut locked_removed_slot_ids = self.accounts_index.removed_slot_ids.lock().unwrap(); - for (_slot, remove_slot_id) in remove_slots.iter() { - locked_removed_slot_ids.insert(*remove_slot_id); + let mut locked_removed_bank_ids = self.accounts_index.removed_bank_ids.lock().unwrap(); + for (_slot, remove_bank_id) in remove_slots.iter() { + locked_removed_bank_ids.insert(*remove_bank_id); } } @@ -6871,7 +6871,7 @@ pub mod tests { fn run_test_remove_unrooted_slot(is_cached: bool) { let unrooted_slot = 9; - let unrooted_slot_id = 9; + let unrooted_bank_id = 9; let mut db = AccountsDb::new(Vec::new(), &ClusterType::Development); db.caching_enabled = true; let key = Pubkey::default(); @@ -6893,7 +6893,7 @@ pub mod tests { assert_load_account(&db, unrooted_slot, key, 1); // Purge the slot - db.remove_unrooted_slots(&[(unrooted_slot, unrooted_slot_id)]); + db.remove_unrooted_slots(&[(unrooted_slot, unrooted_bank_id)]); assert!(db.load_without_fixed_root(&ancestors, &key).is_none()); assert!(db.bank_hashes.read().unwrap().get(&unrooted_slot).is_none()); assert!(db.accounts_cache.slot_cache(unrooted_slot).is_none()); @@ -6924,14 +6924,14 @@ pub mod tests { fn test_remove_unrooted_slot_snapshot() { solana_logger::setup(); let unrooted_slot = 9; - let unrooted_slot_id = 9; + let unrooted_bank_id = 9; let db = AccountsDb::new(Vec::new(), &ClusterType::Development); let key = solana_sdk::pubkey::new_rand(); let account0 = AccountSharedData::new(1, 0, &key); db.store_uncached(unrooted_slot, &[(&key, &account0)]); // Purge the slot - db.remove_unrooted_slots(&[(unrooted_slot, unrooted_slot_id)]); + db.remove_unrooted_slots(&[(unrooted_slot, unrooted_bank_id)]); // Add a new root let key2 = solana_sdk::pubkey::new_rand(); @@ -7556,10 +7556,10 @@ pub mod tests { // Secondary index should still find both pubkeys let mut found_accounts = HashSet::new(); let index_key = IndexKey::SplTokenMint(mint_key); - let slot_id = 0; + let bank_id = 0; accounts .accounts_index - .index_scan_accounts(&Ancestors::default(), slot_id, index_key, |key, _| { + .index_scan_accounts(&Ancestors::default(), bank_id, index_key, |key, _| { found_accounts.insert(*key); }) .unwrap(); @@ -7576,7 +7576,7 @@ pub mod tests { let found_accounts = accounts .index_scan_accounts( &Ancestors::default(), - slot_id, + bank_id, index_key, |collection: &mut HashSet, account| { collection.insert(*account.unwrap().0); @@ -7594,7 +7594,7 @@ pub mod tests { let found_accounts = accounts .index_scan_accounts( &Ancestors::default(), - slot_id, + bank_id, index_key, |collection: &mut HashSet, account| { collection.insert(*account.unwrap().0); @@ -7629,7 +7629,7 @@ pub mod tests { .accounts_index .index_scan_accounts( &Ancestors::default(), - slot_id, + bank_id, IndexKey::SplTokenMint(mint_key), |key, _| found_accounts.push(*key), ) @@ -10107,7 +10107,7 @@ pub mod tests { fn setup_scan( db: Arc, scan_ancestors: Arc, - slot_id: SlotId, + bank_id: BankId, stall_key: Pubkey, ) -> ScanTracker { let exit = Arc::new(AtomicBool::new(false)); @@ -10120,7 +10120,7 @@ pub mod tests { .spawn(move || { db.scan_accounts( &scan_ancestors, - slot_id, + bank_id, |_collector: &mut Vec<(Pubkey, AccountSharedData)>, maybe_account| { ready_.store(true, Ordering::Relaxed); if let Some((pubkey, _, _)) = maybe_account { @@ -10182,8 +10182,8 @@ pub mod tests { let max_scan_root = 0; db.add_root(max_scan_root); let scan_ancestors: Arc = Arc::new(vec![(0, 1), (1, 1)].into_iter().collect()); - let slot_id = 0; - let scan_tracker = setup_scan(db.clone(), scan_ancestors.clone(), slot_id, account_key2); + let bank_id = 0; + let scan_tracker = setup_scan(db.clone(), scan_ancestors.clone(), bank_id, account_key2); // Add a new root 2 let new_root = 2; @@ -10243,8 +10243,8 @@ pub mod tests { assert_eq!(account.0.lamports(), slot1_account.lamports()); // Simulate dropping the bank, which finally removes the slot from the cache - let slot_id = 1; - db.purge_slot(1, slot_id, false); + let bank_id = 1; + db.purge_slot(1, bank_id, false); assert!(db .do_load( &scan_ancestors, @@ -10349,11 +10349,11 @@ pub mod tests { accounts_db.add_root(*slot as Slot); if Some(*slot) == scan_slot { let ancestors = Arc::new(vec![(stall_slot, 1), (*slot, 1)].into_iter().collect()); - let slot_id = 0; + let bank_id = 0; scan_tracker = Some(setup_scan( accounts_db.clone(), ancestors, - slot_id, + bank_id, scan_stall_key, )); assert_eq!( @@ -11188,10 +11188,10 @@ pub mod tests { account.set_lamports(lamports); // Pick random 50% of the slots to pass to `remove_unrooted_slots()` - let mut all_slots: Vec<(Slot, SlotId)> = (0..num_cached_slots) + let mut all_slots: Vec<(Slot, BankId)> = (0..num_cached_slots) .map(|slot| { - let slot_id = slot + 1; - (slot, slot_id) + let bank_id = slot + 1; + (slot, bank_id) }) .collect(); all_slots.shuffle(&mut rand::thread_rng()); @@ -11239,7 +11239,7 @@ pub mod tests { // Wait for flush to finish before starting next trial flush_done_receiver.recv().unwrap(); - for (slot, slot_id) in slots_to_keep { + for (slot, bank_id) in slots_to_keep { let account_in_slot = slot_to_pubkey_map[slot]; assert!(db .load( @@ -11250,7 +11250,7 @@ pub mod tests { .is_some()); // Clear for next iteration so that `assert!(self.storage.get_slot_stores(purged_slot).is_none());` // in `purge_slot_pubkeys()` doesn't trigger - db.remove_unrooted_slots(&[(*slot, *slot_id)]); + db.remove_unrooted_slots(&[(*slot, *bank_id)]); } } diff --git a/runtime/src/accounts_index.rs b/runtime/src/accounts_index.rs index d531f36a7ee..ca69c1be70e 100644 --- a/runtime/src/accounts_index.rs +++ b/runtime/src/accounts_index.rs @@ -10,7 +10,7 @@ use log::*; use ouroboros::self_referencing; use solana_measure::measure::Measure; use solana_sdk::{ - clock::{Slot, SlotId}, + clock::{BankId, Slot}, pubkey::{Pubkey, PUBKEY_BYTES}, }; use std::{ @@ -58,8 +58,8 @@ impl IsCached for u64 { #[derive(Error, Debug, PartialEq)] pub enum ScanError { - #[error("Node detected it replayed bad version of slot {slot:?} with id {slot_id:?}, thus the scan on said slot was aborted")] - SlotRemoved { slot: Slot, slot_id: SlotId }, + #[error("Node detected it replayed bad version of slot {slot:?} with id {bank_id:?}, thus the scan on said slot was aborted")] + SlotRemoved { slot: Slot, bank_id: BankId }, } enum ScanTypes> { @@ -626,10 +626,10 @@ pub struct AccountsIndex { // `AccountsDb::remove_unrooted_slots()`. When the scan finishes, it'll realize that the // results of the scan may have been corrupted by `remove_unrooted_slots` and abort its results. // - // `removed_slot_ids` tracks all the slot ids that were removed via `remove_unrooted_slots()` so any attempted scans + // `removed_bank_ids` tracks all the slot ids that were removed via `remove_unrooted_slots()` so any attempted scans // on any of these slots fails. This is safe to purge once the associated Bank is dropped and // scanning the fork with that Bank at the tip is no longer possible. - pub removed_slot_ids: Mutex>, + pub removed_bank_ids: Mutex>, zero_lamport_pubkeys: DashSet, } @@ -648,7 +648,7 @@ impl Default for AccountsIndex { ), roots_tracker: RwLock::::default(), ongoing_scan_roots: RwLock::>::default(), - removed_slot_ids: Mutex::>::default(), + removed_bank_ids: Mutex::>::default(), zero_lamport_pubkeys: DashSet::::default(), } } @@ -666,7 +666,7 @@ impl AccountsIndex { &self, metric_name: &'static str, ancestors: &Ancestors, - scan_slot_id: SlotId, + scan_bank_id: BankId, func: F, scan_type: ScanTypes, ) -> Result<(), ScanError> @@ -675,11 +675,11 @@ impl AccountsIndex { R: RangeBounds, { { - let locked_removed_slot_ids = self.removed_slot_ids.lock().unwrap(); - if locked_removed_slot_ids.contains(&scan_slot_id) { + let locked_removed_bank_ids = self.removed_bank_ids.lock().unwrap(); + if locked_removed_bank_ids.contains(&scan_bank_id) { return Err(ScanError::SlotRemoved { slot: ancestors.max_slot(), - slot_id: scan_slot_id, + bank_id: scan_bank_id, }); } } @@ -861,18 +861,18 @@ impl AccountsIndex { } } - // If the fork with tip at bank `scan_slot_id` was removed durin our scan, then the scan + // If the fork with tip at bank `scan_bank_id` was removed durin our scan, then the scan // may have been corrupted, so abort the results. let was_scan_corrupted = self - .removed_slot_ids + .removed_bank_ids .lock() .unwrap() - .contains(&scan_slot_id); + .contains(&scan_bank_id); if was_scan_corrupted { Err(ScanError::SlotRemoved { slot: ancestors.max_slot(), - slot_id: scan_slot_id, + bank_id: scan_bank_id, }) } else { Ok(()) @@ -1079,7 +1079,7 @@ impl AccountsIndex { pub(crate) fn scan_accounts( &self, ancestors: &Ancestors, - scan_slot_id: SlotId, + scan_bank_id: BankId, func: F, ) -> Result<(), ScanError> where @@ -1089,7 +1089,7 @@ impl AccountsIndex { self.do_checked_scan_accounts( "", ancestors, - scan_slot_id, + scan_bank_id, func, ScanTypes::Unindexed(None::>), ) @@ -1125,7 +1125,7 @@ impl AccountsIndex { pub(crate) fn index_scan_accounts( &self, ancestors: &Ancestors, - scan_slot_id: SlotId, + scan_bank_id: BankId, index_key: IndexKey, func: F, ) -> Result<(), ScanError> @@ -1136,7 +1136,7 @@ impl AccountsIndex { self.do_checked_scan_accounts( "", ancestors, - scan_slot_id, + scan_bank_id, func, ScanTypes::>::Indexed(index_key), ) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 815cb90fd20..c8bc7780afa 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -68,7 +68,7 @@ use solana_sdk::{ AccountSharedData, InheritableAccountFields, ReadableAccount, WritableAccount, }, clock::{ - Epoch, Slot, SlotCount, SlotId, SlotIndex, UnixTimestamp, DEFAULT_TICKS_PER_SECOND, + BankId, Epoch, Slot, SlotCount, SlotIndex, UnixTimestamp, DEFAULT_TICKS_PER_SECOND, INITIAL_RENT_EPOCH, MAX_PROCESSING_AGE, MAX_RECENT_BLOCKHASHES, MAX_TRANSACTION_FORWARDING_DELAY, SECONDS_PER_DAY, }, @@ -417,7 +417,7 @@ pub struct BankRc { /// Current slot pub(crate) slot: Slot, - pub(crate) slot_id_generator: Arc, + pub(crate) bank_id_generator: Arc, } #[cfg(RUSTC_WITH_SPECIALIZATION)] @@ -432,7 +432,7 @@ impl AbiExample for BankRc { // AbiExample for Accounts is specially implemented to contain a storage example accounts: AbiExample::example(), slot: AbiExample::example(), - slot_id_generator: Arc::new(AtomicU64::new(0)), + bank_id_generator: Arc::new(AtomicU64::new(0)), } } } @@ -443,7 +443,7 @@ impl BankRc { accounts: Arc::new(accounts), parent: RwLock::new(None), slot, - slot_id_generator: Arc::new(AtomicU64::new(0)), + bank_id_generator: Arc::new(AtomicU64::new(0)), } } @@ -913,7 +913,7 @@ pub struct Bank { /// Bank slot (i.e. block) slot: Slot, - slot_id: SlotId, + bank_id: BankId, /// Bank epoch epoch: Epoch, @@ -1135,7 +1135,7 @@ impl Bank { )), parent: RwLock::new(Some(parent.clone())), slot, - slot_id_generator: parent.rc.slot_id_generator.clone(), + bank_id_generator: parent.rc.bank_id_generator.clone(), }; let src = StatusCacheRc { status_cache: parent.src.status_cache.clone(), @@ -1144,12 +1144,12 @@ impl Bank { let fee_rate_governor = FeeRateGovernor::new_derived(&parent.fee_rate_governor, parent.signature_count()); - let slot_id = rc.slot_id_generator.fetch_add(1, Relaxed) + 1; + let bank_id = rc.bank_id_generator.fetch_add(1, Relaxed) + 1; let mut new = Bank { rc, src, slot, - slot_id, + bank_id, epoch, blockhash_queue: RwLock::new(parent.blockhash_queue.read().unwrap().clone()), @@ -1328,7 +1328,7 @@ impl Bank { slots_per_year: fields.slots_per_year, unused: genesis_config.unused, slot: fields.slot, - slot_id: 0, + bank_id: 0, epoch: fields.epoch, block_height: fields.block_height, collector_id: fields.collector_id, @@ -1447,8 +1447,8 @@ impl Bank { self.slot } - pub fn slot_id(&self) -> SlotId { - self.slot_id + pub fn bank_id(&self) -> BankId { + self.bank_id } pub fn epoch(&self) -> Epoch { @@ -2668,7 +2668,7 @@ impl Bank { } } - pub fn remove_unrooted_slots(&self, slots: &[(Slot, SlotId)]) { + pub fn remove_unrooted_slots(&self, slots: &[(Slot, BankId)]) { self.rc.accounts.accounts_db.remove_unrooted_slots(slots) } @@ -4351,7 +4351,7 @@ impl Bank { ) -> ScanResult> { self.rc .accounts - .load_by_program(&self.ancestors, self.slot_id, program_id) + .load_by_program(&self.ancestors, self.bank_id, program_id) } pub fn get_filtered_program_accounts bool>( @@ -4361,7 +4361,7 @@ impl Bank { ) -> ScanResult> { self.rc.accounts.load_by_program_with_filter( &self.ancestors, - self.slot_id, + self.bank_id, program_id, filter, ) @@ -4374,7 +4374,7 @@ impl Bank { ) -> ScanResult> { self.rc.accounts.load_by_index_key_with_filter( &self.ancestors, - self.slot_id, + self.bank_id, index_key, filter, ) @@ -4387,7 +4387,7 @@ impl Bank { pub fn get_all_accounts_with_modified_slots( &self, ) -> ScanResult> { - self.rc.accounts.load_all(&self.ancestors, self.slot_id) + self.rc.accounts.load_all(&self.ancestors, self.bank_id) } pub fn get_program_accounts_modified_since_parent( @@ -4445,7 +4445,7 @@ impl Bank { ) -> ScanResult> { self.rc.accounts.load_largest_accounts( &self.ancestors, - self.slot_id, + self.bank_id, num, filter_by_address, filter, @@ -5281,7 +5281,7 @@ impl Drop for Bank { // AccountsBackgroundService to perform cleanups yet. self.rc .accounts - .purge_slot(self.slot(), self.slot_id(), false); + .purge_slot(self.slot(), self.bank_id(), false); } } } @@ -11933,7 +11933,7 @@ pub(crate) mod tests { F: Fn( Arc, crossbeam_channel::Sender>, - crossbeam_channel::Receiver, + crossbeam_channel::Receiver, Arc>, Pubkey, u64, @@ -11990,8 +11990,8 @@ pub(crate) mod tests { ) = bounded(1); let (scan_finished_sender, scan_finished_receiver): ( - crossbeam_channel::Sender, - crossbeam_channel::Receiver, + crossbeam_channel::Sender, + crossbeam_channel::Receiver, ) = unbounded(); let num_banks_scanned = Arc::new(AtomicU64::new(0)); let scan_thread = { @@ -12011,7 +12011,7 @@ pub(crate) mod tests { { info!("scanning program accounts for slot {}", bank_to_scan.slot()); let accounts_result = bank_to_scan.get_program_accounts(&program_id); - let _ = scan_finished_sender.send(bank_to_scan.slot_id()); + let _ = scan_finished_sender.send(bank_to_scan.bank_id()); num_banks_scanned.fetch_add(1, Relaxed); match (&acceptable_scan_results, accounts_result.is_err()) { (AcceptableScanResults::DroppedSlotError, _) @@ -12020,7 +12020,7 @@ pub(crate) mod tests { accounts_result, Err(ScanError::SlotRemoved { slot: bank_to_scan.slot(), - slot_id: bank_to_scan.slot_id() + bank_id: bank_to_scan.bank_id() }) ); } @@ -12250,7 +12250,7 @@ pub(crate) mod tests { starting_lamports: u64, num_banks_on_fork: usize, step_size: usize, - ) -> (Arc, Vec<(Slot, SlotId)>, Ancestors) { + ) -> (Arc, Vec<(Slot, BankId)>, Ancestors) { // Need at least 2 keys to create inconsistency in account balances when deleting // slots assert!(pubkeys_to_modify.len() > 1); @@ -12278,7 +12278,7 @@ pub(crate) mod tests { bank_at_fork_tip.slot() + 1, )); if lamports_this_round == 0 { - lamports_this_round = bank_at_fork_tip.slot_id() + starting_lamports + 1; + lamports_this_round = bank_at_fork_tip.bank_id() + starting_lamports + 1; } let pubkey_to_modify_starting_index = i * pubkeys_to_modify_per_slot; let account = AccountSharedData::new(lamports_this_round, 0, program_id); @@ -12289,7 +12289,7 @@ pub(crate) mod tests { bank_at_fork_tip.store_account(&key, &account); } bank_at_fork_tip.freeze(); - slots_on_fork.push((bank_at_fork_tip.slot(), bank_at_fork_tip.slot_id())); + slots_on_fork.push((bank_at_fork_tip.slot(), bank_at_fork_tip.bank_id())); } } @@ -12336,11 +12336,11 @@ pub(crate) mod tests { } // Wait for scan to finish before starting next iteration - let finished_scan_slot_id = scan_finished_receiver.recv(); - if finished_scan_slot_id.is_err() { + let finished_scan_bank_id = scan_finished_receiver.recv(); + if finished_scan_bank_id.is_err() { return; } - assert_eq!(finished_scan_slot_id.unwrap(), bank_at_fork_tip.slot_id()); + assert_eq!(finished_scan_bank_id.unwrap(), bank_at_fork_tip.bank_id()); } }, None, @@ -12398,12 +12398,12 @@ pub(crate) mod tests { return; } - let finished_scan_slot_id = scan_finished_receiver.recv(); - if finished_scan_slot_id.is_err() { + let finished_scan_bank_id = scan_finished_receiver.recv(); + if finished_scan_bank_id.is_err() { return; } // Wait for scan to finish before starting next iteration - assert_eq!(finished_scan_slot_id.unwrap(), prev_bank.slot_id()); + assert_eq!(finished_scan_bank_id.unwrap(), prev_bank.bank_id()); } bank_at_fork_tip.remove_unrooted_slots(&slots_on_fork); prev_bank = bank_at_fork_tip; @@ -12459,15 +12459,15 @@ pub(crate) mod tests { bank_at_fork_tip.remove_unrooted_slots(&[slot_to_remove]); // Wait for scan to finish before starting next iteration - let finished_scan_slot_id = scan_finished_receiver.recv(); - if finished_scan_slot_id.is_err() { + let finished_scan_bank_id = scan_finished_receiver.recv(); + if finished_scan_bank_id.is_err() { return; } - assert_eq!(finished_scan_slot_id.unwrap(), bank_at_fork_tip.slot_id()); + assert_eq!(finished_scan_bank_id.unwrap(), bank_at_fork_tip.bank_id()); // Remove the rest of the slots before the next iteration - for (slot, slot_id) in slots_on_fork { - bank_at_fork_tip.remove_unrooted_slots(&[(slot, slot_id)]); + for (slot, bank_id) in slots_on_fork { + bank_at_fork_tip.remove_unrooted_slots(&[(slot, bank_id)]); } } }, diff --git a/sdk/program/src/clock.rs b/sdk/program/src/clock.rs index 4aa1807eb84..5cf5b27fc4e 100644 --- a/sdk/program/src/clock.rs +++ b/sdk/program/src/clock.rs @@ -79,7 +79,7 @@ pub type Slot = u64; /// Uniquely distinguishes every version of a slot, even if the /// slot number is the same, i.e. duplicate slots -pub type SlotId = u64; +pub type BankId = u64; /// Epoch is a unit of time a given leader schedule is honored, /// some number of Slots. From 2e0a6f5a6ed67e18935f1f1790732d1c1ce503b0 Mon Sep 17 00:00:00 2001 From: Carl Lin Date: Tue, 8 Jun 2021 01:30:10 -0700 Subject: [PATCH 11/12] Address comments --- client/src/rpc_custom_error.rs | 4 ++-- runtime/src/accounts_index.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/client/src/rpc_custom_error.rs b/client/src/rpc_custom_error.rs index cd2c5b070c2..5ce021bc573 100644 --- a/client/src/rpc_custom_error.rs +++ b/client/src/rpc_custom_error.rs @@ -19,7 +19,7 @@ pub const JSON_RPC_SERVER_ERROR_NO_SNAPSHOT: i64 = -32008; pub const JSON_RPC_SERVER_ERROR_LONG_TERM_STORAGE_SLOT_SKIPPED: i64 = -32009; pub const JSON_RPC_SERVER_ERROR_KEY_EXCLUDED_FROM_SECONDARY_INDEX: i64 = -32010; pub const JSON_RPC_SERVER_ERROR_TRANSACTION_HISTORY_NOT_AVAILABLE: i64 = -32011; -pub const JSON_RPC_REMOVED_SLOT: i64 = -32012; +pub const JSON_RPC_SCAN_ERROR: i64 = -32012; #[derive(Error, Debug)] pub enum RpcCustomError { @@ -149,7 +149,7 @@ impl From for Error { data: None, }, RpcCustomError::ScanError(scan_err) => Self { - code: ErrorCode::ServerError(JSON_RPC_REMOVED_SLOT), + code: ErrorCode::ServerError(JSON_RPC_SCAN_ERROR), message: scan_err.to_string(), data: None, }, diff --git a/runtime/src/accounts_index.rs b/runtime/src/accounts_index.rs index ca69c1be70e..71454803867 100644 --- a/runtime/src/accounts_index.rs +++ b/runtime/src/accounts_index.rs @@ -861,7 +861,7 @@ impl AccountsIndex { } } - // If the fork with tip at bank `scan_bank_id` was removed durin our scan, then the scan + // If the fork with tip at bank `scan_bank_id` was removed during our scan, then the scan // may have been corrupted, so abort the results. let was_scan_corrupted = self .removed_bank_ids From 5973ab47b83d1d8a09cacb3b19dc5055fab43b5b Mon Sep 17 00:00:00 2001 From: Carl Lin Date: Fri, 11 Jun 2021 15:32:17 -0700 Subject: [PATCH 12/12] Remove runtime from client --- Cargo.lock | 1 - client/Cargo.toml | 1 - client/src/rpc_custom_error.rs | 8 ++--- programs/bpf/Cargo.lock | 1 - rpc/src/rpc.rs | 56 ++++++++++++++++++++++------------ 5 files changed, 40 insertions(+), 27 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ac964156941..83fc19c1202 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4476,7 +4476,6 @@ dependencies = [ "solana-faucet", "solana-logger 1.8.0", "solana-net-utils", - "solana-runtime", "solana-sdk", "solana-transaction-status", "solana-version", diff --git a/client/Cargo.toml b/client/Cargo.toml index b7a5b7da057..1ea43fb8377 100644 --- a/client/Cargo.toml +++ b/client/Cargo.toml @@ -28,7 +28,6 @@ solana-account-decoder = { path = "../account-decoder", version = "=1.8.0" } solana-clap-utils = { path = "../clap-utils", version = "=1.8.0" } solana-faucet = { path = "../faucet", version = "=1.8.0" } solana-net-utils = { path = "../net-utils", version = "=1.8.0" } -solana-runtime = { path = "../runtime", version = "=1.8.0" } solana-sdk = { path = "../sdk", version = "=1.8.0" } solana-transaction-status = { path = "../transaction-status", version = "=1.8.0" } solana-version = { path = "../version", version = "=1.8.0" } diff --git a/client/src/rpc_custom_error.rs b/client/src/rpc_custom_error.rs index 5ce021bc573..3e80c1a5228 100644 --- a/client/src/rpc_custom_error.rs +++ b/client/src/rpc_custom_error.rs @@ -1,6 +1,4 @@ //! Implementation defined RPC server errors - -use solana_runtime::accounts_index::ScanError; use thiserror::Error; use { crate::rpc_response::RpcSimulateTransactionResult, @@ -52,7 +50,7 @@ pub enum RpcCustomError { #[error("TransactionHistoryNotAvailable")] TransactionHistoryNotAvailable, #[error("ScanError")] - ScanError(#[from] ScanError), + ScanError { message: String }, } #[derive(Debug, Serialize, Deserialize)] @@ -148,9 +146,9 @@ impl From for Error { message: "Transaction history is not available from this node".to_string(), data: None, }, - RpcCustomError::ScanError(scan_err) => Self { + RpcCustomError::ScanError { message } => Self { code: ErrorCode::ServerError(JSON_RPC_SCAN_ERROR), - message: scan_err.to_string(), + message, data: None, }, } diff --git a/programs/bpf/Cargo.lock b/programs/bpf/Cargo.lock index c897893ba46..b979dbf876a 100644 --- a/programs/bpf/Cargo.lock +++ b/programs/bpf/Cargo.lock @@ -3170,7 +3170,6 @@ dependencies = [ "solana-clap-utils", "solana-faucet", "solana-net-utils", - "solana-runtime", "solana-sdk", "solana-transaction-status", "solana-version", diff --git a/rpc/src/rpc.rs b/rpc/src/rpc.rs index 3c2ef6706b1..6aea3fe50c9 100644 --- a/rpc/src/rpc.rs +++ b/rpc/src/rpc.rs @@ -718,7 +718,12 @@ impl JsonRpcRequestProcessor { }) } else { let (addresses, address_filter) = if let Some(filter) = config.clone().filter { - let non_circulating_supply = calculate_non_circulating_supply(&bank)?; + let non_circulating_supply = + calculate_non_circulating_supply(&bank).map_err(|e| { + RpcCustomError::ScanError { + message: e.to_string(), + } + })?; let addresses = non_circulating_supply.accounts.into_iter().collect(); let address_filter = match filter { RpcLargestAccountsFilter::Circulating => AccountAddressFilter::Exclude, @@ -729,7 +734,10 @@ impl JsonRpcRequestProcessor { (HashSet::new(), AccountAddressFilter::Exclude) }; let accounts = bank - .get_largest_accounts(NUM_LARGEST_ACCOUNTS, &addresses, address_filter)? + .get_largest_accounts(NUM_LARGEST_ACCOUNTS, &addresses, address_filter) + .map_err(|e| RpcCustomError::ScanError { + message: e.to_string(), + })? .into_iter() .map(|(address, lamports)| RpcAccountBalance { address: address.to_string(), @@ -747,7 +755,10 @@ impl JsonRpcRequestProcessor { commitment: Option, ) -> RpcCustomResult> { let bank = self.bank(commitment); - let non_circulating_supply = calculate_non_circulating_supply(&bank)?; + let non_circulating_supply = + calculate_non_circulating_supply(&bank).map_err(|e| RpcCustomError::ScanError { + message: e.to_string(), + })?; let total_supply = bank.capitalization(); Ok(new_response( &bank, @@ -1760,19 +1771,24 @@ impl JsonRpcRequestProcessor { index_key: program_id.to_string(), }); } - Ok(bank.get_filtered_indexed_accounts( - &IndexKey::ProgramId(*program_id), - |account| { + Ok(bank + .get_filtered_indexed_accounts(&IndexKey::ProgramId(*program_id), |account| { // The program-id account index checks for Account owner on inclusion. However, due // to the current AccountsDb implementation, an account may remain in storage as a // zero-lamport AccountSharedData::Default() after being wiped and reinitialized in later // updates. We include the redundant filters here to avoid returning these // accounts. account.owner() == program_id && filter_closure(account) - }, - )?) + }) + .map_err(|e| RpcCustomError::ScanError { + message: e.to_string(), + })?) } else { - Ok(bank.get_filtered_program_accounts(program_id, filter_closure)?) + Ok(bank + .get_filtered_program_accounts(program_id, filter_closure) + .map_err(|e| RpcCustomError::ScanError { + message: e.to_string(), + })?) } } @@ -1809,16 +1825,17 @@ impl JsonRpcRequestProcessor { index_key: owner_key.to_string(), }); } - Ok(bank.get_filtered_indexed_accounts( - &IndexKey::SplTokenOwner(*owner_key), - |account| { + Ok(bank + .get_filtered_indexed_accounts(&IndexKey::SplTokenOwner(*owner_key), |account| { account.owner() == &spl_token_id_v2_0() && filters.iter().all(|filter_type| match filter_type { RpcFilterType::DataSize(size) => account.data().len() as u64 == *size, RpcFilterType::Memcmp(compare) => compare.bytes_match(&account.data()), }) - }, - )?) + }) + .map_err(|e| RpcCustomError::ScanError { + message: e.to_string(), + })?) } else { self.get_filtered_program_accounts(bank, &spl_token_id_v2_0(), filters) } @@ -1856,16 +1873,17 @@ impl JsonRpcRequestProcessor { index_key: mint_key.to_string(), }); } - Ok(bank.get_filtered_indexed_accounts( - &IndexKey::SplTokenMint(*mint_key), - |account| { + Ok(bank + .get_filtered_indexed_accounts(&IndexKey::SplTokenMint(*mint_key), |account| { account.owner() == &spl_token_id_v2_0() && filters.iter().all(|filter_type| match filter_type { RpcFilterType::DataSize(size) => account.data().len() as u64 == *size, RpcFilterType::Memcmp(compare) => compare.bytes_match(&account.data()), }) - }, - )?) + }) + .map_err(|e| RpcCustomError::ScanError { + message: e.to_string(), + })?) } else { self.get_filtered_program_accounts(bank, &spl_token_id_v2_0(), filters) }