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

WIP: Fix Cliff Val Bug Caught in Simulation #2332

Closed
wants to merge 2 commits into from

Conversation

alexanderbez
Copy link
Contributor

@alexanderbez alexanderbez commented Sep 13, 2018

TODO:

  • Describe issue
  • Write unit test(s)

Note: $ make test_sim_gaia_fast will still fail due to #2224.

closes: #2289


  • 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.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added entries in PENDING.md with issue #

  • rereviewed Files changed in the github PR explorer


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 was newly bonded and has greater power
k.beginUnbondingValidator(ctx, oldCliffVal)
if bytes.Equal(validatorToBond.OperatorAddr, affectedValidator.OperatorAddr) {
if bytes.Compare(affectedValRank, oldCliffValRank) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

should these two if statement's want to be combined? (if enters the first but not the second this function won't move on to the else statement below - doesn't sound like the desired functionality)


Think we need to understand the reasons for this solution a bit more... (not sure if worth if for upcoming refactor though).. This fix doesn't really make sense to me

@alexanderbez
Copy link
Contributor Author

I actually want to close this. I don't fully understand what is happening and I do not feel comfortable with the changes in this PR. Ultimately this boils down to intra-block validator state transitions involving cliff validators, particularly of equal power. Once #2332 gets going, this will be obsolete anyway.

My gut tells me this is a very tricky corner-case and I would think we should just proceed with the release without some fix for this? Idk...

@rigelrozanski
Copy link
Contributor

I don't think we should proceed with the release without fixing this, but likely the fix should be made via the refactoring out of the cliff validator altogether

@alexanderbez alexanderbez deleted the bez/2289-yet-another-cliff-bugfix branch September 13, 2018 21:16
@alexanderbez
Copy link
Contributor Author

alexanderbez commented Sep 13, 2018

So do you recommend we start on #2332 asap?

@rigelrozanski
Copy link
Contributor

rigelrozanski commented Sep 13, 2018

Sorry which issue did you mean to point too? that's THIS issue. If you're referring to the Radical Refactor 🤙 then yes we need to work on that one ASAP - but will certainly add delay to the release if we block on it.

@alexanderbez
Copy link
Contributor Author

err, I meant #2312 , sorry.

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.

Randomized Fuzzer: cliff validator has not been changed, yet we bonded a new validator
2 participants