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

Staking ValidatorPowerRank should use "Potential Consensus Power" #3985

Closed
rigelrozanski opened this issue Mar 27, 2019 · 3 comments · Fixed by #4524
Closed

Staking ValidatorPowerRank should use "Potential Consensus Power" #3985

rigelrozanski opened this issue Mar 27, 2019 · 3 comments · Fixed by #4524
Assignees

Comments

@rigelrozanski
Copy link
Contributor

rigelrozanski commented Mar 27, 2019

Referring to "Potential Tendermint Power" as the tokens reduced by a threshold (as is currently used within the ABCI)

func TokensToTendermintPower(tokens Int) int64 {

This issue is followup to this comment in the code which contains incorrect variable names:

func getValidatorPowerRank(validator types.Validator) []byte {
potentialPower := validator.Tokens
// todo: deal with cases above 2**64, ref https://github.com/cosmos/cosmos-sdk/issues/2439#issuecomment-427167556
tendermintPower := potentialPower.Int64()
tendermintPowerBytes := make([]byte, 8)
binary.BigEndian.PutUint64(tendermintPowerBytes[:], uint64(tendermintPower))

Which was referenced recently in #2439


By using the Potential Tendermint Power we remove any Int64 overflow issues, not to mention make the variable names in the code reflective of reality (currently these variable names are misleading/incorrect - this is NOT the tendermint power, the tendermint power is equivalent to Potential Tendermint Power)

CC @alexanderbez @keichiri @cwgoes

@rigelrozanski
Copy link
Contributor Author

the actionable item is modify:

tendermintPower := potentialPower.Int64() 

to

tendermintPower := TokensToTendermintPower(validator.Tokens)

As well as any legacy conversion necessary

@ValarDragon
Copy link
Contributor

Perhaps it would make sense to make tendermintPower its own type? (Or rather "ConsensusPower" to be mechanism agnostic)

@rigelrozanski
Copy link
Contributor Author

nice, I like ConsensusPower - and yeah making its own type makes sense I think

@rigelrozanski rigelrozanski changed the title Staking ValidatorPowerRank should use "Potential Tendermint Power" Staking ValidatorPowerRank should use "Potential Consensus Power" Apr 2, 2019
@rigelrozanski rigelrozanski self-assigned this Apr 2, 2019
@rigelrozanski rigelrozanski mentioned this issue Apr 3, 2019
5 tasks
@alexanderbez alexanderbez removed this from the v0.36.0 milestone Jun 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants