x/staking ValidatorPowerRank will have incorrect orderings

[ValarDragon]

Summary of Bug

This issue is demonstrated quite clearly in: https://github.com/cosmos/cosmos-sdk/pull/2438/files#diff-1fe0bb213f8721408b446de8df30add8R41

Basically, we don't do fixed length encoding for the decimals. Therefore a greater decimal will be longer. However, this doesn't preserve ordering as we'd like. From the linked example, the first line corresponds to tokens=1, and the second tokens=2^40:

05303030303030303030303130ffffffffffffffffffff
0531303939353131363237373736ffffffffffffffffffff

The tokens=1 case will occur first in the iteration, since the ff occurs first.

Solution

The solution to this is to create a fixed size byte encoding of decimals. At the same time, we should change the encoding to be more sensible for keys. Currently it converts to a string, and then casts to []byte. Thats good for UI's, not for encoding to state. (You see the 0 decimal encoded as a 30 in the above hex)

[cwgoes]

Hmm, I wonder if we should actually rank validators by Tendermint power (RoundInt64) in any case.

[alexanderbez]

Good catch, since we send TM updates in ABCI power (rounded), it seems to make sense.

[ValarDragon]

How do we go from decimal / int to Tendermint power?

[cwgoes]

How do we go from decimal / int to Tendermint power?

https://github.com/cosmos/cosmos-sdk/blob/develop/x/stake/types/validator.go#L313

[ValarDragon]

I do agree with making what is stored in the key align with tendermint power, depending on the implementation of tendermint power. AFAICT the implementation for deriving tendermint power is broken. (It may not surface as an issue on the hub for a few years though, but it will surface in other chains almost immediately)

RoundInt64 in that function just panics if the int is over 2**64. However token amounts can and will be greater than 2**64 bits long. (This is the point of making it 2**256 bits long) If we make our base unit of atoms 10**(-10) atom, then you get ~10**9 atoms of precision in this bonded token amount.

We should instead make the tendermint power either a big.Int, or just the bits corresponding to the 64 MSB for the validator with most stake. To illustrate what I mean, suppose the validator with the most power's token amount has 2**90 bits of precision. Then the tmpower for all validators will be the bits corresponding to 2**90 - 2**27. This range would only need to be recomputed when the number one validator's stake changes. Fees would use the full bonded token amount, not this. However under this implementation, I don't think the store keys should match the tmpower, and should instead be what I described at the top of the issue.

[rigelrozanski]

or just the bits corresponding to the 64 MSB for the validator with most stake.

or alternatively we could always just use a different base unit (aka order by atoms not nano-atoms) - def don't think we need to be making TendermintPower an Int - seems overkill - but this is a good catch, we certainly don't have regard for this overflow potential at the moment.

---

This bug probably describes some of the weirdo ordering bugs that were cropping up previously - correcting this should be priority - I'd say should probably be 0.25

[alexanderbez]

That latter of what you suggested @ValarDragon just sounds overly complex imo. Using a different unit or type sounds like the more straight forward approach.

[ValarDragon]

What I described was making the unit the minimum it could possibly be. We could window it by a few bits, but it fundamentally does need to remain upgradable imo, since we are going for high inflation. (AFAIK)

You made a good point though Rigel, we could just fix a base unit, theres not really a need for dynamic updates. Governance can control this bit range via param updates, and we can get it from the Int / Dec with pretty straightforward bitmasks.

[alexanderbez]

I suppose we don't have to worry about infinitesimal amounts as this is for the validator power store, right?

[ValarDragon]

This is also for consensus. The amounts being negligible compared to MSB is why we don't have to worry about it. (One third attack success rate would be computed off of these truncated numbers. The thing is, if you can succeed with the truncation, you could succeed in the normal case with a negligible increase in money you have)
The logic for the power store is the same as well.

[jackzampolin]

Closing this in favor of #2513

[keichiri]

Hello, this issue does not seem to be solved with the issue that this one got closed in favor of.

cosmos-sdk/x/staking/keeper/key.go

Line 76 in ea46da7

// todo: deal with cases above 2**64, ref https://github.com/cosmos/cosmos-sdk/issues/2439#issuecomment-427167556

This part is causing the problems for us, when validators have large amount of tokens staked, in our case e.g. 20000000000000000000000 (we use a lot of decimal places)

Is this planned to be reopened in future?

[alexanderbez]

/cc @rigelrozanski

[rigelrozanski]

opened up a new issue for this #3985, worth resolving soon