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 20, 2024
1 parent 9c6f4b7 commit 771c631
Show file tree
Hide file tree
Showing 17 changed files with 111 additions and 197 deletions.
20 changes: 9 additions & 11 deletions core/src/commitment_service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -202,19 +202,17 @@ impl AggregateCommitmentService {
}
let vote_state = if pubkey == node_vote_pubkey {
// Override old vote_state in bank with latest one for my own vote pubkey
Ok(node_vote_state)
node_vote_state
} else {
account.vote_state()
};
if let Ok(vote_state) = vote_state {
Self::aggregate_commitment_for_vote_account(
&mut commitment,
&mut rooted_stake,
vote_state,
ancestors,
*lamports,
);
}
Self::aggregate_commitment_for_vote_account(
&mut commitment,
&mut rooted_stake,
vote_state,
ancestors,
*lamports,
);
}

(commitment, rooted_stake)
Expand Down Expand Up @@ -546,7 +544,7 @@ mod tests {
fn test_highest_super_majority_root_advance() {
fn get_vote_state(vote_pubkey: Pubkey, bank: &Bank) -> VoteState {
let vote_account = bank.get_vote_account(&vote_pubkey).unwrap();
vote_account.vote_state().cloned().unwrap()
vote_account.vote_state().clone()
}

let block_commitment_cache = RwLock::new(BlockCommitmentCache::new_for_tests());
Expand Down
24 changes: 4 additions & 20 deletions core/src/consensus.rs
Original file line number Diff line number Diff line change
Expand Up @@ -379,20 +379,7 @@ impl Tower {
continue;
}
trace!("{} {} with stake {}", vote_account_pubkey, key, voted_stake);
let mut vote_state = match account.vote_state().cloned() {
Err(_) => {
datapoint_warn!(
"tower_warn",
(
"warn",
format!("Unable to get vote_state from account {key}"),
String
),
);
continue;
}
Ok(vote_state) => vote_state,
};
let mut vote_state = account.vote_state().clone();
for vote in &vote_state.votes {
lockout_intervals
.entry(vote.lockout.last_locked_out_slot())
Expand Down Expand Up @@ -591,7 +578,7 @@ impl Tower {
pub fn last_voted_slot_in_bank(bank: &Bank, vote_account_pubkey: &Pubkey) -> Option<Slot> {
let vote_account = bank.get_vote_account(vote_account_pubkey)?;
let vote_state = vote_account.vote_state();
vote_state.as_ref().ok()?.last_voted_slot()
vote_state.last_voted_slot()
}

pub fn record_bank_vote(&mut self, bank: &Bank) -> Option<Slot> {
Expand Down Expand Up @@ -1576,10 +1563,7 @@ impl Tower {
bank: &Bank,
) {
if let Some(vote_account) = bank.get_vote_account(vote_account_pubkey) {
self.vote_state = vote_account
.vote_state()
.cloned()
.expect("vote_account isn't a VoteState?");
self.vote_state = vote_account.vote_state().clone();
self.initialize_root(root);
self.initialize_lockouts(|v| v.slot() > root);
trace!(
Expand Down Expand Up @@ -2428,7 +2412,7 @@ pub mod test {
.get_vote_account(&vote_pubkey)
.unwrap();
let state = observed.vote_state();
info!("observed tower: {:#?}", state.as_ref().unwrap().votes);
info!("observed tower: {:#?}", state.votes);

let num_slots_to_try = 200;
cluster_votes
Expand Down
31 changes: 26 additions & 5 deletions core/src/consensus/progress_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -422,15 +422,36 @@ impl ProgressMap {
mod test {
use {
super::*,
solana_sdk::account::{Account, AccountSharedData},
rand::Rng as _,
solana_sdk::{account::AccountSharedData, clock::Clock},
solana_vote::vote_account::VoteAccount,
solana_vote_program::vote_state::{VoteInit, VoteState, VoteStateVersions},
};

fn new_test_vote_account() -> VoteAccount {
let account = AccountSharedData::from(Account {
owner: solana_vote_program::id(),
..Account::default()
});
let mut rng = rand::thread_rng();

let vote_init = VoteInit {
node_pubkey: Pubkey::new_unique(),
authorized_voter: Pubkey::new_unique(),
authorized_withdrawer: Pubkey::new_unique(),
commission: rng.gen(),
};
let clock = Clock {
slot: rng.gen(),
epoch_start_timestamp: rng.gen(),
epoch: rng.gen(),
leader_schedule_epoch: rng.gen(),
unix_timestamp: rng.gen(),
};
let vote_state = VoteState::new(&vote_init, &clock);
let account = AccountSharedData::new_data(
rng.gen(), // lamports
&VoteStateVersions::new_current(vote_state.clone()),
&solana_sdk::vote::program::id(), // owner
)
.unwrap();

VoteAccount::try_from(account).unwrap()
}

Expand Down
20 changes: 2 additions & 18 deletions core/src/replay_stage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2470,17 +2470,6 @@ impl ReplayStage {
Some(vote_account) => vote_account,
};
let vote_state = vote_account.vote_state();
let vote_state = match vote_state.as_ref() {
Err(_) => {
warn!(
"Vote account {} is unreadable. Unable to vote",
vote_account_pubkey,
);
return GenerateVoteTxResult::Failed;
}
Ok(vote_state) => vote_state,
};

if vote_state.node_pubkey != node_keypair.pubkey() {
info!(
"Vote account node_pubkey mismatch: {} (expected: {}). Unable to vote",
Expand Down Expand Up @@ -3458,9 +3447,7 @@ impl ReplayStage {
let Some(vote_account) = bank.get_vote_account(my_vote_pubkey) else {
return;
};
let Ok(mut bank_vote_state) = vote_account.vote_state().cloned() else {
return;
};
let mut bank_vote_state = vote_account.vote_state().clone();
if bank_vote_state.last_voted_slot() <= tower.vote_state.last_voted_slot() {
return;
}
Expand Down Expand Up @@ -7931,10 +7918,7 @@ pub(crate) mod tests {
let vote_account = expired_bank_child
.get_vote_account(&my_vote_pubkey)
.unwrap();
assert_eq!(
vote_account.vote_state().as_ref().unwrap().tower(),
vec![0, 1]
);
assert_eq!(vote_account.vote_state().tower(), vec![0, 1]);
expired_bank_child.fill_bank_with_ticks_for_tests();
expired_bank_child.freeze();

Expand Down
2 changes: 1 addition & 1 deletion core/src/validator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2488,7 +2488,7 @@ fn get_stake_percent_in_gossip(bank: &Bank, cluster_info: &ClusterInfo, log: boo
if activated_stake == 0 {
continue;
}
let vote_state_node_pubkey = vote_account.node_pubkey().copied().unwrap_or_default();
let vote_state_node_pubkey = *vote_account.node_pubkey();

if let Some(peer) = peers.get(&vote_state_node_pubkey) {
if peer.shred_version() == my_shred_version {
Expand Down
9 changes: 2 additions & 7 deletions core/src/vote_simulator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ impl VoteSimulator {
let tower_sync = if let Some(vote_account) =
parent_bank.get_vote_account(&keypairs.vote_keypair.pubkey())
{
let mut vote_state = vote_account.vote_state().unwrap().clone();
let mut vote_state = vote_account.vote_state().clone();
process_vote_unchecked(
&mut vote_state,
solana_vote_program::vote_state::Vote::new(
Expand Down Expand Up @@ -143,12 +143,7 @@ impl VoteSimulator {
.get_vote_account(&keypairs.vote_keypair.pubkey())
.unwrap();
let state = vote_account.vote_state();
assert!(state
.as_ref()
.unwrap()
.votes
.iter()
.any(|lockout| lockout.slot() == parent));
assert!(state.votes.iter().any(|lockout| lockout.slot() == parent));
}
}
while new_bank.tick_height() < new_bank.max_tick_height() {
Expand Down
3 changes: 0 additions & 3 deletions ledger-tool/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,6 @@ fn graph_forks(bank_forks: &BankForks, config: &GraphConfig) -> String {

// Search all forks and collect the last vote made by each validator
let mut last_votes = HashMap::new();
let default_vote_state = VoteState::default();
for fork_slot in &fork_slots {
let bank = &bank_forks[*fork_slot];

Expand All @@ -217,7 +216,6 @@ fn graph_forks(bank_forks: &BankForks, config: &GraphConfig) -> String {
.sum();
for (stake, vote_account) in bank.vote_accounts().values() {
let vote_state = vote_account.vote_state();
let vote_state = vote_state.unwrap_or(&default_vote_state);
if let Some(last_vote) = vote_state.votes.iter().last() {
let entry = last_votes.entry(vote_state.node_pubkey).or_insert((
last_vote.slot(),
Expand Down Expand Up @@ -258,7 +256,6 @@ fn graph_forks(bank_forks: &BankForks, config: &GraphConfig) -> String {
loop {
for (_, vote_account) in bank.vote_accounts().values() {
let vote_state = vote_account.vote_state();
let vote_state = vote_state.unwrap_or(&default_vote_state);
if let Some(last_vote) = vote_state.votes.iter().last() {
let validator_votes = all_votes.entry(vote_state.node_pubkey).or_default();
validator_votes
Expand Down
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
5 changes: 2 additions & 3 deletions rpc/src/rpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ use {
TransactionBinaryEncoding, TransactionConfirmationStatus, TransactionStatus,
UiConfirmedBlock, UiTransactionEncoding,
},
solana_vote_program::vote_state::{VoteState, MAX_LOCKOUT_HISTORY},
solana_vote_program::vote_state::MAX_LOCKOUT_HISTORY,
spl_token_2022::{
extension::{
interest_bearing_mint::InterestBearingConfig, BaseStateWithExtensions,
Expand Down Expand Up @@ -1004,7 +1004,6 @@ impl JsonRpcRequestProcessor {
let epoch_vote_accounts = bank
.epoch_vote_accounts(bank.get_epoch_and_slot_index(bank.slot()).0)
.ok_or_else(Error::invalid_request)?;
let default_vote_state = VoteState::default();
let delinquent_validator_slot_distance = config
.delinquent_slot_distance
.unwrap_or(DELINQUENT_VALIDATOR_SLOT_DISTANCE);
Expand All @@ -1021,7 +1020,6 @@ impl JsonRpcRequestProcessor {
}

let vote_state = account.vote_state();
let vote_state = vote_state.unwrap_or(&default_vote_state);
let last_vote = if let Some(vote) = vote_state.votes.iter().last() {
vote.slot()
} else {
Expand Down Expand Up @@ -4360,6 +4358,7 @@ pub mod tests {
transaction::{
self, SimpleAddressLoader, Transaction, TransactionError, TransactionVersion,
},
vote::state::VoteState,
},
solana_transaction_status::{
EncodedConfirmedBlock, EncodedTransaction, EncodedTransactionWithStatusMeta,
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 @@ -5874,6 +5869,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
Loading

0 comments on commit 771c631

Please sign in to comment.