Phragmén Validator Election #1915

kianenigma · 2019-03-04T11:22:19Z

Replaces the naive validator selection in the staking srml, as explained in #1772.

The old approach was: Validators are chosen merely by sorting their staked values. If anyone nominated, then it would have been automatically assigned to its first vote. This PR entirely removes this logic. The new algorithm requires nominators, otherwise, previous generation validators are chosen naively.

TODOs:

Complete test cases: leftovers of Stash/controller model for staking #1782 + a few ones dedicated to the new logic. Special attention should be paid to have test-cases that demonstrate the aforementioned goals of the module.
(OPTIONAL) Improve readability and castings between BalanceOf<T> and other numeric types if possible.

Come up to a conclusion about some edge cases @gavofyork @shawntabrizi :

Should a validator candidate with no vote be considered?
- Current status: NO. validators are filtered by having approval_stake >= 0 before the first round of the election, where approval_stake is the sum of stakes coming from votes.
- Is a vote with stake == 0 completely null and useless? if yes, then the above point is correct. If no, then the above point is already incorrect since we are currently treating a validator with no votes and a validator with tons of zero-votes the same => both are filtered out. Correct.
What's the expected behavior when the number of validator candidates (after being filtered by approval_stake) is less than ValidatorCount<T>?
- Current status: All of them are elected without any computation. Correct.
The same condition as above + the number of possible candidates is less than ValidatorCount<T> and MinimumValidatorCount<T>?
- Current status: Nothing special. This should most likely change and some emergency condition be triggered (e.g. Emit an event to other modules and stop processing blocks?). Correct: Keep the previous set and do nothing further.

parity-cla-bot · 2019-03-04T11:22:22Z

It looks like @kianenigma signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

core/sr-primitives/src/lib.rs

node/executor/src/lib.rs

srml/staking/src/lib.rs

gavofyork · 2019-03-07T15:12:42Z

srml/staking/src/lib.rs

@@ -43,6 +43,47 @@ const DEFAULT_MINIMUM_VALIDATOR_COUNT: u32 = 4;
 const MAX_NOMINATIONS: usize = 16;
 const MAX_UNSTAKE_THRESHOLD: u32 = 10;

+// a wrapper around validation candidates list and some metadata needed for election process.


Phragmén specific data structures should be in their own module along with the algorithm.

Maybe check the new function signature etc. and see if it is ok? as suggested uses closures, a bit verbose but works fine.

srml/staking/src/mock.rs

kianenigma · 2019-03-13T09:48:10Z

I've added one final test to check the internal scores of the algorithm and they seem to be in sync with the same config executed in python (e.g. this and this). I believe that based on the info available (reference + discussion here) this PR is good to go.

gavofyork · 2019-03-13T19:12:12Z

srml/staking/src/lib.rs

 		);
-
+
+		<SlotStake<T>>::put(&slot_stake);


any reason why this was moved down here?

While automatic merges probably. Will move it back up in the final merge.

gavofyork · 2019-03-13T19:16:12Z

srml/staking/src/phragmen.rs

+		minimum_validator_count: usize,
+	) -> Vec<Candidate<T::AccountId, BalanceOf<T>>> where
+		FR: Fn() -> usize,
+		FV: Fn() -> Box<dyn Iterator<


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

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.

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

bkchr

Some more stuff for a follow-up @kianenigma

bkchr · 2019-03-14T10:46:07Z

srml/staking/src/phragmen.rs

+		get_nominators: FN,
+		stash_of: FS,
+		minimum_validator_count: usize,
+	) -> Vec<Candidate<T::AccountId, BalanceOf<T>>> where


Suggested change

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

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

bkchr · 2019-03-14T10:46:22Z

srml/staking/src/phragmen.rs

+///
+/// @returns a vector of elected candidates
+pub fn elect<T: Trait + 'static, FR, FN, FV, FS>(
+		get_rounds: FR,


Please one tab here for all parameters.

bkchr · 2019-03-14T10:47:59Z

srml/staking/src/phragmen.rs

+///
+/// Reference implementation: https://github.com/w3f/consensus
+///
+/// @returns a vector of elected candidates


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

Or just Returns bla

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.

bkchr · 2019-03-14T10:51:18Z

node/executor/src/lib.rs

@@ -441,7 +445,13 @@ mod tests {
 			]
 		);

-		let digest = generic::Digest::<Log>::default();
+		// let mut digest = generic::Digest::<Log>::default();


No un-commented code please.

bkchr · 2019-03-14T10:51:37Z

node/executor/src/lib.rs

@@ -574,6 +584,14 @@ mod tests {
 					phase: Phase::Finalization,
 					event: Event::session(session::RawEvent::NewSession(1))
 				},
+				// EventRecord { // TODO: this might be wrong.


Same and no TODO without an issue in the code.

bkchr · 2019-03-14T11:43:50Z

srml/staking/src/phragmen.rs

+			}
+
+			elected_candidates.push(winner);
+


Suggested change

bkchr · 2019-03-14T11:45:13Z

srml/staking/src/phragmen.rs

+		FS: Fn(T::AccountId) -> BalanceOf<T>,
+{
+	let rounds = get_rounds();
+	let mut elected_candidates = vec![];


Suggested change

let mut elected_candidates = vec![];

let mut elected_candidates = Vec::with_capacity(rounds);

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.

bkchr · 2019-03-14T11:47:42Z

srml/staking/src/phragmen.rs

+					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 }


Suggested change

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

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

bkchr · 2019-03-14T11:48:20Z

srml/staking/src/phragmen.rs

+				// 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_()


Join these 3 lines.

bkchr · 2019-03-14T11:48:53Z

srml/staking/src/phragmen.rs

+					if let Some(c) = elected_candidates.iter_mut().find(|c| c.who == v.who) {
+						c.exposure.total += n.stake;
+						c.exposure.others.push(
+							IndividualExposure {who: n.who.clone(), value: n.stake }


Suggested change

IndividualExposure {who: n.who.clone(), value: n.stake }

IndividualExposure { who: n.who.clone(), value: n.stake }

kianenigma · 2019-03-14T13:05:04Z

@bkchr Thanks for the new review. All fixed here.
#1951

* adds first draft phragmen with tuned tests and sr-primitives * fix a few docs and code styles * clean, organize and finish remaining test cases * more and more tests * update edge cases behavior and relavent tests * fix global test issues * updated wasm files * all tests fixed * cleanup * fix some global issues * fix global tests * bump versions * fix typo * first step toward extracting phragmen * Fix most of the grumbles. * fix rest of the grumbles * spaces to tabs * update wasm * Removed nightly feature. * More tests * Fix broken tests. * Bump and update wasm.

@kianenigma

# Membership Request Hi, I am Kian Paimani, known as @kianenigma. I have been working on Polkadot/Kusama through Parity since February 2019 and I can categorize my main contributions to Polkadot's ecosystem as follows: 1. Maintaining and developing the staking sub-system. 2. General FRAME development, especially testing and quality assurance. 3. Polkadot-native side-projects. 4. Education > My first contribution to Polkadot is also indeed related to staking: paritytech/substrate#1915 ### Staking system I joke as the Polkadot staking to be both my blessing and my curse over the years. I started working on it since the first days that I joined this ecosystem and the work [is ongoing ever since](https://github.com/orgs/paritytech/projects/33/views/9). In the past, I focused on making sure that the staking system is secure and to some extent scalable. More recently, I coordinated the (imminent) launch of Nomination Pools. Nowadays I also put an extra effort on making sure that this sub-system of Polkadot is *sustainable*, through code refactor and educating other core developers. Lastly, I have been the main author of the [Polkadot staking newsletter](https://gist.github.com/kianenigma/aa835946455b9a3f167821b9d05ba376), which is my main attempt at making the entire complexity and development of this part of the protocol transparent to the end-users. I expect myself to contribute *directly* to the staking system for at least another ~12, if not more, and afterwards having the role of an advisor. Some notable contributions: - paritytech/substrate#4517 - paritytech/substrate#7910 - paritytech/substrate#6242 - paritytech/substrate#9415 - paritytech/polkadot#3141 - paritytech/substrate#11212 - paritytech/substrate#12129 ### FRAME Historically, I have contributed a variety of domains in FRAME, namely: - Early version of the weight system paritytech/substrate#3816 paritytech/substrate#3157 - Early version of the transaction fee system - Primitive arithmetic types paritytech/substrate#3456 - Council election pallet paritytech/substrate#3364 Many of which were, admittedly, a PoC at most, if not considered "poor". I am happy that nowadays many of the above have been refactored and are being maintained by new domain experts. These days, I put most of my FRAME focus on testing and quality assurance. Through my work in the staking system, I have had to deal with the high sensitivity and liveness requirement of protocol development first hand (I believe I had to do among the [very first storage migrations](paritytech/substrate#3948) in Kusama) and consequently I felt the need to make better testing facilities, all of which have been formulated in https://forum.polkadot.network/t/testing-complex-frame-pallets-discussion-tools/356. Some relevant PRs: - paritytech/substrate#8038 - paritytech/substrate#9788 - paritytech/substrate#10174 Regardless of wearing the staking hat, I plan to remain a direct contributor to FRAME, namely because I consider it to be an important requirements of successfully delivering more features to Polkadot's ecosystem. ### Polkadot-Native Side Projects I have started multiple small, mostly non-RUST projects in the polkadot ecosystem that I am very happy about, and I plan to continue doing so. I have not yet found the time to make a "polished product" out of any of these, but I hope that I can help foster our community such that someday a team will do so. I consider my role, for the time being, to *put ideas out there* through these side projects. - https://github.com/substrate-portfolio/polkadot-portfolio/ - https://github.com/kianenigma/polkadot-basic-notification/ - https://github.com/paritytech/polkadot-scripts/ - https://github.com/paritytech/substrate-debug-kit/ ### Education Lastly, aside from having had a number of educational talks over the years (all of which [are listed](https://hello.kianenigma.nl/talks/) in my personal website), I am a big enthusiast of the newly formed Polkadot Blockchain Academy. I have [been an instructor](https://singular.app/collectibles/statemine/16/2) in the first cohort, and continue to contribute for as long and as much as I can, whilst still attending to the former 3 duties. --- With all of that being said and done, I consider myself at the beginning of the path to Dan 4, but happy to start at a lower one as well.

@kianenigma

# Membership Request Hi, I am Kian Paimani, known as @kianenigma. I have been working on Polkadot/Kusama through Parity since February 2019 and I can categorize my main contributions to Polkadot's ecosystem as follows: 1. Maintaining and developing the staking sub-system. 2. General FRAME development, especially testing and quality assurance. 3. Polkadot-native side-projects. 4. Education > My first contribution to Polkadot is also indeed related to staking: paritytech/substrate#1915 ### Staking system I joke as the Polkadot staking to be both my blessing and my curse over the years. I started working on it since the first days that I joined this ecosystem and the work [is ongoing ever since](https://github.com/orgs/paritytech/projects/33/views/9). In the past, I focused on making sure that the staking system is secure and to some extent scalable. More recently, I coordinated the (imminent) launch of Nomination Pools. Nowadays I also put an extra effort on making sure that this sub-system of Polkadot is *sustainable*, through code refactor and educating other core developers. Lastly, I have been the main author of the [Polkadot staking newsletter](https://gist.github.com/kianenigma/aa835946455b9a3f167821b9d05ba376), which is my main attempt at making the entire complexity and development of this part of the protocol transparent to the end-users. I expect myself to contribute *directly* to the staking system for at least another ~12, if not more, and afterwards having the role of an advisor. Some notable contributions: - paritytech/substrate#4517 - paritytech/substrate#7910 - paritytech/substrate#6242 - paritytech/substrate#9415 - paritytech/polkadot#3141 - paritytech/substrate#11212 - paritytech/substrate#12129 ### FRAME Historically, I have contributed a variety of domains in FRAME, namely: - Early version of the weight system paritytech/substrate#3816 paritytech/substrate#3157 - Early version of the transaction fee system - Primitive arithmetic types paritytech/substrate#3456 - Council election pallet paritytech/substrate#3364 Many of which were, admittedly, a PoC at most, if not considered "poor". I am happy that nowadays many of the above have been refactored and are being maintained by new domain experts. These days, I put most of my FRAME focus on testing and quality assurance. Through my work in the staking system, I have had to deal with the high sensitivity and liveness requirement of protocol development first hand (I believe I had to do among the [very first storage migrations](paritytech/substrate#3948) in Kusama) and consequently I felt the need to make better testing facilities, all of which have been formulated in https://forum.polkadot.network/t/testing-complex-frame-pallets-discussion-tools/356. Some relevant PRs: - paritytech/substrate#8038 - paritytech/substrate#9788 - paritytech/substrate#10174 Regardless of wearing the staking hat, I plan to remain a direct contributor to FRAME, namely because I consider it to be an important requirements of successfully delivering more features to Polkadot's ecosystem. ### Polkadot-Native Side Projects I have started multiple small, mostly non-RUST projects in the polkadot ecosystem that I am very happy about, and I plan to continue doing so. I have not yet found the time to make a "polished product" out of any of these, but I hope that I can help foster our community such that someday a team will do so. I consider my role, for the time being, to *put ideas out there* through these side projects. - https://github.com/substrate-portfolio/polkadot-portfolio/ - https://github.com/kianenigma/polkadot-basic-notification/ - https://github.com/paritytech/polkadot-scripts/ - https://github.com/paritytech/substrate-debug-kit/ ### Education Lastly, aside from having had a number of educational talks over the years (all of which [are listed](https://hello.kianenigma.nl/talks/) in my personal website), I am a big enthusiast of the newly formed Polkadot Blockchain Academy. I have [been an instructor](https://singular.app/collectibles/statemine/16/2) in the first cohort, and continue to contribute for as long and as much as I can, whilst still attending to the former 3 duties. --- With all of that being said and done, I consider myself at the beginning of the path to Dan 4, but happy to start at a lower one as well. Co-authored-by: Bastian Köcher <git@kchr.de>

kianenigma added 2 commits March 4, 2019 11:43

adds first draft phragmen with tuned tests and sr-primitives

89417a1

fix a few docs and code styles

cf8157c

kianenigma added the A3-in_progress Pull request is in progress. No review needed at this stage. label Mar 4, 2019

This comment has been minimized.

Sign in to view

devops-parity added the A4-gotissues label Mar 4, 2019

kianenigma and others added 11 commits March 4, 2019 15:54

clean, organize and finish remaining test cases

b0b8e21

more and more tests

653456f

update edge cases behavior and relavent tests

ee38f86

fix global test issues

f969335

updated wasm files

2c02b54

merged and tests almost patched

89185ec

all tests fixed

baf8778

cleanup

cfaf7b9

fix some global issues

9408b95

fix global tests

b2973a3

bump versions

e9b5244

devops-parity added the B2-breaksapi label Mar 7, 2019

fix typo

cabdc5b

kianenigma requested review from bkchr and gavofyork March 7, 2019 11:36

kianenigma added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. A4-gotissues labels Mar 7, 2019