From df3800d17d710e7710be73d6a49b02bdb695910c Mon Sep 17 00:00:00 2001 From: Mikhail Date: Fri, 28 Feb 2020 12:13:39 -0800 Subject: [PATCH] fix(epoch_manager): Some fixes and comments Some fixes, refactoring and comments. Test plan --------- Run existing tests Add more asserts --- chain/epoch_manager/src/lib.rs | 29 ++++++- chain/epoch_manager/src/proposals.rs | 118 +++++++++++--------------- chain/epoch_manager/src/test_utils.rs | 18 ---- chain/epoch_manager/src/types.rs | 14 +-- core/primitives/src/types.rs | 2 + 5 files changed, 83 insertions(+), 98 deletions(-) diff --git a/chain/epoch_manager/src/lib.rs b/chain/epoch_manager/src/lib.rs index 3b60aacf541..65321588ffd 100644 --- a/chain/epoch_manager/src/lib.rs +++ b/chain/epoch_manager/src/lib.rs @@ -77,6 +77,11 @@ impl EpochManager { validator_reward, 0, )?; + // Dummy block info. + // Artificial block we add to simplify implementation: dummy block is the + // parent of genesis block that points to itself. + // If we view it as block in epoch -1 and height -1, it naturally extends the + // EpochId formula using T-2 for T=1, and height field is unused. let block_info = BlockInfo::default(); let mut store_update = epoch_manager.store.store_update(); epoch_manager.save_epoch_info(&mut store_update, &genesis_epoch_id, epoch_info)?; @@ -86,6 +91,21 @@ impl EpochManager { Ok(epoch_manager) } + /// # Parameters + /// epoch_info + /// block_validator_tracker + /// chunk_validator_tracker + /// + /// slashed: set of slashed validators + /// prev_validator_kickout: previously kicked out + /// + /// # Returns + /// (set of validators to kickout, set of validators to reward with stats) + /// + /// - Slashed validators are ignored (they are handled separately) + /// - A validator is kicked out if he produced too few blocks or chunks + /// - If all validators are either previously kicked out or to be kicked out, we choose one not to + /// kick out fn compute_kickout_info( &self, epoch_info: &EpochInfo, @@ -203,7 +223,7 @@ impl EpochManager { &slashed_validators, &prev_validator_kickout, ); - validator_kickout = validator_kickout.union(&kickout).cloned().collect(); + validator_kickout.extend(kickout); debug!( "All proposals: {:?}, Kickouts: {:?}, Block Tracker: {:?}, Shard Tracker: {:?}", all_proposals, validator_kickout, block_validator_tracker, chunk_validator_tracker @@ -938,6 +958,10 @@ impl EpochManager { } } + /// Get BlockInfo for a block + /// # Errors + /// EpochError::Other if storage returned an error + /// EpochError::MissingBlock if block is not in storage pub fn get_block_info(&mut self, hash: &CryptoHash) -> Result<&BlockInfo, EpochError> { if self.blocks_info.cache_get(hash).is_none() { let block_info = self @@ -1404,8 +1428,11 @@ mod tests { epoch_manager.get_slashed_validators(&h[2]).unwrap().clone().into_iter().collect(); let slashed2: Vec<_> = epoch_manager.get_slashed_validators(&h[3]).unwrap().clone().into_iter().collect(); + let slashed3: Vec<_> = + epoch_manager.get_slashed_validators(&h[5]).unwrap().clone().into_iter().collect(); assert_eq!(slashed1, vec![("test1".to_string(), SlashState::Other)]); assert_eq!(slashed2, vec![("test1".to_string(), SlashState::AlreadySlashed)]); + assert_eq!(slashed3, vec![("test1".to_string(), SlashState::AlreadySlashed)]); } /// Test that double sign interacts with other challenges in the correct way. diff --git a/chain/epoch_manager/src/proposals.rs b/chain/epoch_manager/src/proposals.rs index b6319c16638..ca362ae5d87 100644 --- a/chain/epoch_manager/src/proposals.rs +++ b/chain/epoch_manager/src/proposals.rs @@ -1,4 +1,3 @@ -use std::collections::btree_map::Entry; use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; use std::iter; @@ -42,36 +41,31 @@ pub fn proposals_to_epoch_info( ) -> Result { // Combine proposals with rollovers. let mut ordered_proposals = BTreeMap::new(); + // Account -> new_stake let mut stake_change = BTreeMap::new(); let mut fishermen = vec![]; - let mut fishermen_to_index = HashMap::new(); + debug_assert!( + proposals.iter().map(|stake| &stake.account_id).collect::>().len() + == proposals.len(), + "Proposals should not have duplicates" + ); for p in proposals { if validator_kickout.contains(&p.account_id) { - stake_change.insert(p.account_id, (0, p.stake)); + stake_change.insert(p.account_id, 0); } else { + stake_change.insert(p.account_id.clone(), p.stake); ordered_proposals.insert(p.account_id.clone(), p); } } for r in epoch_info.validators.iter() { - match ordered_proposals.entry(r.account_id.clone()) { - Entry::Occupied(mut e) => { - let p = e.get_mut(); - let return_stake = if r.stake > p.stake { r.stake - p.stake } else { 0 }; - let reward = *validator_reward.get(&r.account_id).unwrap_or(&0); - p.stake += reward; - stake_change.insert(r.account_id.clone(), (p.stake, return_stake)); - } - Entry::Vacant(e) => { - if !validator_kickout.contains(&r.account_id) { - let mut proposal = r.clone(); - proposal.stake += *validator_reward.get(&r.account_id).unwrap_or(&0); - e.insert(proposal); - } else { - stake_change.insert(r.account_id.clone(), (0, r.stake)); - } - } + if validator_kickout.contains(&r.account_id) { + stake_change.insert(r.account_id.clone(), 0); + continue; } + let p = ordered_proposals.entry(r.account_id.clone()).or_insert_with(|| r.clone()); + p.stake += *validator_reward.get(&p.account_id).unwrap_or(&0); + stake_change.insert(p.account_id.clone(), p.stake); } for r in epoch_info.fishermen.iter() { @@ -80,9 +74,8 @@ pub fn proposals_to_epoch_info( { // safe to do this here because fishermen from previous epoch is guaranteed to have no // duplicates. - fishermen_to_index.insert(r.account_id.clone(), fishermen.len() as ValidatorId); fishermen.push(r.clone()); - stake_change.insert(r.account_id.clone(), (r.stake, 0)); + stake_change.insert(r.account_id.clone(), r.stake); } } @@ -97,25 +90,12 @@ pub fn proposals_to_epoch_info( for (account_id, p) in ordered_proposals { if p.stake >= threshold { - if !stake_change.contains_key(&account_id) { - stake_change.insert(account_id, (p.stake, 0)); - } final_proposals.push(p); } else if p.stake >= epoch_config.fishermen_threshold { // Do not return stake back since they will become fishermen - stake_change.entry(account_id.clone()).or_insert((p.stake, 0)); - fishermen_to_index.insert(account_id, fishermen.len() as ValidatorId); fishermen.push(p); } else { - stake_change - .entry(account_id.clone()) - .and_modify(|(new_stake, return_stake)| { - if *new_stake != 0 { - *return_stake += *new_stake; - *new_stake = 0; - } - }) - .or_insert((0, p.stake)); + *stake_change.get_mut(&account_id).unwrap() = 0; if epoch_info.validator_to_index.contains_key(&account_id) || epoch_info.fishermen_to_index.contains_key(&account_id) { @@ -130,12 +110,8 @@ pub fn proposals_to_epoch_info( .enumerate() .flat_map(|(i, p)| iter::repeat(i as u64).take((p.stake / threshold) as usize)) .collect::>(); - if dup_proposals.len() < num_total_seats as usize { - return Err(EpochError::SelectedSeatsMismatch( - dup_proposals.len() as NumSeats, - num_total_seats, - )); - } + + assert!(dup_proposals.len() >= num_total_seats as usize, "bug in find_threshold"); { use protocol_defining_rand::seq::SliceRandom; use protocol_defining_rand::{rngs::StdRng, SeedableRng}; @@ -148,29 +124,24 @@ pub fn proposals_to_epoch_info( let mut block_producers_settlement = dup_proposals[..epoch_config.num_block_producer_seats as usize].to_vec(); // remove proposals that are not selected - let mut indices_to_remove = (0..final_proposals.len()).collect::>(); - for index in block_producers_settlement.iter() { - indices_to_remove.remove(&(*index as usize)); - } - let (final_proposals, proposals_to_remove, validator_to_index) = - final_proposals.into_iter().enumerate().fold( - (vec![], vec![], HashMap::new()), - |(mut proposals, mut to_remove, mut indices), (i, p)| { - if indices_to_remove.contains(&i) { - to_remove.push(p); - } else { - indices.insert(p.account_id.clone(), proposals.len() as ValidatorId); - proposals.push(p); - } - (proposals, to_remove, indices) - }, - ); + let indices_to_keep = block_producers_settlement.iter().copied().collect::>(); + let (final_proposals, proposals_to_remove) = final_proposals.into_iter().enumerate().fold( + (vec![], vec![]), + |(mut proposals, mut to_remove), (i, p)| { + if indices_to_keep.contains(&(i as u64)) { + proposals.push(p); + } else { + to_remove.push(p); + } + (proposals, to_remove) + }, + ); for p in proposals_to_remove { + debug_assert!(p.stake >= threshold); if p.stake >= epoch_config.fishermen_threshold { - fishermen_to_index.insert(p.account_id.clone(), fishermen.len() as ValidatorId); fishermen.push(p); } else { - stake_change.insert(p.account_id.clone(), (0, p.stake)); + stake_change.insert(p.account_id.clone(), 0); if epoch_info.validator_to_index.contains_key(&p.account_id) || epoch_info.fishermen_to_index.contains_key(&p.account_id) { @@ -181,7 +152,7 @@ pub fn proposals_to_epoch_info( // reset indices for index in block_producers_settlement.iter_mut() { - *index -= indices_to_remove.range(..(*index as usize)).count() as u64; + *index = indices_to_keep.range(..*index).count() as u64; } // Collect proposals into block producer assignments. @@ -189,18 +160,27 @@ pub fn proposals_to_epoch_info( let mut last_index: u64 = 0; for num_seats_in_shard in epoch_config.num_block_producer_seats_per_shard.iter() { let mut shard_settlement: Vec = vec![]; - for i in 0..*num_seats_in_shard { - let proposal_index = block_producers_settlement - [((i + last_index) % epoch_config.num_block_producer_seats) as usize]; + for _ in 0..*num_seats_in_shard { + let proposal_index = block_producers_settlement[last_index as usize]; shard_settlement.push(proposal_index); + last_index = (last_index + 1) % epoch_config.num_block_producer_seats; } chunk_producers_settlement.push(shard_settlement); - last_index = (last_index + num_seats_in_shard) % epoch_config.num_block_producer_seats; } - // TODO(1050): implement fishermen allocation. + let fishermen_to_index = fishermen + .iter() + .enumerate() + .map(|(index, s)| (s.account_id.clone(), index as ValidatorId)) + .collect::>(); - let final_stake_change = stake_change.into_iter().map(|(k, (v, _))| (k, v)).collect(); + let validator_to_index = final_proposals + .iter() + .enumerate() + .map(|(index, s)| (s.account_id.clone(), index as ValidatorId)) + .collect::>(); + + // TODO(1050): implement fishermen allocation. Ok(EpochInfo { validators: final_proposals, @@ -209,7 +189,7 @@ pub fn proposals_to_epoch_info( block_producers_settlement, chunk_producers_settlement, hidden_validators_settlement: vec![], - stake_change: final_stake_change, + stake_change, validator_reward, inflation, validator_kickout, diff --git a/chain/epoch_manager/src/test_utils.rs b/chain/epoch_manager/src/test_utils.rs index b08e70e7bef..03a142eeefe 100644 --- a/chain/epoch_manager/src/test_utils.rs +++ b/chain/epoch_manager/src/test_utils.rs @@ -105,24 +105,6 @@ pub fn stake(account_id: &str, amount: Balance) -> ValidatorStake { ValidatorStake::new(account_id.to_string(), public_key, amount) } -pub fn reward_calculator( - max_inflation_rate: u8, - num_blocks_per_year: u64, - epoch_length: BlockHeightDelta, - validator_reward_percentage: u8, - protocol_reward_percentage: u8, - protocol_treasury_account: AccountId, -) -> RewardCalculator { - RewardCalculator { - max_inflation_rate, - num_blocks_per_year, - epoch_length, - validator_reward_percentage, - protocol_reward_percentage, - protocol_treasury_account, - } -} - /// No-op reward calculator. Will produce no reward pub fn default_reward_calculator() -> RewardCalculator { RewardCalculator { diff --git a/chain/epoch_manager/src/types.rs b/chain/epoch_manager/src/types.rs index c4b97b2c1d2..129b79f2227 100644 --- a/chain/epoch_manager/src/types.rs +++ b/chain/epoch_manager/src/types.rs @@ -77,6 +77,7 @@ pub struct BlockInfo { pub epoch_id: EpochId, pub proposals: Vec, pub chunk_mask: Vec, + /// Validators slashed since the start of epoch or in previous epoch pub slashed: HashMap, /// Total rent paid in this block. pub rent_paid: Balance, @@ -196,8 +197,6 @@ pub enum EpochError { ThresholdError(Balance, u64), /// Requesting validators for an epoch that wasn't computed yet. EpochOutOfBounds, - /// Number of selected seats doesn't match requested. - SelectedSeatsMismatch(NumSeats, NumSeats), /// Missing block hash in the storage (means there is some structural issue). MissingBlock(CryptoHash), /// Other error. @@ -215,11 +214,6 @@ impl fmt::Debug for EpochError { stakes_sum, num_seats ), EpochError::EpochOutOfBounds => write!(f, "Epoch out of bounds"), - EpochError::SelectedSeatsMismatch(selected, required) => write!( - f, - "Number of selected seats {} < total number of seats {}", - selected, required - ), EpochError::MissingBlock(hash) => write!(f, "Missing block {}", hash), EpochError::Other(err) => write!(f, "Other: {}", err), } @@ -233,9 +227,6 @@ impl fmt::Display for EpochError { write!(f, "ThresholdError({}, {})", stake, num_seats) } EpochError::EpochOutOfBounds => write!(f, "EpochOutOfBounds"), - EpochError::SelectedSeatsMismatch(selected, required) => { - write!(f, "SelectedSeatsMismatch({}, {})", selected, required) - } EpochError::MissingBlock(hash) => write!(f, "MissingBlock({})", hash), EpochError::Other(err) => write!(f, "Other({})", err), } @@ -261,8 +252,11 @@ impl From for near_chain::Error { pub struct EpochSummary { pub prev_epoch_last_block_hash: CryptoHash, + // Proposals from the epoch, only the latest one per account pub all_proposals: Vec, + // Kickout set, includes slashed pub validator_kickout: HashSet, + // Only for validators who met the threshold and didn't get slashed pub validator_block_chunk_stats: HashMap, pub total_storage_rent: Balance, pub total_validator_reward: Balance, diff --git a/core/primitives/src/types.rs b/core/primitives/src/types.rs index a644c98f5fc..076f3310f97 100644 --- a/core/primitives/src/types.rs +++ b/core/primitives/src/types.rs @@ -147,6 +147,8 @@ impl StateRootNode { } /// Epoch identifier -- wrapped hash, to make it easier to distinguish. +/// EpochId of epoch T is the hash of last block in T-2 +/// EpochId of first two epochs is 0 #[derive( Hash, Eq,