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: Inflation bug fixes #2982

Merged
merged 13 commits into from
Dec 4, 2018
Merged

R4R: Inflation bug fixes #2982

merged 13 commits into from
Dec 4, 2018

Conversation

cwgoes
Copy link
Contributor

@cwgoes cwgoes commented Dec 3, 2018

Closes #2967

Changes:

  • Tokens are minted for the previous block before distributing rewards
  • Inflation rate is now recalculated each block instead of each hour
  • For zero-height export, delegation withdrawal infos / validator withdrawal infos are not deleted but instead reset to height 0

Standard checklist:

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

@cwgoes cwgoes added wip C:x/distribution distribution module related C:x/mint labels Dec 3, 2018
@codecov
Copy link

codecov bot commented Dec 3, 2018

Codecov Report

Merging #2982 into develop will increase coverage by 0.06%.
The diff coverage is 40%.

@@             Coverage Diff             @@
##           develop    #2982      +/-   ##
===========================================
+ Coverage     55.5%   55.57%   +0.06%     
===========================================
  Files          120      120              
  Lines         8494     8488       -6     
===========================================
+ Hits          4715     4717       +2     
+ Misses        3457     3449       -8     
  Partials       322      322

@cwgoes cwgoes requested a review from zramsay as a code owner December 3, 2018 11:06
@cwgoes cwgoes changed the title WIP: Inflation bug fixes (BeginBlocker ordering, hourly calculation) R4R: Inflation bug fixes (BeginBlocker ordering, hourly calculation) Dec 3, 2018
@cwgoes cwgoes changed the title R4R: Inflation bug fixes (BeginBlocker ordering, hourly calculation) R4R: Inflation bug fixes (hourly calculation) Dec 3, 2018
@cwgoes cwgoes changed the title R4R: Inflation bug fixes (hourly calculation) R4R: Inflation bug fixes Dec 3, 2018
@cwgoes
Copy link
Contributor Author

cwgoes commented Dec 3, 2018

Interesting, one seed failing:

go test ./cmd/gaia/app -run TestGaiaSimulationAfterImport -SimulationEnabled=true -SimulationNumBlocks=50 -SimulationVerbose=true -SimulationCommit=true -SimulationSeed=981928 -v -timeout 24h

with

panic: invariant broken: loose token invariance:
	pool.LooseTokens: 78540599.9265062257
	sum of account tokens: 78540599.9265062218 [recovered]
	panic: invariant broken: loose token invariance:
	pool.LooseTokens: 78540599.9265062257
	sum of account tokens: 78540599.9265062218

@cwgoes
Copy link
Contributor Author

cwgoes commented Dec 3, 2018

Pinned to these lines in cmd/gaia/app/export.go:

  app.assertRuntimeInvariantsWith(ctx)
  fmt.Printf("pre-deletion")

  // delete all distribution infos
  // these will be recreated in InitGenesis
  app.distrKeeper.RemoveValidatorDistInfos(ctx)
  app.distrKeeper.RemoveDelegationDistInfos(ctx)

  // assert again
  app.assertRuntimeInvariantsWith(ctx)
  fmt.Printf("post-deletion")

Fails between "pre-deletion" and "post-deletion".

Somehow, it's not safe to delete distribution infos after (theoretically) withdrawing everything from them.

@cwgoes
Copy link
Contributor Author

cwgoes commented Dec 3, 2018

One ValidatorDistributionInfo has leftover delegator-pool stake:

--- FAIL: TestGaiaSimulationAfterImport (6.25s)
panic: valInfo: {OperatorAddr:35D5DAA716545B6F6A574E9F75061912DD08D0DF FeePoolWithdrawalHeight:50 DelAccum:{UpdateHeight:50 Accum:0.0000000000} DelPool:[{Denom:STAKE Amount:0.0000000039}] ValCommission:[]} [recovered]
	panic: valInfo: {OperatorAddr:35D5DAA716545B6F6A574E9F75061912DD08D0DF FeePoolWithdrawalHeight:50 DelAccum:{UpdateHeight:50 Accum:0.0000000000} DelPool:[{Denom:STAKE Amount:0.0000000039}] ValCommission:[]}

@cwgoes
Copy link
Contributor Author

cwgoes commented Dec 3, 2018

OK, confirmed that all the delegation infos have zero rewards, looks like occasional DelPool can be leftover by rounding. Instead now resetting delegation info height to 0 (and not deleting).

@rigelrozanski
Copy link
Contributor

Let's benchmark the actual cost per block of this!

@cwgoes
Copy link
Contributor Author

cwgoes commented Dec 3, 2018

@rigelrozanski Added benchmarks in 60cf1f4 - total for both looks like ~2200 ns.


## NextInflationRate

The target annual inflation rate is recalculated at the first block of each new
hour. The inflation is also subject to a rate change (positive or negative)
The target annual inflation rate is recalculated each block.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this warrant any changes to the distribution spec (e.g. Affects on Staking)?

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 good. Not sure if any further documentation needs to be updated? Also, not sure if this has any other implications on the state machine.

@jackzampolin
Copy link
Member

Sounds like we should get this in for GoS as #2984 depends on it?

@cwgoes
Copy link
Contributor Author

cwgoes commented Dec 3, 2018

Sounds like we should get this in for GoS as #2984 depends on it?

Yup, and more importantly because it fixes #2967.

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.

few minor comments which should be addressed still, but approving

cmd/gaia/app/export.go Outdated Show resolved Hide resolved
cmd/gaia/app/export.go Outdated Show resolved Hide resolved
x/mint/minter_test.go Outdated Show resolved Hide resolved
x/mint/minter_test.go Show resolved Hide resolved
x/mint/minter.go Show resolved Hide resolved
x/mint/abci_app.go Show resolved Hide resolved
@cwgoes cwgoes mentioned this pull request Dec 4, 2018
5 tasks
@jackzampolin jackzampolin merged commit dfd00a6 into develop Dec 4, 2018
@cwgoes cwgoes deleted the cwgoes/minter-ordering-fixes branch December 4, 2018 18:54
mircea-c pushed a commit that referenced this pull request Dec 5, 2018
* PENDING.md; swap BeginBlocker ordering
* Calculate inflation every block
* Update x/mint spec
* Reset distribution info bond height instead
@rigelrozanski rigelrozanski mentioned this pull request Dec 7, 2018
4 tasks
@hleb-albau
Copy link
Contributor

@cwgoes @jackzampolin is it correct, that NextAnnualProvisions() calculated using only bonded part of the tokens, but not really total supply?

// BeginBlocker
	totalSupply := k.sk.TotalPower(ctx)
	bondedRatio := k.sk.BondedRatio(ctx)
	minter.Inflation = minter.NextInflationRate(params, bondedRatio)
	minter.AnnualProvisions = minter.NextAnnualProvisions(params, totalSupply)

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/mint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants