From 63b2da072902dd05806dd182595be4b0fac0c388 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 --- core/Cargo.toml | 1 + core/src/commitment_service.rs | 20 ++- core/src/consensus.rs | 24 +--- core/src/consensus/progress_map.rs | 18 +-- core/src/replay_stage.rs | 20 +-- core/src/validator.rs | 2 +- core/src/vote_simulator.rs | 9 +- ledger-tool/src/main.rs | 3 - ledger/src/blockstore_processor.rs | 28 +--- programs/bpf_loader/Cargo.toml | 2 +- programs/bpf_loader/src/syscalls/mod.rs | 10 +- rpc/src/rpc.rs | 5 +- 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/Cargo.toml | 4 +- vote/src/vote_account.rs | 134 ++++++++++-------- 21 files changed, 129 insertions(+), 219 deletions(-) diff --git a/core/Cargo.toml b/core/Cargo.toml index 4d3c59a8ada4f8..d107296bba0e6e 100644 --- a/core/Cargo.toml +++ b/core/Cargo.toml @@ -110,6 +110,7 @@ solana-stake-program = { workspace = true } solana-unified-scheduler-pool = { workspace = true, features = [ "dev-context-only-utils", ] } +solana-vote = { workspace = true, features = ["dev-context-only-utils"] } static_assertions = { workspace = true } systemstat = { workspace = true } test-case = { workspace = true } diff --git a/core/src/commitment_service.rs b/core/src/commitment_service.rs index 03c04ad9ab4c96..5cdcfa94ec593b 100644 --- a/core/src/commitment_service.rs +++ b/core/src/commitment_service.rs @@ -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) @@ -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()); diff --git a/core/src/consensus.rs b/core/src/consensus.rs index 93c80c554c6ab1..96fbbc6b68d0bd 100644 --- a/core/src/consensus.rs +++ b/core/src/consensus.rs @@ -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()) @@ -591,7 +578,7 @@ impl Tower { pub fn last_voted_slot_in_bank(bank: &Bank, vote_account_pubkey: &Pubkey) -> Option { 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 { @@ -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!( @@ -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 diff --git a/core/src/consensus/progress_map.rs b/core/src/consensus/progress_map.rs index a06f51b2001534..447ebff0f8361e 100644 --- a/core/src/consensus/progress_map.rs +++ b/core/src/consensus/progress_map.rs @@ -420,19 +420,7 @@ impl ProgressMap { #[cfg(test)] mod test { - use { - super::*, - solana_sdk::account::{Account, AccountSharedData}, - solana_vote::vote_account::VoteAccount, - }; - - fn new_test_vote_account() -> VoteAccount { - let account = AccountSharedData::from(Account { - owner: solana_vote_program::id(), - ..Account::default() - }); - VoteAccount::try_from(account).unwrap() - } + use {super::*, solana_vote::vote_account::VoteAccount}; #[test] fn test_add_vote_pubkey() { @@ -467,7 +455,7 @@ mod test { let epoch_vote_accounts: HashMap<_, _> = vote_account_pubkeys .iter() .skip(num_vote_accounts - staked_vote_accounts) - .map(|pubkey| (*pubkey, (1, new_test_vote_account()))) + .map(|pubkey| (*pubkey, (1, VoteAccount::new_random()))) .collect(); let mut stats = PropagatedStats::default(); @@ -509,7 +497,7 @@ mod test { let epoch_vote_accounts: HashMap<_, _> = vote_account_pubkeys .iter() .skip(num_vote_accounts - staked_vote_accounts) - .map(|pubkey| (*pubkey, (1, new_test_vote_account()))) + .map(|pubkey| (*pubkey, (1, VoteAccount::new_random()))) .collect(); stats.add_node_pubkey_internal(&node_pubkey, &vote_account_pubkeys, &epoch_vote_accounts); assert!(stats.propagated_node_ids.contains(&node_pubkey)); diff --git a/core/src/replay_stage.rs b/core/src/replay_stage.rs index 7f7d0f61157d9c..010688cfbcd764 100644 --- a/core/src/replay_stage.rs +++ b/core/src/replay_stage.rs @@ -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", @@ -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; } @@ -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(); diff --git a/core/src/validator.rs b/core/src/validator.rs index 016514dd817166..0f99e4c4768497 100644 --- a/core/src/validator.rs +++ b/core/src/validator.rs @@ -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 { diff --git a/core/src/vote_simulator.rs b/core/src/vote_simulator.rs index f886d2821af4b0..74fa0a1ebd3476 100644 --- a/core/src/vote_simulator.rs +++ b/core/src/vote_simulator.rs @@ -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( @@ -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() { diff --git a/ledger-tool/src/main.rs b/ledger-tool/src/main.rs index 06f17a55e03a2a..ca7ae870420ae7 100644 --- a/ledger-tool/src/main.rs +++ b/ledger-tool/src/main.rs @@ -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]; @@ -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(), @@ -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 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/programs/bpf_loader/Cargo.toml b/programs/bpf_loader/Cargo.toml index ace6f2ed9c0b83..4c085663513300 100644 --- a/programs/bpf_loader/Cargo.toml +++ b/programs/bpf_loader/Cargo.toml @@ -34,7 +34,7 @@ assert_matches = { workspace = true } memoffset = { workspace = true } rand = { workspace = true } solana-sdk = { workspace = true, features = ["dev-context-only-utils"] } -solana-vote = { workspace = true } +solana-vote = { workspace = true, features = ["dev-context-only-utils"] } test-case = { workspace = true } [lib] diff --git a/programs/bpf_loader/src/syscalls/mod.rs b/programs/bpf_loader/src/syscalls/mod.rs index e70a266f340917..876b734af19156 100644 --- a/programs/bpf_loader/src/syscalls/mod.rs +++ b/programs/bpf_loader/src/syscalls/mod.rs @@ -4820,15 +4820,7 @@ mod tests { let mut vote_accounts_map = HashMap::new(); vote_accounts_map.insert( vote_address, - ( - expected_epoch_stake, - VoteAccount::try_from(AccountSharedData::new( - 0, - 0, - &solana_sdk::vote::program::id(), - )) - .unwrap(), - ), + (expected_epoch_stake, VoteAccount::new_random()), ); with_mock_invoke_context!(invoke_context, transaction_context, vec![]); diff --git a/rpc/src/rpc.rs b/rpc/src/rpc.rs index db4d7bf9e69b53..0cd32ab3ff84b1 100644 --- a/rpc/src/rpc.rs +++ b/rpc/src/rpc.rs @@ -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, @@ -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); @@ -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 { @@ -4360,6 +4358,7 @@ pub mod tests { transaction::{ self, SimpleAddressLoader, Transaction, TransactionError, TransactionVersion, }, + vote::state::VoteState, }, solana_transaction_status::{ EncodedConfirmedBlock, EncodedTransaction, EncodedTransactionWithStatusMeta, diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index f54e5f46793c64..da1d9c2eb6b503 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 = @@ -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 { + // 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/Cargo.toml b/vote/Cargo.toml index 2eb821eac407a4..03c22c88d31226 100644 --- a/vote/Cargo.toml +++ b/vote/Cargo.toml @@ -12,6 +12,7 @@ edition = { workspace = true } [dependencies] itertools = { workspace = true } log = { workspace = true } +rand = { workspace = true, optional = true } serde = { workspace = true, features = ["rc"] } serde_derive = { workspace = true } solana-frozen-abi = { workspace = true, optional = true } @@ -25,7 +26,6 @@ name = "solana_vote" [dev-dependencies] bincode = { workspace = true } -rand = { workspace = true } [package.metadata.docs.rs] targets = ["x86_64-unknown-linux-gnu"] @@ -34,7 +34,7 @@ targets = ["x86_64-unknown-linux-gnu"] rustc_version = { workspace = true, optional = true } [features] -dev-context-only-utils = [] +dev-context-only-utils = ["dep:rand"] frozen-abi = [ "dep:rustc_version", "dep:solana-frozen-abi", diff --git a/vote/src/vote_account.rs b/vote/src/vote_account.rs index 14cc788cca13d9..233cacfb6ac41d 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,49 @@ 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 + } + + #[cfg(feature = "dev-context-only-utils")] + pub fn new_random() -> VoteAccount { + use { + rand::Rng as _, + solana_sdk::{ + clock::Clock, + vote::state::{VoteInit, VoteStateVersions}, + }, + }; + + 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() } } @@ -103,9 +130,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 +183,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 +201,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 +242,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 +271,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 +321,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 +459,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 +474,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 +482,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 +496,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 +507,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 +698,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!(