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: remove rationals in endblock #1751

Merged
merged 7 commits into from
Jul 23, 2018
Merged

Conversation

rigelrozanski
Copy link
Contributor

@rigelrozanski rigelrozanski commented Jul 19, 2018

After thinking about it a bit more, I think the rational calculation actually makes a bunch of sense, we were just using outrageous precision, I drastically reduced the precision to acceptable amount as well as added rounding in all the appropriate places to improve performance.

Additionally, there was an unnecessary store read and write in the end-blocker which was isolated to it's required location.

closes #1743

  • Updated all relevant documentation (docs/)
  • Updated all relevant code comments
  • Wrote tests
  • Updated CHANGELOG.md
  • 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)

@rigelrozanski rigelrozanski added the T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). label Jul 19, 2018
@codecov
Copy link

codecov bot commented Jul 19, 2018

Codecov Report

Merging #1751 into develop will decrease coverage by <.01%.
The diff coverage is 77.77%.

@@             Coverage Diff             @@
##           develop    #1751      +/-   ##
===========================================
- Coverage    63.46%   63.46%   -0.01%     
===========================================
  Files          117      117              
  Lines         6922     6927       +5     
===========================================
+ Hits          4393     4396       +3     
- Misses        2274     2276       +2     
  Partials       255      255

@rigelrozanski rigelrozanski changed the title WIP: remove rationals in endblock R4R: remove rationals in endblock Jul 20, 2018
@@ -82,41 +82,6 @@ func TestProcessProvisions(t *testing.T) {
checkFinalPoolValues(t, pool, sdk.NewRat(initialTotalTokens), cumulativeExpProvs)
}

// Benchmark precision
func BenchmarkProcessProvisions10000(b *testing.B) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be easily adapted to just a benchmark of the inflation calculation? I think thats valuable, regardless of whether or not we're doing rational math.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed couldn't get the benchmarking to work as expected - should add benchmarks in a separate PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But yeah I mean the inflation calculation is within here, we could benchmark both, but honestly think it's probably enough to benchmark processing provisions because it's so tightly coupled with the inflation calculation (and is the higher level calculation)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good! Thanks for looking into it!

inflationRateChangePerYear := sdk.OneRat().Sub(p.BondedRatio().Quo(params.GoalBonded)).Mul(params.InflationRateChange)
inflationRateChangePerYear := sdk.OneRat().
Sub(p.BondedRatio().Round(precision).
Quo(params.GoalBonded)).
Copy link
Contributor

Choose a reason for hiding this comment

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

Not super related, but we should benchmark if the order of quotient and multiplication affects performance. Such testing could be done in the rational package.

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

If there are any edge cases (particular choices of inflation parameters) where this rounding leads to substantial divergence from the "pure" model, would be nice to document.

@rigelrozanski rigelrozanski merged commit a19cc0f into develop Jul 23, 2018
@rigelrozanski rigelrozanski deleted the rigel/no-endblock-rat-calcs branch July 23, 2018 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/staking T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reduce rational calculations in end-block
3 participants