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

R4R: Fix validator power key #2508

Merged
merged 2 commits into from
Oct 16, 2018
Merged

R4R: Fix validator power key #2508

merged 2 commits into from
Oct 16, 2018

Conversation

cwgoes
Copy link
Contributor

@cwgoes cwgoes commented Oct 16, 2018

Ref #2439, although it doesn't address all of that issue (only fixing the current key, not changing the Tendermint power calculation).

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added entries in PENDING.md with issue #
  • rereviewed Files changed in the github PR explorer

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)

@cwgoes cwgoes changed the title WIP: Fix validator power key R4R: Fix validator power key Oct 16, 2018
@codecov
Copy link

codecov bot commented Oct 16, 2018

Codecov Report

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

@@           Coverage Diff           @@
##           develop   #2508   +/-   ##
=======================================
  Coverage       60%     60%           
=======================================
  Files          151     151           
  Lines         8844    8844           
=======================================
  Hits          5307    5307           
  Misses        3167    3167           
  Partials       370     370

@cwgoes
Copy link
Contributor Author

cwgoes commented Oct 16, 2018

Ref #2509 as a followup

Copy link
Contributor

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

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

👍

@@ -71,8 +71,15 @@ func GetBondedValidatorIndexKey(operator sdk.ValAddress) []byte {
func getValidatorPowerRank(validator types.Validator) []byte {

potentialPower := validator.Tokens
powerBytes := []byte(potentialPower.ToLeftPadded(maxDigitsForAccount)) // power big-endian (more powerful validators first)
Copy link
Contributor

Choose a reason for hiding this comment

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

can to delete ToLeftPadded now I believe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm it could be useful in general for other serialization purposes though.

@cwgoes cwgoes merged commit b70c26a into develop Oct 16, 2018
@cwgoes cwgoes deleted the cwgoes/fix-validator-power-key branch October 16, 2018 20:09
// todo: deal with cases above 2**64, ref https://github.com/cosmos/cosmos-sdk/issues/2439#issuecomment-427167556
tendermintPower := potentialPower.RoundInt64()
tendermintPowerBytes := make([]byte, 8)
binary.BigEndian.PutUint64(tendermintPowerBytes[:], uint64(tendermintPower))
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is the approach we want to take. Now we lose sorting past the tmpower. Instead we should make the entire Decimal/Int get marshalled in constant size for the store key, imo.

Copy link
Contributor Author

@cwgoes cwgoes Oct 16, 2018

Choose a reason for hiding this comment

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

Maybe... what's the benefit of sorting past TM power precision?

(I agree with changing how TM power is calculated as was discussed in the issue)

Copy link
Contributor

Choose a reason for hiding this comment

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

per discussion in slack, I now agree. Lets only use tmpower everywhere.

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

Successfully merging this pull request may close these issues.

3 participants