diff --git a/frame/session/src/lib.rs b/frame/session/src/lib.rs index 3243d81a3d631..780e7e71c5675 100644 --- a/frame/session/src/lib.rs +++ b/frame/session/src/lib.rs @@ -313,8 +313,9 @@ impl SessionHandler for Tuple { #( let our_keys: Box> = Box::new(validators.iter() .filter_map(|k| - (&k.0, k.1.get::(::ID))) - ); + k.1.get::(::ID).map(|k1| (&k.0, k1)) + ) + ); Tuple::on_genesis_session(our_keys); )* @@ -330,12 +331,12 @@ impl SessionHandler for Tuple { #( let our_keys: Box> = Box::new(validators.iter() .filter_map(|k| - (&k.0, k.1.get::(::ID))) - ); + k.1.get::(::ID).map(|k1| (&k.0, k1)) + )); let queued_keys: Box> = Box::new(queued_validators.iter() .filter_map(|k| - (&k.0, k.1.get::(::ID))) - ); + k.1.get::(::ID).map(|k1| (&k.0, k1)) + )); Tuple::on_new_session(changed, our_keys, queued_keys); )* ) diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index f2e5f0f783895..081ff7d1e4290 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -364,7 +364,7 @@ pub struct ActiveEraInfo { /// Reward points of an era. Used to split era total payout between validators. /// /// This points will be used to reward validators and their respective nominators. -#[derive(PartialEq, Encode, Decode, Default, RuntimeDebug, TypeInfo)] +#[derive(PartialEq, Encode, Decode, RuntimeDebug, TypeInfo)] pub struct EraRewardPoints { /// Total number of points. Equals the sum of reward points for each validator. total: RewardPoint, @@ -372,6 +372,12 @@ pub struct EraRewardPoints { individual: BTreeMap, } +impl Default for EraRewardPoints { + fn default() -> Self { + EraRewardPoints { total: Default::default(), individual: BTreeMap::new() } + } +} + /// Indicates the initial status of the staker. #[derive(RuntimeDebug, TypeInfo)] #[cfg_attr(feature = "std", derive(serde::Serialize, serde::Deserialize))] @@ -593,9 +599,7 @@ pub struct IndividualExposure { } /// A snapshot of the stake backing a single validator in the system. -#[derive( - PartialEq, Eq, PartialOrd, Ord, Clone, Encode, Decode, Default, RuntimeDebug, TypeInfo, -)] +#[derive(PartialEq, Eq, PartialOrd, Ord, Clone, Encode, Decode, RuntimeDebug, TypeInfo)] pub struct Exposure { /// The total balance backing this validator. #[codec(compact)] @@ -607,6 +611,12 @@ pub struct Exposure { pub others: Vec>, } +impl Default for Exposure { + fn default() -> Self { + Self { total: Default::default(), own: Default::default(), others: vec![] } + } +} + /// A pending slash record. The value of the slash has been computed but not applied yet, /// rather deferred for several eras. #[derive(Encode, Decode, Default, RuntimeDebug, TypeInfo)] diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index edcc061112d6f..dd6515c500db1 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -1108,9 +1108,12 @@ where fn note_author(author: T::AccountId) { Self::reward_by_ids(vec![(author, 20)]) } - fn note_uncle(author: T::AccountId, _age: T::BlockNumber) { - if let Some(author) = >::author() { - Self::reward_by_ids(vec![(author, 2), (author, 1)]) + fn note_uncle(uncle_author: T::AccountId, _age: T::BlockNumber) { + // defensive-only: block author must exist. + if let Some(block_author) = >::author() { + Self::reward_by_ids(vec![(block_author, 2), (uncle_author, 1)]) + } else { + crate::log!(warn, "block author not set, this should never happen"); } } } diff --git a/primitives/npos-elections/src/lib.rs b/primitives/npos-elections/src/lib.rs index b1e1ed2745174..0b10b4c7c1b8f 100644 --- a/primitives/npos-elections/src/lib.rs +++ b/primitives/npos-elections/src/lib.rs @@ -193,6 +193,15 @@ pub struct Edge { weight: ExtendedBalance, } +#[cfg(test)] +impl Edge { + fn new(candidate: Candidate, weight: ExtendedBalance) -> Self { + let who = candidate.who.clone(); + let candidate = Rc::new(RefCell::new(candidate)); + Self { weight, who, candidate, load: Default::default() } + } +} + #[cfg(feature = "std")] impl sp_std::fmt::Debug for Edge { fn fmt(&self, f: &mut sp_std::fmt::Formatter<'_>) -> sp_std::fmt::Result { @@ -223,7 +232,12 @@ impl std::fmt::Debug for Voter { impl Voter { /// Create a new `Voter`. pub fn new(who: AccountId) -> Self { - Self { who, edges: Default::default(), budget: Default::default(), load: Default::default() } + Self { + who, + edges: Default::default(), + budget: Default::default(), + load: Default::default(), + } } /// Returns `true` if `self` votes for `target`. @@ -350,10 +364,7 @@ pub struct Support { impl Default for Support { fn default() -> Self { - Self { - total: Default::default(), - voters: vec![], - } + Self { total: Default::default(), voters: vec![] } } } @@ -477,7 +488,8 @@ pub fn setup_inputs( backed_stake: Default::default(), elected: Default::default(), round: Default::default(), - }.to_ptr() + } + .to_ptr() }) .collect::>>(); diff --git a/primitives/npos-elections/src/reduce.rs b/primitives/npos-elections/src/reduce.rs index 9a31a4aa6da58..98e015497fad1 100644 --- a/primitives/npos-elections/src/reduce.rs +++ b/primitives/npos-elections/src/reduce.rs @@ -355,7 +355,7 @@ fn reduce_all(assignments: &mut Vec>) -> u32 .or_insert_with(|| Node::new(target_id).into_ref()) .clone(); - // If one exists but the other one doesn't, or if both does not, then set the existing + // If one exists but the other one doesn't, or if both do not, then set the existing // one as the parent of the non-existing one and move on. Else, continue with the rest // of the code. match (voter_exists, target_exists) { @@ -389,39 +389,43 @@ fn reduce_all(assignments: &mut Vec>) -> u32 let common_count = trailing_common(&voter_root_path, &target_root_path); // because roots are the same. - #[cfg(feature = "std")] debug_assert_eq!(target_root_path.last().unwrap(), voter_root_path.last().unwrap()); + // the common path must be non-void.. debug_assert!(common_count > 0); + // and smaller than btoh + debug_assert!(common_count <= voter_root_path.len()); + debug_assert!(common_count <= target_root_path.len()); // cycle part of each path will be `path[path.len() - common_count - 1 : 0]` // NOTE: the order of chaining is important! it is always build from [target, ..., // voter] let cycle = target_root_path .iter() - .take(target_root_path.len() - common_count + 1) + .take(target_root_path.len().saturating_sub(common_count).saturating_add(1)) .cloned() .chain( voter_root_path .iter() - .take(voter_root_path.len() - common_count) + .take(voter_root_path.len().saturating_sub(common_count)) .rev() .cloned(), ) .collect::>>(); // a cycle's length shall always be multiple of two. - #[cfg(feature = "std")] debug_assert_eq!(cycle.len() % 2, 0); // find minimum of cycle. let mut min_value: ExtendedBalance = Bounded::max_value(); - // The voter and the target pair that create the min edge. - let mut min_target: Option = None; - let mut min_voter: Option = None; + // The voter and the target pair that create the min edge. These MUST be set by the + // end of this code block, otherwise we skip. + let mut maybe_min_target: Option = None; + let mut maybe_min_voter: Option = None; // The index of the min in opaque cycle list. - let mut min_index = 0usize; + let mut maybe_min_index: Option = None; // 1 -> next // 0 -> prev - let mut min_direction = 0u32; + let mut maybe_min_direction: Option = None; + // helpers let next_index = |i| { if i < (cycle.len() - 1) { @@ -437,6 +441,7 @@ fn reduce_all(assignments: &mut Vec>) -> u32 cycle.len() - 1 } }; + for i in 0..cycle.len() { if cycle[i].borrow().id.role == NodeRole::Voter { // NOTE: sadly way too many clones since I don't want to make A: Copy @@ -447,10 +452,10 @@ fn reduce_all(assignments: &mut Vec>) -> u32 ass.distribution.iter().find(|d| d.0 == next).map(|(_, w)| { if *w < min_value { min_value = *w; - min_target = Some(next.clone()); - min_voter = Some(current.clone()); - min_index = i; - min_direction = 1; + maybe_min_target = Some(next.clone()); + maybe_min_voter = Some(current.clone()); + maybe_min_index = Some(i); + maybe_min_direction = Some(1); } }) }); @@ -458,16 +463,39 @@ fn reduce_all(assignments: &mut Vec>) -> u32 ass.distribution.iter().find(|d| d.0 == prev).map(|(_, w)| { if *w < min_value { min_value = *w; - min_target = Some(prev.clone()); - min_voter = Some(current.clone()); - min_index = i; - min_direction = 0; + maybe_min_target = Some(prev.clone()); + maybe_min_voter = Some(current.clone()); + maybe_min_index = Some(i); + maybe_min_direction = Some(0); } }) }); } } + // all of these values must be set by now, we assign them to un-mut values, no + // longer being optional either. + let (min_value, min_target, min_voter, min_index, min_direction) = + match ( + min_value, + maybe_min_target, + maybe_min_voter, + maybe_min_index, + maybe_min_direction, + ) { + ( + min_value, + Some(min_target), + Some(min_voter), + Some(min_index), + Some(min_direction), + ) => (min_value, min_target, min_voter, min_index, min_direction), + _ => { + sp_runtime::print("UNREACHABLE code reached in `reduce` algorithm. This must be a bug."); + break + }, + }; + // if the min edge is in the voter's sub-chain. // [target, ..., X, Y, ... voter] let target_chunk = target_root_path.len() - common_count; @@ -577,7 +605,7 @@ fn reduce_all(assignments: &mut Vec>) -> u32 // re-org. if should_reorg { - let min_edge = min_voter.into_iter().chain(min_target.into_iter()).collect::>(); + let min_edge = vec![min_voter, min_target]; if min_chain_in_voter { // NOTE: safe; voter_root_path is always bigger than 1 element. for i in 0..voter_root_path.len() - 1 { @@ -623,8 +651,8 @@ fn reduce_all(assignments: &mut Vec>) -> u32 num_changed } -/// Reduce the given [`Vec>`]. This removes redundant edges from -/// without changing the overall backing of any of the elected candidates. +/// Reduce the given [`Vec>`]. This removes redundant edges without +/// changing the overall backing of any of the elected candidates. /// /// Returns the number of edges removed. /// diff --git a/primitives/npos-elections/src/tests.rs b/primitives/npos-elections/src/tests.rs index bf9ca57677efa..c745ee1d7706d 100644 --- a/primitives/npos-elections/src/tests.rs +++ b/primitives/npos-elections/src/tests.rs @@ -199,9 +199,9 @@ fn voter_normalize_ops_works() { let c2 = Candidate { who: 20, elected: false, ..Default::default() }; let c3 = Candidate { who: 30, elected: false, ..Default::default() }; - let e1 = Edge { candidate: Rc::new(RefCell::new(c1)), weight: 30, ..Default::default() }; - let e2 = Edge { candidate: Rc::new(RefCell::new(c2)), weight: 33, ..Default::default() }; - let e3 = Edge { candidate: Rc::new(RefCell::new(c3)), weight: 30, ..Default::default() }; + let e1 = Edge::new(c1, 30); + let e2 = Edge::new(c2, 33); + let e3 = Edge::new(c3, 30); let mut v = Voter { who: 1, budget: 100, edges: vec![e1, e2, e3], ..Default::default() }; @@ -214,9 +214,9 @@ fn voter_normalize_ops_works() { let c2 = Candidate { who: 20, elected: true, ..Default::default() }; let c3 = Candidate { who: 30, elected: true, ..Default::default() }; - let e1 = Edge { candidate: Rc::new(RefCell::new(c1)), weight: 30, ..Default::default() }; - let e2 = Edge { candidate: Rc::new(RefCell::new(c2)), weight: 33, ..Default::default() }; - let e3 = Edge { candidate: Rc::new(RefCell::new(c3)), weight: 30, ..Default::default() }; + let e1 = Edge::new(c1, 30); + let e2 = Edge::new(c2, 33); + let e3 = Edge::new(c3, 30); let mut v = Voter { who: 1, budget: 100, edges: vec![e1, e2, e3], ..Default::default() };