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

@@ -521,7 +521,7 @@ impl Tower {
         last_voted_slot_in_bank: Option<Slot>,
     ) -> 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);
+            let result = process_vote_unchecked(&mut self.vote_state, vote);
-            VoteTransaction::from(VoteStateUpdate::new(
+            if result.is_err() {
-                self.vote_state
+                error!(
-                    .votes
+                    "Error while recording vote {} {} in local tower {:?}",
-                    .iter()
+                    vote_slot, vote_hash, result
-                    .map(|vote| vote.lockout)
+                );
-                    .collect(),
+            }
-                self.vote_state.root_slot,
+            self.update_last_vote_from_vote_state(vote_hash);
-                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()));
+            new_vote
-        self.last_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

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<Pubkey>,
     ) -> (Option<Slot>, Option<Slot>) {
         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<Pubkey>| {
+            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));
+    }
 }

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