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: Removal of Mandatory Self-Delegation Reward #2984

Merged
merged 10 commits into from
Dec 4, 2018

Conversation

alexanderbez
Copy link
Contributor

@alexanderbez alexanderbez commented Dec 3, 2018

closes: #2914


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

@alexanderbez alexanderbez added wip C:x/distribution distribution module related labels Dec 3, 2018
@alexanderbez alexanderbez changed the title WIP: Mandatory Self-Delegation Reward Withdraw Removal WIP: Removal of Mandatory Self-Delegation Reward Withdraw Dec 3, 2018
@codecov
Copy link

codecov bot commented Dec 3, 2018

Codecov Report

Merging #2984 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #2984   +/-   ##
========================================
  Coverage    55.55%   55.55%           
========================================
  Files          120      120           
  Lines         8490     8490           
========================================
  Hits          4717     4717           
  Misses        3451     3451           
  Partials       322      322

@alexanderbez alexanderbez changed the title WIP: Removal of Mandatory Self-Delegation Reward Withdraw WIP: Removal of Mandatory Self-Delegation Reward Dec 3, 2018
@alexanderbez alexanderbez reopened this Dec 3, 2018
@alexanderbez
Copy link
Contributor Author

alexanderbez commented Dec 3, 2018

With #2982, the test_sim_gaia_simulation_after_import stage passes.

@rigelrozanski lmk if this is what you had in mind. I'm not sure if any documentation needs to be updated?

@alexanderbez alexanderbez changed the title WIP: Removal of Mandatory Self-Delegation Reward R4R: Removal of Mandatory Self-Delegation Reward Dec 3, 2018
@jackzampolin jackzampolin mentioned this pull request Dec 3, 2018
5 tasks
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.

Biggest comment is need to investigate these failing tests - few other herein... thanks! :)

EDIT - should merge in the bug fixes from #2982 to this branch before merge to confirm the fixes

@@ -116,7 +125,6 @@ func (h Hooks) OnDelegationCreated(ctx sdk.Context, delAddr sdk.AccAddress, valA
h.k.onDelegationCreated(ctx, delAddr, valAddr)
Copy link
Contributor

Choose a reason for hiding this comment

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

The new hook route should also be called within the OnDelegationCreated hook here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there is a need now as I just moved the new call into onValidatorModified.

if bytes.Equal(delAddr.Bytes(), valAddr.Bytes()) {
// On updates to a self bond/unbond, we must update the validator's dist
// info without withdrawing any rewards.
k.UpdateValidatorDistInfoFromPool(ctx, valAddr)
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't need to be exposed (should keep consistency)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! Updated.

But consistency with what exactly? Everything in x/distribution/keeper/validator.go is exposed (which is where this lives). I wasn't able to find a consistent pattern for these methods. I think a large bulk of them should be unexposed.

if bytes.Equal(delAddr.Bytes(), valAddr.Bytes()) {
// On updates to a self bond/unbond, we must update the validator's dist
// info without withdrawing any rewards.
k.UpdateValidatorDistInfoFromPool(ctx, valAddr)
Copy link
Contributor

Choose a reason for hiding this comment

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

Also if you look at what's happening within onValidatorModified you'll see we skip running this at height of 0 - don't see why we wouldn't replicate this for the new function too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ditto: I don't think there is a need now as I just moved the new call into onValidatorModified.

x/distribution/keeper/validator.go Show resolved Hide resolved
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.

This doesn't fix the problem - onValidatorModified is still called when another delegator (not the validator themself) changes the amount of their delegation, and onValidatorModified withdraws the validator's rewards.

I think we need to change onValidatorModified - when any delegation is updated, we should not need to withdraw rewards for any other delegation, we just need to move fees from the global pool to the validator-specific pool.

@alexanderbez
Copy link
Contributor Author

@cwgoes I'm slightly confused. My interpretation was that this is only to be done when a validator bonds/unbonds their self-delegation. I guess I misunderstood the scope of the original issue. I will update.

@alexanderbez
Copy link
Contributor Author

@cwgoes @rigelrozanski updated.

@cwgoes
Copy link
Contributor

cwgoes commented Dec 4, 2018

This looks good to me. Additional testcases would be nice, if only to clarify exactly what will happen in various scenarios.

I still think we should go through the exact hooks here again, maybe on a call, and make sure we're only doing the minimum amount of computation necessary. That doesn't need to happen in this PR; this PR just needs to fix the immediate issue, which I think it does.

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.

Yeah this makes sense that we can avoid the validator withdrawal in all hook situations as you've done. nice!

@rigelrozanski rigelrozanski merged commit 6395e2a into develop Dec 4, 2018
@rigelrozanski rigelrozanski deleted the bez/2914-val-withdraw-self-del branch December 4, 2018 22:54
mircea-c pushed a commit that referenced this pull request Dec 5, 2018
* Some minor cleanup and reformatting to make things easier to understand

* Update onDelegationSharesModified hook

* Add pending log

* Address PR comments

* Fix linting
jaekwon added a commit that referenced this pull request Dec 8, 2018
Fixes regression introduced by #2984.
Continuiation of #3033 , which didn't fix the simulation issues.
(candidate) Complete solution for #3019, 9002 halt bug.

From #2984, it isn't sufficient to take the fee pool rewards of a validator. Since we don't track delegator accums (as we do with validator accums), and because onValidatorModified >updateValidatorDistInfoFromPool is also being called upon delegation updates (or at least I believe this is the reason), it is necessary to also withdraw self delegation.

TODO: I don't think self-delegation should be required to be modified here... consider using a delegation hook to do the self-delegation withdraw part instead, e.g. splitting the updateValidatorDistInfoFromPool function into two. It might not result in cleaner code, however. Think hard.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/distribution distribution module related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interaction of mandatory reward-withdraw and reward truncation
4 participants