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

WIP: Automatic Validator count adjust with dampening #7231

Closed
wants to merge 10 commits into from
Closed

WIP: Automatic Validator count adjust with dampening #7231

wants to merge 10 commits into from

Conversation

g2udevelopment
Copy link
Contributor

@g2udevelopment g2udevelopment commented Sep 29, 2020

This PR build on top of the previous work of @shawntabrizi. It implements the features in paritytech/polkadot-sdk#470. The goal is to have an automatic adjustment of the validator count when a new era starts if certain conditions are matched.
It also adds a flag to turn on/off the use of the dampening function and it adds an extra MaxValidatorCount storage item.
On top of that I added mocks and a few tests, to test the basic functioning of the dampening function.

I think a few test could be added so it would be helpfull if someone can add them. Also validation of the rules is still needed.

@g2udevelopment g2udevelopment changed the title WIP: Validator dampening WIP: Automatic Validator count adjust with dampening Sep 29, 2020
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.

I disagree with the logic of validator count update being baked into staking.

With ValidatorCountAdjust , you allow a user of substrate to opt out of this feature, which is good. But what if someone wants to have some update function, like the one proposed for kusama, but slightly different? this won't be possible with your API.

As recommended, I think having a type ValidatorConvert: Convert<u32, u32>is best, but I am fine with any lgic that respects the flexibility of substrate and staking pallet and will not enforce this business of 40% on all chains.

With Convert: For a chain that wants to opt out, they will pass in an IdentityConvert which doesn't touch the count. It will always convert x to x. Kusama can have its own custom implementation, and an other chain is free to customize this.

You use the simple () or IdentityConvert for substrate-node, and make a polkadot companion which sets polkadot-runtime to the same IdentityConvert, and finally kusama-runtime to whatever is the requirements of kusama.

Let me know if you want more explanation on any of the above.

@kianenigma kianenigma added A4-gotissues B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Sep 29, 2020
@g2udevelopment
Copy link
Contributor Author

I disagree with the logic of validator count update being baked into staking.

With ValidatorCountAdjust , you allow a user of substrate to opt out of this feature, which is good. But what if someone wants to have some update function, like the one proposed for kusama, but slightly different? this won't be possible with your API.

As recommended, I think having a type ValidatorConvert: Convert<u32, u32>. For a chain that wants to opt out, they will pass in an IdentityConvert which doesn't touch the count. Kusama can have its own custom implementation, and an other chain is free to customize this.

Sorry I missed the conversation about baking in the function in staking in the first commits. I think this is a good suggestion.

So I will remove the boolean flag , add the Convert type and provide both an Identity function and the current implementation. Then the Identity Implementation will serve as the bool false in the current implementation.

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
@kianenigma
Copy link
Contributor

add the Convert type and provide both an Identity function.

Yes, and this is already implemented in substrate

/// A structure that performs identity conversion.
pub struct Identity;
impl<T> Convert<T, T> for Identity {
fn convert(a: T) -> T { a }
}

So use this for identity.

@gnunicorn gnunicorn added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Oct 19, 2020
@gnunicorn
Copy link
Contributor

Closed because it's been stale for a while.

@gnunicorn gnunicorn closed this Nov 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants