CreateValidator self delegation must be at least one consensus power #8909
Conversation
…nimum MsgCreateValidator ValidateBasic requires a self delegation of at least one consensus power, this prevents a common, but hard to debug error
colin-axner
requested review from
aaronc,
alessio and
alexanderbez
as code owners
|
Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=bb4f407742694a8389e78fc9d62644a2 |
|
tested on a local backport to ensure it covers the error case described in the 768 issue linked above, here is the new error
The only tricky part left is the user needs to know to run |
No this could cause issues since existing genesis files might have gentx's which break this validate basic check. The error this avoids only occurs when all the delegated validators have below one consensus power. I'm pretty sure the validator is still set in the store but not added to the validator set until someone delegates at least one consensus power |
Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
There was a problem hiding this comment.
Suppose I want to create a validator but do not have enough tokens myself to be in top 125 (taking Hub's case as an example). I might still want to create a validator and then stay dormant initially with the hopes that future delegations bump me into the active validator set.
I believe this should be allowed, but it looks like this PR will limit people to only creating validators if they have enough funds on their own to join the validator set.
fedekunze
added
the
A:automerge
|
Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=d26ac41c304a04a4c2715b2c1b7c4338c72b1768 |
|
Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=b3bdcf8d4a12408eab285e539d13e350 |
colin-axner
removed
the
A:automerge
|
Looks like I have some tests and simulations to fix |
Construct successfuly MsgCreateValidator to use a self delegation which is greater than the PowerReduction
|
Fixed, lets wait to make sure sims pass |
|
Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=29821db0b1c23a41b1327a01d4d9f65abaf16090 |
|
Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=fb33cd2e33c340e9960045147d59dad8 |
|
Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=a4cd80dd1678d81ab2734ad5698a30d1fd1b9b52 |
|
Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=2b847503f47346158b25c8d0a4293c3b |
Codecov Report
@@ Coverage Diff @@
## master #8909 +/- ##
==========================================
+ Coverage 59.27% 59.29% +0.01%
==========================================
Files 571 572 +1
Lines 31827 31846 +19
==========================================
+ Hits 18867 18884 +17
- Misses 10757 10758 +1
- Partials 2203 2204 +1
|
|
Adding automerge, the test fixes are pretty straightforward. The only test failing seems to be unrelated since I see it failing on other prs |
colin-axner
added
the
A:automerge
|
Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=087403b7940216139ff84f31cc404afd969c0e36 |
|
@alessio do you know the sims are skipped? |
|
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. |
|
You bring up a good point @sunnya97. I do also agree that to be the most flexible here, we should apply the intent of minimum consensus token power validation at genesis validation. Otherwise, we don't apply any validation, i.e. the old behavior. Thoughts @colin-axner? |
|
Thanks for bringing this up @sunnya97. I agree with the proposal It looks like we can add that check here. If my understanding is correct, than one of either staking, genutil (or potentially a different module) must make validator set changes. Since the initial state relative to Does this sound right? |
|
Yes, that sounds correct @colin-axner |
|
Opened #8960 Turns out every test in the SDK that uses simapp, runs with an empty validator set. Fixing this involves changing default This is beyond my available bandwidth so I'll likely just put this information into an issue and hope someone from the SDK team picks it up |
<!-- The default pull request template is for types feat, fix, or refactor. For other templates, add one of the following parameters to the url: - template=docs.md - template=other.md --> ## Description Closes: #8961 SDK allows InitGenesis to return with an empty validator set. In practice, the error for an empty validator set gets thrown in tendermint. To fix this, * Add non-empty validator set check to the `mm.InitGenesis` function. This will break `simapp.Setup` because it relies on an empty validator set [#comment](#8909 (comment)). * Update `simapp.Setup` to use a single validator. * Fix failing tests (Most of them are keeper tests). <!-- Add a description of the changes that this PR introduces and the files that are the most critical to review. --> --- ### 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... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [x] added `!` to the type prefix if API or client breaking change - [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [x] provided a link to the relevant issue or specification - [x] 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) - [x] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [x] 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)
Description
closes: #8908
Inspired by 768 on gaia (and all the other folks who have faced this infamous error)
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.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes