Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Changes from 1 commit
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
spaces to tabs
  • Loading branch information
kianenigma committed Mar 9, 2019
commit d3ad7b477528e4f373b0eb0f530a43314cb6a060
270 changes: 135 additions & 135 deletions srml/staking/src/phragmen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ pub struct Candidate<AccountId, Balance: HasCompact> {
// Accumulator of the stake of this candidate based on received votes.
approval_stake: Balance,
// Intermediary value used to sort candidates.
// See Phragmén reference implementation.
// See Phragmén reference implementation.
score: Perquintill,
}

Expand All @@ -46,7 +46,7 @@ pub struct Nominations<AccountId, Balance: HasCompact> {
// List of validators proposed by this nominator.
nominees: Vec<Vote<AccountId, Balance>>,
// the stake amount proposed by the nominator as a part of the vote.
// Same as `nom.budget` in Phragmén reference.
// Same as `nom.budget` in Phragmén reference.
stake: Balance,
// Incremented each time a nominee that this nominator voted for has been elected.
load: Perquintill,
Expand All @@ -71,137 +71,137 @@ pub struct Vote<AccountId, Balance: HasCompact> {
///
/// @returns a vector of elected candidates
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"@returns" Is that any special rustdoc syntax? I don't think so, maybe just add # Returns as heading.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or just Returns bla

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed but I am pretty sure I saw it somewhere else in substrate and assumed it is the de facto way for some reason. Will refactor all.

pub fn elect<T: Trait + 'static, FR, FN, FV, FS>(
get_rounds: FR,
get_validators: FV,
get_nominators: FN,
stash_of: FS,
minimum_validator_count: usize,
) -> Vec<Candidate<T::AccountId, BalanceOf<T>>> where
FR: Fn() -> usize,
FV: Fn() -> Box<dyn Iterator<
Item =(T::AccountId, ValidatorPrefs<BalanceOf<T>>)
>>,
FN: Fn() -> Box<dyn Iterator<
Item =(T::AccountId, Vec<T::AccountId>)
>>,
FS: Fn(T::AccountId) -> BalanceOf<T>,
get_rounds: FR,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please one tab here for all parameters.

get_validators: FV,
get_nominators: FN,
stash_of: FS,
minimum_validator_count: usize,
) -> Vec<Candidate<T::AccountId, BalanceOf<T>>> where
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
) -> Vec<Candidate<T::AccountId, BalanceOf<T>>> where
) -> Vec<Candidate<T::AccountId, BalanceOf<T>>> where

FR: Fn() -> usize,
FV: Fn() -> Box<dyn Iterator<
Copy link
Member

@gavofyork gavofyork Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Box<..> looks a bit nasty. any reason the iterators can't be passed in directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually the type returned by the .enumerate(); passing in the storage item as it via a closure led to an even more nasty type.

What looks cleaner but was not applied in favor of keeping the code in lib.rs as simple as possible is to pre-process everything in lib.rs and pass them in as a closure that returns e.g. a Vector of validators. Though, this is computationaly not optimal in my opinion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok - something to check for optimisation at a later date.

Item =(T::AccountId, ValidatorPrefs<BalanceOf<T>>)
>>,
FN: Fn() -> Box<dyn Iterator<
Item =(T::AccountId, Vec<T::AccountId>)
>>,
FS: Fn(T::AccountId) -> BalanceOf<T>,
{
let rounds = get_rounds();
let mut elected_candidates = vec![];
// 1- Pre-process candidates and place them in a container
let mut candidates = get_validators().map(|(who, _)| {
let stash_balance = stash_of(who.clone());
Candidate {
who,
approval_stake: BalanceOf::<T>::zero(),
score: Perquintill::zero(),
exposure: Exposure { total: stash_balance, own: stash_balance, others: vec![] },
}
}).collect::<Vec<Candidate<T::AccountId, BalanceOf<T>>>>();

// Just to be used when we are below minimum validator count
let original_candidates = candidates.clone();
// 2- Collect the nominators with the associated votes.
// Also collect approval stake along the way.
let mut nominations = get_nominators().map(|(who, nominees)| {
let nominator_stake = stash_of(who.clone());
for n in &nominees {
let rounds = get_rounds();
let mut elected_candidates = vec![];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let mut elected_candidates = vec![];
let mut elected_candidates = Vec::with_capacity(rounds);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, maybe it is better let mut elected_candidates;
And initiliaze the vector with with_capacity in the if branch. The else branch does not need allocation at all.

// 1- Pre-process candidates and place them in a container
let mut candidates = get_validators().map(|(who, _)| {
let stash_balance = stash_of(who.clone());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you make stash_of take a reference, you don't need to call clone.

Candidate {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Candidate {
			who,
			exposure: Exposure { total: stash_balance, own: stash_balance, others: vec![] },
      ..Default::default()
}

who,
approval_stake: BalanceOf::<T>::zero(),
score: Perquintill::zero(),
exposure: Exposure { total: stash_balance, own: stash_balance, others: vec![] },
}
}).collect::<Vec<Candidate<T::AccountId, BalanceOf<T>>>>();

// Just to be used when we are below minimum validator count
let original_candidates = candidates.clone();
// 2- Collect the nominators with the associated votes.
// Also collect approval stake along the way.
let mut nominations = get_nominators().map(|(who, nominees)| {
let nominator_stake = stash_of(who.clone());
for n in &nominees {
candidates.iter_mut().filter(|i| i.who == *n).for_each(|c| {
c.approval_stake += nominator_stake;
});
}

Nominations {
who,
nominees: nominees.into_iter()
.map(|n| Vote {who: n, load: Perquintill::zero(), backing_stake: BalanceOf::<T>::zero()})
.collect::<Vec<Vote<T::AccountId, BalanceOf<T>>>>(),
stake: nominator_stake,
load : Perquintill::zero(),
}
}).collect::<Vec<Nominations<T::AccountId, BalanceOf<T>>>>();
// 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 > BalanceOf::<T>::zero())
.collect::<Vec<Candidate<T::AccountId, BalanceOf<T>>>>();

// 4- If we have more candidates then needed, run Phragmén.
if candidates.len() > rounds {
// Main election loop
for _round in 0..rounds {
// Loop 1: initialize score
for nominaotion in &nominations {
for vote in &nominaotion.nominees {
let candidate = &vote.who;
if let Some(c) = candidates.iter_mut().find(|i| i.who == *candidate) {
let approval_stake = c.approval_stake;
c.score = Perquintill::from_xth(approval_stake.as_());
}
}
}
// Loop 2: increment score.
for nominaotion in &nominations {
for vote in &nominaotion.nominees {
let candidate = &vote.who;
if let Some(c) = candidates.iter_mut().find(|i| i.who == *candidate) {
let approval_stake = c.approval_stake;
let temp =
nominaotion.stake.as_()
* *nominaotion.load
/ approval_stake.as_();
c.score = Perquintill::from_quintillionths(*c.score + temp);
}
}
}

// Find the best
let (winner_index, _) = candidates.iter().enumerate().min_by_key(|&(_i, c)| *c.score)
.expect("candidates length is checked to be >0; qed");

// loop 3: update nominator and vote load
let winner = candidates.remove(winner_index);
for n in &mut nominations {
for v in &mut n.nominees {
if v.who == winner.who {
v.load =
Perquintill::from_quintillionths(
*winner.score
- *n.load
);
n.load = winner.score;
}
}
}

elected_candidates.push(winner);

} // end of all rounds

// 4.1- Update backing stake of candidates and nominators
for n in &mut nominations {
for v in &mut n.nominees {
// if the target of this vote is among the winners, otherwise let go.
if let Some(c) = elected_candidates.iter_mut().find(|c| c.who == v.who) {
v.backing_stake = <BalanceOf<T> as As<u64>>::sa(
n.stake.as_()
* *v.load
/ *n.load
);
c.exposure.total += v.backing_stake;
// Update IndividualExposure of those who nominated and their vote won
c.exposure.others.push(
IndividualExposure {who: n.who.clone(), value: v.backing_stake }
);
}
}
}
} else {
if candidates.len() > minimum_validator_count {
// if we don't have enough candidates, just choose all that have some vote.
elected_candidates = candidates;
}

Nominations {
who,
nominees: nominees.into_iter()
.map(|n| Vote {who: n, load: Perquintill::zero(), backing_stake: BalanceOf::<T>::zero()})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.map(|n| Vote {who: n, load: Perquintill::zero(), backing_stake: BalanceOf::<T>::zero()})
.map(|n| Vote { who: n, ..Default::default() })

.collect::<Vec<Vote<T::AccountId, BalanceOf<T>>>>(),
stake: nominator_stake,
load : Perquintill::zero(),
}
}).collect::<Vec<Nominations<T::AccountId, BalanceOf<T>>>>();
// 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 > BalanceOf::<T>::zero())
.collect::<Vec<Candidate<T::AccountId, BalanceOf<T>>>>();

// 4- If we have more candidates then needed, run Phragmén.
if candidates.len() > rounds {
// Main election loop
for _round in 0..rounds {
// Loop 1: initialize score
for nominaotion in &nominations {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for nominaotion in &nominations {
for nomination in &nominations {

for vote in &nominaotion.nominees {
let candidate = &vote.who;
if let Some(c) = candidates.iter_mut().find(|i| i.who == *candidate) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if let Some(c) = candidates.iter_mut().find(|i| i.who == *candidate) {
if let Some(c) = candidates.iter_mut().find(|i| i.who == vote.who) {

let approval_stake = c.approval_stake;
c.score = Perquintill::from_xth(approval_stake.as_());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
c.score = Perquintill::from_xth(approval_stake.as_());
c.score = Perquintill::from_xth(c.approval_stake.as_());

}
}
}
// Loop 2: increment score.
for nominaotion in &nominations {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for nominaotion in &nominations {
for nomination in &nominations {

for vote in &nominaotion.nominees {
let candidate = &vote.who;
if let Some(c) = candidates.iter_mut().find(|i| i.who == *candidate) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if let Some(c) = candidates.iter_mut().find(|i| i.who == *candidate) {
if let Some(c) = candidates.iter_mut().find(|i| i.who == vote.who) {

let approval_stake = c.approval_stake;
let temp =
nominaotion.stake.as_()
* *nominaotion.load
/ approval_stake.as_();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/ approval_stake.as_();
/ c.approval_stake.as_();

c.score = Perquintill::from_quintillionths(*c.score + temp);
}
}
}

// Find the best
let (winner_index, _) = candidates.iter().enumerate().min_by_key(|&(_i, c)| *c.score)
.expect("candidates length is checked to be >0; qed");

// loop 3: update nominator and vote load
let winner = candidates.remove(winner_index);
for n in &mut nominations {
for v in &mut n.nominees {
if v.who == winner.who {
v.load =
Perquintill::from_quintillionths(
*winner.score
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
*winner.score
*winner.score - *n.load

- *n.load
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- *n.load

);
n.load = winner.score;
}
}
}

elected_candidates.push(winner);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

} // end of all rounds

// 4.1- Update backing stake of candidates and nominators
for n in &mut nominations {
for v in &mut n.nominees {
// if the target of this vote is among the winners, otherwise let go.
if let Some(c) = elected_candidates.iter_mut().find(|c| c.who == v.who) {
v.backing_stake = <BalanceOf<T> as As<u64>>::sa(
n.stake.as_()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Join these 3 lines.

* *v.load
/ *n.load
);
c.exposure.total += v.backing_stake;
// Update IndividualExposure of those who nominated and their vote won
c.exposure.others.push(
IndividualExposure {who: n.who.clone(), value: v.backing_stake }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
IndividualExposure {who: n.who.clone(), value: v.backing_stake }
IndividualExposure { who: n.who.clone(), value: v.backing_stake }

);
}
}
}
} else {
if candidates.len() > minimum_validator_count {
// if we don't have enough candidates, just choose all that have some vote.
elected_candidates = candidates;
// `Exposure.others` still needs an update
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gavofyork this has room for debate. Current descision:

  • With code yellow where we have some votes but less than needed: exposure is updated.
  • With code red where we have some votes and less than Minimum: exposure is NOT updated. This is because technically in this scenario votes don't really matter. Though, note that Stakers<T> is cleaned outside of the functions so they will actually also NOT have even the previous era's exposure. An alternative is to keep the exposure that they used to have if this code red happened.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's fine for now. it's something we should put to the w3f researchers though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will post a detailed description there.

for n in &mut nominations {
for v in &mut n.nominees {
Expand All @@ -213,11 +213,11 @@ pub fn elect<T: Trait + 'static, FR, FN, FV, FS>(
}
}
}
} else {
// if we have less than minimum, use the previous validator set.
elected_candidates = original_candidates;
}
}
} else {
// if we have less than minimum, use the previous validator set.
elected_candidates = original_candidates;
}
}

elected_candidates
elected_candidates
}