Fix callers of VoteAccounts::vote_state() holding the lock too long

Author: brooksprumo

core/src/consensus.rs

@@ -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",
@@ -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

core/src/replay_stage.rs

@@ -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!(

core/src/validator.rs

@@ -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 {

ledger/src/blockstore_processor.rs

@@ -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();
 

runtime/src/bank.rs

@@ -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::{
@@ -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;
@@ -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))
                 }
             })

runtime/src/epoch_stakes.rs

@@ -72,21 +72,20 @@ impl EpochStakes {
         let epoch_authorized_voters = epoch_vote_accounts
             .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,
-                };
+                let vote_state_lock = account.vote_state();
+                if vote_state_lock.is_err() {
+                    drop(vote_state_lock);
+                    datapoint_warn!(
+                        "parse_epoch_vote_accounts",
+                        (
+                            "warn",
+                            format!("Unable to get vote_state from account {}", key),
+                            String
+                        ),
+                    );
+                    return None;
+                }
+                let vote_state = vote_state_lock.as_ref().unwrap();
 
                 if *stake > 0 {
                     if let Some(authorized_voter) = vote_state

runtime/src/stakes.rs

@@ -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()
     }
 }
 

runtime/src/vote_account.rs

@@ -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),
@@ -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)
     }
 }