From 9f160853aee55ea9e91a3f3cf8257572ac134e1b Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Mon, 19 Aug 2024 16:03:07 +0000 Subject: [PATCH] VoteAccount: remove OnceLock around VoteState The lazy-init OnceLock logic wasn't really being used. Execution/perf wise this code doesn't change anything since we were already forcing early deserialization in StakesCache::check_and_store: - // Called to eagerly deserialize vote state - let _res = vote_account.vote_state(); For more context see https://discord.com/channels/428295358100013066/439194979856809985/1268759289531596872 https://discord.com/channels/428295358100013066/439194979856809985/1272661616399224892 --- ledger/src/blockstore_processor.rs | 28 ++---- runtime/src/bank.rs | 13 ++- runtime/src/bank/fee_distribution.rs | 2 +- .../partitioned_epoch_rewards/calculation.rs | 14 ++- runtime/src/bank/tests.rs | 8 +- runtime/src/epoch_stakes.rs | 14 --- runtime/src/snapshot_minimizer.rs | 5 +- runtime/src/stakes.rs | 12 +-- vote/src/vote_account.rs | 98 +++++++------------ 9 files changed, 65 insertions(+), 129 deletions(-) diff --git a/ledger/src/blockstore_processor.rs b/ledger/src/blockstore_processor.rs index d34543db73993c..4af5663f78c515 100644 --- a/ledger/src/blockstore_processor.rs +++ b/ledger/src/blockstore_processor.rs @@ -1870,7 +1870,6 @@ fn load_frozen_forks( let new_root_bank = { if bank_forks.read().unwrap().root() >= max_root { supermajority_root_from_vote_accounts( - bank.slot(), bank.total_epoch_stake(), &bank.vote_accounts(), ).and_then(|supermajority_root| { @@ -2005,27 +2004,17 @@ fn supermajority_root(roots: &[(Slot, u64)], total_epoch_stake: u64) -> Option Option { let mut roots_stakes: Vec<(Slot, u64)> = vote_accounts - .iter() - .filter_map(|(key, (stake, account))| { + .values() + .filter_map(|(stake, account)| { if *stake == 0 { return None; } - match account.vote_state().as_ref() { - Err(_) => { - warn!( - "Unable to get vote_state from account {} in bank: {}", - key, bank_slot - ); - None - } - Ok(vote_state) => Some((vote_state.root_slot?, *stake)), - } + Some((account.vote_state().root_slot?, *stake)) }) .collect(); @@ -4579,23 +4568,20 @@ pub mod tests { }; let total_stake = 10; - let slot = 100; // Supermajority root should be None - assert!( - supermajority_root_from_vote_accounts(slot, total_stake, &HashMap::default()).is_none() - ); + assert!(supermajority_root_from_vote_accounts(total_stake, &HashMap::default()).is_none()); // Supermajority root should be None let roots_stakes = vec![(8, 1), (3, 1), (4, 1), (8, 1)]; let accounts = convert_to_vote_accounts(roots_stakes); - assert!(supermajority_root_from_vote_accounts(slot, total_stake, &accounts).is_none()); + assert!(supermajority_root_from_vote_accounts(total_stake, &accounts).is_none()); // Supermajority root should be 4, has 7/10 of the stake let roots_stakes = vec![(8, 1), (3, 1), (4, 1), (8, 5)]; let accounts = convert_to_vote_accounts(roots_stakes); assert_eq!( - supermajority_root_from_vote_accounts(slot, total_stake, &accounts).unwrap(), + supermajority_root_from_vote_accounts(total_stake, &accounts).unwrap(), 4 ); @@ -4603,7 +4589,7 @@ pub mod tests { let roots_stakes = vec![(8, 1), (3, 1), (4, 1), (8, 6)]; let accounts = convert_to_vote_accounts(roots_stakes); assert_eq!( - supermajority_root_from_vote_accounts(slot, total_stake, &accounts).unwrap(), + supermajority_root_from_vote_accounts(total_stake, &accounts).unwrap(), 8 ); } diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 0a0bacea0eef1c..00830043d134e3 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -2280,12 +2280,8 @@ impl Bank { invalid_vote_keys.insert(vote_pubkey, InvalidCacheEntryReason::WrongOwner); return None; } - let Ok(vote_state) = vote_account.vote_state().cloned() else { - invalid_vote_keys.insert(vote_pubkey, InvalidCacheEntryReason::BadState); - return None; - }; let vote_with_stake_delegations = VoteWithStakeDelegations { - vote_state: Arc::new(vote_state), + vote_state: Arc::new(vote_account.vote_state().clone()), vote_account: AccountSharedData::from(vote_account), delegations: Vec::default(), }; @@ -2703,7 +2699,6 @@ impl Bank { let vote_accounts = self.vote_accounts(); let recent_timestamps = vote_accounts.iter().filter_map(|(pubkey, (_, account))| { let vote_state = account.vote_state(); - let vote_state = vote_state.as_ref().ok()?; let slot_delta = self.slot().checked_sub(vote_state.last_timestamp.slot)?; (slot_delta <= slots_per_epoch).then_some({ ( @@ -2879,7 +2874,7 @@ impl Bank { // up and can be used to set the collector id to the highest staked // node. If no staked nodes exist, allow fallback to an unstaked test // collector id during tests. - let collector_id = self.stakes_cache.stakes().highest_staked_node(); + let collector_id = self.stakes_cache.stakes().highest_staked_node().copied(); #[cfg(feature = "dev-context-only-utils")] let collector_id = collector_id.or(collector_id_for_tests); self.collector_id = @@ -5871,6 +5866,10 @@ impl Bank { /// current vote accounts for this bank along with the stake /// attributed to each account pub fn vote_accounts(&self) -> Arc { + // log::error!( + // "VOTE ACCOUNTS {}", + // std::backtrace::Backtrace::force_capture() + // ); let stakes = self.stakes_cache.stakes(); Arc::from(stakes.vote_accounts()) } diff --git a/runtime/src/bank/fee_distribution.rs b/runtime/src/bank/fee_distribution.rs index 4dc511a5eee95c..383521c016179f 100644 --- a/runtime/src/bank/fee_distribution.rs +++ b/runtime/src/bank/fee_distribution.rs @@ -203,7 +203,7 @@ impl Bank { None } else { total_staked += *staked; - Some((*account.node_pubkey()?, *staked)) + Some((*account.node_pubkey(), *staked)) } }) .collect::>(); diff --git a/runtime/src/bank/partitioned_epoch_rewards/calculation.rs b/runtime/src/bank/partitioned_epoch_rewards/calculation.rs index 9d929accb5cdb1..257a531f3e04d4 100644 --- a/runtime/src/bank/partitioned_epoch_rewards/calculation.rs +++ b/runtime/src/bank/partitioned_epoch_rewards/calculation.rs @@ -385,7 +385,7 @@ impl Bank { if vote_account.owner() != &solana_vote_program { return None; } - let vote_state = vote_account.vote_state().cloned().ok()?; + let vote_state = vote_account.vote_state(); let pre_lamport = stake_account.lamports(); @@ -393,7 +393,7 @@ impl Bank { rewarded_epoch, stake_state, &mut stake_account, - &vote_state, + vote_state, &point_value, stake_history, reward_calc_tracer.as_ref(), @@ -407,13 +407,14 @@ impl Bank { "calculated reward: {} {} {} {}", stake_pubkey, pre_lamport, post_lamport, stakers_reward ); + let commission = vote_state.commission; // track voter rewards let mut voters_reward_entry = vote_account_rewards .entry(vote_pubkey) .or_insert(VoteReward { + commission, vote_account: vote_account.into(), - commission: vote_state.commission, vote_rewards: 0, vote_needs_store: false, }); @@ -438,7 +439,7 @@ impl Bank { reward_type: RewardType::Staking, lamports: i64::try_from(stakers_reward).unwrap(), post_balance, - commission: Some(vote_state.commission), + commission: Some(commission), }, stake, }); @@ -508,13 +509,10 @@ impl Bank { if vote_account.owner() != &solana_vote_program { return 0; } - let Ok(vote_state) = vote_account.vote_state() else { - return 0; - }; solana_stake_program::points::calculate_points( stake_account.stake_state(), - vote_state, + vote_account.vote_state(), stake_history, new_warmup_cooldown_rate_epoch, ) diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index 36d3ca8d1c7f8b..2647ede9ec783d 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -3896,12 +3896,8 @@ fn test_bank_epoch_vote_accounts() { accounts .iter() .filter_map(|(pubkey, (stake, account))| { - if let Ok(vote_state) = account.vote_state().as_ref() { - if vote_state.node_pubkey == leader_pubkey { - Some((*pubkey, *stake)) - } else { - None - } + if account.node_pubkey() == &leader_pubkey { + Some((*pubkey, *stake)) } else { None } diff --git a/runtime/src/epoch_stakes.rs b/runtime/src/epoch_stakes.rs index 84b6bdc40a6345..fa4de74d8e23cb 100644 --- a/runtime/src/epoch_stakes.rs +++ b/runtime/src/epoch_stakes.rs @@ -101,20 +101,6 @@ impl EpochStakes { .iter() .filter_map(|(key, (stake, account))| { let vote_state = account.vote_state(); - let vote_state = match vote_state.as_ref() { - Err(_) => { - datapoint_warn!( - "parse_epoch_vote_accounts", - ( - "warn", - format!("Unable to get vote_state from account {key}"), - String - ), - ); - return None; - } - Ok(vote_state) => vote_state, - }; if *stake > 0 { if let Some(authorized_voter) = vote_state diff --git a/runtime/src/snapshot_minimizer.rs b/runtime/src/snapshot_minimizer.rs index b48f1832fc0256..7aeba40adea04d 100644 --- a/runtime/src/snapshot_minimizer.rs +++ b/runtime/src/snapshot_minimizer.rs @@ -158,9 +158,8 @@ impl<'a> SnapshotMinimizer<'a> { .par_iter() .for_each(|(pubkey, (_stake, vote_account))| { self.minimized_account_set.insert(*pubkey); - if let Ok(vote_state) = vote_account.vote_state().as_ref() { - self.minimized_account_set.insert(vote_state.node_pubkey); - } + self.minimized_account_set + .insert(*vote_account.node_pubkey()); }); } diff --git a/runtime/src/stakes.rs b/runtime/src/stakes.rs index d79d8e43492687..f878510402e929 100644 --- a/runtime/src/stakes.rs +++ b/runtime/src/stakes.rs @@ -98,11 +98,6 @@ impl StakesCache { if VoteStateVersions::is_correct_size_and_initialized(account.data()) { match VoteAccount::try_from(account.to_account_shared_data()) { Ok(vote_account) => { - { - // Called to eagerly deserialize vote state - let _res = vote_account.vote_state(); - } - // drop the old account after releasing the lock let _old_vote_account = { let mut stakes = self.0.write().unwrap(); @@ -428,7 +423,6 @@ impl Stakes { new_rate_activation_epoch: Option, ) -> Option { debug_assert_ne!(vote_account.lamports(), 0u64); - debug_assert!(vote_account.is_deserialized()); let stake_delegations = &self.stake_delegations; self.vote_accounts.insert(*vote_pubkey, vote_account, || { @@ -507,9 +501,9 @@ impl Stakes { &self.stake_delegations } - pub(crate) fn highest_staked_node(&self) -> Option { + pub(crate) fn highest_staked_node(&self) -> Option<&Pubkey> { let vote_account = self.vote_accounts.find_max_by_delegated_stake()?; - vote_account.node_pubkey().copied() + Some(vote_account.node_pubkey()) } } @@ -835,7 +829,7 @@ pub(crate) mod tests { let vote11_node_pubkey = vote_state::from(&vote11_account).unwrap().node_pubkey; - let highest_staked_node = stakes_cache.stakes().highest_staked_node(); + let highest_staked_node = stakes_cache.stakes().highest_staked_node().copied(); assert_eq!(highest_staked_node, Some(vote11_node_pubkey)); } diff --git a/vote/src/vote_account.rs b/vote/src/vote_account.rs index 14cc788cca13d9..225ddd15fc8074 100644 --- a/vote/src/vote_account.rs +++ b/vote/src/vote_account.rs @@ -34,7 +34,7 @@ pub enum Error { #[derive(Debug)] struct VoteAccountInner { account: AccountSharedData, - vote_state: OnceLock>, + vote_state: VoteState, } pub type VoteAccountsHashMap = HashMap; @@ -68,22 +68,13 @@ impl VoteAccount { self.0.account.owner() } - pub fn vote_state(&self) -> Result<&VoteState, &Error> { - // VoteState::deserialize deserializes a VoteStateVersions and then - // calls VoteStateVersions::convert_to_current. - self.0 - .vote_state - .get_or_init(|| VoteState::deserialize(self.0.account.data()).map_err(Error::from)) - .as_ref() - } - - pub fn is_deserialized(&self) -> bool { - self.0.vote_state.get().is_some() + pub fn vote_state(&self) -> &VoteState { + &self.0.vote_state } /// VoteState.node_pubkey of this vote-account. - pub fn node_pubkey(&self) -> Option<&Pubkey> { - self.vote_state().ok().map(|s| &s.node_pubkey) + pub fn node_pubkey(&self) -> &Pubkey { + &self.0.vote_state.node_pubkey } } @@ -103,9 +94,7 @@ impl VoteAccounts { self.vote_accounts .values() .filter(|(stake, _)| *stake != 0u64) - .filter_map(|(stake, vote_account)| { - Some((*vote_account.node_pubkey()?, stake)) - }) + .map(|(stake, vote_account)| (*vote_account.node_pubkey(), stake)) .into_grouping_map() .aggregate(|acc, _node_pubkey, stake| { Some(acc.unwrap_or_default() + stake) @@ -158,13 +147,14 @@ impl VoteAccounts { let (stake, old_vote_account) = entry.get_mut(); if let Some(staked_nodes) = self.staked_nodes.get_mut() { + log::error!("LOLLL doing the stake thing"); let old_node_pubkey = old_vote_account.node_pubkey(); let new_node_pubkey = new_vote_account.node_pubkey(); if new_node_pubkey != old_node_pubkey { // The node keys have changed, we move the stake from the old node to the // new one Self::do_sub_node_stake(staked_nodes, *stake, old_node_pubkey); - Self::do_add_node_stake(staked_nodes, *stake, new_node_pubkey.copied()); + Self::do_add_node_stake(staked_nodes, *stake, *new_node_pubkey); } } @@ -175,11 +165,7 @@ impl VoteAccounts { // This is a new vote account. We don't know the stake yet, so we need to compute it. let (stake, vote_account) = entry.insert((calculate_stake(), new_vote_account)); if let Some(staked_nodes) = self.staked_nodes.get_mut() { - Self::do_add_node_stake( - staked_nodes, - *stake, - vote_account.node_pubkey().copied(), - ); + Self::do_add_node_stake(staked_nodes, *stake, *vote_account.node_pubkey()); } None } @@ -220,24 +206,22 @@ impl VoteAccounts { return; }; - VoteAccounts::do_add_node_stake(staked_nodes, stake, vote_account.node_pubkey().copied()); + VoteAccounts::do_add_node_stake(staked_nodes, stake, *vote_account.node_pubkey()); } fn do_add_node_stake( staked_nodes: &mut Arc>, stake: u64, - node_pubkey: Option, + node_pubkey: Pubkey, ) { if stake == 0u64 { return; } - node_pubkey.map(|node_pubkey| { - Arc::make_mut(staked_nodes) - .entry(node_pubkey) - .and_modify(|s| *s += stake) - .or_insert(stake) - }); + Arc::make_mut(staked_nodes) + .entry(node_pubkey) + .and_modify(|s| *s += stake) + .or_insert(stake); } fn sub_node_stake(&mut self, stake: u64, vote_account: &VoteAccount) { @@ -251,24 +235,22 @@ impl VoteAccounts { fn do_sub_node_stake( staked_nodes: &mut Arc>, stake: u64, - node_pubkey: Option<&Pubkey>, + node_pubkey: &Pubkey, ) { if stake == 0u64 { return; } - if let Some(node_pubkey) = node_pubkey { - let staked_nodes = Arc::make_mut(staked_nodes); - let current_stake = staked_nodes - .get_mut(node_pubkey) - .expect("this should not happen"); - match (*current_stake).cmp(&stake) { - Ordering::Less => panic!("subtraction value exceeds node's stake"), - Ordering::Equal => { - staked_nodes.remove(node_pubkey); - } - Ordering::Greater => *current_stake -= stake, + let staked_nodes = Arc::make_mut(staked_nodes); + let current_stake = staked_nodes + .get_mut(node_pubkey) + .expect("this should not happen"); + match (*current_stake).cmp(&stake) { + Ordering::Less => panic!("subtraction value exceeds node's stake"), + Ordering::Equal => { + staked_nodes.remove(node_pubkey); } + Ordering::Greater => *current_stake -= stake, } } } @@ -303,8 +285,8 @@ impl TryFrom for VoteAccountInner { return Err(Error::InvalidOwner(*account.owner())); } Ok(Self { + vote_state: VoteState::deserialize(account.data()).map_err(Error::InstructionError)?, account, - vote_state: OnceLock::new(), }) } } @@ -441,12 +423,10 @@ mod tests { .into_iter() .filter(|(_, (stake, _))| *stake != 0) { - if let Some(node_pubkey) = vote_account.node_pubkey() { - staked_nodes - .entry(*node_pubkey) - .and_modify(|s| *s += *stake) - .or_insert(*stake); - } + staked_nodes + .entry(*vote_account.node_pubkey()) + .and_modify(|s| *s += *stake) + .or_insert(*stake); } staked_nodes } @@ -458,9 +438,7 @@ mod tests { let lamports = account.lamports(); let vote_account = VoteAccount::try_from(account).unwrap(); assert_eq!(lamports, vote_account.lamports()); - assert_eq!(vote_state, *vote_account.vote_state().unwrap()); - // 2nd call to .vote_state() should return the cached value. - assert_eq!(vote_state, *vote_account.vote_state().unwrap()); + assert_eq!(vote_state, *vote_account.vote_state()); } #[test] @@ -468,8 +446,8 @@ mod tests { let mut rng = rand::thread_rng(); let (account, vote_state) = new_rand_vote_account(&mut rng, None); let vote_account = VoteAccount::try_from(account.clone()).unwrap(); - assert_eq!(vote_state, *vote_account.vote_state().unwrap()); - // Assert than VoteAccount has the same wire format as Account. + assert_eq!(vote_state, *vote_account.vote_state()); + // Assert that VoteAccount has the same wire format as Account. assert_eq!( bincode::serialize(&account).unwrap(), bincode::serialize(&vote_account).unwrap() @@ -482,10 +460,10 @@ mod tests { let (account, vote_state) = new_rand_vote_account(&mut rng, None); let data = bincode::serialize(&account).unwrap(); let vote_account = VoteAccount::try_from(account).unwrap(); - assert_eq!(vote_state, *vote_account.vote_state().unwrap()); + assert_eq!(vote_state, *vote_account.vote_state()); let other_vote_account: VoteAccount = bincode::deserialize(&data).unwrap(); assert_eq!(vote_account, other_vote_account); - assert_eq!(vote_state, *other_vote_account.vote_state().unwrap()); + assert_eq!(vote_state, *other_vote_account.vote_state()); } #[test] @@ -493,12 +471,12 @@ mod tests { let mut rng = rand::thread_rng(); let (account, vote_state) = new_rand_vote_account(&mut rng, None); let vote_account = VoteAccount::try_from(account).unwrap(); - assert_eq!(vote_state, *vote_account.vote_state().unwrap()); + assert_eq!(vote_state, *vote_account.vote_state()); let data = bincode::serialize(&vote_account).unwrap(); let other_vote_account: VoteAccount = bincode::deserialize(&data).unwrap(); // Assert that serialize->deserialized returns the same VoteAccount. assert_eq!(vote_account, other_vote_account); - assert_eq!(vote_state, *other_vote_account.vote_state().unwrap()); + assert_eq!(vote_state, *other_vote_account.vote_state()); } #[test] @@ -684,7 +662,7 @@ mod tests { let staked_nodes = vote_accounts.staked_nodes(); let (pubkey, (more_stake, vote_account)) = accounts.find(|(_, (stake, _))| *stake != 0).unwrap(); - let node_pubkey = *vote_account.node_pubkey().unwrap(); + let node_pubkey = *vote_account.node_pubkey(); vote_accounts.insert(pubkey, vote_account, || more_stake); assert_ne!(staked_nodes, vote_accounts.staked_nodes()); assert_eq!(