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

Allow Unjail of Non-Bonded Jailed Validator #6061

Merged
merged 11 commits into from
Apr 26, 2020
Merged

Conversation

alexanderbez
Copy link
Contributor

@alexanderbez alexanderbez commented Apr 23, 2020

Description

Allow a validator to immediately unjail due to being jailed for falling below their minimum self-delegation and never having been bonded (i.e. no signing info present), assuming they've now met their minimum self-delegation.

closes: #6004


For contributor use:

  • 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

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

// if the delegation is the operator of the validator and undelegating will decrease the validator's self delegation below their minimum
// trigger a jail validator
// If the delegation is the operator of the validator and undelegating will decrease the validator's
// self-delegation below their minimum, we jail the validator.
if isValidatorOperator && !validator.Jailed &&
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note there is no change here apart from keeping the code DRY and calling k.Jail instead of manually calling the private internal APIs.

// cannot be unjailed until out of jail
if ctx.BlockHeader().Time.Before(info.JailedUntil) {
return types.ErrValidatorJailed
if found {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The core change is here. We now allow a validator to unjail if no ValidatorSigningInfo is present. This should only ever be the case under the condition of the reported issue. Essentially, we allow a validator to unjail immediately (once they've met their minimum self-delegation).

@alexanderbez alexanderbez marked this pull request as ready for review April 24, 2020 14:52
Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, although it not feel like I have enough knowledge of the staking module internals to know whether there could be any unintended consequences.

@cwgoes cwgoes requested a review from rigelrozanski April 24, 2020 15:12
Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

I think this is fine, 👀 from @rigelrozanski would be good too.

@codecov
Copy link

codecov bot commented Apr 24, 2020

Codecov Report

Merging #6061 into master will increase coverage by 0.04%.
The diff coverage is 54.54%.

@@            Coverage Diff             @@
##           master    #6061      +/-   ##
==========================================
+ Coverage   54.35%   54.39%   +0.04%     
==========================================
  Files         430      430              
  Lines       26103    26106       +3     
==========================================
+ Hits        14187    14201      +14     
+ Misses      10937    10922      -15     
- Partials      979      983       +4     

Copy link
Contributor

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK - few comments. Good idea

x/staking/keeper/delegation.go Outdated Show resolved Hide resolved
// A validator that is jailed but has no ValidatorSigningInfo object signals
// that the validator was never bonded and must've been jailed due to falling
// below their minimum self-delegation. The validator can unjail at any point
// assuming they've now bonded above their minimum self-delegation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to have some form of context by which we can verify the reason for being jailed for situations like this. In the future it may be that there are other reasons that an unbonded validator may be jailed (and other privileges to being a validator "in waiting"). I could see the Validator Jailed bool field being replaced with a JailedStatus uint8 kind whereby the reason for being jailed could be specified (here, 0 value would probably indicate "not-jailed"). Anyways that's a breaking change, but I think it could be helpful.... probably for another PR or issue!

@alexanderbez alexanderbez added the A:automerge Automatically merge PR once all prerequisites pass. label Apr 26, 2020
@mergify mergify bot merged commit d90b2e7 into master Apr 26, 2020
@mergify mergify bot deleted the bez/6004-fix-unbond-unjail branch April 26, 2020 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:x/slashing C:x/staking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't unjail a validator which was jailed because of undelegating self delegation
4 participants