Turn staking power reduction into an on-chain param #8505

sunnya97 · 2021-02-03T14:43:46Z

closes: #8365

Copied description from issue:

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.

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

…duction change

…aram

…on_chain_param power reduction as on-chain param

codecov · 2021-02-03T14:55:39Z

Codecov Report

Merging #8505 (4b778ab) into master (04ab271) will decrease coverage by 0.18%.
The diff coverage is 37.50%.

@@            Coverage Diff             @@
##           master    #8505      +/-   ##
==========================================
- Coverage   59.00%   58.82%   -0.19%     
==========================================
  Files         577      580       +3     
  Lines       32481    32643     +162     
==========================================
+ Hits        19167    19203      +36     
- Misses      11052    11174     +122     
- Partials     2262     2266       +4

Impacted Files	Coverage Δ
x/gov/types/params.go	`4.54% <ø> (ø)`
x/staking/client/cli/tx.go	`61.11% <ø> (ø)`
x/staking/keeper/invariants.go	`50.49% <0.00%> (ø)`
x/staking/keeper/migrations.go	`50.00% <0.00%> (ø)`
x/staking/legacy/v040/keys.go	`0.00% <0.00%> (ø)`
x/staking/legacy/v040/types.go	`0.00% <0.00%> (ø)`
x/staking/legacy/v043/json.go	`36.98% <36.98%> (ø)`
types/staking.go	`50.00% <50.00%> (ø)`
x/staking/genesis.go	`55.90% <50.00%> (-4.73%)`	⬇️
x/staking/types/params.go	`44.87% <55.55%> (+1.39%)`	⬆️
... and 17 more

github-actions · 2021-03-21T00:13:26Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

alexanderbez · 2021-03-22T16:32:41Z

Each param change set has a validation function, e.g. https://github.com/cosmos/cosmos-sdk/blob/master/x/mint/types/params.go#L93

sunnya97 · 2021-03-22T17:04:48Z

@alexanderbez No, I mean that #8909 makes it such that a CreateValidator message with < 1 consensus power of self-bond fails the ValidateBasic of the MsgCreateValidator. But this can't be done now, because in this PR the MsgCreateValidator.ValidateBasic function doesn't have the ctx, and so can't know what the current power reduction is.

alexanderbez · 2021-03-22T17:08:07Z

Ahhh, yes, you're right. I think our only option is to remove it from the stateless ValidateBasic and add the check to the MsgCreateValidator message handler.

sunnya97 · 2021-03-22T17:14:30Z

Yeah, that's what I started doing, but actually I think that might be doubling down on the wrong solution. I wrote this on the #8909 (comment) PR.

I think always requiring a CreateValidator to have a minimum of 1 consensus power isn't correct. At the moment, power-reduction is really low, so it is fine. However, #8505 makes power reduction an on-chain param, with the intent that some use cases might want the number of tokens needed to get one consensus power gets really high. In these cases, we want validators to be able to declare the validator without needing to provide 1 full consensus power by themselves.

To solve the problem this was meant to tackle (which I agree is quite annoying an issue), I believe an error should be thrown during the Genesis validation. A genesis file to be valid, should require that the total consensus power of all the genesis validators is >0. This seems like a better place to solve this issue than requiring all CreateValidators to have 1 consensus power self-bond, even after genesis.

amaury1093

Code lgtm, but it seems like migrations (both JSON and in-place) are not handled correctly

CHANGELOG.md

x/staking/legacy/v040/migrate.go

x/staking/legacy/v043/store_test.go

sunnya97 · 2021-04-08T00:01:32Z

@AmauryM, fixed the migrations issues

sunnya97 · 2021-04-08T00:16:58Z

Still not sure why test-rosetta is failing though

clevinson · 2021-04-09T03:48:14Z

Maybe @jgimeno or @fdymylja have ideas?

fdymylja · 2021-04-09T07:36:49Z

If the PR breaks state then node data needs to be rebuilt.

To rebuild rosetta with new state:

make rosetta-data
it's gonna output a cosmos address and a validator address; take the cosmos address and put it in ./contrib/rosetta/configuration/bootstrap.json

after that you can run: make test-rosetta to check if it passes.

sunnya97 · 2021-04-10T23:24:52Z

Thanks @fdymylja!

addressed

sunnya97 · 2021-04-11T01:52:00Z

Oops, I forgot the squash and merge 🤦

Is there a way to fix that?

## Description Closes: #9447 This PR partially reverts #8505. Namely: - it removes PowerReduction as a staking on-chain param - however, it keeps #8505's API changes regarding adding a `powerReduction` function argument to staking functions. This allows us to rely less on global variables in said functions.  --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable)

xbuidler and others added 15 commits January 26, 2021 23:28

power reduction as on-chain param

fec543d

on-chain power reduction param conversion basic work

4f476d6

use on-chain params on tests + fix tests + add new tests for power re…

cb17fed

…duction change

comment update

86844b2

legacy genesis migration test and feedbacks

60c6c0a

remove dependency between min deposit tokens and power reduction

7b5cf61

Less function to not use default power reduction

1c1e67f

reduce power reduction var name

5acdc07

increase gas limit for redelegation only

2ce6d4b

Merge cosmos-sdk master

cc4ef50

Merge branch 'master' into issue#8365/staking_power_reduction_chain_p…

0e6b9a6

…aram

regen staking.pb.go

7130544

golang-ci lint

36bc29a

reduce legacy code

2a43a2d

Merge pull request #65 from psaradev/issue#8365/staking_power_reducti…

2c1ef71

…on_chain_param power reduction as on-chain param

sunnya97 added C:x/staking genesis-breaking C:x/params labels Feb 3, 2021

sunnya97 requested review from aaronc, alessio and alexanderbez as code owners February 3, 2021 14:43

github-actions bot added the stale label Mar 21, 2021

alessio removed the stale label Mar 21, 2021

alessio requested review from amaury1093, jgimeno, clevinson, ethanfrey and gsora March 21, 2021 00:39

sunnya97 mentioned this pull request Mar 22, 2021

CreateValidator self delegation must be at least one consensus power #8909

Merged

9 tasks

sunnya97 added this to the v0.43 milestone Apr 2, 2021

sunnya97 self-assigned this Apr 2, 2021

sunnya97 added 2 commits April 6, 2021 19:19

merged in master

05e2e45

merged master

ca34808

amaury1093 previously requested changes Apr 7, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

x/staking/legacy/v040/migrate.go Outdated Show resolved Hide resolved

x/staking/legacy/v043/store_test.go Show resolved Hide resolved

sunnya97 added 3 commits April 7, 2021 13:59

fix merge conflict

aee3ca8

Merge branch 'master' into powerreduction_param

61b608f

fixed migrations

04e483d

add nolint to pb.go files

8cf5fe0

sunnya97 added 3 commits April 9, 2021 13:32

fix rosetta-test

cd8186e

Merge branch 'master' into powerreduction_param

10b2b08

merged master

4b778ab

sunnya97 requested a review from amaury1093 April 10, 2021 23:25

sunnya97 merged commit a4c7fd7 into cosmos:master Apr 10, 2021

This was referenced Jun 9, 2021

Changing PowerReduction param causing network halt #9447

Closed

revert: Turn staking power reduction into an on-chain param #9495

Merged

LeCodeurDuDimanche mentioned this pull request Nov 24, 2021

Make the yield calculation use the newly introduced PowerReduction iov-one/starnamed#146

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Turn staking power reduction into an on-chain param #8505

Turn staking power reduction into an on-chain param #8505

sunnya97 commented Feb 3, 2021 •

edited

Loading

codecov bot commented Feb 3, 2021 •

edited

Loading

github-actions bot commented Mar 21, 2021

alexanderbez commented Mar 22, 2021

sunnya97 commented Mar 22, 2021

alexanderbez commented Mar 22, 2021 •

edited

Loading

sunnya97 commented Mar 22, 2021 •

edited

Loading

amaury1093 left a comment

sunnya97 commented Apr 8, 2021

sunnya97 commented Apr 8, 2021

clevinson commented Apr 9, 2021

fdymylja commented Apr 9, 2021 •

edited

Loading

sunnya97 commented Apr 10, 2021

sunnya97 commented Apr 11, 2021

Turn staking power reduction into an on-chain param #8505

Turn staking power reduction into an on-chain param #8505

Conversation

sunnya97 commented Feb 3, 2021 • edited Loading

codecov bot commented Feb 3, 2021 • edited Loading

Codecov Report

github-actions bot commented Mar 21, 2021

alexanderbez commented Mar 22, 2021

sunnya97 commented Mar 22, 2021

alexanderbez commented Mar 22, 2021 • edited Loading

sunnya97 commented Mar 22, 2021 • edited Loading

amaury1093 left a comment

Choose a reason for hiding this comment

sunnya97 commented Apr 8, 2021

sunnya97 commented Apr 8, 2021

clevinson commented Apr 9, 2021

fdymylja commented Apr 9, 2021 • edited Loading

sunnya97 commented Apr 10, 2021

sunnya97 commented Apr 11, 2021

sunnya97 commented Feb 3, 2021 •

edited

Loading

codecov bot commented Feb 3, 2021 •

edited

Loading

alexanderbez commented Mar 22, 2021 •

edited

Loading

sunnya97 commented Mar 22, 2021 •

edited

Loading

fdymylja commented Apr 9, 2021 •

edited

Loading