Follow up to persistent tower with tests and API cleaning (#12350) (#12972)

* Follow up to persistent tower

* Ignore for now...

* Hard-code validator identities for easy reasoning

* Add a test for opt. conf violation without tower

* Fix compile with rust < 1.47

* Remove unused method

* More move of assert tweak to the asser pr

* Add comments

* Clean up

* Clean the test addressing various review comments

* Clean up a bit

(cherry picked from commit 54517ea)

Co-authored-by: Ryo Onodera <ryoqun@gmail.com>

Author: mergify[bot]

Cargo.lock

@@ -60,7 +60,7 @@ impl ForkChoice for BankWeightForkChoice {
         trace!("frozen_banks {}", frozen_banks.len());
         let num_old_banks = frozen_banks
             .iter()
-            .filter(|b| b.slot() < tower.root().unwrap_or(0))
+            .filter(|b| b.slot() < tower.root())
             .count();
 
         let last_voted_slot = tower.last_voted_slot();

core/src/bank_weight_fork_choice.rs

@@ -387,17 +387,14 @@ impl Tower {
     pub fn record_bank_vote(&mut self, vote: Vote) -> Option<Slot> {
         let slot = vote.last_voted_slot().unwrap_or(0);
         trace!("{} record_vote for {}", self.node_pubkey, slot);
-        let root_slot = self.lockouts.root_slot;
+        let old_root = self.root();
         self.lockouts.process_vote_unchecked(&vote);
         self.last_vote = vote;
+        let new_root = self.root();
 
-        datapoint_info!(
-            "tower-vote",
-            ("latest", slot, i64),
-            ("root", self.lockouts.root_slot.unwrap_or(0), i64)
-        );
-        if root_slot != self.lockouts.root_slot {
-            Some(self.lockouts.root_slot.unwrap())
+        datapoint_info!("tower-vote", ("latest", slot, i64), ("root", new_root, i64));
+        if old_root != new_root {
+            Some(new_root)
         } else {
             None
         }
@@ -446,8 +443,8 @@ impl Tower {
     // which establishes the origin of trust (i.e. root) whether booting from genesis (slot 0) or
     // snapshot (slot N). In other words, there should be no possibility a Tower doesn't have
     // root, unlike young vote accounts.
-    pub fn root(&self) -> Option<Slot> {
-        self.lockouts.root_slot
+    pub fn root(&self) -> Slot {
+        self.lockouts.root_slot.unwrap()
     }
 
     // a slot is recent if it's newer than the last vote we have
@@ -514,7 +511,7 @@ impl Tower {
     ) -> SwitchForkDecision {
         self.last_voted_slot()
             .map(|last_voted_slot| {
-                let root = self.lockouts.root_slot.unwrap_or(0);
+                let root = self.root();
                 let empty_ancestors = HashSet::default();
 
                 let last_vote_ancestors =
@@ -837,8 +834,7 @@ impl Tower {
         slot_history: &SlotHistory,
     ) -> Result<Self> {
         // sanity assertions for roots
-        assert!(self.root().is_some());
-        let tower_root = self.root().unwrap();
+        let tower_root = self.root();
         info!(
             "adjusting lockouts (after replay up to {}): {:?} tower root: {}",
             replayed_root,
@@ -1160,28 +1156,27 @@ pub fn reconcile_blockstore_roots_with_tower(
     tower: &Tower,
     blockstore: &Blockstore,
 ) -> blockstore_db::Result<()> {
-    if let Some(tower_root) = tower.root() {
-        let last_blockstore_root = blockstore.last_root();
-        if last_blockstore_root < tower_root {
-            // Ensure tower_root itself to exist and be marked as rooted in the blockstore
-            // in addition to its ancestors.
-            let new_roots: Vec<_> = AncestorIterator::new_inclusive(tower_root, &blockstore)
-                .take_while(|current| match current.cmp(&last_blockstore_root) {
-                    Ordering::Greater => true,
-                    Ordering::Equal => false,
-                    Ordering::Less => panic!(
-                        "couldn't find a last_blockstore_root upwards from: {}!?",
-                        tower_root
-                    ),
-                })
-                .collect();
-            assert!(
-                !new_roots.is_empty(),
-                "at least 1 parent slot must be found"
-            );
+    let tower_root = tower.root();
+    let last_blockstore_root = blockstore.last_root();
+    if last_blockstore_root < tower_root {
+        // Ensure tower_root itself to exist and be marked as rooted in the blockstore
+        // in addition to its ancestors.
+        let new_roots: Vec<_> = AncestorIterator::new_inclusive(tower_root, &blockstore)
+            .take_while(|current| match current.cmp(&last_blockstore_root) {
+                Ordering::Greater => true,
+                Ordering::Equal => false,
+                Ordering::Less => panic!(
+                    "couldn't find a last_blockstore_root upwards from: {}!?",
+                    tower_root
+                ),
+            })
+            .collect();
+        assert!(
+            !new_roots.is_empty(),
+            "at least 1 parent slot must be found"
+        );
 
-            blockstore.set_roots(&new_roots)?
-        }
+        blockstore.set_roots(&new_roots)?
     }
     Ok(())
 }
@@ -2744,13 +2739,13 @@ pub mod test {
             .unwrap();
 
         assert_eq!(tower.voted_slots(), vec![2, 3]);
-        assert_eq!(tower.root(), Some(replayed_root_slot));
+        assert_eq!(tower.root(), replayed_root_slot);
 
         tower = tower
             .adjust_lockouts_after_replay(replayed_root_slot, &slot_history)
             .unwrap();
         assert_eq!(tower.voted_slots(), vec![2, 3]);
-        assert_eq!(tower.root(), Some(replayed_root_slot));
+        assert_eq!(tower.root(), replayed_root_slot);
     }
 
     #[test]
@@ -2772,7 +2767,7 @@ pub mod test {
             .unwrap();
 
         assert_eq!(tower.voted_slots(), vec![2, 3]);
-        assert_eq!(tower.root(), Some(replayed_root_slot));
+        assert_eq!(tower.root(), replayed_root_slot);
     }
 
     #[test]
@@ -2796,7 +2791,7 @@ pub mod test {
             .unwrap();
 
         assert_eq!(tower.voted_slots(), vec![] as Vec<Slot>);
-        assert_eq!(tower.root(), Some(replayed_root_slot));
+        assert_eq!(tower.root(), replayed_root_slot);
         assert_eq!(tower.stray_restored_slot, None);
     }
 
@@ -2819,7 +2814,7 @@ pub mod test {
             .adjust_lockouts_after_replay(MAX_ENTRIES, &slot_history)
             .unwrap();
         assert_eq!(tower.voted_slots(), vec![] as Vec<Slot>);
-        assert_eq!(tower.root(), Some(MAX_ENTRIES));
+        assert_eq!(tower.root(), MAX_ENTRIES);
     }
 
     #[test]
@@ -2842,7 +2837,7 @@ pub mod test {
             .unwrap();
 
         assert_eq!(tower.voted_slots(), vec![3, 4]);
-        assert_eq!(tower.root(), Some(replayed_root_slot));
+        assert_eq!(tower.root(), replayed_root_slot);
     }
 
     #[test]
@@ -2863,7 +2858,7 @@ pub mod test {
             .unwrap();
 
         assert_eq!(tower.voted_slots(), vec![5, 6]);
-        assert_eq!(tower.root(), Some(replayed_root_slot));
+        assert_eq!(tower.root(), replayed_root_slot);
     }
 
     #[test]
@@ -2907,7 +2902,7 @@ pub mod test {
             .unwrap();
 
         assert_eq!(tower.voted_slots(), vec![3, 4, 5]);
-        assert_eq!(tower.root(), Some(replayed_root_slot));
+        assert_eq!(tower.root(), replayed_root_slot);
     }
 
     #[test]
@@ -2923,7 +2918,7 @@ pub mod test {
             .unwrap();
 
         assert_eq!(tower.voted_slots(), vec![] as Vec<Slot>);
-        assert_eq!(tower.root(), Some(replayed_root_slot));
+        assert_eq!(tower.root(), replayed_root_slot);
     }
 
     #[test]

core/src/consensus.rs

@@ -11,6 +11,7 @@ homepage = "https://solana.com/"
 [dependencies]
 itertools = "0.9.0"
 gag = "0.1.10"
+fs_extra = "1.1.0"
 log = "0.4.8"
 rand = "0.7.0"
 solana-config-program = { path = "../programs/config", version = "1.4.2" }

local-cluster/Cargo.toml

@@ -370,6 +370,15 @@ impl LocalCluster {
         validator_pubkey
     }
 
+    pub fn ledger_path(&self, validator_pubkey: &Pubkey) -> std::path::PathBuf {
+        self.validators
+            .get(validator_pubkey)
+            .unwrap()
+            .info
+            .ledger_path
+            .clone()
+    }
+
     fn close(&mut self) {
         self.close_preserve_ledgers();
     }