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

Choose a uniform power calculation method #2513

Closed
cwgoes opened this issue Oct 16, 2018 · 21 comments · Fixed by #3400
Closed

Choose a uniform power calculation method #2513

cwgoes opened this issue Oct 16, 2018 · 21 comments · Fixed by #3400
Assignees

Comments

@cwgoes
Copy link
Contributor

cwgoes commented Oct 16, 2018

Either full sdk.Decimal precision or limited Tendermint power precision, and use that uniformly - in fee distribution, staking, power ranking, etc.

ref https://github.com/cosmos/cosmos-sdk/pull/2508/files#r225712555
ref #2439 (comment)

Maybe we should have an even higher threshold for delegation/undelegation/redelegation, in part to mitigate fee distribution DoS risk.

cc @ValarDragon

@ValarDragon
Copy link
Contributor

ValarDragon commented Oct 16, 2018

This seems good to me. (Presuming we're going with the fixed "Unit" approach, which will be specified by a governance change-able parameter, and is a power of 2. All relevant stake will be determined in integer quantities of Unit, with the integer not exceeding 2**63. (Or perhaps 2**51))

We do need to note and have a plan for the complications which will arise when upgrading the threshold, and how we would have to correctly reward the rewards from delegations/undelegations etc. along with keeping the correct amount slashable.

I do think this is a good idea. Then we can get faster computations on the int64's + less on-chain data usage.

@rigelrozanski
Copy link
Contributor

Yeah I could see us migrating to int's all the way around for the power representation of a validator however we will still need to use the decimals for a wide variety of mechanism calculations, we just want to limit its use in the final results (aka withdrawn fees, withdrawn inflation, etc) - like there are many circumstances where folks are continuously accumulating "fractions" of rewards which really add up over blocks - just need to have consideration for this and use decimal in those place appropriately

@jackzampolin
Copy link
Member

This discussion carried over from: #2439

@rigelrozanski
Copy link
Contributor

At @cwgoes were you planning on doing this soon? was also on my roster but if you want to take on that's also dope.

I do not see why GoS should require blocking on this.

@cwgoes
Copy link
Contributor Author

cwgoes commented Nov 16, 2018

@rigelrozanski At first I thought this was necessary for GoS but upon reflection decided that this method would be annoying since we would have to change the unit constant mid-Game. For now I think we'll just decrease the inflation slightly so we won't hit the int64 limit - but we should definitely still do this after GoS. Glad to pick it up but if you want to do it that's fine too.

@jackzampolin
Copy link
Member

Going to go ahead close this as the Dec -> Int PR looks like it close this.

@rigelrozanski
Copy link
Contributor

rigelrozanski commented Jan 10, 2019

that's incorrect my PR did not address this issue (hence, the PR did not say "closes #2513" but instead "ref #2513")

@rigelrozanski rigelrozanski reopened this Jan 10, 2019
@jackzampolin
Copy link
Member

@rigelrozanski what else is necessary here?

@rigelrozanski
Copy link
Contributor

my understanding - we need to upgrade the "power" so it doesn't overflow int64 which is < than sdk.Int -> thus we want to "scale back" the power calculation used when submitting power to tendermint

@cwgoes
Copy link
Contributor Author

cwgoes commented Jan 10, 2019

Yes; exactly, this is still an issue (especially as we want to use micro-Atoms).

@rigelrozanski
Copy link
Contributor

k I can take this one, AFAIU these are the requirements for the solution:

  • development of a threshold parameter to define how "scaled back" the Tendermint power is from the sdk.Validator power
  • development of an upgrade procedure for the threshold changes (includes sending an entire validator update to Tendermint)
  • creation of an endblock check and trigger for threshold upgrade
  • ensuring that slashing power (as received from tendermint) is equivalent across thresholds
    • AFAICT this means we will need to store historical thresholds (up to an unbonding period)

AFAICT we do not need to worry about about the effect on delegation power, withdrawn power etc - these are all internal to the SDK and do not involve the tendermint power whatsoever

Further thoughts @cwgoes ?

@rigelrozanski rigelrozanski self-assigned this Jan 23, 2019
@cwgoes
Copy link
Contributor Author

cwgoes commented Jan 23, 2019

k I can take this one, AFAIU these are the requirements for the solution:

* development of a threshold parameter to define how "scaled back" the Tendermint power is from the sdk.Validator power

* development of an upgrade procedure for the threshold changes (includes sending an entire validator update to Tendermint)

* creation of an endblock check and trigger for threshold upgrade

* ensuring that slashing power (as received from tendermint) is equivalent across thresholds
  
  * AFAICT this means we will need to store historical thresholds (up to an unbonding period)

AFAICT we do not need to worry about about the effect on delegation power, withdrawn power etc - these are all internal to the SDK and do not involve the tendermint power whatsoever

Further thoughts @cwgoes ?

We can implement that, but I don't know if it's worth the complexity - choosing a constant ratio should provide sufficient precision for long periods of operation (assuming inflation isn't ridiculous, which will be true in most real blockchains) - I think we just need to choose the particulars of Atoms / micro-Atoms, calculate our expected maximum total supply, and choose an appropriate ratio (considering several decades of inflation) - the ratio can easily be changed later with a hard-fork.

@rigelrozanski
Copy link
Contributor

I like the idea, there are some additional considerations to this hard coded approach:

  • The value should actually be hard coded and not a governance parameter, any change to this parameter would disrupt a variety of systems (as previously mentioned) - as such when the time comes that we need to upgrade it, we should hard fork to include the new mechanisms to allow the existing systems to smoothly transition.
  • We will need now have consideration for Tendermint 0 power if somebody is in the bonded validator set, however does not meet the minimum value for this ratio - this could lead to a situation were there are 100 bonded validators but <100 registered with Tendermint - which may have interesting considerations around launch.
  • I'm imagining we should truncate values for Tendermint Power instead of standard round.

@jackzampolin
Copy link
Member

We will need now have consideration for Tendermint 0 power if somebody is in the bonded validator set, however does not meet the minimum value for this ratio - this could lead to a situation were there are 100 bonded validators but <100 registered with Tendermint - which may have interesting considerations around launch.

This seems to be a pretty serious concern.

@cwgoes
Copy link
Contributor Author

cwgoes commented Jan 24, 2019

The value should actually be hard coded and not a governance parameter, any change to this parameter would disrupt a variety of systems (as previously mentioned) - as such when the time comes that we need to upgrade it, we should hard fork to include the new mechanisms to allow the existing systems to smoothly transition.

Agreed.

We will need now have consideration for Tendermint 0 power if somebody is in the bonded validator set, however does not meet the minimum value for this ratio - this could lead to a situation were there are 100 bonded validators but <100 registered with Tendermint - which may have interesting considerations around launch.
This seems to be a pretty serious concern.

I don't think so, we can just threshold directly based on TM power - https://github.com/cosmos/cosmos-sdk/blob/develop/x/staking/keeper/val_state_change.go#L50.

I'm imagining we should truncate values for Tendermint Power instead of standard round.

Yes, that sounds prudent.

@rigelrozanski
Copy link
Contributor

I don't think so, we can just threshold directly based on TM power

yeah this was my first thought, if you don't meet the threshold for 1 bonded power in tendermint, then you're not considered bonded. That's fine so long as we launch with a some validators who have more than the threshold to start the chain with

@rigelrozanski
Copy link
Contributor

Thoughts on defining the threshold:

  • total supply: ~218 000 000 ATOMs (let's say 2.2*10^8)
  • inflation (at max) is 20% per year
    • at 100% bonded then over 10 years that's (2.210^8)(1.2^10) ~= 1.4 * 10^9 atoms
  • assuming we're going to use "nano" atoms (quarks) as the base denom which are 10-9 atoms
    then the max number of base-tokens is 1.4 * 10^9 * 10^9 = 1.4 * 10^18
  • The max value for tendermint power is int64 which is 9,223,372,036,854,775,807 ~ 9*10^18

Huh........ so if all the atoms after 10 years inflation (repr. as quarks) where put into one account which was the only bonded account then it would still fit into a tendermint power. Extrolating it looks like full inflation would need to exist for about 20 years before this could even begin to be a threat

So I think we're actually fine and don't need to implement this?

Again if we choose a much smaller denom than 10^9 then this could be an issue but I think we're good for now

@cwgoes
Copy link
Contributor Author

cwgoes commented Jan 25, 2019

We want a much tighter bound on Tendermint power, because it's added / multiplied in several ways in Tendermint itself (mostly in proposer selection). I think we've decided to bound individual validator power at max(int32) = 2147483647 (cc @liamsi) - which the number of base tokens will definitely overflow. Tendermint doesn't yet enforce this at the ABCI level, but it will in the next release.

Otherwise your calculations look fine, so I think dividing by 10 ^ 9 would be about right - which would also have the advantage of making the threshold exactly one Atom.

@rigelrozanski
Copy link
Contributor

Is this is Int32 Spec'd out anywhere on the Tendermint side? lol probably a good idea

@rigelrozanski
Copy link
Contributor

rigelrozanski commented Feb 5, 2019

We chose to rename to uatoms as the base unit as 10^-6

@jackzampolin @jaekwon @sunnya97 @cwgoes

Which means that the reduction from tokens should be 10^-6 instead of 10^-9

@cwgoes
Copy link
Contributor Author

cwgoes commented Feb 6, 2019

Sure, makes sense - ref #3510 - let's double-check that total power will not exceed int32 max size.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants