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

lower kusama staking limits #5000

Merged
merged 2 commits into from Mar 23, 2022
Merged

lower kusama staking limits #5000

merged 2 commits into from Mar 23, 2022

Conversation

kianenigma
Copy link
Contributor

@kianenigma kianenigma commented Feb 28, 2022

This PR is part of my effort to re-configure Kusama's staking system for long term safety, and prepare it for nomination pools.

Recall that the NPoS election process, in order to be both successful and safe, contains 4 main steps.

  1. Snapshot creation.
  2. Signed submissions from staking-miner.
  3. Unsigned submissions from validators, as backup.
  4. Solution verification.

Our experimentation has unraveled that, as it stands now, the bottleneck for the Kusama network (and Polkadot and Westend) is the third step, and the largest input data for which this step could be executed, in WASM, without running out of resources (weight, memory, block-length), is when there are at most around 12500 nominators in the system. Note that this number is expectedly lower than Polkadot’s 22500, because Kusama opted to allow for up to 24 nominations per nominator. Thus, we need to assume the worst case in our experimentation, namely that each nominator is indeed using all of their 24 nominations.

More information: https://docs.google.com/document/d/1aKWbaYnzyIp7mnWlQYccrQtGLOIuVw8SD277HZuR1vw/edit?usp=sharing

@kianenigma kianenigma added A0-please_review Pull request needs code review. B7-runtimenoteworthy D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Feb 28, 2022
@shawntabrizi shawntabrizi added the C1-low PR touches the given topic and has a low impact on builders. label Mar 1, 2022
@shawntabrizi
Copy link
Contributor

shawntabrizi commented Mar 1, 2022

Note that this number is expectedly lower than Polkadot’s 22500, because Kusama opted to allow for up to 24 nominations per nominator. Thus, we need to assume the worst case in our experimentation, namely that each nominator is indeed using all of their 24 nominations.

I think with this in mind, it is better to reduce this number from 24 than to reduce the voter snapshot per block.

The upside of having people nominate 24 people instead of 16 people seems way smaller than the upside of including another 10,000 people into the staking system calculation logic. I would even say to reduce the max nominations from 16 to something like 8 if we can basically double the number of participants as a result.

@bkchr
Copy link
Member

bkchr commented Mar 2, 2022

I'm seconding @shawntabrizi

@jonasW3F
Copy link

jonasW3F commented Mar 2, 2022

I am looking at the current situation on Kusama at session 19786.

number_degree

We can see that many nominators use the flexibility of choosing up to 24 validators. We can also see that many stick with 16, probably they haven't updated their nominations in a while. Another large group only has one degree.

The following graph plots the respective stakes (in % of all staked tokens) by grouping nominators into how many degrees they have.

bond_degree

From this graph we can see that those two groups (plus probably a few but very large stake-holders with 12 degrees) also have a large share of the total stake. In addition, nominators that are only having one degree seem to be smaller nominators who do not contribute much to the total stake.

As per our discussion around November, I am in favor of leaving the flexibility to nominators to choose a large amount of validators. Reducing to 16 seems fine on Kusama, but I wouldn't go lower, because this is an important tool to drive decentralization.

@kianenigma
Copy link
Contributor Author

One minor problem with going down from 24 to 16 will be that we need to give sufficient heads up for people to update their nominations, which might be tricky. If the maximum is set back to 16, all of those who are voting for more than 16 will no longer be treated as a nominator, and can also be chilled via chill_other.

Nonetheless, I am open to explore this path as well.

Copy link
Contributor

@emostov emostov left a comment

Choose a reason for hiding this comment

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

If we want to go down the path of 16 targets on Kusama I think that should be done as a follow up since it's more controversial and would force any nominators with more than 16 targets to change their strategy. Conversely, this current proposed change shouldn't require any intervention by active nominators and will improves safety by ensuring OCW (or any stage in the election process) won't OOM.

I don't see any reason to punt off this fix while the community does bikesheding on the ideal number of targets.

@kianenigma
Copy link
Contributor Author

kianenigma commented Mar 7, 2022

Indeed, and looking at the numbers again, Kusama is only about halfway into filling the proposed 12_500 limit, and with the current growth rate, it would be a long time until it is reached.

Also:

  • We also have PRs like Allow for a dynamic number of nominators substrate#10340 which could solve this problem in a more novel way. This system if fixed upper bound of nominations is in itself questionable.
  • With multi-block elections, we might not need any of this, and easily stick to "safe" number of voters per block rather than maxing in out to something more risky, simply because we can scale by increasing the number of blocks.

In the meantime though, I strongly advice we move forward with this.

Copy link
Contributor

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

We will use this as a stop gap until we lower the number of nominations per nominator.

/// very large value. Once the `BagsList` is in full motion, staking might open its door to many
/// more nominators, and this value should instead be what is a "safe" number (e.g. 22500).
pub const VoterSnapshotPerBlock: u32 = 22_500;
pub const VoterSnapshotPerBlock: u32 = 12_500;
Copy link
Contributor

Choose a reason for hiding this comment

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

But y u remove comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

comment was referring to the pre-bags-list world, we are passed that now :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah but what I mean was, it should still have a comment explaining why we have set it to be 12_500.

@kianenigma
Copy link
Contributor Author

Will leave this open for more feedback, or pushback, but ideally it whould be merged to be part of 9190 runtime.

@kianenigma
Copy link
Contributor Author

next release might branch off soon, so merging this.

@kianenigma
Copy link
Contributor Author

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot
Copy link

Merge cancelled due to error. Error: Checks failed for dbc487b

@kianenigma kianenigma merged commit 7960eb1 into master Mar 23, 2022
@kianenigma kianenigma deleted the kiz-lower-ksm-staking-limits branch March 23, 2022 10:48
@coderobe coderobe mentioned this pull request Apr 8, 2022
16 tasks
@jakoblell jakoblell removed the D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. label May 10, 2022
@jakoblell jakoblell added the D1-audited 👍 PR contains changes to critical logic that has been properly reviewed and externally audited. label May 10, 2022
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. C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to critical logic that has been properly reviewed and externally audited.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants