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 F1 mechanism rounding fix #3788

Merged

Conversation

rigelrozanski
Copy link
Contributor

@rigelrozanski rigelrozanski commented Mar 2, 2019

REF #3750

due to order of operations F1 was actually calculating stake slightly differently than staking... see the diff in the code comments for more explanation

fixes problem found here:
#3750 (comment)

Also added a defensive minimum if rewards > outstanding (which could occur in the old code if everyone withdrew in the same block)

(feel free to push to this branch)

@rigelrozanski
Copy link
Contributor Author

unforch the a test case now fails due to the correction factor - so we should patch it however that's a bit ugly - a better solution here may be to fundamentally modify how F1 works so this form of rounding error doesn't show up... at least we know what's causing it.

feel free to push to dis branch @cwgoes

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.

A few questions, also this doesn't seem to fix the simulator.

x/distribution/keeper/delegation.go Outdated Show resolved Hide resolved
x/distribution/keeper/delegation.go Outdated Show resolved Hide resolved
x/distribution/keeper/delegation.go Outdated Show resolved Hide resolved
@jackzampolin jackzampolin added this to the v0.33.0 (Launch) milestone Mar 3, 2019
@cwgoes
Copy link
Contributor

cwgoes commented Mar 4, 2019

Simulations are failing due to division-by-zero on line 72 of x/stake/keeper/delegation.go:

  roundingCorrectionFactor := currentStake.QuoTruncate(currentStakeF1)

@cwgoes
Copy link
Contributor

cwgoes commented Mar 4, 2019

FYI @rigelrozanski - once the division by zero is removed, this fixes the simulator except for import/export - #3798 (comment).

I prefer the approach in #3788 (comment) though - and I even prefer just the MinSet safety check (which also fixes the sim) over applying this correction on every slash event, because I think applying this correction every time could lead to substantial reward underestimates.

@rigelrozanski
Copy link
Contributor Author

@cwgoes we're in agreement I will implement now

@codecov
Copy link

codecov bot commented Mar 4, 2019

Codecov Report

Merging #3788 into cwgoes/outstanding-per-validator will decrease coverage by 0.03%.
The diff coverage is 37.25%.

@@                         Coverage Diff                         @@
##           cwgoes/outstanding-per-validator   #3788      +/-   ##
===================================================================
- Coverage                             61.33%   61.3%   -0.04%     
===================================================================
  Files                                   189     189              
  Lines                                 14156   14167      +11     
===================================================================
+ Hits                                   8683    8685       +2     
- Misses                                 4913    4922       +9     
  Partials                                560     560

types/dec_coin.go Outdated 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.

Basic logic looks good; a few minor comments & the import/export sim still fails.

types/dec_coin.go Outdated Show resolved Hide resolved
PENDING.md Outdated Show resolved Hide resolved
x/staking/keeper/delegation.go Show resolved Hide resolved
x/distribution/keeper/delegation.go Outdated Show resolved Hide resolved
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Changes look reasonable to me and @cwgoes covers anything I'd have to add :)

@rigelrozanski
Copy link
Contributor Author

Alright so I finally got to the bottom of the bug seen in test_sim_gaia_simulation_after_import - NOTE that this bug was not introduced in this PR but in the parent PR cwgoes/outstanding-per-validator ( #3750)

see the solution in commit d9e5211

Basically what is happening is because we switched to storing outstanding per validator instead of per the whole network, when we initialized for zero-genesis-export we were reinitializing (resetting to zero) the distribution-validator record which held the scrap "decimal" amounts of outstanding coins. Normally these scraps are kept in the validator to be withdrawn later once they reach a full coin amount.

My solution just has these scraps donated to the community pool during zero-height export.

@cwgoes cwgoes dismissed alexanderbez’s stale review March 6, 2019 11:15

Changes addressed.

@cwgoes cwgoes merged commit afdac45 into cwgoes/outstanding-per-validator Mar 6, 2019
@cwgoes cwgoes mentioned this pull request Mar 6, 2019
@rigelrozanski rigelrozanski deleted the rigel/outstanding-per-validator2 branch March 6, 2019 19:03
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 C:x/staking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants