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

R4R: Fix broken invariant of bonded validator power decrease #2083

Merged
merged 4 commits into from
Aug 18, 2018

Conversation

alexanderbez
Copy link
Contributor

@alexanderbez alexanderbez commented Aug 17, 2018

  • Fix broken invariant when previously bonded validator decreases in power below cliff validator

closes: #2071


  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Updated all relevant documentation (docs/)
  • Updated all relevant code comments
  • Wrote tests
  • Added entries in PENDING.md that include links to the relevant issue or PR that most accurately describes the change.
  • Updated cmd/gaia and examples/

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

validator = k.bondValidator(ctx, validatorToBond)
if bytes.Equal(validator.Owner, affectedValidator.Owner) {
return validator, true
}

return affectedValidator, true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this is ideal, but in case line # 472 get executed, I need to make sure UpdateValidator set's the newly modified affectedValidator which should have it's status set to unbonded.

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems right

if !found {
panic(fmt.Sprintf("validator record not found for address: %v\n", oldCliffValidatorAddr))
}

k.unbondValidator(ctx, cliffVal)
affectedValRank := GetValidatorsByPowerIndexKey(affectedValidator, pool)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cwgoes lmk if this is analogous to what you had in your initial fix?

@alexanderbez alexanderbez changed the title Fix broken invariant of bonded validator power decrease WIP: Fix broken invariant of bonded validator power decrease Aug 17, 2018
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.

TestGetValidatorsEdgeCases is now failing too, maybe we should debug that before debugging the simulation.

PENDING.md Outdated
@@ -47,6 +47,7 @@ BUG FIXES
* Gaia CLI (`gaiacli`)

* Gaia
* [x/stake] [#2083] Fix broken invariant of bonded validator power decrease
Copy link
Contributor

Choose a reason for hiding this comment

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

Put this in CHANGELOG.md since we're merging into the release

validator = k.bondValidator(ctx, validatorToBond)
if bytes.Equal(validator.Owner, affectedValidator.Owner) {
return validator, true
}

return affectedValidator, true
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems right

@codecov
Copy link

codecov bot commented Aug 18, 2018

Codecov Report

Merging #2083 into release/v0.24.0 will increase coverage by 0.02%.
The diff coverage is 100%.

@@                 Coverage Diff                 @@
##           release/v0.24.0    #2083      +/-   ##
===================================================
+ Coverage            63.73%   63.75%   +0.02%     
===================================================
  Files                  113      113              
  Lines                 6667     6671       +4     
===================================================
+ Hits                  4249     4253       +4     
  Misses                2133     2133              
  Partials               285      285

@alexanderbez alexanderbez changed the title WIP: Fix broken invariant of bonded validator power decrease R4R: Fix broken invariant of bonded validator power decrease Aug 18, 2018
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, thanks!

@cwgoes cwgoes merged commit 5794f3c into release/v0.24.0 Aug 18, 2018
@cwgoes cwgoes deleted the bez/2071-fix-broken-cliff-val-invariant branch August 18, 2018 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants