Add a test for opt. conf violation without tower

ryoqun · ryoqun · commit 3f5ecc971f88 · 2020-10-09T16:51:36.000+09:00
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/core/src/consensus.rs b/core/src/consensus.rs
@@ -30,11 +30,11 @@ use std::{
 };
 use thiserror::Error;
 
-#[derive(PartialEq, Clone, Debug)]
+#[derive(PartialEq, Clone, Debug, AbiExample)]
 pub enum SwitchForkDecision {
     SwitchProof(Hash),
-    NoSwitch,
-    FailedSwitchThreshold,
+    SameFork,
+    FailedSwitchThreshold(u64, u64),
 }
 
 impl SwitchForkDecision {
@@ -45,8 +45,8 @@ impl SwitchForkDecision {
         authorized_voter_pubkey: &Pubkey,
     ) -> Option<Instruction> {
         match self {
-            SwitchForkDecision::FailedSwitchThreshold => None,
-            SwitchForkDecision::NoSwitch => Some(vote_instruction::vote(
+            SwitchForkDecision::FailedSwitchThreshold(_, _) => None,
+            SwitchForkDecision::SameFork => Some(vote_instruction::vote(
                 vote_account_pubkey,
                 authorized_voter_pubkey,
                 vote,
@@ -101,6 +101,8 @@ pub struct Tower {
     // This could be emptied after some time; but left intact indefinitely for easier
     // implementation
     stray_restored_slot: Option<Slot>,
+    #[serde(skip)]
+    pub last_switch_threshold_check: Option<(Slot, SwitchForkDecision)>,
 }
 
 impl Default for Tower {
@@ -115,6 +117,7 @@ impl Default for Tower {
             path: PathBuf::default(),
             tmp_path: PathBuf::default(),
             stray_restored_slot: Option::default(),
+            last_switch_threshold_check: Option::default(),
         };
         // VoteState::root_slot is ensured to be Some in Tower
         tower.lockouts.root_slot = Some(Slot::default());
@@ -440,6 +443,10 @@ impl Tower {
         self.lockouts.root_slot.unwrap()
     }
 
+    pub fn node_pubkey(&self) -> Pubkey {
+        self.node_pubkey
+    }
+
     // a slot is recent if it's newer than the last vote we have
     pub fn is_recent(&self, slot: Slot) -> bool {
         if let Some(last_voted_slot) = self.lockouts.last_voted_slot() {
@@ -520,9 +527,16 @@ impl Tower {
                             // all of them.
                             panic!("no ancestors found with slot: {}", last_voted_slot);
                         } else {
-                            // bank_forks doesn't have corresponding data for the stray restored last vote,
-                            // meaning some inconsistency between saved tower and ledger.
-                            // (newer snapshot, or only a saved tower is moved over to new setup?)
+                            // compare slots not to error! just because of newer snapshots
+                            if self.last_switch_threshold_check.is_none() && switch_slot < last_voted_slot {
+                                error!(
+                                  "{} bank_forks doesn't have corresponding data for the stray restored \
+                                   last vote({}), meaning some inconsistency between saved tower and ledger.",
+                                  self.node_pubkey,
+                                  last_voted_slot
+                                );
+                            }
+                            // (system crash? process crash? newer snapshot, or only a saved tower is moved over to new setup?)
                             &empty_ancestors
                         }
                     });
@@ -532,12 +546,22 @@ impl Tower {
                 if switch_slot == last_voted_slot || switch_slot_ancestors.contains(&last_voted_slot) {
                     // If the `switch_slot is a descendant of the last vote,
                     // no switching proof is necessary
-                    return SwitchForkDecision::NoSwitch;
+                    return SwitchForkDecision::SameFork;
                 }
 
-                // Should never consider switching to an ancestor
-                // of your last vote
-                assert!(!last_vote_ancestors.contains(&switch_slot));
+                // 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 {
+                        panic!("Don't switch 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,0);
+                    }
+                }
 
                 // By this point, we know the `switch_slot` is on a different fork
                 // (is neither an ancestor nor descendant of `last_vote`), so a
@@ -622,10 +646,10 @@ impl Tower {
                 if (locked_out_stake as f64 / total_stake as f64) > SWITCH_FORK_THRESHOLD {
                     SwitchForkDecision::SwitchProof(switch_proof)
                 } else {
-                    SwitchForkDecision::FailedSwitchThreshold
+                    SwitchForkDecision::FailedSwitchThreshold(locked_out_stake, total_stake)
                 }
             })
-            .unwrap_or(SwitchForkDecision::NoSwitch)
+            .unwrap_or(SwitchForkDecision::SameFork)
     }
 
     pub fn check_vote_stake_threshold(
@@ -931,9 +955,9 @@ impl Tower {
             self.lockouts = vote_state;
             self.do_initialize_lockouts(root, |v| v.slot > root);
             trace!(
-                "{} lockouts initialized to {:?}",
+                "Lockouts in tower for {} is initialized using bank {}",
                 self.node_pubkey,
-                self.lockouts
+                bank.slot(),
             );
             assert_eq!(
                 self.lockouts.node_pubkey, self.node_pubkey,
@@ -985,6 +1009,11 @@ impl Tower {
             bincode::serialize_into(&mut file, &saved_tower)?;
             // file.sync_all() hurts performance; pipeline sync-ing and submitting votes to the cluster!
         }
+        trace!(
+            "{} persisted votes: {:?}",
+            node_keypair.pubkey(),
+            self.voted_slots()
+        );
         fs::rename(&new_filename, &filename)?;
         // self.path.parent().sync_all() hurts performance same as the above sync
 
@@ -1046,6 +1075,16 @@ pub enum TowerError {
     FatallyInconsistent(&'static str),
 }
 
+impl TowerError {
+    pub fn is_file_missing(&self) -> bool {
+        if let TowerError::IOError(io_err) = &self {
+            io_err.kind() == std::io::ErrorKind::NotFound
+        } else {
+            false
+        }
+    }
+}
+
 #[frozen_abi(digest = "Gaxfwvx5MArn52mKZQgzHmDCyn5YfCuTHvp5Et3rFfpp")]
 #[derive(Default, Clone, Serialize, Deserialize, Debug, PartialEq, AbiExample)]
 pub struct SavedTower {
@@ -1268,7 +1307,7 @@ pub mod test {
                 &ancestors,
                 &descendants,
                 &self.progress,
-                &tower,
+                tower,
             );
 
             // Make sure this slot isn't locked out or failing threshold
@@ -1465,11 +1504,11 @@ pub mod test {
     #[test]
     fn test_to_vote_instruction() {
         let vote = Vote::default();
-        let mut decision = SwitchForkDecision::FailedSwitchThreshold;
+        let mut decision = SwitchForkDecision::FailedSwitchThreshold(0, 0);
         assert!(decision
             .to_vote_instruction(vote.clone(), &Pubkey::default(), &Pubkey::default())
             .is_none());
-        decision = SwitchForkDecision::NoSwitch;
+        decision = SwitchForkDecision::SameFork;
         assert_eq!(
             decision.to_vote_instruction(vote.clone(), &Pubkey::default(), &Pubkey::default()),
             Some(vote_instruction::vote(
@@ -1572,7 +1611,7 @@ pub mod test {
                 total_stake,
                 bank0.epoch_vote_accounts(0).unwrap(),
             ),
-            SwitchForkDecision::NoSwitch
+            SwitchForkDecision::SameFork
         );
 
         // Trying to switch to another fork at 110 should fail
@@ -1585,7 +1624,7 @@ pub mod test {
                 total_stake,
                 bank0.epoch_vote_accounts(0).unwrap(),
             ),
-            SwitchForkDecision::FailedSwitchThreshold
+            SwitchForkDecision::FailedSwitchThreshold(0, 20000)
         );
 
         // Adding another validator lockout on a descendant of last vote should
@@ -1600,7 +1639,7 @@ pub mod test {
                 total_stake,
                 bank0.epoch_vote_accounts(0).unwrap(),
             ),
-            SwitchForkDecision::FailedSwitchThreshold
+            SwitchForkDecision::FailedSwitchThreshold(0, 20000)
         );
 
         // Adding another validator lockout on an ancestor of last vote should
@@ -1615,7 +1654,7 @@ pub mod test {
                 total_stake,
                 bank0.epoch_vote_accounts(0).unwrap(),
             ),
-            SwitchForkDecision::FailedSwitchThreshold
+            SwitchForkDecision::FailedSwitchThreshold(0, 20000)
         );
 
         // Adding another validator lockout on a different fork, but the lockout
@@ -1630,7 +1669,7 @@ pub mod test {
                 total_stake,
                 bank0.epoch_vote_accounts(0).unwrap(),
             ),
-            SwitchForkDecision::FailedSwitchThreshold
+            SwitchForkDecision::FailedSwitchThreshold(0, 20000)
         );
 
         // Adding another validator lockout on a different fork, and the lockout
@@ -1647,7 +1686,7 @@ pub mod test {
                 total_stake,
                 bank0.epoch_vote_accounts(0).unwrap(),
             ),
-            SwitchForkDecision::FailedSwitchThreshold
+            SwitchForkDecision::FailedSwitchThreshold(0, 20000)
         );
 
         // Adding another validator lockout on a different fork, and the lockout
@@ -1698,7 +1737,7 @@ pub mod test {
                 total_stake,
                 bank0.epoch_vote_accounts(0).unwrap(),
             ),
-            SwitchForkDecision::FailedSwitchThreshold
+            SwitchForkDecision::FailedSwitchThreshold(0, 20000)
         );
     }
 
@@ -2366,7 +2405,7 @@ pub mod test {
                 total_stake,
                 bank0.epoch_vote_accounts(0).unwrap(),
             ),
-            SwitchForkDecision::NoSwitch
+            SwitchForkDecision::SameFork
         );
 
         // Trying to switch to another fork at 110 should fail
@@ -2379,7 +2418,7 @@ pub mod test {
                 total_stake,
                 bank0.epoch_vote_accounts(0).unwrap(),
             ),
-            SwitchForkDecision::FailedSwitchThreshold
+            SwitchForkDecision::FailedSwitchThreshold(0, 20000)
         );
 
         vote_simulator.simulate_lockout_interval(111, (10, 49), &other_vote_account);
@@ -2457,7 +2496,7 @@ pub mod test {
                 total_stake,
                 bank0.epoch_vote_accounts(0).unwrap(),
             ),
-            SwitchForkDecision::FailedSwitchThreshold
+            SwitchForkDecision::FailedSwitchThreshold(0, 20000)
         );
 
         // Add lockout_interval which should be excluded
@@ -2471,7 +2510,7 @@ pub mod test {
                 total_stake,
                 bank0.epoch_vote_accounts(0).unwrap(),
             ),
-            SwitchForkDecision::FailedSwitchThreshold
+            SwitchForkDecision::FailedSwitchThreshold(0, 20000)
         );
 
         // Add lockout_interval which should not be excluded
diff --git a/core/src/replay_stage.rs b/core/src/replay_stage.rs
@@ -405,7 +405,7 @@ impl ReplayStage {
                         &ancestors,
                         &descendants,
                         &progress,
-                        &tower,
+                        &mut tower,
                     );
                     select_vote_and_reset_forks_time.stop();
 
@@ -1511,7 +1511,7 @@ impl ReplayStage {
         ancestors: &HashMap<u64, HashSet<u64>>,
         descendants: &HashMap<u64, HashSet<u64>>,
         progress: &ProgressMap,
-        tower: &Tower,
+        tower: &mut Tower,
     ) -> SelectVoteAndResetForkResult {
         // Try to vote on the actual heaviest fork. If the heaviest bank is
         // locked out or fails the threshold check, the validator will:
@@ -1538,7 +1538,19 @@ impl ReplayStage {
                     .epoch_vote_accounts(heaviest_bank.epoch())
                     .expect("Bank epoch vote accounts must contain entry for the bank's own epoch"),
             );
-            if switch_fork_decision == SwitchForkDecision::FailedSwitchThreshold {
+            let a = Some((heaviest_bank.slot(), switch_fork_decision.clone()));
+            if tower.last_switch_threshold_check != a {
+                let (slot, choice) = a.clone().unwrap();
+                trace!(
+                    "{}: switch threshold is checked: slot {} (parent: {}): {:?}",
+                    tower.node_pubkey(),
+                    slot,
+                    heaviest_bank.parent_slot(),
+                    choice
+                );
+                tower.last_switch_threshold_check = a;
+            }
+            if let SwitchForkDecision::FailedSwitchThreshold(_, _) = switch_fork_decision {
                 // If we can't switch, then reset to the the next votable
                 // bank on the same fork as our last vote, but don't vote
                 info!(
@@ -1584,11 +1596,13 @@ impl ReplayStage {
                 failure_reasons.push(HeaviestForkFailures::NoPropagatedConfirmation(bank.slot()));
             }
 
-            if !is_locked_out
-                && vote_threshold
-                && propagation_confirmed
-                && switch_fork_decision != SwitchForkDecision::FailedSwitchThreshold
-            {
+            if let SwitchForkDecision::FailedSwitchThreshold(_, _) = switch_fork_decision {
+                SelectVoteAndResetForkResult {
+                    vote_bank: None,
+                    reset_bank: Some(bank.clone()),
+                    heaviest_fork_failures: failure_reasons,
+                }
+            } else if !is_locked_out && vote_threshold && propagation_confirmed {
                 info!("voting: {} {}", bank.slot(), fork_weight);
                 SelectVoteAndResetForkResult {
                     vote_bank: Some((bank.clone(), switch_fork_decision)),
diff --git a/core/src/validator.rs b/core/src/validator.rs
@@ -6,7 +6,7 @@ use crate::{
     cluster_info::{ClusterInfo, Node},
     cluster_info_vote_listener::VoteTracker,
     completed_data_sets_service::CompletedDataSetsService,
-    consensus::{reconcile_blockstore_roots_with_tower, Tower, TowerError},
+    consensus::{reconcile_blockstore_roots_with_tower, Tower},
     contact_info::ContactInfo,
     gossip_service::{discover_cluster, GossipService},
     poh_recorder::{PohRecorder, GRACE_TICKS_FACTOR, MAX_GRACE_SLOTS},
@@ -644,12 +644,7 @@ fn post_process_restored_tower(
         .unwrap_or_else(|err| {
             let voting_has_been_active =
                 active_vote_account_exists_in_bank(&bank_forks.working_bank(), &vote_account);
-            let saved_tower_is_missing = if let TowerError::IOError(io_err) = &err {
-                io_err.kind() == std::io::ErrorKind::NotFound
-            } else {
-                false
-            };
-            if !saved_tower_is_missing {
+            if !err.is_file_missing() {
                 datapoint_error!(
                     "tower_error",
                     (
@@ -667,7 +662,7 @@ fn post_process_restored_tower(
                 );
                 process::exit(1);
             }
-            if saved_tower_is_missing && !voting_has_been_active {
+            if err.is_file_missing() && !voting_has_been_active {
                 // Currently, don't protect against spoofed snapshots with no tower at all
                 info!(
                     "Ignoring expected failed tower restore because this is the initial \
diff --git a/local-cluster/Cargo.toml b/local-cluster/Cargo.toml
@@ -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.0" }
diff --git a/local-cluster/tests/local_cluster.rs b/local-cluster/tests/local_cluster.rs
