Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Turn staking Power Reduction into an on-chain param #8365

Open
3 of 4 tasks
sunnya97 opened this issue Jan 18, 2021 · 8 comments · Fixed by #8505
Open
3 of 4 tasks

Turn staking Power Reduction into an on-chain param #8365

sunnya97 opened this issue Jan 18, 2021 · 8 comments · Fixed by #8505

Comments

@sunnya97
Copy link
Member

sunnya97 commented Jan 18, 2021

Summary

Background: #7655

Currently, the power reduction variable in the staking module is defaulted to 10^6. While it is possible to change this in the app.go in the codebase, we would like to remove the reliance on global variables throughout the codebase. Instead, the power reduction should be an on-chain param.

Along with better code hygiene, by making PowerReduction an on-chain param instead of code variable, we would be able to update it on a running chain. This could be useful for things like asset redenominations, but also more advanced voting power distributions.

For example, you can use a dynamic PowerReduction to create a "less granular" voting power (i.e. if Validator A has a stake 54 tokens, and validator B has stake 103 tokens, the chain can dynamically set the power reduction to be 50, so validator A has voting power 1, and validator B has voting power 2.). This is useful for things like threshold cryptography and sharding.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@amaury1093
Copy link
Contributor

This change causes a consensus failure: #9447. The current decision is that we'll revert if for now, i.e. won't include it in v0.43.

Re-opening as to find a strategy to include this feature in v0.44.

@alexanderbez
Copy link
Contributor

alexanderbez commented Jun 11, 2021

IMHO keeping this as a tune-able variable is not the worst, although I do agree globals are the root of many evils.

Making this an on-chain param would require some non-trivial amount of effort and thought. We attempted it and saw consensus failures which is not surprising because validator power is crucial to validator iteration, specifically during computing and returning validator set updates to Tendermint during x/staking EndBlock.

If we were to make this on-chain, we'd probably need to utilize hooks and iterate over every single validator and re-compute and update their power based on the new param.

@aaronc
Copy link
Member

aaronc commented Jun 14, 2021

A middle ground which avoids the global is to make this a keeper member variable rather than a global var.

@yihuang
Copy link
Collaborator

yihuang commented Aug 16, 2021

If we were to make this on-chain, we'd probably need to utilize hooks and iterate over every single validator and re-compute and update their power based on the new param.

So I guess to sync the validator set with tendermint, we better to change the parameter through a message?

@alexanderbez
Copy link
Contributor

alexanderbez commented Aug 16, 2021

The problem isn't the how to tune the parameter (on-chain gov parameter makes total sense). The reason this was re-opened and non-trivial to solve is because you need to go into the x/staking store and modify all validator powers based on the new parameter value. In the current x/params context, this is pretty much impossible. Once we refactor the way modules handle parameters (i.e. stateful and can execute arbitrary logic before/after the param change), this should become pretty trivial.

@fedekunze
Copy link
Collaborator

fedekunze commented Nov 30, 2021

we def need this feature for Evmos, I just realized that the power reduction is hardcoded on the staking module and wanted to know timelines for this feature.

// PowerReduction - is the amount of staking tokens required for 1 unit of consensus-engine power.
// Currently, this returns a global variable that the app developer can tweak.
// TODO: we might turn this into an on-chain param:
// https://github.com/cosmos/cosmos-sdk/issues/8365
func (k Keeper) PowerReduction(ctx sdk.Context) sdk.Int {
return sdk.DefaultPowerReduction
}

@fedekunze
Copy link
Collaborator

fedekunze commented Nov 30, 2021

it would be relevant to test overflows with MaxTotalVotingPower on Tendermint

https://github.com/tendermint/tendermint/blob/99ee730ee77037f002503e0e833760a21a4bf5e7/types/validator_set.go#L18-L25

@alexanderbez
Copy link
Contributor

Feel free to submit a PR @fedekunze. The work is non-trivial. You can't simply make the value a param. In fact, we've already tried. You'll need to execute logic post-param-change that updates all validator powers in state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants