From d32a0721900bc45e0b2048350c08b6504a541fde Mon Sep 17 00:00:00 2001 From: Pankaj Garg Date: Tue, 19 Nov 2019 17:55:42 -0800 Subject: [PATCH] Use ticks_per_slot to calculate maximum grace ticks (#7024) * Use ticks_per_slot to calculate maximum grace ticks * fix test * fix votable candidate ordering * fixes to pick_best_fork() and a unit test * fixes --- core/src/poh_recorder.rs | 8 +-- core/src/replay_stage.rs | 145 +++++++++++++++++++++++++++++++++++++-- 2 files changed, 143 insertions(+), 10 deletions(-) diff --git a/core/src/poh_recorder.rs b/core/src/poh_recorder.rs index 31c2ec24b2..423383a17e 100644 --- a/core/src/poh_recorder.rs +++ b/core/src/poh_recorder.rs @@ -29,7 +29,7 @@ use std::sync::{Arc, Mutex}; use std::time::Instant; const GRACE_TICKS_FACTOR: u64 = 2; -const MAX_GRACE_TICKS: u64 = 12; +const MAX_GRACE_SLOTS: u64 = 3; #[derive(Debug, PartialEq, Eq, Clone)] pub enum PohRecorderError { @@ -173,7 +173,7 @@ impl PohRecorder { let last_tick_height = (last_slot + 1) * ticks_per_slot; let num_slots = last_slot - first_slot + 1; let grace_ticks = cmp::min( - MAX_GRACE_TICKS, + ticks_per_slot * MAX_GRACE_SLOTS, ticks_per_slot * num_slots / GRACE_TICKS_FACTOR, ); ( @@ -186,7 +186,7 @@ impl PohRecorder { None, 0, cmp::min( - MAX_GRACE_TICKS, + ticks_per_slot * MAX_GRACE_SLOTS, ticks_per_slot * NUM_CONSECUTIVE_LEADER_SLOTS / GRACE_TICKS_FACTOR, ), )) @@ -1323,7 +1323,7 @@ mod tests { assert_eq!( PohRecorder::compute_leader_slot_tick_heights(Some((4, 7)), 8), - (Some(45), 64, MAX_GRACE_TICKS) + (Some(49), 64, 2 * 8) ); assert_eq!( diff --git a/core/src/replay_stage.rs b/core/src/replay_stage.rs index 49e729a381..40a4fdc18e 100644 --- a/core/src/replay_stage.rs +++ b/core/src/replay_stage.rs @@ -792,7 +792,7 @@ impl ReplayStage { return (None, None); } let mut vote = None; - let (best_bank, best_stats) = best_banks.last().unwrap(); + let (mut best_bank, best_stats) = best_banks.last().unwrap(); debug!("best bank: {:?}", best_stats); let mut by_slot: Vec<_> = best_banks.iter().collect(); by_slot.sort_by_key(|x| x.1.slot); @@ -808,14 +808,12 @@ impl ReplayStage { debug!("best bank found ancestor: {}", parent_stats.slot); inc_new_counter_info!("replay_stage-pick_best_fork-ancestor", 1); vote = Some(((*parent).clone(), parent_stats.total_staked)); + break; } } - //look for the oldest child of the best bank + //look for the heaviest child of the best bank if vote.is_none() { - for (child, child_stats) in by_slot.iter().rev() { - if child_stats.is_locked_out || !child_stats.vote_threshold { - continue; - } + for (child, child_stats) in best_banks.iter().rev() { let has_best = best_stats.slot == child_stats.slot || ancestors .get(&child.slot()) @@ -824,9 +822,14 @@ impl ReplayStage { if !has_best { continue; } + best_bank = child; + if child_stats.is_locked_out || !child_stats.vote_threshold { + continue; + } inc_new_counter_info!("replay_stage-pick_best_fork-child", 1); debug!("best bank found child: {}", child_stats.slot); vote = Some(((*child).clone(), child_stats.total_staked)); + break; } } if vote.is_none() { @@ -1080,6 +1083,7 @@ mod test { use solana_sdk::transaction::TransactionError; use solana_vote_api::vote_state::VoteState; use std::fs::remove_dir_all; + use std::iter::FromIterator; use std::sync::{Arc, RwLock}; #[test] @@ -1479,4 +1483,133 @@ mod test { &expected2 ); } + + #[test] + fn test_pick_best_fork() { + let leader_pubkey = Pubkey::new_rand(); + let leader_lamports = 3; + let genesis_config_info = + create_genesis_config_with_leader(50, &leader_pubkey, leader_lamports); + let mut genesis_config = genesis_config_info.genesis_config; + genesis_config.epoch_schedule.warmup = false; + genesis_config.ticks_per_slot = 4; + let bank_default = Arc::new(Bank::new(&genesis_config)); + + // Create bank forks such that + // /- 9 - 11 + // 7 - 8 + // \- 10 - 12 + // + let mut ancestors: HashMap> = HashMap::new(); + ancestors.insert(8, HashSet::from_iter(vec![7u64])); + ancestors.insert(9, HashSet::from_iter(vec![8, 7])); + ancestors.insert(11, HashSet::from_iter(vec![9, 8, 7])); + ancestors.insert(10, HashSet::from_iter(vec![8, 7])); + ancestors.insert(12, HashSet::from_iter(vec![10, 8, 7])); + + let bank7 = Arc::new(Bank::new_from_parent(&bank_default, &Pubkey::new_rand(), 7)); + let bank8 = Arc::new(Bank::new_from_parent(&bank7, &Pubkey::new_rand(), 8)); + let bank9 = Arc::new(Bank::new_from_parent(&bank8, &Pubkey::new_rand(), 9)); + let bank11 = Arc::new(Bank::new_from_parent(&bank9, &Pubkey::new_rand(), 11)); + let bank10 = Arc::new(Bank::new_from_parent(&bank8, &Pubkey::new_rand(), 10)); + let bank12 = Arc::new(Bank::new_from_parent(&bank10, &Pubkey::new_rand(), 12)); + let banks = vec![ + (bank7, ForkStats::default()), + (bank8, ForkStats::default()), + (bank9, ForkStats::default()), + (bank10, ForkStats::default()), + (bank11, ForkStats::default()), + (bank12, ForkStats::default()), + ]; + + // Fork stats are: no lockouts, and vote_threshold is met for all banks + let mut best_banks: Vec<_> = banks + .into_iter() + .map(|(b, mut f)| { + f.slot = b.slot(); + f.is_locked_out = false; + f.vote_threshold = true; + (b, f) + }) + .collect(); + + let best_banks_ref: Vec<_> = best_banks.iter().map(|(b, f)| (b, f)).collect(); + + let res = ReplayStage::pick_best_fork(&ancestors, &best_banks_ref[..]); + assert!(res.0.is_some()); + assert_eq!(res.0.unwrap().0.slot(), 7); + + // Set bank7 as locked out. Should return bank8 as best fork + best_banks[0].1.is_locked_out = true; + + let best_banks_ref: Vec<_> = best_banks.iter().map(|(b, f)| (b, f)).collect(); + + let res = ReplayStage::pick_best_fork(&ancestors, &best_banks_ref[..]); + assert!(res.0.is_some()); + assert_eq!(res.0.unwrap().0.slot(), 8); + + // Set bank8 as missing vote threshold. Should return bank10 as best fork (as 9 on a different fork) + best_banks[1].1.vote_threshold = false; + + let best_banks_ref: Vec<_> = best_banks.iter().map(|(b, f)| (b, f)).collect(); + + let res = ReplayStage::pick_best_fork(&ancestors, &best_banks_ref[..]); + assert!(res.0.is_some()); + assert_eq!(res.0.unwrap().0.slot(), 10); + + // Set bank10 also as missing vote threshold. It should return 12 (oldest child of best bank) + best_banks[3].1.vote_threshold = false; + + let best_banks_ref: Vec<_> = best_banks.iter().map(|(b, f)| (b, f)).collect(); + + let res = ReplayStage::pick_best_fork(&ancestors, &best_banks_ref[..]); + assert!(res.0.is_some()); + assert_eq!(res.0.unwrap().0.slot(), 12); + + // Set bank12 also as missing vote threshold. + best_banks[5].1.vote_threshold = false; + + let best_banks_ref: Vec<_> = best_banks.iter().map(|(b, f)| (b, f)).collect(); + + let res = ReplayStage::pick_best_fork(&ancestors, &best_banks_ref[..]); + assert!(res.0.is_none()); + assert!(res.1.is_some()); + assert_eq!(res.1.unwrap().slot(), 12); + + // Test if heaviest bank has a leaf which is not the heaviest leaf + // e.g. heaviest bank is 9, but it's leaf (11) is lighter than 12 + let bank7 = Arc::new(Bank::new_from_parent(&bank_default, &Pubkey::new_rand(), 7)); + let bank8 = Arc::new(Bank::new_from_parent(&bank7, &Pubkey::new_rand(), 8)); + let bank9 = Arc::new(Bank::new_from_parent(&bank8, &Pubkey::new_rand(), 9)); + let bank11 = Arc::new(Bank::new_from_parent(&bank9, &Pubkey::new_rand(), 11)); + let bank10 = Arc::new(Bank::new_from_parent(&bank8, &Pubkey::new_rand(), 10)); + let bank12 = Arc::new(Bank::new_from_parent(&bank10, &Pubkey::new_rand(), 12)); + let banks = vec![ + (bank7, ForkStats::default()), + (bank8, ForkStats::default()), + (bank10, ForkStats::default()), + (bank11, ForkStats::default()), + (bank12, ForkStats::default()), + (bank9, ForkStats::default()), + ]; + + // Assume everything is locked out (or does not have threshold) + let best_banks: Vec<_> = banks + .into_iter() + .map(|(b, mut f)| { + f.slot = b.slot(); + f.is_locked_out = true; + f.vote_threshold = true; + (b, f) + }) + .collect(); + + let best_banks_ref: Vec<_> = best_banks.iter().map(|(b, f)| (b, f)).collect(); + + // It should return 11, as it's child of heaviest bank + let res = ReplayStage::pick_best_fork(&ancestors, &best_banks_ref[..]); + assert!(res.0.is_none()); + assert!(res.1.is_some()); + assert_eq!(res.1.unwrap().slot(), 11); + } }