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

Allow for a dynamic number of nominators #10340

Closed
wants to merge 11 commits into from

Conversation

kianenigma
Copy link
Contributor

@kianenigma kianenigma commented Nov 22, 2021

This PR adds two flexible pieces of logic to pallet-staking:

  1. The number of votes that each nominator can cast can be configurable now. A simple implementation would use a constant value for this. A fancier implementation would make this a function of the amount of stake.

  2. The amount of voters that is taken for the election snapshot is now dynamic over two axis. A SnapshotBound is defined as a cap on both the count and byte-size of the snapshot. The snapshot creation will, if bounded, stop whenever either of the bounds are met.

Breaking Changes

  • MAX_NOMINATIONS is removed from the API of pallet-staking and is replaced by NominationQuota. To replicate the same behaviour, use pallet_staking::FixedNomintionQuota<OLD_VALUE>.
const OLD_MAX_NOMINATIONS: u32 = 16;
impl pallet_staking::Config {
  ... 
  type NominationQuota = pallet_staking::FixedNomintionQuota<OLD_MAX_NOMINATIONS >;
}
  • VoterSnapshotPerBlock is removed. Two replacements are introduced, VoterSnapshotBounds and TargetSnapshotBounds. To replicate exactly the old behaviour, VoterSnapshotBounds should be SnapshotBounds::new_count(OLD_VALUE) and TargetSnapshotBounds should be SnapshotBounds::new_unbounded(). Note that this is only safe when the number of validators in staking is well bounded (MaxValidatorsCount is set to some small value). As seen in this PR (node/runtime/src/lib.rs), it is recommended to replace this with a small sane bound.

@kianenigma kianenigma added the A3-in_progress Pull request is in progress. No review needed at this stage. label Nov 22, 2021
@kianenigma
Copy link
Contributor Author

For the sake of prosperity, I hereby confirm that commit 2f629a9 which is not properly signed was made by me.

/// Essentially, draws a line between `(MinBalance, MinQuota)` and `(MaxBalance, MaxQuota)`, Also,
/// values less than `MinBalance` have `MinQuota` and values higher than `MaxBalance` have
/// `MaxQuota`.
pub struct LinearNominationQuota<MinBalance, MaxBalance, const MAX_QUOTA: u32, const MIN_QUOTA: u32>(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I won't test this further yet because I am not sure if we are going to use it.

/// Whilst doing this, [`size`] will track the entire size of the `Vec<Voter>`, except for the
/// length prefix of the outer `Vec`. To get the final size at any point, use
/// [`final_byte_size_of`].
pub(crate) struct StaticSizeTracker<AccountId> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would make this fully generic and reusable, but it is not really feasible given the nested vec.

@kianenigma kianenigma marked this pull request as ready for review November 30, 2021 07:52
@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 Nov 30, 2021
@kianenigma kianenigma added B7-runtimenoteworthy 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 Nov 30, 2021
@jonasW3F
Copy link

Based on the discussion in the element group, I'd like to summarize my points on this. First, I think reducing the number of nominations per nominator is generally a reasonable approach if we can trade those for increasing the number of total nominators in the short run. But, I think it is crucial to not reduce those too far, because for the following reasons:

  1. The most important reason is that we want our nominators to participate in decentralizing the network, which means that they should seek out the best validators to fit their preferences and include them in their favorite set. Also, we would like them to include inactive validators because of the mentioned reason. Reducing the nominations too much would jeopardize this. Especially including inactive validators becomes increasingly costly which favors large operators and reduces healthy turnover in the active set.
  2. Nominators could frequently miss out on rewards if the number of nominations becomes too low, which would cause frustration.
  3. I think users of the network generally enjoy the freedom of choice that NPoS gives them and it is an important feature that distinguishes us from other networks that restrict the choices of their stakers.
  4. Once we obtain better scaling solutions for staking, sacrificing on the first three mentioned arguments would hurt the network in the long run.

Having said that, I think there is still room to reduce the nominations (currently 16 in Polkadot) to free space for more nominators. As mentioned in the group, I'd propose a rule of setting the minimum number of nominations to 6-8 and giving another nomination per 50k DOT in the stash. That would provide sufficient freedom to smaller stakers as well as for larger token holders.

This strategy of 6 + 1 every 50k (and cap at 32) would give, based on some recent era data, a maximum of 111801 nominations. If we assume that nominators use their free slots similarly as currently (64%), that would bring down the number of nominations to 72k. If we compare that to the current number of 184954 nominations, that's a good gain.

@kianenigma
Copy link
Contributor Author

I'm not quite sure about the prospect of this, so marking it is on ice for now until I know more, or I will temporarily close it.

@kianenigma kianenigma added A1-onice and removed A0-please_review Pull request needs code review. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Jan 3, 2022
@@ -439,3 +443,173 @@ impl<
sp_npos_elections::phragmms(winners, targets, voters, Balancing::get())
}
}

/// The limits imposed on a snapshot, either voters or targets.
#[derive(Clone, Copy, RuntimeDebug, scale_info::TypeInfo, Encode, Decode)]
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 a good option to be backported back to master in either case.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Apr 6, 2022
@paritytech paritytech deleted a comment from stale bot Apr 6, 2022
@stale stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Apr 6, 2022
@stale
Copy link

stale bot commented May 6, 2022

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label May 6, 2022
@kianenigma
Copy link
Contributor Author

YES

@stale stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label May 6, 2022
@kianenigma
Copy link
Contributor Author

too stale, giving up for now

@kianenigma kianenigma closed this May 19, 2022
@kianenigma kianenigma reopened this Jul 6, 2022
@kianenigma
Copy link
Contributor Author

kianenigma commented Jul 6, 2022

This is back from the grave as per a discussion with @xlc, which led us to re-discover this as a useful feature. I think it is a great feature to add, and we can later let the governance decide what the range of nomination should be. We can and should deploy this initially in a backwards compatible way, i.e. with FixedNomintionQuota.

@kianenigma kianenigma closed this Jul 13, 2022
@gpestana
Copy link
Contributor

gpestana commented Dec 12, 2022

I think the logic and design of the PR look good, but the git history has diverged quite significantly from the current master. My plans to revive this PR are to cut a new branch from master and:

  • cherry-pick and resolve conflicts of (all in this PR)
    • 12872c79ed2d26b460952da6035fcb3d002609ec
    • f9a27f1b7809434d6bf653aea1021a4a76ee26aa
    • 9844424cd49eb84d95d878070dffa7e0edbbb8e4
    • 7b4e7a8f3a3f34a302785f7dcf0085d69b3b1488
  • redesign the exhausts_size_count_non_zero api to make sure that the caller passes given_size and given_count in the correct order
  • update benchmarks
  • revisit tests
  • open companion PRs

cc @kianenigma

@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/the-future-of-polkadot-staking/1848/6

@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/the-future-of-polkadot-staking/1848/7

@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/the-future-of-polkadot-staking/1848/8

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
C1-low PR touches the given topic and has a low impact on builders.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants