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 storage efficiency improvements #3333

Merged
merged 21 commits into from
Jan 23, 2019
Merged

Conversation

cwgoes
Copy link
Contributor

@cwgoes cwgoes commented Jan 21, 2019

Closes #3303

Makes the following changes to F1 to improve storage efficiency:

  • Commission is force-withdrawn when a validator is removed, and the store record is cleared.
  • DelegatorStartingInfo is deleted when a delegation's rewards are withdrawn
  • Slash events, historical rewards, and current rewards are cleared when a validator is removed (all delegations will have been withdrawn)
  • Reference counts are kept on historical rewards records, and records are deleted once no references to them (possible delegations which would need to reference them when withdrawing) remain.

F1 should now have storage usage ~= O(D + S + V), where D is the number of current delegations to active validators, S is the number of historical slashes (since the beginning of time) of active validators, and V is the number of active validators (also, the incentives are in favor of withdrawing rewards and decreasing storage usage). This seems reasonable as we already store delegations & active validators and slashes are costly / rare. This storage usage is checked in a simulation invariant.

Because of the way this is implemented, I'm not sure we need to store the reference count at all, but I suggest we leave it in for now until we're sure about that and sure we don't need to reference historical records for any other reason. Ref #3344.

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 labels Jan 21, 2019
@codecov
Copy link

codecov bot commented Jan 21, 2019

Codecov Report

Merging #3333 into develop will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           develop   #3333      +/-   ##
==========================================
- Coverage     54.9%   54.9%   -0.01%     
==========================================
  Files          132     132              
  Lines         9680    9684       +4     
==========================================
+ Hits          5315    5317       +2     
- Misses        4028    4030       +2     
  Partials       337     337

@cwgoes
Copy link
Contributor Author

cwgoes commented Jan 21, 2019

To-do:

  • Reference counting seems to basically work, needs better testcases and docs
  • See if current rewards, slash events, and all historical rewards can be cleared when validator is removed (since no delegations should exist anymore)
  • Calculate some rough numbers on state usage (should be O(num_total_delegations))
  • Ideally testcases could ensure this (no state leaks)

@cwgoes cwgoes changed the title WIP: F1 storage efficiency improvements R4R: F1 storage efficiency improvements Jan 22, 2019
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.

LGTM 👌 ++ on the simulation storage invariant.

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

I think that my biggest comment is I would like to see a paragraph in the "implementation" spec (even if it doesn't exist yet, a new temporary .md file would suffice) describing the reward referencing counting mechanism from a high level.

Otherwise, code looks clean - have some general questions, some minor suggestions within.

x/distribution/keeper/delegation.go Show resolved Hide resolved
x/distribution/keeper/validator.go Show resolved Hide resolved
x/distribution/types/validator.go Outdated Show resolved Hide resolved
x/distribution/keeper/validator.go Show resolved Hide resolved
x/distribution/keeper/validator.go Outdated Show resolved Hide resolved
x/distribution/keeper/validator.go Show resolved Hide resolved
x/distribution/keeper/validator.go Show resolved Hide resolved
x/distribution/keeper/hooks.go Show resolved Hide resolved
@cwgoes cwgoes requested a review from zramsay as a code owner January 23, 2019 10:54
@cwgoes
Copy link
Contributor Author

cwgoes commented Jan 23, 2019

@rigelrozanski Added a Markdown doc describing the rationale behind reference counting - bd0ebc8.

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.

⭐️ 🚁 💾 🦅

@cwgoes cwgoes merged commit b5e245f into develop Jan 23, 2019
@cwgoes cwgoes deleted the cwgoes/f1-storage-efficiency branch January 23, 2019 11:37
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 T: Performance Performance improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants