substrate icon indicating copy to clipboard operation
substrate copied to clipboard

[Feature] StakeTracker P1 - VotersList

Open ruseinov opened this issue 2 years ago • 22 comments

Original issue: https://github.com/paritytech/substrate/issues/12719 Splitting up https://github.com/paritytech/substrate/pull/12744

polkadot companion: https://github.com/paritytech/polkadot/pull/6527

Terminology

Approval stake - a sum of all the nominator/nomination pools stakes that are backing this validator + self-stake.

Goals

The original goal of this series of PR's is to introduce a list of validators sorted by their approval stake, therefore eliminating the need for Max Validator Intentions parameter. This way we'll be able to look into top x validators in the list to get a set of electable ones. While doing that we also introduce a concept of EventListeners into Staking, that would allow us to hook into various staking methods acting upon emitted events. In this particular case this allows us to delegate the sorted list maintenance to a 3rd party EventListener.

Technical details

This PR introduces a new Staking-related abstraction called OnStakingUpdate that basically provides the ability to subscribe to certain events that Staking fires and act upon them.


/// A generic staking event listener.
/// Note that the interface is designed in a way that the events are fired post-action, so any
/// pre-action data that is needed needs to be passed to interface methods.
/// The rest of the data can be retrieved by using `StakingInterface`.
pub trait OnStakingUpdate<AccountId: Clone, Balance: Copy> {
	/// Fired when the stake amount of someone updates.
	///
	/// Also called when someone stakes for the first time. (TODO: is it? this is why we need unit
	/// tests for this pallet alone).
	///
	/// This is effectively any changes to the bond amount, such as bonding more funds, and
	/// unbonding.
	fn on_stake_update(who: &AccountId, prev_stake: Option<Stake<AccountId, Balance>>);
	/// Fired when someone sets their intention to nominate, either new, or existing one.
	fn on_nominator_update(who: &AccountId, prev_nominations: Vec<AccountId>);
	/// Fired when someone sets their intention to validate, either new, or existing one.
	fn on_validator_update(who: &AccountId);
	/// Fired when someone removes their intention to validate, either due to chill or nominating.
	fn on_validator_remove(who: &AccountId); // only fire this event when this is an actual Validator
	/// Fired when someone removes their intention to nominate, either due to chill or validating.
	fn on_nominator_remove(who: &AccountId, nominations: Vec<AccountId>); // only fire this if this is an actual Nominator
	/// fired when someone is fully unstaked.
	fn on_unstake(who: &AccountId); // -> basically `kill_stash`
}

The first event listener implementing this interface, also introduced in this PR, is called pallet-stake-tracker which currently takes care of maintaining VoterList (a sorted list of nominators and validator's self-stake), effectively moving this logic out of Staking. It's also supplying Staking with a read-only version of this list, which is currently implemented by adding some defensive checks into insert/updated/remove methods.

Some other things that were introduced in this PR include:

  1. Staking tests that verify the correct OnStakingUpdate events are being fired with the right arguments.
  2. A read-only wrapper for SortedListProvider, that stubs out update/insert/remove methods and only leaves unsafe methods to allow for easier benchmarking/testing/migrations.
  3. CurrencyToVote moved from frame_support to primitives/staking.

ruseinov avatar Jan 05 '23 14:01 ruseinov

@kianenigma This does not have dedicated tests for StakeTracker yet, but it runs Staking tests against StakeTracker and it all works. Separate unit-tests for tracker are going to be introduced, however I think we shall retain the Staking part of those tests too as Staking relies on the outcome of the VotersList.

There is one test that is failing: https://github.com/paritytech/substrate/pull/13079/files#diff-64c049b4e94a55bdeaf757c725cf7d1df623b754557af4ced157024a943c4703R5145 that I think has to be removed. As discussed in Frame channel before - we don't want to cater for un-decodables due to MaxNominations decrease, this should be accompanied by a migration. And it simplifies the code by quite a bit.

ruseinov avatar Jan 05 '23 20:01 ruseinov

bot rebase

ruseinov avatar Jan 09 '23 14:01 ruseinov

Rebased

bot rebase

ruseinov avatar Jan 17 '23 09:01 ruseinov

Rebased

bot rebase

ruseinov avatar Jan 17 '23 09:01 ruseinov

Branch is already up-to-date

bot rebase

ruseinov avatar Jan 19 '23 08:01 ruseinov

Rebased

bot rebase

ruseinov avatar Jan 24 '23 09:01 ruseinov

Rebased

bot rebase

ruseinov avatar Feb 10 '23 13:02 ruseinov

Branch is already up-to-date

bot rebase

ruseinov avatar Feb 15 '23 10:02 ruseinov

Rebased

bot rebase

ruseinov avatar Feb 17 '23 11:02 ruseinov

Rebased

bot rebase

ruseinov avatar Feb 26 '23 23:02 ruseinov

Rebased

bot rebase

ruseinov avatar Mar 05 '23 19:03 ruseinov

Rebased

bot help

kianenigma avatar Mar 10 '23 14:03 kianenigma

Here's a link to docs

command-bot[bot] avatar Mar 10 '23 14:03 command-bot[bot]

bot rebase

ruseinov avatar Mar 11 '23 09:03 ruseinov

Rebased

bot rebase

ruseinov avatar Mar 13 '23 20:03 ruseinov

Branch is already up-to-date

bot rebase

ruseinov avatar Mar 18 '23 21:03 ruseinov

Rebased

The CI pipeline was cancelled due to failure one of the required jobs. Job name: test-linux-stable Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2548672

paritytech-cicd-pr avatar Mar 18 '23 21:03 paritytech-cicd-pr