Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VoteAccount: remove OnceLock around VoteState #2659

Merged
merged 3 commits into from
Aug 24, 2024
Merged

VoteAccount: remove OnceLock around VoteState #2659

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f1561f4
VoteAccount: remove OnceLock around VoteState
alessandrod Aug 19, 2024
2dba7d1
VoteAccounts: discard invalid accounts on deserialization
alessandrod Aug 23, 2024
1978c05
VoteAccount: remove Deserialize impl
alessandrod Aug 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions core/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
20 changes: 9 additions & 11 deletions core/src/commitment_service.rs
View file
Open in desktop
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 {
Copy link
Author

@alessandrod alessandrod Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is a bit noisy because there are a lot of changes like this: vote_state() used to return Result, but votes that failed to be parsed were never inserted into VoteAccounts. So effectively the Err branches were never entered, this PR removes them, but there's no actual logic change.

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
View file
Open in desktop
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
18 changes: 3 additions & 15 deletions core/src/consensus/progress_map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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));
Expand Down
20 changes: 2 additions & 18 deletions core/src/replay_stage.rs
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion programs/bpf_loader/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
10 changes: 1 addition & 9 deletions programs/bpf_loader/src/syscalls/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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![]);
Expand Down
5 changes: 2 additions & 3 deletions rpc/src/rpc.rs
View file
Open in desktop
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
9 changes: 2 additions & 7 deletions runtime/src/bank.rs
View file
Open in desktop
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 {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: in the get_vote_account closure above we now have a redundant check for vote owner and deserializability since that's exactly what VoteAccount::try_from is doing now. There are some other redundant checks scattered around for things like vote owner as well. Can be cleaned up in a follow up.

            let account = self.get_account_with_fixed_root(vote_pubkey)?;
            if account.owner() == &solana_vote_program
                && VoteState::deserialize(account.data()).is_ok()
            {
                vote_accounts_cache_miss_count.fetch_add(1, Relaxed);
            }
            VoteAccount::try_from(account).ok()

Copy link
Author

@alessandrod alessandrod Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code has been dead for a long time I think. We check that VoteAccounts contains all vote accounts at snapshot load time. We have the vote_accounts_cache_miss_count metric that's never been non-zero anytime I've looked since I started this cursed work.

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
Loading