From 952d8861c64a6a39e96ae92cdab2600b7d9c9248 Mon Sep 17 00:00:00 2001 From: behzad nouri Date: Fri, 21 Jul 2023 19:52:44 +0000 Subject: [PATCH] removes feature-gate code for updating rewards from cached accounts (#32514) --- runtime/src/accounts_db.rs | 14 --- runtime/src/bank.rs | 212 ++--------------------------------- runtime/src/bank/metrics.rs | 18 --- runtime/src/bank/tests.rs | 16 +-- runtime/src/stake_account.rs | 15 --- runtime/src/stakes.rs | 13 +-- 6 files changed, 10 insertions(+), 278 deletions(-) diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index 051ba57277..4beac5eda1 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -476,7 +476,6 @@ pub const ACCOUNTS_DB_CONFIG_FOR_TESTING: AccountsDbConfig = AccountsDbConfig { ancient_append_vec_offset: None, skip_initial_hash_calc: false, exhaustively_verify_refcounts: false, - assert_stakes_cache_consistency: true, create_ancient_storage: CreateAncientStorage::Pack, test_partitioned_epoch_rewards: TestPartitionedEpochRewards::CompareResults, }; @@ -488,7 +487,6 @@ pub const ACCOUNTS_DB_CONFIG_FOR_BENCHMARKS: AccountsDbConfig = AccountsDbConfig ancient_append_vec_offset: None, skip_initial_hash_calc: false, exhaustively_verify_refcounts: false, - assert_stakes_cache_consistency: false, create_ancient_storage: CreateAncientStorage::Pack, test_partitioned_epoch_rewards: TestPartitionedEpochRewards::None, }; @@ -550,8 +548,6 @@ pub struct AccountsDbConfig { pub ancient_append_vec_offset: Option, pub skip_initial_hash_calc: bool, pub exhaustively_verify_refcounts: bool, - /// when stakes cache consistency check occurs, assert that cached accounts match accounts db - pub assert_stakes_cache_consistency: bool, /// how to create ancient storages pub create_ancient_storage: CreateAncientStorage, pub test_partitioned_epoch_rewards: TestPartitionedEpochRewards, @@ -1378,9 +1374,6 @@ pub struct AccountsDb { pub(crate) storage: AccountStorage, - /// from AccountsDbConfig - pub(crate) assert_stakes_cache_consistency: bool, - #[allow(dead_code)] /// from AccountsDbConfig create_ancient_storage: CreateAncientStorage, @@ -2390,7 +2383,6 @@ impl AccountsDb { const ACCOUNTS_STACK_SIZE: usize = 8 * 1024 * 1024; AccountsDb { - assert_stakes_cache_consistency: false, bank_progress: BankCreationFreezingProgress::default(), create_ancient_storage: CreateAncientStorage::Pack, verify_accounts_hash_in_bg: VerifyAccountsHashInBackground::default(), @@ -2515,11 +2507,6 @@ impl AccountsDb { .map(|config| config.exhaustively_verify_refcounts) .unwrap_or_default(); - let assert_stakes_cache_consistency = accounts_db_config - .as_ref() - .map(|config| config.assert_stakes_cache_consistency) - .unwrap_or_default(); - let create_ancient_storage = accounts_db_config .as_ref() .map(|config| config.create_ancient_storage) @@ -2549,7 +2536,6 @@ impl AccountsDb { accounts_update_notifier, filler_accounts_config, filler_account_suffix, - assert_stakes_cache_consistency, create_ancient_storage, write_cache_limit_bytes: accounts_db_config .as_ref() diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index e4e7668fb6..e7d175adb9 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -68,7 +68,7 @@ use { serde_snapshot::BankIncrementalSnapshotPersistence, snapshot_hash::SnapshotHash, sorted_storages::SortedStorages, - stake_account::{self, StakeAccount}, + stake_account::StakeAccount, stake_history::StakeHistory, stake_rewards::StakeReward, stake_weighted_timestamp::{ @@ -174,7 +174,7 @@ use { self, InflationPointCalculationEvent, PointValue, StakeState, }, solana_system_program::{get_system_account_kind, SystemAccountKind}, - solana_vote_program::vote_state::{VoteState, VoteStateVersions}, + solana_vote_program::vote_state::VoteState, std::{ borrow::Cow, cell::RefCell, @@ -819,8 +819,7 @@ pub struct Bank { struct VoteWithStakeDelegations { vote_state: Arc, vote_account: AccountSharedData, - // TODO: use StakeAccount once the old code is deleted. - delegations: Vec<(Pubkey, StakeAccount<()>)>, + delegations: Vec<(Pubkey, StakeAccount)>, } type VoteWithStakeDelegationsMap = DashMap; @@ -829,11 +828,7 @@ type InvalidCacheKeyMap = DashMap; struct LoadVoteAndStakeAccountsResult { vote_with_stake_delegations_map: VoteWithStakeDelegationsMap, - invalid_stake_keys: InvalidCacheKeyMap, invalid_vote_keys: InvalidCacheKeyMap, - invalid_cached_vote_accounts: usize, - invalid_cached_stake_accounts: usize, - invalid_cached_stake_accounts_rent_epoch: usize, vote_accounts_cache_miss_count: usize, } @@ -2547,9 +2542,6 @@ impl Bank { } = self.calculate_previous_epoch_inflation_rewards(capitalization, prev_epoch); let old_vote_balance_and_staked = self.stakes_cache.stakes().vote_balance_and_staked(); - let update_rewards_from_cached_accounts = self - .feature_set - .is_active(&feature_set::update_rewards_from_cached_accounts::id()); self.pay_validator_rewards_with_thread_pool( prev_epoch, @@ -2557,7 +2549,6 @@ impl Bank { reward_calc_tracer, thread_pool, metrics, - update_rewards_from_cached_accounts, ); let new_vote_balance_and_staked = self.stakes_cache.stakes().vote_balance_and_staked(); @@ -2621,167 +2612,6 @@ impl Bank { ); } - /// map stake delegations into resolved (pubkey, account) pairs - /// returns a map (has to be copied) of loaded - /// ( Vec<(staker info)> (voter account) ) keyed by voter pubkey - /// - /// Filters out invalid pairs - fn _load_vote_and_stake_accounts_with_thread_pool( - &self, - thread_pool: &ThreadPool, - reward_calc_tracer: Option, - ) -> LoadVoteAndStakeAccountsResult { - let stakes = self.stakes_cache.stakes(); - let cached_vote_accounts = stakes.vote_accounts(); - let vote_with_stake_delegations_map = DashMap::with_capacity(cached_vote_accounts.len()); - let invalid_stake_keys: DashMap = DashMap::new(); - let invalid_vote_keys: DashMap = DashMap::new(); - let invalid_cached_stake_accounts = AtomicUsize::default(); - let invalid_cached_vote_accounts = AtomicUsize::default(); - let invalid_cached_stake_accounts_rent_epoch = AtomicUsize::default(); - - let stake_delegations = self.filter_stake_delegations(&stakes); - thread_pool.install(|| { - stake_delegations - .into_par_iter() - .for_each(|(stake_pubkey, cached_stake_account)| { - let delegation = cached_stake_account.delegation(); - let vote_pubkey = &delegation.voter_pubkey; - if invalid_vote_keys.contains_key(vote_pubkey) { - return; - } - let Some(stake_account) = self.get_account_with_fixed_root(stake_pubkey) else { - invalid_stake_keys - .insert(*stake_pubkey, InvalidCacheEntryReason::Missing); - invalid_cached_stake_accounts.fetch_add(1, Relaxed); - return; - }; - if cached_stake_account.account() != &stake_account { - if self.rc.accounts.accounts_db.assert_stakes_cache_consistency { - panic!( - "stakes cache accounts mismatch {cached_stake_account:?} {stake_account:?}" - ); - } - invalid_cached_stake_accounts.fetch_add(1, Relaxed); - let cached_stake_account = cached_stake_account.account(); - if cached_stake_account.lamports() == stake_account.lamports() - && cached_stake_account.data() == stake_account.data() - && cached_stake_account.owner() == stake_account.owner() - && cached_stake_account.executable() == stake_account.executable() - { - invalid_cached_stake_accounts_rent_epoch.fetch_add(1, Relaxed); - } else { - debug!( - "cached stake account mismatch: {}: {:?}, {:?}", - stake_pubkey, stake_account, cached_stake_account - ); - } - } - let stake_account = match StakeAccount::<()>::try_from(stake_account) { - Ok(stake_account) => stake_account, - Err(stake_account::Error::InvalidOwner { .. }) => { - invalid_stake_keys - .insert(*stake_pubkey, InvalidCacheEntryReason::WrongOwner); - return; - } - Err(stake_account::Error::InstructionError(_)) => { - invalid_stake_keys - .insert(*stake_pubkey, InvalidCacheEntryReason::BadState); - return; - } - Err(stake_account::Error::InvalidDelegation(_)) => { - // This should not happen. - error!( - "Unexpected code path! StakeAccount<()> \ - should not check if stake-state is a \ - Delegation." - ); - return; - } - }; - let stake_delegation = (*stake_pubkey, stake_account); - let mut vote_delegations = if let Some(vote_delegations) = - vote_with_stake_delegations_map.get_mut(vote_pubkey) - { - vote_delegations - } else { - let cached_vote_account = cached_vote_accounts.get(vote_pubkey); - let vote_account = match self.get_account_with_fixed_root(vote_pubkey) { - Some(vote_account) => { - if vote_account.owner() != &solana_vote_program::id() { - invalid_vote_keys - .insert(*vote_pubkey, InvalidCacheEntryReason::WrongOwner); - if cached_vote_account.is_some() { - invalid_cached_vote_accounts.fetch_add(1, Relaxed); - } - return; - } - vote_account - } - None => { - if cached_vote_account.is_some() { - invalid_cached_vote_accounts.fetch_add(1, Relaxed); - } - invalid_vote_keys - .insert(*vote_pubkey, InvalidCacheEntryReason::Missing); - return; - } - }; - - let vote_state = if let Ok(vote_state) = - StateMut::::state(&vote_account) - { - vote_state.convert_to_current() - } else { - invalid_vote_keys - .insert(*vote_pubkey, InvalidCacheEntryReason::BadState); - if cached_vote_account.is_some() { - invalid_cached_vote_accounts.fetch_add(1, Relaxed); - } - return; - }; - match cached_vote_account { - Some(cached_vote_account) - if cached_vote_account.account() == &vote_account => {} - _ => { - invalid_cached_vote_accounts.fetch_add(1, Relaxed); - } - }; - vote_with_stake_delegations_map - .entry(*vote_pubkey) - .or_insert_with(|| VoteWithStakeDelegations { - vote_state: Arc::new(vote_state), - vote_account, - delegations: vec![], - }) - }; - - if let Some(reward_calc_tracer) = reward_calc_tracer.as_ref() { - reward_calc_tracer(&RewardCalculationEvent::Staking( - stake_pubkey, - &InflationPointCalculationEvent::Delegation( - delegation, - solana_vote_program::id(), - ), - )); - } - - vote_delegations.delegations.push(stake_delegation); - }); - }); - invalid_cached_stake_accounts.fetch_add(invalid_stake_keys.len(), Relaxed); - LoadVoteAndStakeAccountsResult { - vote_with_stake_delegations_map, - invalid_vote_keys, - invalid_stake_keys, - invalid_cached_vote_accounts: invalid_cached_vote_accounts.into_inner(), - invalid_cached_stake_accounts: invalid_cached_stake_accounts.into_inner(), - invalid_cached_stake_accounts_rent_epoch: invalid_cached_stake_accounts_rent_epoch - .into_inner(), - vote_accounts_cache_miss_count: 0, - } - } - fn filter_stake_delegations<'a>( &self, stakes: &'a Stakes>, @@ -2906,8 +2736,7 @@ impl Bank { let event = RewardCalculationEvent::Staking(stake_pubkey, &delegation); reward_calc_tracer(&event); } - let stake_account = StakeAccount::from(stake_account.clone()); - let stake_delegation = (*stake_pubkey, stake_account); + let stake_delegation = (*stake_pubkey, stake_account.clone()); vote_delegations.delegations.push(stake_delegation); }; thread_pool.install(|| { @@ -2918,10 +2747,6 @@ impl Bank { LoadVoteAndStakeAccountsResult { vote_with_stake_delegations_map, invalid_vote_keys, - invalid_stake_keys: DashMap::default(), - invalid_cached_vote_accounts: 0, - invalid_cached_stake_accounts: 0, - invalid_cached_stake_accounts_rent_epoch: 0, vote_accounts_cache_miss_count: vote_accounts_cache_miss_count.into_inner(), } } @@ -2982,15 +2807,10 @@ impl Bank { reward_calc_tracer: Option, thread_pool: &ThreadPool, metrics: &mut RewardsMetrics, - update_rewards_from_cached_accounts: bool, ) { let stake_history = self.stakes_cache.stakes().history().clone(); - let vote_with_stake_delegations_map = self.load_vote_and_stake_accounts( - thread_pool, - reward_calc_tracer.as_ref(), - metrics, - update_rewards_from_cached_accounts, - ); + let vote_with_stake_delegations_map = + self.load_vote_and_stake_accounts(thread_pool, reward_calc_tracer.as_ref(), metrics); let point_value = self.calculate_reward_points( &vote_with_stake_delegations_map, @@ -3130,39 +2950,23 @@ impl Bank { thread_pool: &ThreadPool, reward_calc_tracer: Option, metrics: &mut RewardsMetrics, - update_rewards_from_cached_accounts: bool, ) -> VoteWithStakeDelegationsMap { let ( LoadVoteAndStakeAccountsResult { vote_with_stake_delegations_map, - invalid_stake_keys, invalid_vote_keys, - invalid_cached_vote_accounts, - invalid_cached_stake_accounts, - invalid_cached_stake_accounts_rent_epoch, vote_accounts_cache_miss_count, }, measure, ) = measure!({ - if update_rewards_from_cached_accounts { - self._load_vote_and_stake_accounts(thread_pool, reward_calc_tracer.as_ref()) - } else { - self._load_vote_and_stake_accounts_with_thread_pool( - thread_pool, - reward_calc_tracer.as_ref(), - ) - } + self._load_vote_and_stake_accounts(thread_pool, reward_calc_tracer.as_ref()) }); metrics .load_vote_and_stake_accounts_us .fetch_add(measure.as_us(), Relaxed); - metrics.invalid_cached_vote_accounts += invalid_cached_vote_accounts; - metrics.invalid_cached_stake_accounts += invalid_cached_stake_accounts; - metrics.invalid_cached_stake_accounts_rent_epoch += - invalid_cached_stake_accounts_rent_epoch; metrics.vote_accounts_cache_miss_count += vote_accounts_cache_miss_count; self.stakes_cache - .handle_invalid_keys(invalid_stake_keys, invalid_vote_keys, self.slot()); + .handle_invalid_keys(invalid_vote_keys, self.slot()); vote_with_stake_delegations_map } diff --git a/runtime/src/bank/metrics.rs b/runtime/src/bank/metrics.rs index 1cde4251d7..1fa33b2e7f 100644 --- a/runtime/src/bank/metrics.rs +++ b/runtime/src/bank/metrics.rs @@ -19,9 +19,6 @@ pub(crate) struct RewardsMetrics { pub(crate) redeem_rewards_us: u64, pub(crate) store_stake_accounts_us: AtomicU64, pub(crate) store_vote_accounts_us: AtomicU64, - pub(crate) invalid_cached_vote_accounts: usize, - pub(crate) invalid_cached_stake_accounts: usize, - pub(crate) invalid_cached_stake_accounts_rent_epoch: usize, pub(crate) vote_accounts_cache_miss_count: usize, pub(crate) hash_partition_rewards_us: u64, } @@ -96,21 +93,6 @@ pub(crate) fn report_new_epoch_metrics( metrics.store_vote_accounts_us.load(Relaxed), i64 ), - ( - "invalid_cached_vote_accounts", - metrics.invalid_cached_vote_accounts, - i64 - ), - ( - "invalid_cached_stake_accounts", - metrics.invalid_cached_stake_accounts, - i64 - ), - ( - "invalid_cached_stake_accounts_rent_epoch", - metrics.invalid_cached_stake_accounts_rent_epoch, - i64 - ), ( "vote_accounts_cache_miss_count", metrics.vote_accounts_cache_miss_count, diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index e2ab8aca19..2e5fa95df7 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -2071,9 +2071,6 @@ fn test_rent_eager_collect_rent_zero_lamport_deterministic() { #[test] fn test_bank_update_vote_stake_rewards() { let thread_pool = ThreadPoolBuilder::new().num_threads(1).build().unwrap(); - check_bank_update_vote_stake_rewards(|bank: &Bank| { - bank._load_vote_and_stake_accounts_with_thread_pool(&thread_pool, null_tracer()) - }); check_bank_update_vote_stake_rewards(|bank: &Bank| { bank._load_vote_and_stake_accounts(&thread_pool, null_tracer()) }); @@ -9143,12 +9140,6 @@ fn test_get_inflation_num_slots_already_activated() { #[test] fn test_stake_vote_account_validity() { let thread_pool = ThreadPoolBuilder::new().num_threads(1).build().unwrap(); - check_stake_vote_account_validity( - true, // check owner change, - |bank: &Bank| { - bank._load_vote_and_stake_accounts_with_thread_pool(&thread_pool, null_tracer()) - }, - ); // TODO: stakes cache should be hardened for the case when the account // owner is changed from vote/stake program to something else. see: // https://github.com/solana-labs/solana/pull/24200#discussion_r849935444 @@ -9179,11 +9170,7 @@ where AccountSecondaryIndexes::default(), AccountShrinkThreshold::default(), false, - Some(AccountsDbConfig { - // at least one tests hit this assert, so disable it - assert_stakes_cache_consistency: false, - ..ACCOUNTS_DB_CONFIG_FOR_TESTING - }), + Some(ACCOUNTS_DB_CONFIG_FOR_TESTING), None, Arc::default(), )); @@ -11953,7 +11940,6 @@ fn test_stake_account_consistency_with_rent_epoch_max_feature( Epoch::default() }; - assert!(bank.rc.accounts.accounts_db.assert_stakes_cache_consistency); let mut pubkey_bytes_early = [0u8; 32]; pubkey_bytes_early[31] = 2; let stake_id1 = Pubkey::from(pubkey_bytes_early); diff --git a/runtime/src/stake_account.rs b/runtime/src/stake_account.rs index e52e67e3b9..e4cd79e48c 100644 --- a/runtime/src/stake_account.rs +++ b/runtime/src/stake_account.rs @@ -44,10 +44,6 @@ impl StakeAccount { pub(crate) fn stake_state(&self) -> &StakeState { &self.stake_state } - - pub(crate) fn account(&self) -> &AccountSharedData { - &self.account - } } impl StakeAccount { @@ -91,17 +87,6 @@ impl TryFrom for StakeAccount { } } -impl From> for StakeAccount<()> { - #[inline] - fn from(stake_account: StakeAccount) -> Self { - Self { - account: stake_account.account, - stake_state: stake_account.stake_state, - _phantom: PhantomData, - } - } -} - impl From> for (AccountSharedData, StakeState) { #[inline] fn from(stake_account: StakeAccount) -> Self { diff --git a/runtime/src/stakes.rs b/runtime/src/stakes.rs index 8d39f03cc5..614a65e142 100644 --- a/runtime/src/stakes.rs +++ b/runtime/src/stakes.rs @@ -124,11 +124,10 @@ impl StakesCache { pub(crate) fn handle_invalid_keys( &self, - invalid_stake_keys: DashMap, invalid_vote_keys: DashMap, current_slot: Slot, ) { - if invalid_stake_keys.is_empty() && invalid_vote_keys.is_empty() { + if invalid_vote_keys.is_empty() { return; } @@ -136,16 +135,6 @@ impl StakesCache { // not properly evicted in normal operation. let mut stakes = self.0.write().unwrap(); - for (stake_pubkey, reason) in invalid_stake_keys { - stakes.remove_stake_delegation(&stake_pubkey); - datapoint_warn!( - "bank-stake_delegation_accounts-invalid-account", - ("slot", current_slot as i64, i64), - ("stake-address", format!("{stake_pubkey:?}"), String), - ("reason", reason.to_i64().unwrap_or_default(), i64), - ); - } - for (vote_pubkey, reason) in invalid_vote_keys { stakes.remove_vote_account(&vote_pubkey); datapoint_warn!(