cosmos / cosmos-sdk

:chains: A Framework for Building High Value Public Blockchains :sparkles:
https://cosmos.network/
Apache License 2.0
6.17k stars 3.57k forks source link

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

Open sunnya97 opened 3 years ago

sunnya97 commented 3 years ago

Summary

Background: https://github.com/cosmos/cosmos-sdk/issues/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

amaury1093 commented 3 years ago

This change causes a consensus failure: https://github.com/cosmos/cosmos-sdk/issues/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 commented 3 years ago

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 commented 3 years ago

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

yihuang commented 3 years ago

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 commented 3 years ago

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 commented 2 years ago

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.

https://github.com/cosmos/cosmos-sdk/blob/a660eea7532f94fa23e7c4b8ad6eaa30ab546d14/x/staking/keeper/params.go#L42-L48

fedekunze commented 2 years ago

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 commented 2 years ago

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.