Reflow the adjust tower fn code path and new tests

Author: ryoqun

core/src/consensus.rs

@@ -569,22 +569,18 @@ impl Tower {
                     return SwitchForkDecision::SameFork;
                 }
 
-                // Generally, should never consider switching to an ancestor of your last vote
                 if last_vote_ancestors.contains(&switch_slot) {
-                    let restored_stray_exception = self.is_stray_last_vote() && switch_slot < last_voted_slot;
-                    if !restored_stray_exception {
+                    if !self.is_stray_last_vote() {
                         panic!(
                             "Should never consider switching to slot ({}), which is ancestors({:?}) of last vote: {}",
                             switch_slot,
                             last_vote_ancestors,
                             last_voted_slot
                         );
                     } else {
-                        // If last vote is stray, it's possible to reach here, because we don't have complete view of voting history.
-                        // In that case, pretend we're always failing to switch, regarless switch_slot is actually on same frok or not.
-                        // this is for safety; otherwise we accidentally might doubl#e-vte on ancestors.
-                        // siwtch_slot must be `< last_voted_slot` because switch_slot must be one of last_vote_ancestors in this if clause.
-                        return SwitchForkDecision::FailedSwitchThreshold(0, 1);
+                        // If last vote is stray, it's possible to reach here, because bank fork might not have the slot which (???)
+                        // lockout check should prevent double vote and slashing.
+                        return SwitchForkDecision::SameFork;
                     }
                 }
 
@@ -857,14 +853,9 @@ impl Tower {
         );
         assert_eq!(slot_history.check(replayed_root), Check::Found);
 
-        // reconcile_blockstore_roots_with_tower() should already have aligned these.
-        assert!(
-            tower_root <= replayed_root,
-            format!(
-                "tower root: {:?} >= replayed root slot: {}",
-                tower_root, replayed_root
-            )
-        );
+        if tower_root > replayed_root {
+            return Err(TowerError::TooOldBlockstore(tower_root, replayed_root));
+        }
         assert!(
             self.last_vote == Vote::default() && self.lockouts.votes.is_empty()
                 || self.last_vote != Vote::default() && !self.lockouts.votes.is_empty(),
@@ -1126,6 +1117,12 @@ pub enum TowerError {
     )]
     TooOldTower(Slot, Slot),
 
+    #[error(
+        "The blockstore is too old: \
+        root in tower ({0}) > root in blockstore ({1})"
+    )]
+    TooOldBlockstore(Slot, Slot),
+
     #[error("The tower is fatally inconsistent with blockstore: {0}")]
     FatallyInconsistent(&'static str),
 }
@@ -1184,14 +1181,24 @@ pub fn reconcile_blockstore_roots_with_tower(
                 ),
             })
             .collect();
-        // adjust this assertion as well; this can be triggered by being empty if tower is moved to new ledger
-        // which has older max slot.
-        assert!(
-            !new_roots.is_empty(),
-            "at least 1 parent slot must be found"
-        );
-
-        blockstore.set_roots(&new_roots)?;
+        if !new_roots.is_empty() {
+            info!(
+                "marking slots as root based on tower root: {:?} ({}..{}) ",
+                new_roots, tower_root, last_blockstore_root
+            );
+            blockstore.set_roots(&new_roots)?;
+        } else {
+            // This indicates we're in bad state; but still don't panic here.
+            // That's because we might have a chance of recovering properly with
+            // newer snapshot. Ultimately TooOldBlockstore should handle
+            // this bad condition.
+            warn!(
+                "Couldn't find any ancestor slots from tower root ({}) \
+                 towards ({}) in blockstore; blockstore pruned or only \
+                 tower moved?",
+                tower_root, last_blockstore_root,
+            );
+        }
     }
     Ok(())
 }
@@ -2713,8 +2720,7 @@ pub mod test {
     }
 
     #[test]
-    #[should_panic(expected = "at least 1 parent slot must be found")]
-    fn test_reconcile_blockstore_roots_with_tower_panic_no_parent() {
+    fn test_reconcile_blockstore_roots_with_tower_nop_no_parent() {
         solana_logger::setup();
         let blockstore_path = get_tmp_ledger_path!();
         {
@@ -2730,7 +2736,9 @@ pub mod test {
 
             let mut tower = Tower::new_with_key(&Pubkey::default());
             tower.lockouts.root_slot = Some(4);
+            assert_eq!(blockstore.last_root(), 0);
             reconcile_blockstore_roots_with_tower(&tower, &blockstore).unwrap();
+            assert_eq!(blockstore.last_root(), 0);
         }
         Blockstore::destroy(&blockstore_path).expect("Expected successful database destruction");
     }
@@ -3052,6 +3060,23 @@ pub mod test {
             .unwrap();
     }
 
+    #[test]
+    fn test_adjust_lockouts_after_replay_vote_on_root() {
+        let mut tower = Tower::new_for_tests(10, 0.9);
+        tower.lockouts.root_slot = Some(42);
+        tower.lockouts.votes.push_back(Lockout::new(42));
+        tower.lockouts.votes.push_back(Lockout::new(43));
+        tower.lockouts.votes.push_back(Lockout::new(44));
+        let vote = Vote::new(vec![44], Hash::default());
+        tower.last_vote = vote;
+
+        let mut slot_history = SlotHistory::default();
+        slot_history.add(42);
+
+        let tower = tower.adjust_lockouts_after_replay(42, &slot_history);
+        assert_eq!(tower.unwrap().voted_slots(), [43, 44]);
+    }
+
     #[test]
     fn test_adjust_lockouts_after_replay_vote_on_genesis() {
         let mut tower = Tower::new_for_tests(10, 0.9);

core/src/validator.rs

@@ -746,6 +746,20 @@ fn post_process_restored_tower(
                     "And there is an existing vote_account containing actual votes. \
                      Aborting due to possible conflicting duplicate votes",
                 );
+                if let crate::consensus::TowerError::TooOldBlockstore(tower_root, _) = err {
+                    error!(
+                        "This error indicates there had been an unexpeted manipulation to \
+                         the ledger causing severe inconsistency between blockstore and tower.",
+                    );
+                    error!(
+                        "Please restore rocksdb/tower if you modified/copied/moved those files or \
+                         use snapshot (newer than {}) fetched from the cluster by running without \
+                         --no-snapshot-fertch or populate missing data in blockstore by running \
+                         with --no-voting and without --identity and --vote-account until rooting {}.",
+                         tower_root,
+                         tower_root,
+                    );
+                }
                 process::exit(1);
             }
             if err.is_file_missing() && !voting_has_been_active {