From 329c6f131bbf551597293009e8f15e7fbc37b570 Mon Sep 17 00:00:00 2001 From: Ashwin Sekar Date: Wed, 23 Aug 2023 13:15:57 -0700 Subject: [PATCH] tower: when syncing from vote state, update last_vote (#32944) * tower: when syncing from vote state, update last_vote * pr: bubble error through unchecked --- core/src/consensus.rs | 54 +++++++++----- core/src/replay_stage.rs | 108 +++++++++++++++++++++++++++- programs/vote/src/vote_state/mod.rs | 15 ++-- 3 files changed, 150 insertions(+), 27 deletions(-) diff --git a/core/src/consensus.rs b/core/src/consensus.rs index 7b2622ac3..2b30a9be9 100644 --- a/core/src/consensus.rs +++ b/core/src/consensus.rs @@ -521,7 +521,7 @@ impl Tower { last_voted_slot_in_bank: Option, ) -> VoteTransaction { let vote = Vote::new(vec![slot], hash); - process_vote_unchecked(local_vote_state, vote); + let _ignored = process_vote_unchecked(local_vote_state, vote); let slots = if let Some(last_voted_slot) = last_voted_slot_in_bank { local_vote_state .votes @@ -554,6 +554,23 @@ impl Tower { ) } + /// If we've recently updated the vote state by applying a new vote + /// or syncing from a bank, generate the proper last_vote. + pub(crate) fn update_last_vote_from_vote_state(&mut self, vote_hash: Hash) { + let mut new_vote = VoteTransaction::from(VoteStateUpdate::new( + self.vote_state + .votes + .iter() + .map(|vote| vote.lockout) + .collect(), + self.vote_state.root_slot, + vote_hash, + )); + + new_vote.set_timestamp(self.maybe_timestamp(self.last_voted_slot().unwrap_or_default())); + self.last_vote = new_vote; + } + fn record_bank_vote_and_update_lockouts( &mut self, vote_slot: Slot, @@ -564,29 +581,28 @@ impl Tower { trace!("{} record_vote for {}", self.node_pubkey, vote_slot); let old_root = self.root(); - let mut new_vote = if is_direct_vote_state_update_enabled { + if is_direct_vote_state_update_enabled { let vote = Vote::new(vec![vote_slot], vote_hash); - process_vote_unchecked(&mut self.vote_state, vote); - VoteTransaction::from(VoteStateUpdate::new( - self.vote_state - .votes - .iter() - .map(|vote| vote.lockout) - .collect(), - self.vote_state.root_slot, - vote_hash, - )) + let result = process_vote_unchecked(&mut self.vote_state, vote); + if result.is_err() { + error!( + "Error while recording vote {} {} in local tower {:?}", + vote_slot, vote_hash, result + ); + } + self.update_last_vote_from_vote_state(vote_hash); } else { - Self::apply_vote_and_generate_vote_diff( + let mut new_vote = Self::apply_vote_and_generate_vote_diff( &mut self.vote_state, vote_slot, vote_hash, last_voted_slot_in_bank, - ) - }; + ); - new_vote.set_timestamp(self.maybe_timestamp(self.last_voted_slot().unwrap_or_default())); - self.last_vote = new_vote; + new_vote + .set_timestamp(self.maybe_timestamp(self.last_voted_slot().unwrap_or_default())); + self.last_vote = new_vote; + }; let new_root = self.root(); @@ -2541,7 +2557,7 @@ pub mod test { hash: Hash::default(), timestamp: None, }; - vote_state::process_vote_unchecked(&mut local, vote); + let _ = vote_state::process_vote_unchecked(&mut local, vote); assert_eq!(local.votes.len(), 1); let vote = Tower::apply_vote_and_generate_vote_diff(&mut local, 1, Hash::default(), Some(0)); @@ -2557,7 +2573,7 @@ pub mod test { hash: Hash::default(), timestamp: None, }; - vote_state::process_vote_unchecked(&mut local, vote); + let _ = vote_state::process_vote_unchecked(&mut local, vote); assert_eq!(local.votes.len(), 1); // First vote expired, so should be evicted from tower. Thus even with diff --git a/core/src/replay_stage.rs b/core/src/replay_stage.rs index 48fc58ff1..de8c4a7fa 100644 --- a/core/src/replay_stage.rs +++ b/core/src/replay_stage.rs @@ -3074,6 +3074,37 @@ impl ReplayStage { tower.vote_state.root_slot = bank_vote_state.root_slot; tower.vote_state.votes = bank_vote_state.votes; + + let last_voted_slot = tower.vote_state.last_voted_slot().unwrap_or( + // If our local root is higher than the highest slot in `bank_vote_state` due to + // supermajority roots, then it's expected that the vote state will be empty. + // In this case we use the root as our last vote. This root cannot be None, because + // `tower.vote_state.last_voted_slot()` is None only if `tower.vote_state.root_slot` + // is Some. + tower + .vote_state + .root_slot + .expect("root_slot cannot be None here"), + ); + // This is safe because `last_voted_slot` is now equal to + // `bank_vote_state.last_voted_slot()` or `local_root`. + // Since this vote state is contained in `bank`, which we have frozen, + // we must have frozen all slots contained in `bank_vote_state`, + // and by definition we must have frozen `local_root`. + // + // If `bank` is a duplicate, since we are able to replay it successfully, any slots + // in its vote state must also be part of the duplicate fork, and thus present in our + // progress map. + // + // Finally if both `bank` and `bank_vote_state.last_voted_slot()` are duplicate, + // we must have the compatible versions of both duplicates in order to replay `bank` + // successfully, so we are once again guaranteed that `bank_vote_state.last_voted_slot()` + // is present in progress map. + tower.update_last_vote_from_vote_state( + progress + .get_hash(last_voted_slot) + .expect("Must exist for us to have frozen descendant"), + ); } } } @@ -6253,6 +6284,7 @@ pub(crate) mod tests { &mut tower, &mut vote_simulator.heaviest_subtree_fork_choice, &mut vote_simulator.latest_validator_votes_for_frozen_banks, + None, ); assert_eq!(vote_fork.unwrap(), 4); assert_eq!(reset_fork.unwrap(), 4); @@ -6271,6 +6303,7 @@ pub(crate) mod tests { &mut tower, &mut vote_simulator.heaviest_subtree_fork_choice, &mut vote_simulator.latest_validator_votes_for_frozen_banks, + None, ); assert!(vote_fork.is_none()); assert_eq!(reset_fork, Some(5)); @@ -6313,6 +6346,7 @@ pub(crate) mod tests { &mut tower, &mut vote_simulator.heaviest_subtree_fork_choice, &mut vote_simulator.latest_validator_votes_for_frozen_banks, + None, ); assert!(vote_fork.is_none()); assert!(reset_fork.is_none()); @@ -6348,6 +6382,7 @@ pub(crate) mod tests { &mut tower, &mut vote_simulator.heaviest_subtree_fork_choice, &mut vote_simulator.latest_validator_votes_for_frozen_banks, + None, ); // Should now pick 5 as the heaviest fork from last vote again. assert!(vote_fork.is_none()); @@ -6399,6 +6434,7 @@ pub(crate) mod tests { &mut tower, &mut vote_simulator.heaviest_subtree_fork_choice, &mut vote_simulator.latest_validator_votes_for_frozen_banks, + None, ); assert_eq!(vote_fork.unwrap(), 4); assert_eq!(reset_fork.unwrap(), 4); @@ -6445,6 +6481,7 @@ pub(crate) mod tests { &mut tower, &mut vote_simulator.heaviest_subtree_fork_choice, &mut vote_simulator.latest_validator_votes_for_frozen_banks, + None, ); assert!(vote_fork.is_none()); assert_eq!(reset_fork, Some(3)); @@ -6479,6 +6516,7 @@ pub(crate) mod tests { &mut tower, &mut vote_simulator.heaviest_subtree_fork_choice, &mut vote_simulator.latest_validator_votes_for_frozen_banks, + None, ); // Should now pick the next heaviest fork that is not a descendant of 2, which is 6. @@ -6518,6 +6556,7 @@ pub(crate) mod tests { &mut tower, &mut vote_simulator.heaviest_subtree_fork_choice, &mut vote_simulator.latest_validator_votes_for_frozen_banks, + None, ); // Should now pick the heaviest fork 4 again, but lockouts apply so fork 4 // is not votable, which avoids voting for 4 again. @@ -7869,6 +7908,7 @@ pub(crate) mod tests { tower: &mut Tower, heaviest_subtree_fork_choice: &mut HeaviestSubtreeForkChoice, latest_validator_votes_for_frozen_banks: &mut LatestValidatorVotesForFrozenBanks, + my_vote_pubkey: Option, ) -> (Option, Option) { let mut frozen_banks: Vec<_> = bank_forks .read() @@ -7880,7 +7920,7 @@ pub(crate) mod tests { let ancestors = &bank_forks.read().unwrap().ancestors(); let descendants = &bank_forks.read().unwrap().descendants(); ReplayStage::compute_bank_stats( - &Pubkey::default(), + &my_vote_pubkey.unwrap_or_default(), &bank_forks.read().unwrap().ancestors(), &mut frozen_banks, tower, @@ -7977,4 +8017,70 @@ pub(crate) mod tests { ReplayStage::check_for_vote_only_mode(10, 0, &in_vote_only_mode, &bank_forks); assert!(!in_vote_only_mode.load(Ordering::Relaxed)); } + + #[test] + fn test_tower_sync_from_bank() { + solana_logger::setup_with_default( + "error,solana_core::replay_stage=info,solana_core::consensus=info", + ); + /* + Fork structure: + + slot 0 + | + slot 1 + / \ + slot 2 | + | slot 3 + slot 4 | + slot 5 + | + slot 6 + + We had some point voted 0 - 6, while the rest of the network voted 0 - 4. + We are sitting with an oudated tower that has voted until 1. We see that 2 is the heaviest slot, + however in the past we have voted up to 6. We must acknowledge the vote state present at 6, + adopt it as our own and *not* vote on 2 or 4, to respect slashing rules. + */ + + let generate_votes = |pubkeys: Vec| { + pubkeys + .into_iter() + .zip(iter::once(vec![0, 1, 3, 5, 6]).chain(iter::repeat(vec![0, 1, 2, 4]).take(2))) + .collect() + }; + let (mut vote_simulator, _blockstore) = + setup_default_forks(3, Some(Box::new(generate_votes))); + let (bank_forks, mut progress) = (vote_simulator.bank_forks, vote_simulator.progress); + let bank_hash = |slot| bank_forks.read().unwrap().bank_hash(slot).unwrap(); + let my_vote_pubkey = vote_simulator.vote_pubkeys[0]; + let mut tower = Tower::default(); + tower.node_pubkey = vote_simulator.node_pubkeys[0]; + tower.record_vote(0, bank_hash(0)); + tower.record_vote(1, bank_hash(1)); + + let (vote_fork, reset_fork) = run_compute_and_select_forks( + &bank_forks, + &mut progress, + &mut tower, + &mut vote_simulator.heaviest_subtree_fork_choice, + &mut vote_simulator.latest_validator_votes_for_frozen_banks, + Some(my_vote_pubkey), + ); + + assert_eq!(vote_fork, None); + assert_eq!(reset_fork, Some(6)); + + let (vote_fork, reset_fork) = run_compute_and_select_forks( + &bank_forks, + &mut progress, + &mut tower, + &mut vote_simulator.heaviest_subtree_fork_choice, + &mut vote_simulator.latest_validator_votes_for_frozen_banks, + Some(my_vote_pubkey), + ); + + assert_eq!(vote_fork, None); + assert_eq!(reset_fork, Some(6)); + } } diff --git a/programs/vote/src/vote_state/mod.rs b/programs/vote/src/vote_state/mod.rs index 5aa0f9540..407cede49 100644 --- a/programs/vote/src/vote_state/mod.rs +++ b/programs/vote/src/vote_state/mod.rs @@ -708,7 +708,7 @@ pub fn process_new_vote_state( Ok(()) } -fn process_vote_unfiltered( +pub fn process_vote_unfiltered( vote_state: &mut VoteState, vote_slots: &[Slot], vote: &Vote, @@ -745,18 +745,18 @@ pub fn process_vote( } /// "unchecked" functions used by tests and Tower -pub fn process_vote_unchecked(vote_state: &mut VoteState, vote: Vote) { +pub fn process_vote_unchecked(vote_state: &mut VoteState, vote: Vote) -> Result<(), VoteError> { if vote.slots.is_empty() { - return; + return Err(VoteError::EmptySlots); } let slot_hashes: Vec<_> = vote.slots.iter().rev().map(|x| (*x, vote.hash)).collect(); - let _ignored = process_vote_unfiltered( + process_vote_unfiltered( vote_state, &vote.slots, &vote, &slot_hashes, vote_state.current_epoch(), - ); + ) } #[cfg(test)] @@ -767,7 +767,7 @@ pub fn process_slot_votes_unchecked(vote_state: &mut VoteState, slots: &[Slot]) } pub fn process_slot_vote_unchecked(vote_state: &mut VoteState, slot: Slot) { - process_vote_unchecked(vote_state, Vote::new(vec![slot], Hash::default())); + let _ = process_vote_unchecked(vote_state, Vote::new(vec![slot], Hash::default())); } /// Authorize the given pubkey to withdraw or sign votes. This may be called multiple times, @@ -1684,7 +1684,8 @@ mod tests { hash: Hash::new_unique(), timestamp: None, }, - ); + ) + .unwrap(); // Now use the resulting new vote state to perform a vote state update on vote_state assert_eq!(