Remove unneeded BankWeight fork choice (#13978)

Co-authored-by: Carl Lin <carl@solana.com>

core/src/bank_weight_fork_choice.rs

@@ -1,151 +0,0 @@
-use crate::{
-    consensus::{ComputedBankState, Tower},
-    fork_choice::ForkChoice,
-    progress_map::{ForkStats, ProgressMap},
-};
-use solana_runtime::{bank::Bank, bank_forks::BankForks};
-use solana_sdk::timing;
-use std::time::Instant;
-use std::{
-    collections::{HashMap, HashSet},
-    sync::{Arc, RwLock},
-};
-
-#[derive(Default)]
-pub struct BankWeightForkChoice {}
-
-impl ForkChoice for BankWeightForkChoice {
-    fn compute_bank_stats(
-        &mut self,
-        bank: &Bank,
-        _tower: &Tower,
-        progress: &mut ProgressMap,
-        computed_bank_state: &ComputedBankState,
-    ) {
-        let bank_slot = bank.slot();
-        // Only time progress map should be missing a bank slot
-        // is if this node was the leader for this slot as those banks
-        // are not replayed in replay_active_banks()
-        let parent_weight = bank
-            .parent()
-            .and_then(|b| progress.get(&b.slot()))
-            .map(|x| x.fork_stats.fork_weight)
-            .unwrap_or(0);
-
-        let stats = progress
-            .get_fork_stats_mut(bank_slot)
-            .expect("All frozen banks must exist in the Progress map");
-
-        let ComputedBankState { bank_weight, .. } = computed_bank_state;
-        stats.weight = *bank_weight;
-        stats.fork_weight = stats.weight + parent_weight;
-    }
-
-    // Returns:
-    // 1) The heaviest overall bank
-    // 2) The heaviest bank on the same fork as the last vote (doesn't require a
-    // switching proof to vote for)
-    fn select_forks(
-        &self,
-        frozen_banks: &[Arc<Bank>],
-        tower: &Tower,
-        progress: &ProgressMap,
-        ancestors: &HashMap<u64, HashSet<u64>>,
-        _bank_forks: &RwLock<BankForks>,
-    ) -> (Arc<Bank>, Option<Arc<Bank>>) {
-        let tower_start = Instant::now();
-        assert!(!frozen_banks.is_empty());
-        let num_frozen_banks = frozen_banks.len();
-
-        trace!("frozen_banks {}", frozen_banks.len());
-        let num_old_banks = frozen_banks
-            .iter()
-            .filter(|b| b.slot() < tower.root())
-            .count();
-
-        let last_voted_slot = tower.last_voted_slot();
-        let mut heaviest_bank_on_same_fork = None;
-        let mut heaviest_same_fork_weight = 0;
-        let stats: Vec<&ForkStats> = frozen_banks
-            .iter()
-            .map(|bank| {
-                // Only time progress map should be missing a bank slot
-                // is if this node was the leader for this slot as those banks
-                // are not replayed in replay_active_banks()
-                let stats = progress
-                    .get_fork_stats(bank.slot())
-                    .expect("All frozen banks must exist in the Progress map");
-
-                if let Some(last_voted_slot) = last_voted_slot {
-                    if ancestors
-                        .get(&bank.slot())
-                        .expect("Entry in frozen banks must exist in ancestors")
-                        .contains(&last_voted_slot)
-                    {
-                        // Descendant of last vote cannot be locked out
-                        assert!(!stats.is_locked_out);
-
-                        // ancestors(slot) should not contain the slot itself,
-                        // so we should never get the same bank as the last vote
-                        assert_ne!(bank.slot(), last_voted_slot);
-                        // highest weight, lowest slot first. frozen_banks is sorted
-                        // from least slot to greatest slot, so if two banks have
-                        // the same fork weight, the lower slot will be picked
-                        if stats.fork_weight > heaviest_same_fork_weight {
-                            heaviest_bank_on_same_fork = Some(bank.clone());
-                            heaviest_same_fork_weight = stats.fork_weight;
-                        }
-                    }
-                }
-
-                stats
-            })
-            .collect();
-        let num_not_recent = stats.iter().filter(|s| !s.is_recent).count();
-        let num_has_voted = stats.iter().filter(|s| s.has_voted).count();
-        let num_empty = stats.iter().filter(|s| s.is_empty).count();
-        let num_threshold_failure = stats.iter().filter(|s| !s.vote_threshold).count();
-        let num_votable_threshold_failure = stats
-            .iter()
-            .filter(|s| s.is_recent && !s.has_voted && !s.vote_threshold)
-            .count();
-
-        let mut candidates: Vec<_> = frozen_banks.iter().zip(stats.iter()).collect();
-
-        //highest weight, lowest slot first
-        candidates.sort_by_key(|b| (b.1.fork_weight, 0i64 - b.0.slot() as i64));
-        let rv = candidates
-            .last()
-            .expect("frozen banks was nonempty so candidates must also be nonempty");
-        let ms = timing::duration_as_ms(&tower_start.elapsed());
-        let weights: Vec<(u128, u64, u64)> = candidates
-            .iter()
-            .map(|x| (x.1.weight, x.0.slot(), x.1.block_height))
-            .collect();
-        debug!(
-            "@{:?} tower duration: {:?} len: {}/{} weights: {:?}",
-            timing::timestamp(),
-            ms,
-            candidates.len(),
-            stats.iter().filter(|s| !s.has_voted).count(),
-            weights,
-        );
-        datapoint_debug!(
-            "replay_stage-select_forks",
-            ("frozen_banks", num_frozen_banks as i64, i64),
-            ("not_recent", num_not_recent as i64, i64),
-            ("has_voted", num_has_voted as i64, i64),
-            ("old_banks", num_old_banks as i64, i64),
-            ("empty_banks", num_empty as i64, i64),
-            ("threshold_failure", num_threshold_failure as i64, i64),
-            (
-                "votable_threshold_failure",
-                num_votable_threshold_failure as i64,
-                i64
-            ),
-            ("tower_duration", ms as i64, i64),
-        );
-
-        (rv.0.clone(), heaviest_bank_on_same_fork)
-    }
-}

core/src/consensus.rs

@@ -181,7 +181,7 @@ impl Tower {
         vote_account: &Pubkey,
     ) -> Self {
         let root_bank = bank_forks.root_bank();
-        let (_progress, heaviest_subtree_fork_choice, unlock_heaviest_subtree_fork_choice_slot) =
+        let (_progress, heaviest_subtree_fork_choice) =
             crate::replay_stage::ReplayStage::initialize_progress_and_fork_choice(
                 root_bank,
                 bank_forks.frozen_banks().values().cloned().collect(),
@@ -190,14 +190,12 @@ impl Tower {
             );
         let root = root_bank.slot();
 
-        let heaviest_bank = if root > unlock_heaviest_subtree_fork_choice_slot {
+        let heaviest_bank = bank_forks
-            bank_forks
             .get(heaviest_subtree_fork_choice.best_overall_slot())
-                .expect("The best overall slot must be one of `frozen_banks` which all exist in bank_forks")
+            .expect(
-                .clone()
+                "The best overall slot must be one of `frozen_banks` which all exist in bank_forks",
-        } else {
+            )
-            Tower::find_heaviest_bank(&bank_forks, &my_pubkey).unwrap_or_else(|| root_bank.clone())
+            .clone();
-        };
 
         Self::new(
             &my_pubkey,
@@ -785,26 +783,6 @@ impl Tower {
         }
     }
 
-    pub(crate) fn find_heaviest_bank(
-        bank_forks: &BankForks,
-        node_pubkey: &Pubkey,
-    ) -> Option<Arc<Bank>> {
-        let ancestors = bank_forks.ancestors();
-        let mut bank_weights: Vec<_> = bank_forks
-            .frozen_banks()
-            .values()
-            .map(|b| {
-                (
-                    Self::bank_weight(node_pubkey, b, &ancestors),
-                    b.parents().len(),
-                    b.clone(),
-                )
-            })
-            .collect();
-        bank_weights.sort_by_key(|b| (b.0, b.1));
-        bank_weights.pop().map(|b| b.2)
-    }
-
     /// Update stake for all the ancestors.
     /// Note, stake is the same for all the ancestor.
     fn update_ancestor_voted_stakes(
@@ -827,21 +805,6 @@ impl Tower {
         }
     }
 
-    fn bank_weight(
-        node_pubkey: &Pubkey,
-        bank: &Bank,
-        ancestors: &HashMap<Slot, HashSet<Slot>>,
-    ) -> u128 {
-        let ComputedBankState { bank_weight, .. } = Self::collect_vote_lockouts(
-            node_pubkey,
-            bank.slot(),
-            bank.vote_accounts().into_iter(),
-            ancestors,
-            &mut PubkeyReferences::default(),
-        );
-        bank_weight
-    }
-
     fn voted_slots(&self) -> Vec<Slot> {
         self.lockouts
             .votes
@@ -1273,7 +1236,6 @@ pub fn reconcile_blockstore_roots_with_tower(
 pub mod test {
     use super::*;
     use crate::{
-        bank_weight_fork_choice::BankWeightForkChoice,
         cluster_info_vote_listener::VoteTracker,
         cluster_slots::ClusterSlots,
         fork_choice::SelectVoteAndResetForkResult,
@@ -1413,7 +1375,6 @@ pub mod test {
                 &self.bank_forks,
                 &mut PubkeyReferences::default(),
                 &mut self.heaviest_subtree_fork_choice,
-                &mut BankWeightForkChoice::default(),
             );
 
             let vote_bank = self

core/src/lib.rs

@@ -19,7 +19,6 @@ pub mod sample_performance_service;
 pub mod shred_fetch_stage;
 #[macro_use]
 pub mod contact_info;
-pub mod bank_weight_fork_choice;
 pub mod cluster_info;
 pub mod cluster_slots;
 pub mod cluster_slots_service;

core/src/replay_stage.rs

@@ -1,7 +1,6 @@
 //! The `replay_stage` replays transactions broadcast by the leader.
 
 use crate::{
-    bank_weight_fork_choice::BankWeightForkChoice,
     broadcast_stage::RetransmitSlotsSender,
     cache_block_time_service::CacheBlockTimeSender,
     cluster_info::ClusterInfo,
@@ -259,13 +258,11 @@ impl ReplayStage {
                 let (
                     mut progress,
                     mut heaviest_subtree_fork_choice,
-                    unlock_heaviest_subtree_fork_choice_slot,
                 ) = Self::initialize_progress_and_fork_choice_with_locked_bank_forks(
                     &bank_forks,
                     &my_pubkey,
                     &vote_account,
                 );
-                let mut bank_weight_fork_choice = BankWeightForkChoice::default();
                 let mut current_leader = None;
                 let mut last_reset = Hash::default();
                 let mut partition_exists = false;
@@ -355,7 +352,6 @@ impl ReplayStage {
                         &bank_forks,
                         &mut all_pubkeys,
                         &mut heaviest_subtree_fork_choice,
-                        &mut bank_weight_fork_choice,
                     );
                     compute_bank_stats_time.stop();
 
@@ -382,11 +378,7 @@ impl ReplayStage {
 
                     let mut select_forks_time = Measure::start("select_forks_time");
                     let fork_choice: &mut dyn ForkChoice =
-                        if forks_root > unlock_heaviest_subtree_fork_choice_slot {
+                            &mut heaviest_subtree_fork_choice;
-                            &mut heaviest_subtree_fork_choice
-                        } else {
-                            &mut bank_weight_fork_choice
-                        };
                     let (heaviest_bank, heaviest_bank_on_same_voted_fork) = fork_choice
                         .select_forks(&frozen_banks, &tower, &progress, &ancestors, &bank_forks);
                     select_forks_time.stop();
@@ -620,7 +612,7 @@ impl ReplayStage {
         bank_forks: &RwLock<BankForks>,
         my_pubkey: &Pubkey,
         vote_account: &Pubkey,
-    ) -> (ProgressMap, HeaviestSubtreeForkChoice, Slot) {
+    ) -> (ProgressMap, HeaviestSubtreeForkChoice) {
         let (root_bank, frozen_banks) = {
             let bank_forks = bank_forks.read().unwrap();
             (
@@ -642,7 +634,7 @@ impl ReplayStage {
         mut frozen_banks: Vec<Arc<Bank>>,
         my_pubkey: &Pubkey,
         vote_account: &Pubkey,
-    ) -> (ProgressMap, HeaviestSubtreeForkChoice, Slot) {
+    ) -> (ProgressMap, HeaviestSubtreeForkChoice) {
         let mut progress = ProgressMap::default();
 
         frozen_banks.sort_by_key(|bank| bank.slot());
@@ -663,16 +655,10 @@ impl ReplayStage {
             );
         }
         let root = root_bank.slot();
-        let unlock_heaviest_subtree_fork_choice_slot =
-            Self::get_unlock_heaviest_subtree_fork_choice(root_bank.cluster_type());
         let heaviest_subtree_fork_choice =
             HeaviestSubtreeForkChoice::new_from_frozen_banks(root, &frozen_banks);
 
-        (
+        (progress, heaviest_subtree_fork_choice)
-            progress,
-            heaviest_subtree_fork_choice,
-            unlock_heaviest_subtree_fork_choice_slot,
-        )
     }
 
     fn report_memory(
@@ -1379,7 +1365,6 @@ impl ReplayStage {
         bank_forks: &RwLock<BankForks>,
         all_pubkeys: &mut PubkeyReferences,
         heaviest_subtree_fork_choice: &mut dyn ForkChoice,
-        bank_weight_fork_choice: &mut dyn ForkChoice,
     ) -> Vec<Slot> {
         frozen_banks.sort_by_key(|bank| bank.slot());
         let mut new_stats = vec![];
@@ -1409,12 +1394,6 @@ impl ReplayStage {
                         progress,
                         &computed_bank_state,
                     );
-                    bank_weight_fork_choice.compute_bank_stats(
-                        &bank,
-                        tower,
-                        progress,
-                        &computed_bank_state,
-                    );
                     let ComputedBankState {
                         voted_stakes,
                         total_stake,
@@ -1961,17 +1940,6 @@ impl ReplayStage {
         }
     }
 
-    pub fn get_unlock_heaviest_subtree_fork_choice(cluster_type: ClusterType) -> Slot {
-        match cluster_type {
-            ClusterType::Development => 0,
-            ClusterType::Devnet => 0,
-            // Epoch 63
-            ClusterType::Testnet => 21_692_256,
-            // 400_000 slots into epoch 61
-            ClusterType::MainnetBeta => 26_752_000,
-        }
-    }
-
     pub fn join(self) -> thread::Result<()> {
         self.commitment_service.join()?;
         self.t_replay.join().map(|_| ())
@@ -2838,7 +2806,6 @@ pub(crate) mod tests {
             &bank_forks,
             &mut PubkeyReferences::default(),
             &mut heaviest_subtree_fork_choice,
-            &mut BankWeightForkChoice::default(),
         );
 
         // bank 0 has no votes, should not send any votes on the channel
@@ -2884,7 +2851,6 @@ pub(crate) mod tests {
             &bank_forks,
             &mut PubkeyReferences::default(),
             &mut heaviest_subtree_fork_choice,
-            &mut BankWeightForkChoice::default(),
         );
 
         // Bank 1 had one vote
@@ -2920,7 +2886,6 @@ pub(crate) mod tests {
             &bank_forks,
             &mut PubkeyReferences::default(),
             &mut heaviest_subtree_fork_choice,
-            &mut BankWeightForkChoice::default(),
         );
         // No new stats should have been computed
         assert!(newly_computed.is_empty());
@@ -2958,7 +2923,6 @@ pub(crate) mod tests {
             &vote_simulator.bank_forks,
             &mut PubkeyReferences::default(),
             &mut heaviest_subtree_fork_choice,
-            &mut BankWeightForkChoice::default(),
         );
 
         assert_eq!(
@@ -2976,17 +2940,6 @@ pub(crate) mod tests {
 
         // Should pick the lower of the two equally weighted banks
         assert_eq!(heaviest_bank.slot(), 1);
-
-        let (heaviest_bank, _) = BankWeightForkChoice::default().select_forks(
-            &frozen_banks,
-            &tower,
-            &vote_simulator.progress,
-            &ancestors,
-            &vote_simulator.bank_forks,
-        );
-
-        // Should pick the lower of the two equally weighted banks
-        assert_eq!(heaviest_bank.slot(), 1);
     }
 
     #[test]
@@ -3032,7 +2985,6 @@ pub(crate) mod tests {
             &vote_simulator.bank_forks,
             &mut PubkeyReferences::default(),
             &mut vote_simulator.heaviest_subtree_fork_choice,
-            &mut BankWeightForkChoice::default(),
         );
 
         frozen_banks.sort_by_key(|bank| bank.slot());
@@ -3880,7 +3832,6 @@ pub(crate) mod tests {
             &bank_forks,
             &mut PubkeyReferences::default(),
             &mut HeaviestSubtreeForkChoice::new_from_bank_forks(&bank_forks.read().unwrap()),
-            &mut BankWeightForkChoice::default(),
         );
 
         // Check status is true