Skip to content

Commit

Permalink
Fix callers of VoteAccounts::vote_state() holding the lock too long
Browse files Browse the repository at this point in the history
  • Loading branch information
brooksprumo committed Jun 24, 2022
1 parent 877feda commit 956587a
Show file tree
Hide file tree
Showing 8 changed files with 48 additions and 47 deletions.
5 changes: 3 additions & 2 deletions core/src/consensus.rs
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,8 @@ impl Tower {
continue;
}
trace!("{} {} with stake {}", vote_account_pubkey, key, voted_stake);
let mut vote_state = match account.vote_state().as_ref() {
let vote_state = account.vote_state().clone();
let mut vote_state = match vote_state {
Err(_) => {
datapoint_warn!(
"tower_warn",
Expand All @@ -283,7 +284,7 @@ impl Tower {
);
continue;
}
Ok(vote_state) => vote_state.clone(),
Ok(vote_state) => vote_state,
};
for vote in &vote_state.votes {
lockout_intervals
Expand Down
20 changes: 10 additions & 10 deletions core/src/replay_stage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1919,16 +1919,16 @@ impl ReplayStage {
Some((_stake, 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 None;
}
Ok(vote_state) => vote_state,
};
if vote_state.is_err() {
drop(vote_state);
warn!(
"Vote account {} is unreadable. Unable to vote",
vote_account_pubkey,
);
return None;
}
// SAFETY: The `.unwrap()` is safe here because we just checked if vote_state is Err
let vote_state = vote_state.as_ref().unwrap();

if vote_state.node_pubkey != node_keypair.pubkey() {
info!(
Expand Down
6 changes: 1 addition & 5 deletions core/src/validator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1956,11 +1956,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
.vote_state()
.as_ref()
.map(|vote_state| vote_state.node_pubkey)
.unwrap_or_default();
let vote_state_node_pubkey = vote_account.node_pubkey().unwrap_or_default();

if let Some(peer) = peers.get(&vote_state_node_pubkey) {
if peer.shred_version == my_shred_version {
Expand Down
21 changes: 12 additions & 9 deletions ledger/src/blockstore_processor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1462,16 +1462,19 @@ fn supermajority_root_from_vote_accounts(
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)),
let vote_state = account.vote_state();
if vote_state.is_err() {
drop(vote_state);
warn!(
"Unable to get vote_state from account {} in bank: {}",
key, bank_slot
);
return None;
}
// SAFETY: The `.unwrap()` is safe here because we just checked if vote_state is Err
let vote_state = vote_state.as_ref().unwrap();

Some((vote_state.root_slot?, *stake))
})
.collect();

Expand Down
9 changes: 5 additions & 4 deletions runtime/src/bank.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ use {
collections::{HashMap, HashSet},
convert::{TryFrom, TryInto},
fmt, mem,
ops::{Deref, Div, RangeInclusive},
ops::{Div, RangeInclusive},
path::PathBuf,
rc::Rc,
sync::{
Expand Down Expand Up @@ -2977,8 +2977,9 @@ impl Bank {
invalid_vote_keys.insert(vote_pubkey, InvalidCacheEntryReason::WrongOwner);
return None;
}
let vote_state = match vote_account.vote_state().deref() {
Ok(vote_state) => vote_state.clone(),
let vote_state = vote_account.vote_state().clone();
let vote_state = match vote_state {
Ok(vote_state) => vote_state,
Err(_) => {
invalid_vote_keys.insert(vote_pubkey, InvalidCacheEntryReason::BadState);
return None;
Expand Down Expand Up @@ -5043,7 +5044,7 @@ impl Bank {
None
} else {
total_staked += *staked;
let node_pubkey = account.vote_state().as_ref().ok()?.node_pubkey;
let node_pubkey = account.node_pubkey()?;
Some((node_pubkey, *staked))
}
})
Expand Down
28 changes: 14 additions & 14 deletions runtime/src/epoch_stakes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,20 +73,20 @@ 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 vote_state.is_err() {
drop(vote_state);
datapoint_warn!(
"parse_epoch_vote_accounts",
(
"warn",
format!("Unable to get vote_state from account {}", key),
String
),
);
return None;
}
// SAFETY: The `.unwrap()` is safe here because we just checked if vote_state is Err
let vote_state = vote_state.as_ref().unwrap();

if *stake > 0 {
if let Some(authorized_voter) = vote_state
Expand Down
2 changes: 1 addition & 1 deletion runtime/src/stakes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ impl Stakes<StakeAccount> {
pub(crate) fn highest_staked_node(&self) -> Option<Pubkey> {
let key = |(_pubkey, (stake, _vote_account)): &(_, &(u64, _))| *stake;
let (_pubkey, (_stake, vote_account)) = self.vote_accounts.iter().max_by_key(key)?;
Some(vote_account.vote_state().as_ref().ok()?.node_pubkey)
vote_account.node_pubkey()
}
}

Expand Down
4 changes: 2 additions & 2 deletions runtime/src/vote_account.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ const INVALID_VOTE_STATE: Result<VoteState, Error> = Err(Error::InstructionError
#[serde(try_from = "Account")]
pub struct VoteAccount(Arc<VoteAccountInner>);

#[derive(Debug, Error)]
#[derive(Debug, Error, Clone)]
pub enum Error {
#[error(transparent)]
InstructionError(#[from] InstructionError),
Expand Down Expand Up @@ -93,7 +93,7 @@ impl VoteAccount {
}

/// VoteState.node_pubkey of this vote-account.
fn node_pubkey(&self) -> Option<Pubkey> {
pub fn node_pubkey(&self) -> Option<Pubkey> {
Some(self.vote_state().as_ref().ok()?.node_pubkey)
}
}
Expand Down

0 comments on commit 956587a

Please sign in to comment.