From 3687dccda69e9534caa9322c41c87c543e0fd158 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Wed, 9 Dec 2020 03:53:21 +0000 Subject: [PATCH] Remove unneeded BankWeight fork choice (#13978) (#14003) Co-authored-by: Carl Lin (cherry picked from commit 239a191612061eb4b49966c7f316f8c1a052505e) Co-authored-by: carllin --- core/src/bank_weight_fork_choice.rs | 151 ---------------------------- core/src/consensus.rs | 53 ++-------- core/src/lib.rs | 1 - core/src/replay_stage.rs | 57 +---------- 4 files changed, 11 insertions(+), 251 deletions(-) delete mode 100644 core/src/bank_weight_fork_choice.rs diff --git a/core/src/bank_weight_fork_choice.rs b/core/src/bank_weight_fork_choice.rs deleted file mode 100644 index a8efbcb69c..0000000000 --- a/core/src/bank_weight_fork_choice.rs +++ /dev/null @@ -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], - tower: &Tower, - progress: &ProgressMap, - ancestors: &HashMap>, - _bank_forks: &RwLock, - ) -> (Arc, Option>) { - 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) - } -} diff --git a/core/src/consensus.rs b/core/src/consensus.rs index 121ad4e57f..7b12a45ada 100644 --- a/core/src/consensus.rs +++ b/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 { - 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") - .clone() - } else { - Tower::find_heaviest_bank(&bank_forks, &my_pubkey).unwrap_or_else(|| root_bank.clone()) - }; + let heaviest_bank = 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", + ) + .clone(); Self::new( &my_pubkey, @@ -785,26 +783,6 @@ impl Tower { } } - pub(crate) fn find_heaviest_bank( - bank_forks: &BankForks, - node_pubkey: &Pubkey, - ) -> Option> { - 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>, - ) -> 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 { 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 diff --git a/core/src/lib.rs b/core/src/lib.rs index 736aa6a966..b485d31fc5 100644 --- a/core/src/lib.rs +++ b/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; diff --git a/core/src/replay_stage.rs b/core/src/replay_stage.rs index 0a4bc9bc06..b80356d777 100644 --- a/core/src/replay_stage.rs +++ b/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 - } else { - &mut bank_weight_fork_choice - }; + &mut heaviest_subtree_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, 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>, 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, - unlock_heaviest_subtree_fork_choice_slot, - ) + (progress, heaviest_subtree_fork_choice) } fn report_memory( @@ -1363,7 +1349,6 @@ impl ReplayStage { bank_forks: &RwLock, all_pubkeys: &mut PubkeyReferences, heaviest_subtree_fork_choice: &mut dyn ForkChoice, - bank_weight_fork_choice: &mut dyn ForkChoice, ) -> Vec { frozen_banks.sort_by_key(|bank| bank.slot()); let mut new_stats = vec![]; @@ -1393,12 +1378,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, @@ -1945,17 +1924,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(|_| ()) @@ -2822,7 +2790,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 @@ -2868,7 +2835,6 @@ pub(crate) mod tests { &bank_forks, &mut PubkeyReferences::default(), &mut heaviest_subtree_fork_choice, - &mut BankWeightForkChoice::default(), ); // Bank 1 had one vote @@ -2904,7 +2870,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()); @@ -2942,7 +2907,6 @@ pub(crate) mod tests { &vote_simulator.bank_forks, &mut PubkeyReferences::default(), &mut heaviest_subtree_fork_choice, - &mut BankWeightForkChoice::default(), ); assert_eq!( @@ -2960,17 +2924,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] @@ -3016,7 +2969,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()); @@ -3832,7 +3784,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