Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

[NPoS] Implements dynamic number of nominators #12970

Merged
merged 96 commits into from
Aug 10, 2023

Conversation

gpestana
Copy link
Contributor

@gpestana gpestana commented Dec 19, 2022

This PR refactors the implementation of the ElectionDataProvider bounds and implements a dynamic nomination quota per voter, based on the account's balance and an arbitrary curve.

Instead of a fixed MAX_NOMINATIONS per nominator, the ElectionDataProvider uses an implementor of trait NominationsQuota<Balance> to calculate the max votes per nominator. Since the number of votes per nominator is dynamic, the election data provider (pallet staking) keeps track of the size of the snapshot to ensure that the final snapshot size is bounded, even when the number of votes per voter is calculated on the fly. This PR implements an ElectionSizeTracker to achieve this goal.

The FixedNominationsQuota<const MAX: u32> struct implements the nomination quota trait and can be used to set the maximum number of nominations per nominator to 16, which is the current behaviour.

Lazy nomination quota checks: Note that if a nominator's quota decreases after the nomination has been done, all the nominations will be used. The nominations quota limits is only enacted when an account performs a new nomination, in which case, if the number of nomination exceeds the current nominator quota, the fn nominate extrinsic fails with TooManyTargets.

Notable changes:

Trait NominationsQuota

An implementor of NominationsQuota returns the maximum number of votes per nominator. FixedNominationsQuota implements an instance of a fixed number of nominations, similar to the current implementation when set to 16.

ElectionsBounds and DataProviderBounds

The MaxElectableTargets and TargetsBound are replaced by ElectionBounds. ElectionsBounds defines the count and size limits of both voters and targets for an election.

The struct ElectionBounds sets the upper bounds in size (MB) and number of voters/targets to request from the election data provider. The bounds are defined over two axis: number of elements and size (in MB) of the snapshot that is built from the election result.

Struct ElectionSizeTracker

Keeps track of the amount of data (in size and number of elements) that an election data provider prepares in get_npos_voters and get_npos_targets. The election data provider uses an instance of ElectionSizeTracker to know when the number of voters exceeded the bounds in terms of size and return the election data to EPM.

Changes to the ElectionDataProvider interface

Both fn ElectionDataProvider::election_voters and fn ElectionDataProvider::electable_targets take as an input an instance of DataProviderBounds, instead of an optional max in the of number of returned elements.

New events

  • SnapshotVotersSizeExceeded: emitted when the election snapshot was exhausted due to the voters size limit.
  • SnapshotTargetsSizeExceeded: emitted when the election snapshot was exhausted due to the targets size limit.

Runtime API

This PR also adds a new runtime API to allow clients to query the nominations quota for a given balance.


polkadot companion: paritytech/polkadot#6807

Related to and reviving this PR and discussion #10340

Helps #13069
Closes #12968

@gpestana gpestana added A3-in_progress Pull request is in progress. No review needed at this stage. B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Dec 19, 2022
@gpestana gpestana self-assigned this Dec 19, 2022
@gpestana gpestana marked this pull request as draft December 19, 2022 11:01
@github-actions github-actions bot 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. labels Dec 19, 2022
@gpestana gpestana changed the title Allow for a dynamic number of nominators Allow for a dynamic number of nominators (v2) Dec 19, 2022
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Looks in the right direction so far.

@gpestana gpestana requested a review from kianenigma December 27, 2022 21:29
@gpestana gpestana marked this pull request as ready for review December 27, 2022 21:29
@gpestana
Copy link
Contributor Author

gpestana commented Aug 5, 2023

bot fmt

@gpestana
Copy link
Contributor Author

gpestana commented Aug 7, 2023

bot rebase

1 similar comment
@gpestana
Copy link
Contributor Author

gpestana commented Aug 8, 2023

bot rebase

@gpestana
Copy link
Contributor Author

gpestana commented Aug 8, 2023

bot fmt

@gpestana
Copy link
Contributor Author

gpestana commented Aug 8, 2023

bot clean

@command-bot command-bot bot deleted a comment from paritytech-processbot bot Aug 8, 2023
@command-bot command-bot bot deleted a comment from paritytech-processbot bot Aug 8, 2023
@command-bot command-bot bot deleted a comment from paritytech-processbot bot Aug 8, 2023
@command-bot command-bot bot deleted a comment from paritytech-processbot bot Aug 8, 2023
@command-bot command-bot bot deleted a comment from paritytech-processbot bot Aug 8, 2023
@command-bot command-bot bot deleted a comment from paritytech-processbot bot Aug 8, 2023
@gpestana
Copy link
Contributor Author

gpestana commented Aug 9, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@gpestana
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 57d2e53 into master Aug 10, 2023
@paritytech-processbot paritytech-processbot bot deleted the gpestana/staking-dynamic-nominators branch August 10, 2023 07:45
@@ -5120,50 +5297,55 @@ fn min_commission_works() {
}

#[test]
fn change_of_max_nominations() {
#[should_panic]
Copy link
Member

@arkpar arkpar Aug 12, 2023

Choose a reason for hiding this comment

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

Is this intended? If so, Is this test supposed to panic at the last assertion?

@arkpar
Copy link
Member

arkpar commented Aug 12, 2023

cargo test --release -p pallet-staking fails for me after this PR. Apparently some tests are supposed to panic with debug_assert which only works in debug config.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

Allow for a dynamic number of nominators