From 0cd8d2c2cf3c05be517f16ac9323d1e78f0fe8eb Mon Sep 17 00:00:00 2001
From: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Date: Mon, 13 Jan 2020 16:48:00 +0100
Subject: [PATCH] Remove wrong assertion from phragmen (#4515)
* remove assertion
---
substrate/frame/nicks/src/lib.rs | 2 +-
substrate/frame/staking/src/lib.rs | 24 ++++--
substrate/primitives/phragmen/src/lib.rs | 30 ++-----
substrate/primitives/phragmen/src/mock.rs | 2 +-
substrate/primitives/phragmen/src/tests.rs | 91 +++++++++++++++++++++-
5 files changed, 117 insertions(+), 32 deletions(-)
diff --git a/substrate/frame/nicks/src/lib.rs b/substrate/frame/nicks/src/lib.rs
index de39736a5a0..1d33d12ae46 100644
--- a/substrate/frame/nicks/src/lib.rs
+++ b/substrate/frame/nicks/src/lib.rs
@@ -145,7 +145,7 @@ decl_module! {
fn set_name(origin, name: Vec<u8>) {
let sender = ensure_signed(origin)?;
- ensure!(name.len() >= T::MinLength::get(), Error::<T>::TooShort,);
+ ensure!(name.len() >= T::MinLength::get(), Error::<T>::TooShort);
ensure!(name.len() <= T::MaxLength::get(), Error::<T>::TooLong);
let deposit = if let Some((_, deposit)) = <NameOf<T>>::get(&sender) {
diff --git a/substrate/frame/staking/src/lib.rs b/substrate/frame/staking/src/lib.rs
index f2f3fdfb70d..a59aa5d0c8e 100644
--- a/substrate/frame/staking/src/lib.rs
+++ b/substrate/frame/staking/src/lib.rs
@@ -1528,18 +1528,30 @@ impl<T: Trait> Module<T> {
let mut slot_stake = BalanceOf::<T>::max_value();
for (c, s) in supports.into_iter() {
// build `struct exposure` from `support`
+ let mut others = Vec::new();
+ let mut own: BalanceOf<T> = Zero::zero();
+ let mut total: BalanceOf<T> = Zero::zero();
+ s.voters
+ .into_iter()
+ .map(|(who, value)| (who, to_balance(value)))
+ .for_each(|(who, value)| {
+ if who == c {
+ own = own.saturating_add(value);
+ } else {
+ others.push(IndividualExposure { who, value });
+ }
+ total = total.saturating_add(value);
+ });
let exposure = Exposure {
- own: to_balance(s.own),
+ own,
+ others,
// This might reasonably saturate and we cannot do much about it. The sum of
// someone's stake might exceed the balance type if they have the maximum amount
// of balance and receive some support. This is super unlikely to happen, yet
// we simulate it in some tests.
- total: to_balance(s.total),
- others: s.others
- .into_iter()
- .map(|(who, value)| IndividualExposure { who, value: to_balance(value) })
- .collect::<Vec<IndividualExposure<_, _>>>(),
+ total,
};
+
if exposure.total < slot_stake {
slot_stake = exposure.total;
}
diff --git a/substrate/primitives/phragmen/src/lib.rs b/substrate/primitives/phragmen/src/lib.rs
index d9712d6ffbd..c13654543da 100644
--- a/substrate/primitives/phragmen/src/lib.rs
+++ b/substrate/primitives/phragmen/src/lib.rs
@@ -118,14 +118,12 @@ pub struct PhragmenResult<AccountId> {
/// This, at the current version, resembles the `Exposure` defined in the staking SRML module, yet
/// they do not necessarily have to be the same.
#[derive(Default, RuntimeDebug)]
-#[cfg_attr(feature = "std", derive(serde::Serialize, serde::Deserialize))]
+#[cfg_attr(feature = "std", derive(serde::Serialize, serde::Deserialize, Eq, PartialEq))]
pub struct Support<AccountId> {
- /// The amount of support as the effect of self-vote.
- pub own: ExtendedBalance,
/// Total support.
pub total: ExtendedBalance,
/// Support from voters.
- pub others: Vec<PhragmenStakedAssignment<AccountId>>,
+ pub voters: Vec<PhragmenStakedAssignment<AccountId>>,
}
/// A linkage from a candidate and its [`Support`].
@@ -368,20 +366,8 @@ 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) {
- 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));
- }
+ support.voters.push((n.clone(), other_stake));
+ support.total = support.total.saturating_add(other_stake);
}
}
}
@@ -450,8 +436,8 @@ fn do_equalize<Balance, AccountId, C>(
let budget = to_votes(budget_balance);
// Nothing to do. This voter had nothing useful.
- // Defensive only. Assignment list should always be populated.
- if elected_edges.is_empty() { return 0; }
+ // Defensive only. Assignment list should always be populated. 1 might happen for self vote.
+ if elected_edges.is_empty() || elected_edges.len() == 1 { return 0; }
let stake_used = elected_edges
.iter()
@@ -492,7 +478,7 @@ fn do_equalize<Balance, AccountId, C>(
elected_edges.iter_mut().for_each(|e| {
if let Some(support) = support_map.get_mut(&e.0) {
support.total = support.total.saturating_sub(e.1);
- support.others.retain(|i_support| i_support.0 != *voter);
+ support.voters.retain(|i_support| i_support.0 != *voter);
}
e.1 = 0;
});
@@ -529,7 +515,7 @@ fn do_equalize<Balance, AccountId, C>(
.saturating_add(last_stake)
.saturating_sub(support.total);
support.total = support.total.saturating_add(e.1);
- support.others.push((voter.clone(), e.1));
+ support.voters.push((voter.clone(), e.1));
}
});
diff --git a/substrate/primitives/phragmen/src/mock.rs b/substrate/primitives/phragmen/src/mock.rs
index 32de65f0e60..59b8d4eb5c6 100644
--- a/substrate/primitives/phragmen/src/mock.rs
+++ b/substrate/primitives/phragmen/src/mock.rs
@@ -375,7 +375,7 @@ pub(crate) fn run_and_compare(
check_assignments(assignments);
}
-pub(crate) fn build_support_map<FS>(
+pub(crate) fn build_support_map_float<FS>(
result: &mut _PhragmenResult<AccountId>,
stake_of: FS,
) -> _SupportMap<AccountId>
diff --git a/substrate/primitives/phragmen/src/tests.rs b/substrate/primitives/phragmen/src/tests.rs
index f3974d07d3b..b182cfca05a 100644
--- a/substrate/primitives/phragmen/src/tests.rs
+++ b/substrate/primitives/phragmen/src/tests.rs
@@ -19,7 +19,7 @@
#![cfg(test)]
use crate::mock::*;
-use crate::{elect, PhragmenResult};
+use crate::{elect, PhragmenResult, PhragmenStakedAssignment, build_support_map, Support, equalize};
use substrate_test_utils::assert_eq_uvec;
use sp_runtime::Perbill;
@@ -46,7 +46,7 @@ fn float_phragmen_poc_works() {
]
);
- let mut support_map = build_support_map(&mut phragmen_result, &stake_of);
+ let mut support_map = build_support_map_float(&mut phragmen_result, &stake_of);
assert_eq!(
support_map.get(&2).unwrap(),
@@ -405,3 +405,90 @@ fn minimum_to_elect_is_respected() {
assert!(maybe_result.is_none());
}
+
+#[test]
+fn self_votes_should_be_kept() {
+ let candidates = vec![5, 10, 20, 30];
+ let voters = vec![
+ (5, vec![5]),
+ (10, vec![10]),
+ (20, vec![20]),
+ (1, vec![10, 20])
+ ];
+ let stake_of = create_stake_of(&[
+ (5, 5),
+ (10, 10),
+ (20, 20),
+ (1, 8),
+ ]);
+
+ let result = elect::<_, _, _, TestCurrencyToVote>(
+ 2,
+ 2,
+ candidates,
+ voters,
+ &stake_of,
+ ).unwrap();
+
+ assert_eq!(result.winners, vec![(20, 28), (10, 18)]);
+ assert_eq!(
+ result.assignments,
+ vec![
+ (10, vec![(10, Perbill::from_percent(100))]),
+ (20, vec![(20, Perbill::from_percent(100))]),
+ (1, vec![
+ (10, Perbill::from_percent(50)),
+ (20, Perbill::from_percent(50))
+ ]
+ )
+ ],
+ );
+
+ let mut supports = build_support_map::<
+ Balance,
+ AccountId,
+ _,
+ TestCurrencyToVote
+ >(
+ &result.winners.into_iter().map(|(who, _)| who).collect(),
+ &result.assignments,
+ &stake_of
+ );
+
+ assert_eq!(supports.get(&5u64), None);
+ assert_eq!(
+ supports.get(&10u64).unwrap(),
+ &Support { total: 14u128, voters: vec![(10u64, 10u128), (1u64, 4u128)] },
+ );
+ assert_eq!(
+ supports.get(&20u64).unwrap(),
+ &Support { total: 24u128, voters: vec![(20u64, 20u128), (1u64, 4u128)] },
+ );
+
+ let assignments = result.assignments;
+ let mut staked_assignments
+ : Vec<(AccountId, Vec<PhragmenStakedAssignment<AccountId>>)>
+ = Vec::with_capacity(assignments.len());
+ for (n, assignment) in assignments.iter() {
+ let mut staked_assignment
+ : Vec<PhragmenStakedAssignment<AccountId>>
+ = Vec::with_capacity(assignment.len());
+ let stake = stake_of(&n);
+ for (c, per_thing) in assignment.iter() {
+ let vote_stake = *per_thing * stake;
+ staked_assignment.push((c.clone(), vote_stake));
+ }
+ staked_assignments.push((n.clone(), staked_assignment));
+ }
+
+ equalize::<Balance, AccountId, TestCurrencyToVote, _>(staked_assignments, &mut supports, 0, 2usize, &stake_of);
+
+ assert_eq!(
+ supports.get(&10u64).unwrap(),
+ &Support { total: 18u128, voters: vec![(10u64, 10u128), (1u64, 8u128)] },
+ );
+ assert_eq!(
+ supports.get(&20u64).unwrap(),
+ &Support { total: 20u128, voters: vec![(20u64, 20u128)] },
+ );
+}
--
GitLab