R4R: lamborghini distribution & inflation spec upgrade #1702

rigelrozanski · 2018-07-16T23:02:48Z

closes #967

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 · 2018-07-16T23:10:29Z

Codecov Report

Merging #1702 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #1702   +/-   ##
========================================
  Coverage    63.44%   63.44%           
========================================
  Files          117      117           
  Lines         6937     6937           
========================================
  Hits          4401     4401           
  Misses        2281     2281           
  Partials       255      255

fedekunze · 2018-07-25T15:27:01Z

docs/spec/distribution/end_block.md

+and global pool.  When the validator is the proposer of the round, that
+validator (and their delegators) receives between 1% and 5% of fee rewards, the
+reserve tax is then charged, then the remainder is distributed socially by
+voting power to all validators including the proposer validator.  The amount of


Is the remainder distributed to all the validators from the validator set or only to the ones who voted on the pre-commit/pre-vote phase ?

All bonded validators. Not just ones that voted, the only way to not get the social distribution is to get kicked out (which will happen if you're not live enough) - I'll make this text more explicit.

As discussed previously, even though it may not be the best place here, it would help tremendously if some naive examples were linked somehow (similar to the ones on the cosmos.network/docs).

Added more text to make this explicit (addressed)

alexanderbez

Thanks for updating the spec! I left some general comments/remarks.

This stuff can get pretty complex and difficult to just sift through and so I think examples would help greatly (those on cosmos.network/docs helped me greatly), but I know that the spec may not be the best place for those.

It might help to think of ways to introduce the notion of examples somehow.

alexanderbez · 2018-07-26T14:45:37Z

PENDING.md

@@ -38,6 +38,7 @@ IMPROVEMENTS
 * [tools] Remove `rm -rf vendor/` from `make get_vendor_deps`
 * [x/auth] Recover ErrorOutOfGas panic in order to set sdk.Result attributes correctly
 * [x/stake] Add revoked to human-readable validator 
+* [spec] Inflation and distribution specs drastically improved


Lets add the issue (#967) number here 👍

alexanderbez · 2018-07-26T14:50:54Z

docs/spec/distribution/future_improvements.md

+### Power Change
+
+Within the current implementation all power changes ever made are indefinitely stored
+within the current state. In the future this state should be trimmed on an epoch basis. Delegators


This this data be pruned on all nodes? Archive nodes will have to keep this, correct?

It won't be pruned since it's kept in the current state. If we don't need power changes older than an epoch for any calculations, we should be able to remove them from the current state (and then nodes with pruning enabled will delete them).

Is the reason we're not doing this initially because it's complex to implement?

We need all the calculations in the current state for an epoch equal to what we want the maximum allowable time withdrawal your fees is.. I'm guessing at minimum this is something like a couple months and at maximum is something like a year. - There is some complexity surrounding tracking what the last required power change is but the bigger problem is figuring out what to do / the appropriate lazy mechanism which plays nice with the existing fees distribution mechanism for everyone who hasn't withdrawn their fees by that time. There are a number of options: But this is the discussion grounds for that:

Agreed that this merits discussion but it probably can be safely tabled to post-launch as long as we put in place the store key prefixes (block height) to prune easily in the future.

alexanderbez · 2018-07-26T15:15:34Z

docs/spec/distribution/overview.md

+delegators. Each validator has the opportunity to charge commission to the
+delegators on the fees collected on behalf of the delegators by the validators.
+Fees are paid directly into a global fee pool, and validator proposer-reward
+pool. Due to the nature of of passive accounting whenever changes to parameters


of of typo

alexanderbez · 2018-07-26T15:18:14Z

docs/spec/distribution/overview.md

+which affect the rate of fee distribution occurs, withdrawal of fees must also
+occur. 
+
+ - when withdrawing one must withdrawal the maximum amount they are entitled


Capitalization and punctuation should be consistent here 👍

addressed (lower bullet points from here - using lower case in bullet points be cause not sentences.

alexanderbez · 2018-07-26T15:19:09Z

docs/spec/distribution/overview.md

+ - when a validator chooses to change the commission on fees, all accumulated 
+   commission fees must be simultaneously withdrawn.
+
+The above scenarios are covered is `triggers.md`.


Wording is a bit confusing here.

lol thanks - was a type is -> in

alexanderbez · 2018-07-26T19:39:11Z

docs/spec/distribution/triggers.md

+ - triggered-by: `stake.TxCreateValidator`
+
+Whenever a totally new validator is added to the Tendermint validator set they
+are entitled to begin earning rewards of atom provisions and fess. At this


alexanderbez · 2018-07-26T20:06:05Z

docs/spec/inflation/end_block.md

+# End Block
+
+```
+EndBlock() 


What are these for?

Unnecessary - removing

alexanderbez · 2018-07-26T20:09:59Z

docs/spec/inflation/end_block.md

+
+Validator provisions are minted on an hourly basis (the first block of a new
+hour). The annual target of between 7% and 20%. The long-term target ratio of
+bonded tokens to unbonded tokens is 67%.


Would it be possible to describe why the exact target of 67% in a concise manner?

Agreed - or cite the whitepaper / relevant source.

I disagree this IS the spec, it supersedes the white paper - it's not a place where choices always need to be justified. This number for instance comes out of a bunch of conversations with Jae - there is no good reason it just seems like a fine value. For y'alls record if the value is to small its a security concern, if it's to large then there isn't enough liquidity for price discovery -> 67% seems fine.

alexanderbez · 2018-07-26T20:15:26Z

docs/spec/inflation/end_block.md

+        inflation = nextInflation(hrsPerYr).Round(precision)
+        SetInflation(inflation)
+
+        provisions = inflation * (pool.TotalSupply() / hrsPerYr)


Is the pool.TotalSupply() here the total number of bonded tokens in the validator set?

No, it's the total token supply - all tokens bonded, unbonded, deposited-in-governance-proposals, etc.

comment addressed by cwgoes

alexanderbez · 2018-07-26T20:15:55Z

docs/spec/inflation/end_block.md

+
+    bondedRatio = pool.BondedPool / pool.TotalSupply()
+
+    inflationRateChangePerYear = (1 - bondedRatio / params.GoalBonded) * params.InflationRateChange


What exactly is params.InflationRateChange?

Rate of change in the inflation rate (second derivative of the total supply)

comment addressed by cwgoes

cwgoes

Many comments!

cwgoes · 2018-07-26T20:54:20Z

docs/spec/distribution/end_block.md

+At each endblock, the fees received are sorted to the proposer, community fund,
+and global pool.  When the validator is the proposer of the round, that
+validator (and their delegators) receives between 1% and 5% of fee rewards, the
+reserve tax is then charged, then the remainder is distributed socially by


What do you mean "socially"? Perhaps "proportionally"?

Well I did actually mean social distribution which means that you get coins even if you weren't the proposer - but proportional makes more sense in this instance, so I'll change, but then name is the social distribution

OK makes sense - if you just define "social distribution" once that's fine.

cwgoes · 2018-07-26T20:55:26Z

docs/spec/distribution/end_block.md

+validator (and their delegators) receives between 1% and 5% of fee rewards, the
+reserve tax is then charged, then the remainder is distributed socially by
+voting power to all validators including the proposer validator.  The amount of
+proposer reward is calculated from pre-commits Tendermint messages. All


Let's cite why, e.g. "in order to incentivize validators to wait and include additional pre-commits in the block".

sure, included.

cwgoes · 2018-07-26T20:57:08Z

docs/spec/distribution/end_block.md

+                       * sumPowerPrecommitValidators / totalBondedTokens)
+     proposer.ProposerPool += proposerReward
+
+     communityFunding = feesCollectedDec * communityTax


This is the reserve pool? Maybe "tax" is not quite the right name (nbd though).

leaving for now - open to suggestions still

cwgoes · 2018-07-26T20:59:13Z

docs/spec/distribution/future_improvements.md

+### Power Change
+
+Within the current implementation all power changes ever made are indefinitely stored
+within the current state. In the future this state should be trimmed on an epoch basis. Delegators


It won't be pruned since it's kept in the current state. If we don't need power changes older than an epoch for any calculations, we should be able to remove them from the current state (and then nodes with pruning enabled will delete them).

Is the reason we're not doing this initially because it's complex to implement?

cwgoes · 2018-07-26T21:01:02Z

docs/spec/distribution/overview.md

+ - Multi-token fees to be socially distributed 
+ - Proposer reward pool
+ - Inflated atom provisions
+ - Validator commission on delegators 


Commission on atom provisions from delegated stake?

Updated to commission on all rewards earned by their delegators

cwgoes · 2018-07-26T21:23:36Z

docs/spec/distribution/transactions.md

+for that distributor had static shares (equal to the current shares), and no
+recipients had ever withdrawn their entitled rewards. The projected pool
+represents the anticipated recipient's entitlement to the distributors tokens
+base on the current blocks token input to the distributor, and the


"token input" = "contributing stake"?

Good point to clarify, no token input refers to the new input from fees/provision-reward etc. updated to say token input (for example fees reward received)

cwgoes · 2018-07-26T21:30:02Z

docs/spec/distribution/transactions.md

@@ -0,0 +1,396 @@
+# Transactions


I find it really difficult to reason through the correctness of the transaction procedures in this file, likely because we're trying to implement a simple strict model with a lazy process.

I think it would be helpful if we outline for each transaction what the calculation would be in the strict, inefficient case (iterating over everything), then explain our lazy implementation, and even ideally provide a quick (rough) demonstration of equivalence.

Coming up with a good way to comprehend this efficient fee distribution module is going to be an ongoing challenge - Due to this, I agree that equivalent outcome in a "simple" module for each transaction should be documented within the spec beside the complex "efficient" situation already provided.

With regards to the example - this is a tug of worlds here - I don't think that we want to have non-spec information in the spec - to me, the example described doesn't actually belong in the spec itself HOWEVER I think it's super valuable and I think we should have it somewhere. I think maybe IN The spec we could have a non-spec section which actually works through what you're describing. - I think that this is a separate task which should be completed soon, but is a lower priority than distribution implementation to me

I don't mean include an example (although that might be a good idea too), I mean include equations that demonstrate why the complex implementation reduces to the same effects as the simple one (weren't those written up somewhere previously)?

Or "parameterized examples"!

Leaving this for now as we're likely moving away from pre-launch implementation however here is an issue for more discussion #1906

cwgoes · 2018-07-26T21:31:20Z

docs/spec/distribution/triggers.md

+
+## Create validator distribution
+
+ - triggered-by: `stake.TxCreateValidator`


Every time or only when the new validator candidate is bonded?

(and in the latter case - then also by another validator unbonding, for example?)

Also, do we need to anything when a validator is unbonded?

Every time or only when the new validator candidate is bonded?

Good observation, this only happens when the validator enters the bonded validator set. updated.

Also, do we need to anything when a validator is unbonded?

For other validators:
When a validator is unbonded - the power change is be recorded and thus all other's passive calculations will incorporate the changes passively as expected. So nothing additional or special is required for an unbond for them.

For the affected validator:
With regards to everyone affected by the unbond - we will need to incorporate a new mechanism to passively account for this - Just opened up an issue to think about this a bit more #1905.
I'm certain that the appropriate mechanism can be added onto what is spec'd here to account for this situation. THANK YOU FOR BRINGING THIS UP.

cwgoes · 2018-07-26T21:32:08Z

docs/spec/distribution/triggers.md

@@ -0,0 +1,31 @@
+# Triggers


What about slashing events (which modify validator stake and delegation stake, and not necessarily in the same proportion)?

Also, what about parameter changes (e.g. enacted by ParameterChangeProposals)?

Slashing events is a situation that needs to be considered:

Side note: Equal slashing of delegators and validators is a design constraint of this system - we can talk about it more outside of github if you'd like.

These slashing events will need to have a new special computation not yet outlined in this spec: can be processed similarly with a new pool as per to unbonding validators. more discussion here: #1905

Parameter changes will not trigger any new changes because parameters relevant to distribution affect the tokens flowing into the distribution system but do not affect the tokens already contained within this distribution system.

cwgoes · 2018-07-26T21:33:29Z

docs/spec/distribution/triggers.md

+
+## Create or modify delegation distribution
+
+ - triggered-by: `stake.TxDelegate`


Or TxBeginRedelegate, or TxBeginUnbonding?

ValarDragon · 2018-07-27T20:03:23Z

I'd like to review this as well before its merged. Chris had recommended that I wait until his comments were resolved first though. Commenting so I get pinged for updates.

rigelrozanski · 2018-07-30T03:14:06Z

Hopefully can address everything tomorrow -still focused on decimal

cwgoes · 2018-08-02T05:01:49Z

Per discussion with @rigelrozanski we'd like to consider the "piggy bank distribution" instead as a simpler approach for launch.

rigelrozanski · 2018-08-02T05:50:58Z

Yes more on that - this spec should still be merged and reviewed further and expanded based on some of the new issues I've raised out of this. This model is still something we want - but should likely NOT be a prelaunch requirement - instead we should use the simple piggy bank model which should be spec'd out separately.

@cwgoes - I've addressed your comments, although a few of them haven't been fully addressed and I've opened two github issues for further consideration on this mechanism.
@alexanderbez - I've addressed all of your comments besides adding examples - again based on this comment - that's something we should do PRE implementation - however I'm going to hold off on that for now because I don't think we should implement this prelaunch
@ValarDragon - Probably not a good use of your time to focus on reviewing this at the moment because I think we should move away from prelaunch implementation of this spec

rigelrozanski · 2018-08-08T06:11:29Z

Let's merge this?

cwgoes · 2018-08-08T10:08:38Z

Let's merge this?

Before we actually implement it in the future, I want to review again re: transaction correctness - but agreed, fine for now.

cwgoes

utACK

Best description of this mechanism we have at the moment, so merging. Note that transactions have not been reviewed for mathematical equivalence (against the simple model); before we actually implement this (if we do so in the future) they should be further reviewed.

wip

2cc055a

rigelrozanski added spec wip labels Jul 16, 2018

rigelrozanski requested review from cwgoes, ebuchman, jbibla and zramsay as code owners July 16, 2018 23:02

rigelrozanski changed the title ~~WIP provisions spec upgrade~~ WIP distribution & inflation spec upgrade Jul 16, 2018

rigelrozanski added 11 commits July 16, 2018 20:03

seperated out inflation spec

bad0f39

Merge remote-tracking branch 'origin/develop' into rigel/fee-spec

668089e

finalize inflation spec

814a9be

future improvements/state of distribution spec

97c50c8

finish fees overview

54e89b5

...

69cfc1b

big headway

96263bf

complete all the distribution scenarios

162d2f2

distribution transactions complete

d4e6344

distribution end block spec

0a1b191

finish triggers

f512e48

rigelrozanski added ready-for-review and removed wip labels Jul 25, 2018

rigelrozanski added 2 commits July 25, 2018 11:16

Merge remote-tracking branch 'origin/develop' into rigel/fee-spec

1d75328

pending

a1068c7

rigelrozanski changed the title ~~WIP distribution & inflation spec upgrade~~ R4R: distribution & inflation spec upgrade Jul 25, 2018

fedekunze reviewed Jul 25, 2018

View reviewed changes

alexanderbez reviewed Jul 26, 2018

View reviewed changes

cwgoes reviewed Jul 26, 2018

View reviewed changes

Bez and Cwgoes spec comments

398af0e

rigelrozanski changed the title ~~R4R: distribution & inflation spec upgrade~~ R4R: lamborghini distribution & inflation spec upgrade Aug 2, 2018

cwgoes approved these changes Aug 8, 2018

View reviewed changes

cwgoes merged commit 7fb626f into develop Aug 8, 2018

cwgoes deleted the rigel/fee-spec branch August 8, 2018 10:10

chillyvee pushed a commit to chillyvee/cosmos-sdk that referenced this pull request Mar 1, 2024

Update package-lock.json (cosmos#1702)

c05bcd4


		bondedRatio = pool.BondedPool / pool.TotalSupply()

		inflationRateChangePerYear = (1 - bondedRatio / params.GoalBonded) * params.InflationRateChange


		## Create validator distribution

		- triggered-by: `stake.TxCreateValidator`


		## Create or modify delegation distribution

		- triggered-by: `stake.TxDelegate`

R4R: lamborghini distribution & inflation spec upgrade #1702

R4R: lamborghini distribution & inflation spec upgrade #1702

Conversation

rigelrozanski commented Jul 16, 2018 • edited Loading

codecov bot commented Jul 16, 2018 • edited Loading

Codecov Report

fedekunze Jul 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rigelrozanski Aug 1, 2018 • edited Loading

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon commented Jul 27, 2018

rigelrozanski commented Jul 30, 2018

cwgoes commented Aug 2, 2018

rigelrozanski commented Aug 2, 2018

rigelrozanski commented Aug 8, 2018

cwgoes commented Aug 8, 2018

cwgoes left a comment

Choose a reason for hiding this comment

rigelrozanski commented Jul 16, 2018 •

edited

Loading

codecov bot commented Jul 16, 2018 •

edited

Loading

fedekunze Jul 25, 2018 •

edited

Loading

rigelrozanski Aug 1, 2018 •

edited

Loading