Fix several bugs in phragmen elections.

srml/staking/src/phragmen.rs

@@ -108,61 +108,69 @@ pub fn elect<T: Trait + 'static, FN, FV, FS>(
 	>>,
 	for <'r> FS: Fn(&'r T::AccountId) -> BalanceOf<T>,
 {
-	let expand = |b: BalanceOf<T>| <T::CurrencyToVote as Convert<BalanceOf<T>, u64>>::convert(b) as ExtendedBalance;
-	let shrink = |b: ExtendedBalance| <T::CurrencyToVote as Convert<ExtendedBalance, BalanceOf<T>>>::convert(b);
+	let into_currency = |b: BalanceOf<T>| <T::CurrencyToVote as Convert<BalanceOf<T>, u64>>::convert(b) as ExtendedBalance;
+	let into_votes = |b: ExtendedBalance| <T::CurrencyToVote as Convert<ExtendedBalance, BalanceOf<T>>>::convert(b);
 	let mut elected_candidates;
 
-	// 1- Pre-process candidates and place them in a container
-	let mut candidates = get_validators().map(|(who, _)| {
-		let stash_balance = stash_of(&who);
-		Candidate {
-			who,
-			exposure: Exposure { total: stash_balance, own: stash_balance, others: vec![] },
-			..Default::default()
-		}
-	}).collect::<Vec<Candidate<T::AccountId, BalanceOf<T>>>>();
-
-	// 1.1- Add phantom votes.
-	let mut nominators: Vec<Nominator<T::AccountId>> = Vec::with_capacity(candidates.len());
-	candidates.iter_mut().enumerate().for_each(|(idx, c)| {
-		c.approval_stake += expand(c.exposure.total);
-		nominators.push(Nominator {
-			who: c.who.clone(),
-			edges: vec![ Edge { who: c.who.clone(), candidate_index: idx, ..Default::default() }],
-			budget: expand(c.exposure.total),
-			load: Fraction::zero(),
+	let get_validators = get_validators();
+	let get_nominators = get_nominators();
+
+	// 1- Pre-process candidates and place them in a container, optimisation and add phantom votes.
+	// Candidates who have 0 stake => have no votes or all null-votes. Kick them out not.
+	let mut nominators: Vec<Nominator<T::AccountId>> = Vec::with_capacity(get_validators.size_hint().0 + get_nominators.size_hint().0);
+	let mut candidates = get_validators.map(|(who, _)| {
+			let stash_balance = stash_of(&who);
+			Candidate {
+				who,
+				exposure: Exposure { total: stash_balance, own: stash_balance, others: vec![] },
+				..Default::default()
+			}
 		})
-	});
+		.filter_map(|mut c| {
+			c.approval_stake += into_currency(c.exposure.total);
+			if c.approval_stake.is_zero() {
+				None
+			} else {
+				Some(c)
+			}
+		})
+		.enumerate()
+		.map(|(idx, c)| {
+			nominators.push(Nominator {
+				who: c.who.clone(),
+				edges: vec![ Edge { who: c.who.clone(), candidate_index: idx, ..Default::default() }],
+				budget: into_currency(c.exposure.total),
+				load: Fraction::zero(),
+			});
+			c
+		})
+		.collect::<Vec<Candidate<T::AccountId, BalanceOf<T>>>>();
 
 	// 2- Collect the nominators with the associated votes.
 	// Also collect approval stake along the way.
-	nominators.extend(get_nominators().map(|(who, nominees)| {
+	nominators.extend(get_nominators.map(|(who, nominees)| {
 		let nominator_stake = stash_of(&who);
 		let mut edges: Vec<Edge<T::AccountId>> = Vec::with_capacity(nominees.len());
 		for n in &nominees {
 			if let Some(idx) = candidates.iter_mut().position(|i| i.who == *n) {
 				candidates[idx].approval_stake = candidates[idx].approval_stake
-					.saturating_add(expand(nominator_stake));
+					.saturating_add(into_currency(nominator_stake));
 				edges.push(Edge { who: n.clone(), candidate_index: idx, ..Default::default() });
 			}
 		}
 
 		Nominator {
 			who,
 			edges: edges,
-			budget: expand(nominator_stake),
+			budget: into_currency(nominator_stake),
 			load: Fraction::zero(),
 		}
 	}));
 
-
-	// 3- optimization:
-	// Candidates who have 0 stake => have no votes or all null-votes. Kick them out not.
-	let mut candidates = candidates.into_iter().filter(|c| c.approval_stake > 0)
-		.collect::<Vec<Candidate<T::AccountId, BalanceOf<T>>>>();
-
 	// 4- If we have more candidates then needed, run Phragmén.
-	if candidates.len() > validator_count {
+	if candidates.len() >= minimum_validator_count {
+		let validator_count = validator_count.min(candidates.len());
+
 		elected_candidates = Vec::with_capacity(validator_count);
 		// Main election loop
 		for _round in 0..validator_count {
@@ -176,33 +184,36 @@ pub fn elect<T: Trait + 'static, FN, FV, FS>(
 			for n in &nominators {
 				for e in &n.edges {
 					let c = &mut candidates[e.candidate_index];
-					if !c.elected {
+					if !c.elected && !c.approval_stake.is_zero() {
 						let temp = n.budget.saturating_mul(*n.load) / c.approval_stake;
 						c.score = Fraction::from_max_value((*c.score).saturating_add(temp));
 					}
 				}
 			}
 
 			// Find the best
-			let winner = candidates
+			if let Some(winner) = candidates
 				.iter_mut()
 				.filter(|c| !c.elected)
 				.min_by_key(|c| *c.score)
-				.expect("candidates length is checked to be >0; qed");
-
-			// loop 3: update nominator and edge load
-			winner.elected = true;
-			for n in &mut nominators {
-				for e in &mut n.edges {
-					if e.who == winner.who {
-						e.load = Fraction::from_max_value(*winner.score - *n.load);
-						n.load = winner.score;
+			{
+				// loop 3: update nominator and edge load
+				winner.elected = true;
+				for n in &mut nominators {
+					for e in &mut n.edges {
+						if e.who == winner.who {
+							e.load = Fraction::from_max_value(*winner.score - *n.load);
+							n.load = winner.score;
+						}
 					}
 				}
-			}
 
-			elected_candidates.push(winner.clone());
-		} // end of all rounds
+				elected_candidates.push(winner.clone());
+			} else {
+				break
+			}
+		}
+		// end of all rounds
 
 		// 4.1- Update backing stake of candidates and nominators
 		for n in &mut nominators {
@@ -211,13 +222,13 @@ pub fn elect<T: Trait + 'static, FN, FV, FS>(
 				if let Some(c) = elected_candidates.iter_mut().find(|c| c.who == e.who) {
 					e.elected = true;
 					// NOTE: for now, always divide last to avoid collapse to zero.
-					e.backing_stake = n.budget.saturating_mul(*e.load) / *n.load;
+					e.backing_stake = n.budget.saturating_mul(*e.load) / n.load.max(1);
 					c.backing_stake = c.backing_stake.saturating_add(e.backing_stake);
 					if c.who != n.who {
 						// Only update the exposure if this vote is from some other account.
-						c.exposure.total = c.exposure.total.saturating_add(shrink(e.backing_stake));
+						c.exposure.total = c.exposure.total.saturating_add(into_votes(e.backing_stake));
 						c.exposure.others.push(
-							IndividualExposure { who: n.who.clone(), value: shrink(e.backing_stake) }
+							IndividualExposure { who: n.who.clone(), value: into_votes(e.backing_stake) }
 						);
 					}
 				}
@@ -252,24 +263,8 @@ pub fn elect<T: Trait + 'static, FN, FV, FS>(
 			}
 		}
 	} else {
-		if candidates.len() >= minimum_validator_count {
-			// if we don't have enough candidates, just choose all that have some vote.
-			elected_candidates = candidates;
-			for n in &mut nominators {
-				let nominator = n.who.clone();
-				for e in &mut n.edges {
-					if let Some(c) = elected_candidates.iter_mut().find(|c| c.who == e.who && c.who != nominator) {
-						c.exposure.total = c.exposure.total.saturating_add(shrink(n.budget));
-						c.exposure.others.push(
-							IndividualExposure { who: n.who.clone(), value: shrink(n.budget) }
-						);
-					}
-				}
-			}
-		} else {
-			// if we have less than minimum, use the previous validator set.
-			return None
-		}
+		// if we have less than minimum, use the previous validator set.
+		return None
 	}
 	Some(elected_candidates)
 }
@@ -283,9 +278,9 @@ pub fn equalize<T: Trait + 'static>(
 	elected_candidates: &mut Vec<Candidate<T::AccountId, BalanceOf<T>>>,
 	_tolerance: BalanceOf<T>
 ) -> BalanceOf<T> {
-	let expand = |b: BalanceOf<T>|    <T::CurrencyToVote as Convert<BalanceOf<T>, u64>>::convert(b) as ExtendedBalance;
-	let shrink = |b: ExtendedBalance| <T::CurrencyToVote as Convert<ExtendedBalance, BalanceOf<T>>>::convert(b);
-	let tolerance = expand(_tolerance);
+	let into_currency = |b: BalanceOf<T>|    <T::CurrencyToVote as Convert<BalanceOf<T>, u64>>::convert(b) as ExtendedBalance;
+	let into_votes = |b: ExtendedBalance| <T::CurrencyToVote as Convert<ExtendedBalance, BalanceOf<T>>>::convert(b);
+	let tolerance = into_currency(_tolerance);
 
 	let mut elected_edges = nominator.edges
 		.iter_mut()
@@ -322,7 +317,7 @@ pub fn equalize<T: Trait + 'static>(
 		difference = max_stake.saturating_sub(min_stake);
 		difference = difference.saturating_add(nominator.budget.saturating_sub(stake_used));
 		if difference < tolerance {
-			return shrink(difference);
+			return into_votes(difference);
 		}
 	} else {
 		difference = nominator.budget;
@@ -333,7 +328,7 @@ pub fn equalize<T: Trait + 'static>(
 		// NOTE: no assertions in the runtime, but this should nonetheless be indicative.
 		//assert_eq!(elected_candidates[e.elected_idx].who, e.who);
 		elected_candidates[e.elected_idx].backing_stake -= e.backing_stake;
-		elected_candidates[e.elected_idx].exposure.total -= shrink(e.backing_stake);
+		elected_candidates[e.elected_idx].exposure.total -= into_votes(e.backing_stake);
 		e.backing_stake = 0;
 	});
 
@@ -365,11 +360,11 @@ pub fn equalize<T: Trait + 'static>(
 		e.backing_stake = (excess / split_ways as ExtendedBalance)
 			.saturating_add(last_stake)
 			.saturating_sub(c.backing_stake);
-		c.exposure.total = c.exposure.total.saturating_add(shrink(e.backing_stake));
+		c.exposure.total = c.exposure.total.saturating_add(into_votes(e.backing_stake));
 		c.backing_stake = c.backing_stake.saturating_add(e.backing_stake);
 		if let Some(i_expo) = c.exposure.others.iter_mut().find(|i| i.who == nominator_address) {
-			i_expo.value = shrink(e.backing_stake);
+			i_expo.value = into_votes(e.backing_stake);
 		}
 	});
-	shrink(difference)
+	into_votes(difference)
 }

srml/staking/src/tests.rs

@@ -1638,24 +1638,23 @@ fn bond_with_no_staked_value() {
 	.minimum_validator_count(1)
 	.build(), || {
 		// setup
-		assert_ok!(Staking::chill(Origin::signed(30)));
 		assert_ok!(Staking::set_payee(Origin::signed(10), RewardDestination::Controller));
 		let _ = Balances::deposit_creating(&3, 1000);
 		let initial_balance_2 = Balances::free_balance(&2);
 		let initial_balance_4 = Balances::free_balance(&4);
 
 		// Stingy validator.
-		assert_ok!(Staking::bond(Origin::signed(1), 2, 0, RewardDestination::Controller));
+		assert_ok!(Staking::bond(Origin::signed(1), 2, 1, RewardDestination::Controller));
 		assert_ok!(Staking::validate(Origin::signed(2), ValidatorPrefs::default()));
 
 		System::set_block_number(1);
 		Session::check_rotate_session(System::block_number());
 
 		// Not elected even though we want 3.
-		assert_eq_uvec!(Session::validators(), vec![20, 10]);
+		assert_eq_uvec!(Session::validators(), vec![30, 20, 10]);
 
-		// min of 10 and 20.
-		assert_eq!(Staking::slot_stake(), 1000);
+		// min of 10, 20 and 30 (30 got a payout into staking so it raised it from 1 to 11).
+		assert_eq!(Staking::slot_stake(), 11);
 
 		// let's make the stingy one elected.
 		assert_ok!(Staking::bond(Origin::signed(3), 4, 500, RewardDestination::Controller));
@@ -1670,9 +1669,9 @@ fn bond_with_no_staked_value() {
 
 		// Stingy one is selected
 		assert_eq_uvec!(Session::validators(), vec![20, 10, 2]);
-		assert_eq!(Staking::stakers(1), Exposure { own: 0, total: 500, others: vec![IndividualExposure { who: 3, value: 500}]});
+		assert_eq!(Staking::stakers(1), Exposure { own: 1, total: 501, others: vec![IndividualExposure { who: 3, value: 500}]});
 		// New slot stake.
-		assert_eq!(Staking::slot_stake(), 500);
+		assert_eq!(Staking::slot_stake(), 501);
 
 		// no rewards paid to 2 and 4 yet
 		assert_eq!(Balances::free_balance(&2), initial_balance_2);
@@ -1682,10 +1681,10 @@ fn bond_with_no_staked_value() {
 		Session::check_rotate_session(System::block_number());
 
 		let reward = Staking::current_session_reward();
-		// 2 will not get any reward
-		// 4 will get all the reward share
-		assert_eq!(Balances::free_balance(&2), initial_balance_2);
-		assert_eq!(Balances::free_balance(&4), initial_balance_4 + reward);
+		// 2 will not get a reward of only 1
+		// 4 will get the rest
+		assert_eq!(Balances::free_balance(&2), initial_balance_2 + 1);
+		assert_eq!(Balances::free_balance(&4), initial_balance_4 + reward - 1);
 	});
 }