Skip to content

Commit

Permalink
VoteAccount: remove OnceLock around VoteState
Browse files Browse the repository at this point in the history
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
  • Loading branch information
alessandrod committed Aug 19, 2024
1 parent f9316f8 commit 9f16085
Show file tree
Hide file tree
Showing 9 changed files with 65 additions and 129 deletions.
28 changes: 7 additions & 21 deletions ledger/src/blockstore_processor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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| {
Expand Down Expand Up @@ -2005,27 +2004,17 @@ fn supermajority_root(roots: &[(Slot, u64)], total_epoch_stake: u64) -> Option<S
}

fn supermajority_root_from_vote_accounts(
bank_slot: Slot,
total_epoch_stake: u64,
vote_accounts: &VoteAccountsHashMap,
) -> Option<Slot> {
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();

Expand Down Expand Up @@ -4579,31 +4568,28 @@ 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
);

// Supermajority root should be 8, it has 7/10 of the stake
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
);
}
Expand Down
13 changes: 6 additions & 7 deletions runtime/src/bank.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
};
Expand Down Expand Up @@ -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({
(
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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<VoteAccountsHashMap> {
// log::error!(
// "VOTE ACCOUNTS {}",
// std::backtrace::Backtrace::force_capture()
// );
let stakes = self.stakes_cache.stakes();
Arc::from(stakes.vote_accounts())
}
Expand Down
2 changes: 1 addition & 1 deletion runtime/src/bank/fee_distribution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ impl Bank {
None
} else {
total_staked += *staked;
Some((*account.node_pubkey()?, *staked))
Some((*account.node_pubkey(), *staked))
}
})
.collect::<Vec<(Pubkey, u64)>>();
Expand Down
14 changes: 6 additions & 8 deletions runtime/src/bank/partitioned_epoch_rewards/calculation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -385,15 +385,15 @@ 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();

let redeemed = solana_stake_program::rewards::redeem_rewards(
rewarded_epoch,
stake_state,
&mut stake_account,
&vote_state,
vote_state,
&point_value,
stake_history,
reward_calc_tracer.as_ref(),
Expand All @@ -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,
});
Expand All @@ -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,
});
Expand Down Expand Up @@ -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,
)
Expand Down
8 changes: 2 additions & 6 deletions runtime/src/bank/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
14 changes: 0 additions & 14 deletions runtime/src/epoch_stakes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 2 additions & 3 deletions runtime/src/snapshot_minimizer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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());
});
}

Expand Down
12 changes: 3 additions & 9 deletions runtime/src/stakes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -428,7 +423,6 @@ impl Stakes<StakeAccount> {
new_rate_activation_epoch: Option<Epoch>,
) -> Option<VoteAccount> {
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, || {
Expand Down Expand Up @@ -507,9 +501,9 @@ impl Stakes<StakeAccount> {
&self.stake_delegations
}

pub(crate) fn highest_staked_node(&self) -> Option<Pubkey> {
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())
}
}

Expand Down Expand Up @@ -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));
}

Expand Down
Loading

0 comments on commit 9f16085

Please sign in to comment.