Remove Self vote from phragmen (#4081)

* first draft of everything that works

* Some test fixes

substrate/core/phragmen/src/lib.rs

@@ -140,16 +140,17 @@ pub type SupportMap<A> = BTreeMap<A, Support<A>>;
 /// * `initial_candidates`: candidates list to be elected from.
 /// * `initial_voters`: voters list.
 /// * `stake_of`: something that can return the stake stake of a particular candidate or voter.
-/// * `self_vote`. If true, then each candidate will automatically vote for themselves with the a
-///   weight indicated by their stake. Note that when this is `true` candidates are filtered by
-/// having at least some backed stake from themselves.
+///
+/// This function does not strip out candidates who do not have any backing stake. It is the
+/// responsibility of the caller to make sure only those candidates who have a sensible economic
+/// value are passed in. From the perspective of this function, a candidate can easily be among the
+/// winner with no backing stake.
 pub fn elect<AccountId, Balance, FS, C>(
 	candidate_count: usize,
 	minimum_candidate_count: usize,
 	initial_candidates: Vec<AccountId>,
 	initial_voters: Vec<(AccountId, Vec<AccountId>)>,
 	stake_of: FS,
-	self_vote: bool,
 ) -> Option<PhragmenResult<AccountId>> where
 	AccountId: Default + Ord + Member,
 	Balance: Default + Copy + SimpleArithmetic,
@@ -169,36 +170,16 @@ pub fn elect<AccountId, Balance, FS, C>(
 	let num_voters = initial_candidates.len() + initial_voters.len();
 	let mut voters: Vec<Voter<AccountId>> = Vec::with_capacity(num_voters);
 
-	// collect candidates. self vote or filter might apply
-	let mut candidates = if self_vote {
-		// self vote. filter.
-		initial_candidates.into_iter().map(|who| {
-			let stake = stake_of(&who);
-			Candidate { who, approval_stake: to_votes(stake), ..Default::default() }
-		})
-		.filter(|c| !c.approval_stake.is_zero())
+	// Iterate once to create a cache of candidates indexes. This could be optimized by being
+	// provided by the call site.
+	let mut candidates = initial_candidates
+		.into_iter()
 		.enumerate()
-		.map(|(i, c)| {
-			voters.push(Voter {
-				who: c.who.clone(),
-				edges: vec![Edge { who: c.who.clone(), candidate_index: i, ..Default::default() }],
-				budget: c.approval_stake,
-				load: Rational128::zero(),
-			});
-			c_idx_cache.insert(c.who.clone(), i);
-			c
+		.map(|(idx, who)| {
+			c_idx_cache.insert(who.clone(), idx);
+			Candidate { who, ..Default::default() }
 		})
-		.collect::<Vec<Candidate<AccountId>>>()
-	} else {
-		// no self vote. just collect.
-		initial_candidates.into_iter()
-			.enumerate()
-			.map(|(idx, who)| {
-				c_idx_cache.insert(who.clone(), idx);
-				Candidate { who, ..Default::default() }
-			})
-			.collect::<Vec<Candidate<AccountId>>>()
-	};
+		.collect::<Vec<Candidate<AccountId>>>();
 
 	// early return if we don't have enough candidates
 	if candidates.len() < minimum_candidate_count { return None; }
@@ -290,37 +271,33 @@ pub fn elect<AccountId, Balance, FS, C>(
 	for n in &mut voters {
 		let mut assignment = (n.who.clone(), vec![]);
 		for e in &mut n.edges {
-			if let Some(c) = elected_candidates.iter().cloned().find(|(c, _)| *c == e.who) {
-				// if self_vote == false, this branch should always be executed as we want to
-				// collect all nominations
-				if c.0 != n.who || !self_vote  {
-					let per_bill_parts =
-					{
-						if n.load == e.load {
-							// Full support. No need to calculate.
-							Perbill::accuracy().into()
+			if elected_candidates.iter().position(|(ref c, _)| *c == e.who).is_some() {
+				let per_bill_parts =
+				{
+					if n.load == e.load {
+						// Full support. No need to calculate.
+						Perbill::accuracy().into()
+					} else {
+						if e.load.d() == n.load.d() {
+							// return e.load / n.load.
+							let desired_scale: u128 = Perbill::accuracy().into();
+							multiply_by_rational(
+								desired_scale,
+								e.load.n(),
+								n.load.n(),
+							).unwrap_or(Bounded::max_value())
 						} else {
-							if e.load.d() == n.load.d() {
-								// return e.load / n.load.
-								let desired_scale: u128 = Perbill::accuracy().into();
-								multiply_by_rational(
-									desired_scale,
-									e.load.n(),
-									n.load.n(),
-								).unwrap_or(Bounded::max_value())
-							} else {
-								// defensive only. Both edge and nominator loads are built from
-								// scores, hence MUST have the same denominator.
-								Zero::zero()
-							}
+							// defensive only. Both edge and nominator loads are built from
+							// scores, hence MUST have the same denominator.
+							Zero::zero()
 						}
-					};
-					// safer to .min() inside as well to argue as u32 is safe.
-					let per_thing = Perbill::from_parts(
-						per_bill_parts.min(Perbill::accuracy().into()) as u32
-					);
-					assignment.1.push((e.who.clone(), per_thing));
-				}
+					}
+				};
+				// safer to .min() inside as well to argue as u32 is safe.
+				let per_thing = Perbill::from_parts(
+					per_bill_parts.min(Perbill::accuracy().into()) as u32
+				);
+				assignment.1.push((e.who.clone(), per_thing));
 			}
 		}
 
@@ -368,7 +345,6 @@ pub fn build_support_map<Balance, AccountId, FS, C>(
 	elected_stashes: &Vec<AccountId>,
 	assignments: &Vec<(AccountId, Vec<PhragmenAssignment<AccountId>>)>,
 	stake_of: FS,
-	assume_self_vote: bool,
 ) -> SupportMap<AccountId> where
 	AccountId: Default + Ord + Member,
 	Balance: Default + Copy + SimpleArithmetic,
@@ -380,11 +356,7 @@ pub fn build_support_map<Balance, AccountId, FS, C>(
 	let mut supports = <SupportMap<AccountId>>::new();
 	elected_stashes
 		.iter()
-		.map(|e| (e, if assume_self_vote { to_votes(stake_of(e)) } else { Zero::zero() } ))
-		.for_each(|(e, s)| {
-			let item = Support { own: s, total: s, ..Default::default() };
-			supports.insert(e.clone(), item);
-		});
+		.for_each(|e| { supports.insert(e.clone(), Default::default()); });
 
 	// build support struct.
 	for (n, assignment) in assignments.iter() {
@@ -394,10 +366,20 @@ pub fn build_support_map<Balance, AccountId, FS, C>(
 			// per-things to be sound.
 			let other_stake = *per_thing * nominator_stake;
 			if let Some(support) = supports.get_mut(c) {
-				// For an astronomically rich validator with more astronomically rich
-				// set of nominators, this might saturate.
-				support.total = support.total.saturating_add(other_stake);
-				support.others.push((n.clone(), other_stake));
+				if c == n {
+					// This is a nomination from `n` to themselves. This will increase both the
+					// `own` and `total` field.
+					debug_assert!(*per_thing == Perbill::one()); // TODO: deal with this: do we want it?
+					support.own = support.own.saturating_add(other_stake);
+					support.total = support.total.saturating_add(other_stake);
+				} else {
+					// This is a nomination from `n` to someone else. Increase `total` and add an entry
+					// inside `others`.
+					// For an astronomically rich validator with more astronomically rich
+					// set of nominators, this might saturate.
+					support.total = support.total.saturating_add(other_stake);
+					support.others.push((n.clone(), other_stake));
+				}
 			}
 		}
 	}

substrate/core/phragmen/src/mock.rs

@@ -75,13 +75,16 @@ pub(crate) struct _PhragmenResult<A: Clone> {
 	pub assignments: Vec<(A, Vec<_PhragmenAssignment<A>>)>
 }
 
+pub(crate) fn auto_generate_self_voters<A: Clone>(candidates: &[A]) -> Vec<(A, Vec<A>)> {
+	candidates.iter().map(|c| (c.clone(), vec![c.clone()])).collect()
+}
+
 pub(crate) fn elect_float<A, FS>(
 	candidate_count: usize,
 	minimum_candidate_count: usize,
 	initial_candidates: Vec<A>,
 	initial_voters: Vec<(A, Vec<A>)>,
 	stake_of: FS,
-	self_vote: bool,
 ) -> Option<_PhragmenResult<A>> where
 	A: Default + Ord + Member + Copy,
 	for<'r> FS: Fn(&'r A) -> Balance,
@@ -92,36 +95,14 @@ pub(crate) fn elect_float<A, FS>(
 	let num_voters = initial_candidates.len() + initial_voters.len();
 	let mut voters: Vec<_Voter<A>> = Vec::with_capacity(num_voters);
 
-	let mut candidates = if self_vote {
-		initial_candidates.into_iter().map(|who| {
-			let stake = stake_of(&who) as f64;
-			_Candidate { who, approval_stake: stake, ..Default::default() }
-		})
-		.filter(|c| c.approval_stake != 0f64)
+	let mut candidates = initial_candidates
+		.into_iter()
 		.enumerate()
-		.map(|(i, c)| {
-			let who = c.who;
-			voters.push(_Voter {
-				who: who.clone(),
-				edges: vec![
-					_Edge { who: who.clone(), candidate_index: i, ..Default::default() }
-				],
-				budget: c.approval_stake,
-				load: 0f64,
-			});
-			c_idx_cache.insert(c.who.clone(), i);
-			c
+		.map(|(idx, who)| {
+			c_idx_cache.insert(who.clone(), idx);
+			_Candidate { who, ..Default::default() }
 		})
-		.collect::<Vec<_Candidate<A>>>()
-	} else {
-		initial_candidates.into_iter()
-			.enumerate()
-			.map(|(idx, who)| {
-				c_idx_cache.insert(who.clone(), idx);
-				_Candidate { who, ..Default::default() }
-			})
-			.collect::<Vec<_Candidate<A>>>()
-	};
+		.collect::<Vec<_Candidate<A>>>();
 
 	if candidates.len() < minimum_candidate_count {
 		return None;
@@ -359,7 +340,6 @@ pub(crate) fn run_and_compare(
 	stake_of: Box<dyn Fn(&AccountId) -> Balance>,
 	to_elect: usize,
 	min_to_elect: usize,
-	self_vote: bool,
 ) {
 	// run fixed point code.
 	let PhragmenResult { winners, assignments } = elect::<_, _, _, TestCurrencyToVote>(
@@ -368,7 +348,6 @@ pub(crate) fn run_and_compare(
 		candidates.clone(),
 		voters.clone(),
 		&stake_of,
-		self_vote,
 	).unwrap();
 
 	// run float poc code.
@@ -378,7 +357,6 @@ pub(crate) fn run_and_compare(
 		candidates,
 		voters,
 		&stake_of,
-		self_vote,
 	).unwrap();
 
 	assert_eq!(winners, truth_value.winners);

substrate/core/phragmen/src/tests.rs

@@ -32,7 +32,7 @@ fn float_phragmen_poc_works() {
 		(30, vec![2, 3]),
 	];
 	let stake_of = create_stake_of(&[(10, 10), (20, 20), (30, 30), (1, 0), (2, 0), (3, 0)]);
-	let mut phragmen_result = elect_float(2, 2, candidates, voters, &stake_of, false).unwrap();
+	let mut phragmen_result = elect_float(2, 2, candidates, voters, &stake_of).unwrap();
 	let winners = phragmen_result.clone().winners;
 	let assignments = phragmen_result.clone().assignments;
 
@@ -84,7 +84,6 @@ fn phragmen_poc_works() {
 		candidates,
 		voters,
 		create_stake_of(&[(10, 10), (20, 20), (30, 30)]),
-		false,
 	).unwrap();
 
 	assert_eq_uvec!(winners, vec![(2, 40), (3, 50)]);
@@ -114,7 +113,7 @@ fn phragmen_poc_2_works() {
 		(4, 500),
 	]);
 
-	run_and_compare(candidates, voters, stake_of, 2, 2, true);
+	run_and_compare(candidates, voters, stake_of, 2, 2);
 }
 
 #[test]
@@ -132,7 +131,7 @@ fn phragmen_poc_3_works() {
 		(4, 1000),
 	]);
 
-	run_and_compare(candidates, voters, stake_of, 2, 2, true);
+	run_and_compare(candidates, voters, stake_of, 2, 2);
 }
 
 #[test]
@@ -151,24 +150,24 @@ fn phragmen_accuracy_on_large_scale_only_validators() {
 	let PhragmenResult { winners, assignments } = elect::<_, _, _, TestCurrencyToVote>(
 		2,
 		2,
-		candidates,
-		vec![],
+		candidates.clone(),
+		auto_generate_self_voters(&candidates),
 		stake_of,
-		true,
 	).unwrap();
 
 	assert_eq_uvec!(winners, vec![(1, 18446744073709551614u128), (5, 18446744073709551613u128)]);
-	assert_eq!(assignments.len(), 0);
+	assert_eq!(assignments.len(), 2);
 	check_assignments(assignments);
 }
 
 #[test]
 fn phragmen_accuracy_on_large_scale_validators_and_nominators() {
 	let candidates = vec![1, 2, 3, 4, 5];
-	let voters = vec![
+	let mut voters = vec![
 		(13, vec![1, 3, 5]),
 		(14, vec![2, 4]),
 	];
+	voters.extend(auto_generate_self_voters(&candidates));
 	let stake_of = create_stake_of(&[
 		(1,  (u64::max_value() - 1).into()),
 		(2,  (u64::max_value() - 4).into()),
@@ -185,13 +184,17 @@ fn phragmen_accuracy_on_large_scale_validators_and_nominators() {
 		candidates,
 		voters,
 		stake_of,
-		true,
 	).unwrap();
 
 	assert_eq_uvec!(winners, vec![(2, 36893488147419103226u128), (1, 36893488147419103219u128)]);
 	assert_eq!(
 		assignments,
-		vec![(13, vec![(1, Perbill::one())]), (14, vec![(2, Perbill::one())])]
+		vec![
+			(13, vec![(1, Perbill::one())]),
+			(14, vec![(2, Perbill::one())]),
+			(1, vec![(1, Perbill::one())]),
+			(2, vec![(2, Perbill::one())]),
+		]
 	);
 	check_assignments(assignments);
 }
@@ -199,7 +202,7 @@ fn phragmen_accuracy_on_large_scale_validators_and_nominators() {
 #[test]
 fn phragmen_accuracy_on_small_scale_self_vote() {
 	let candidates = vec![40, 10, 20, 30];
-	let voters = vec![];
+	let voters = auto_generate_self_voters(&candidates);
 	let stake_of = create_stake_of(&[
 		(40, 0),
 		(10, 1),
@@ -213,7 +216,6 @@ fn phragmen_accuracy_on_small_scale_self_vote() {
 		candidates,
 		voters,
 		stake_of,
-		true,
 	).unwrap();
 
 	assert_eq_uvec!(winners, vec![(20, 2), (10, 1), (30, 1)]);
@@ -245,7 +247,6 @@ fn phragmen_accuracy_on_small_scale_no_self_vote() {
 		candidates,
 		voters,
 		stake_of,
-		false,
 	).unwrap();
 
 	assert_eq_uvec!(winners, vec![(20, 2), (10, 1), (30, 1)]);
@@ -254,9 +255,10 @@ fn phragmen_accuracy_on_small_scale_no_self_vote() {
 #[test]
 fn phragmen_large_scale_test() {
 	let candidates = vec![2, 4, 6, 8, 10, 12, 14, 16 ,18, 20, 22, 24];
-	let voters = vec![
+	let mut voters = vec![
 		(50, vec![2, 4, 6, 8, 10, 12, 14, 16 ,18, 20, 22, 24]),
 	];
+	voters.extend(auto_generate_self_voters(&candidates));
 	let stake_of = create_stake_of(&[
 		(2,  1),
 		(4,  100),
@@ -279,7 +281,6 @@ fn phragmen_large_scale_test() {
 		candidates,
 		voters,
 		stake_of,
-		true,
 	).unwrap();
 
 	assert_eq_uvec!(winners, vec![(24, 1490000000000200000u128), (22, 1490000000000100000u128)]);
@@ -292,7 +293,8 @@ fn phragmen_large_scale_test_2() {
 	let c_budget: u64 = 4_000_000;
 
 	let candidates = vec![2, 4];
-	let voters = vec![(50, vec![2, 4])];
+	let mut voters = vec![(50, vec![2, 4])];
+	voters.extend(auto_generate_self_voters(&candidates));
 
 	let stake_of = create_stake_of(&[
 		(2,  c_budget.into()),
@@ -306,13 +308,16 @@ fn phragmen_large_scale_test_2() {
 		candidates,
 		voters,
 		stake_of,
-		true,
 	).unwrap();
 
 	assert_eq_uvec!(winners, vec![(2, 1000000000004000000u128), (4, 1000000000004000000u128)]);
 	assert_eq!(
 		assignments,
-		vec![(50, vec![(2, Perbill::from_parts(500000001)), (4, Perbill::from_parts(499999999))])],
+		vec![
+			(50, vec![(2, Perbill::from_parts(500000001)), (4, Perbill::from_parts(499999999))]),
+			(2, vec![(2, Perbill::one())]),
+			(4, vec![(4, Perbill::one())]),
+		],
 	);
 	check_assignments(assignments);
 }
@@ -347,5 +352,5 @@ fn phragmen_linear_equalize() {
 		(130, 1000),
 	]);
 
-	run_and_compare(candidates, voters, stake_of, 2, 2, true);
+	run_and_compare(candidates, voters, stake_of, 2, 2);
 }

substrate/srml/elections-phragmen/src/lib.rs

@@ -542,7 +542,6 @@ impl<T: Trait> Module<T> {
 			candidates,
 			voters_and_votes,
 			Self::locked_stake_of,
-			false,
 		);
 
 		let mut to_release_bond: Vec<T::AccountId> = Vec::with_capacity(desired_seats);
@@ -563,7 +562,6 @@ impl<T: Trait> Module<T> {
 				&new_set,
 				&phragmen_result.assignments,
 				Self::locked_stake_of,
-				false,
 			);
 
 			let to_balance = |e: ExtendedBalance|

substrate/srml/staking/src/lib.rs

@@ -1265,13 +1265,21 @@ impl<T: Trait> Module<T> {
 	///
 	/// Returns the new `SlotStake` value and a set of newly selected _stash_ IDs.
 	fn select_validators() -> (BalanceOf<T>, Option<Vec<T::AccountId>>) {
+		let mut all_nominators: Vec<(T::AccountId, Vec<T::AccountId>)> = Vec::new();
+		let all_validator_candidates_iter = <Validators<T>>::enumerate();
+		let all_validators = all_validator_candidates_iter.map(|(who, _pref)| {
+			let self_vote = (who.clone(), vec![who.clone()]);
+			all_nominators.push(self_vote);
+			who
+		}).collect::<Vec<T::AccountId>>();
+		all_nominators.extend(<Nominators<T>>::enumerate());
+
 		let maybe_phragmen_result = elect::<_, _, _, T::CurrencyToVote>(
 			Self::validator_count() as usize,
 			Self::minimum_validator_count().max(1) as usize,
-			<Validators<T>>::enumerate().map(|(who, _)| who).collect::<Vec<T::AccountId>>(),
-			<Nominators<T>>::enumerate().collect(),
+			all_validators,
+			all_nominators,
 			Self::slashable_balance_of,
-			true,
 		);
 
 		if let Some(phragmen_result) = maybe_phragmen_result {
@@ -1289,7 +1297,6 @@ impl<T: Trait> Module<T> {
 				&elected_stashes,
 				&assignments,
 				Self::slashable_balance_of,
-				true,
 			);
 
 			if cfg!(feature = "equalize") {
@@ -1300,6 +1307,13 @@ impl<T: Trait> Module<T> {
 					let mut staked_assignment
 						: Vec<PhragmenStakedAssignment<T::AccountId>>
 						= Vec::with_capacity(assignment.len());
+
+					// If this is a self vote, then we don't need to equalise it at all. While the
+					// staking system does not allow nomination and validation at the same time,
+					// this must always be 100% support.
+					if assignment.len() == 1 && assignment[0].0 == *n {
+						continue;
+					}
 					for (c, per_thing) in assignment.iter() {
 						let nominator_stake = to_votes(Self::slashable_balance_of(n));
 						let other_stake = *per_thing * nominator_stake;

substrate/srml/treasury/src/lib.rs

@@ -579,7 +579,7 @@ mod tests {
 			<Treasury as OnFinalize<u64>>::on_finalize(2);
 			assert_eq!(Treasury::pot(), 100); // Pot hasn't changed
 
-			Balances::deposit_into_existing(&Treasury::account_id(), 100);
+			let _ = Balances::deposit_into_existing(&Treasury::account_id(), 100).unwrap();
 			<Treasury as OnFinalize<u64>>::on_finalize(4);
 			assert_eq!(Balances::free_balance(&3), 150); // Fund has been spent
 			assert_eq!(Treasury::pot(), 25); // Pot has finally changed