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

Update f1 spec: #2990

Merged
merged 6 commits into from
Dec 12, 2018
Merged

Update f1 spec: #2990

merged 6 commits into from
Dec 12, 2018

Conversation

ValarDragon
Copy link
Contributor

@ValarDragon ValarDragon commented Dec 4, 2018

  • Improves exposition of the base scheme
  • Fix inflation section / add sketch of it (EDIT: Now has a more detailed explanation + proof of correctness)
  • Add future work section

I've also realized that using the ideas of the inflation section,
we could even remove the iteration over slashes when withdrawing!
(Note, The inflation section of this describes
something separate from inflation in the hub. It has inflation happen by
having each staked token produce more staked tokens, instead of having it come
through block rewards. This inflation can also happen per validator,
so as to enable non-signing validators to not get inflation.
The point of this parenthetical being to indicate that F1 can be used
independently of the inflation section.)


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)

* Improves exposition of the base scheme
* Fix inflation section / add sketch of how to do it
* Add future work section

I've also realized that using the ideas of the inflation section,
we could even remove the iteration over slashes when withdrawing!
(Note, inflation section equations / proof of correctness has not been
thoroughly transcribed from my head onto paper, so its not guaranteed to
be correct at the moment. However the inflation section of this describes
something separate from inflation in the hub. It has inflation happen by
having each staked token produce more staked tokens, instead of having it come
through block rewards. This inflation can also happen per validator,
so as to enable non-signing validators to not get inflation)
@ValarDragon ValarDragon added T:Docs Changes and features related to documentation. ready-for-review labels Dec 4, 2018
@codecov
Copy link

codecov bot commented Dec 4, 2018

Codecov Report

Merging #2990 into develop will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           develop   #2990      +/-   ##
==========================================
- Coverage    55.72%   55.7%   -0.03%     
==========================================
  Files          120     120              
  Lines         8468    8468              
==========================================
- Hits          4719    4717       -2     
- Misses        3427    3429       +2     
  Partials       322     322

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.

Pretty compelling compared to our current model, and definitely in the right direction of rigor!

Lots of comments. Still some further analysis to do to make sure no state spam is possible.

This basically requires no change.
In each block you only iterate over the currently bonded validators.
So you simply don't update the "total accrued fees this period" variable for jailed / non-bonded validators.
Withdrawing requires \textit{no} special casing here!
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

The above can be trivially amended if we want inflation to proceed differently for different validators each block.
(e.g. depending on who was offline) It can also be made to be easily adapted in a live chain. (i.e. via governance)
It is unclear if either of these two are more desirable settings.
Note that this inflation model is more fair than the currently implemented inflation algorithm, as this has inflation reward delegators not just validators.

\subsection{Delegation updates}
Updating your delegation amount is equivalent to withdrawing earned rewards and a fully independent new delegation occuring in the same block.
Copy link
Contributor

Choose a reason for hiding this comment

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

Including redelegation or partial redelegation

When creating a new period, check that the previous period (which had to be read anyway) doesn't have a count of 0.
If it does have a count of 0, delete it.
When withdrawing, decrement the period which created this delegation's counter by 1.
If that counter is now 0, delete that period.
Copy link
Contributor

Choose a reason for hiding this comment

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

oh look, you've invented reference counting!

I wonder if we can do this in general actually... would be interesting

Copy link
Contributor

Choose a reason for hiding this comment

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

I also wonder if we could do it in a way which wouldn't break the Merkle root - e.g. if a validator can prove that an entry will never be accessed, they can keep the IAVL key but delete the value. Then these upgrades could be non-breaking.

Copy link
Contributor Author

@ValarDragon ValarDragon Dec 5, 2018

Choose a reason for hiding this comment

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

LOL, you're right this is just reference counting!

The issue with this pruning is that any new syncing node will have to persist this state forever. (As they'd wouldn't get the proof of it never be accessed) Or if a node synced after it was put into state, they will have to keep that entry forever. This makes me feel like it ought to be in consensus, but it could be more intrinsic within the merkle tree rather than the leaf value.

Co-Authored-By: ValarDragon <ValarDragon@users.noreply.github.com>
@rigelrozanski
Copy link
Contributor

I would love to understand this a bit more - not going to review this PR because of capacity, but would like to have some phone detailed phone reviews with @ValarDragon before F1 implementation! Maybe we could organize that convo after I've had a chance to really review and grok.

We should probably not worry about two approvals for merging this PR

@cwgoes
Copy link
Contributor

cwgoes commented Dec 6, 2018

We should probably not worry about two approvals for merging this PR

Sure, but we don't need to merge it with any urgency, it's easier to discuss in the PR format probably.

A phone conversation sounds great, let me know - I'll join!

@ValarDragon
Copy link
Contributor Author

Sounds great! I'd be happy to discuss once you've read through it / have some questions / thoughts!

Just committed a proof of the inflation section's accounting. The proof feels overly verbose imo. (Would recommend reading the latex, makes it easier to read) I didn't show step-by-step why the withdraw is correct. If its not clear from reading it, I'll gladly update it!

@cwgoes
Copy link
Contributor

cwgoes commented Dec 12, 2018

Looks like this spec isn't going to see immediate further review, so I'm going to merge as-is - I think @ValarDragon wants to make more changes, which might then be more easily reviewed as a separate PR.

@cwgoes cwgoes merged commit a31dc20 into develop Dec 12, 2018
@cwgoes cwgoes deleted the dev/update_f1_spec branch December 12, 2018 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T:Docs Changes and features related to documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants