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 Zero Power Block fees to community pool #2905

Merged
merged 2 commits into from
Nov 26, 2018

Conversation

rigelrozanski
Copy link
Contributor

@rigelrozanski rigelrozanski commented Nov 26, 2018

Fixes issues seen with seed 99 in jae/simulator_improvements

reference #2906

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

@codecov
Copy link

codecov bot commented Nov 26, 2018

Codecov Report

Merging #2905 into jae/simulator_improvements will not change coverage.
The diff coverage is n/a.

@@                     Coverage Diff                     @@
##           jae/simulator_improvements    #2905   +/-   ##
===========================================================
  Coverage                       56.35%   56.35%           
===========================================================
  Files                             120      120           
  Lines                            8416     8416           
===========================================================
  Hits                             4743     4743           
  Misses                           3351     3351           
  Partials                          322      322

@cwgoes
Copy link
Contributor

cwgoes commented Nov 26, 2018

This does fix this issue, but I don't think this is quite the root of the problem.

I think the problem is that our "LastTotalPower" is not correct because of the one-block delay.

If we unbond all the validators in block n (in the EndBlocker), LastTotalPower will be zero at block n+1 but there will still be validators signing that block since the updates haven't been applied by Tendermint yet.

The simulator's logic is correct in this case, this is what Tendermint would do (although it is very much an edge case we're unlikely to see in real life).

Perhaps the more concerning part is that I think our fee distribution logic really ought to be offset by another block than it actually is at the moment - we're setting accum values based on powers which haven't actually been set by Tendermint yet. Amortized over lots of blocks with few stake distribution changes this seems fine, but there could be edge cases.

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.

ACK but see comment, let's think further on this

@cwgoes cwgoes merged commit 92917fe into jae/simulator_improvements Nov 26, 2018
@cwgoes cwgoes deleted the rigel/zero-power-block branch November 26, 2018 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants