Use any as validator pubkey #7597

robert-zaremba · 2020-10-19T15:19:00Z

Description

Validator ConsensusPubKey type migration from string (bech32) to Any

closes: #7447 (partially)

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

CHANGELOG.md

codecov · 2020-10-19T23:39:18Z

Codecov Report

Merging #7597 into master will decrease coverage by 0.05%.
The diff coverage is 46.28%.

@@            Coverage Diff             @@
##           master    #7597      +/-   ##
==========================================
- Coverage   54.21%   54.15%   -0.06%     
==========================================
  Files         611      611              
  Lines       38464    38556      +92     
==========================================
+ Hits        20852    20881      +29     
- Misses      15503    15543      +40     
- Partials     2109     2132      +23

blushi

Some tests are still failing, one in particular (TestRandomizedGenState in x/staking/simulation/genesis_test.go) is because you need to implement UnpackInterfaces on staking GenesisState (since it has a list of validators in it with Any values).

Also, it seems like there's an issue in func (v Validator) MinEqual(other Validator) bool in x/staking/types/validator.go, when comparing the ConsensusPubkeys, I'd say you need to GetCachedValue() on the ConsensusPubkeys first and then use Equals function from cryptotypes.PubKey to compare them.

CHANGELOG.md

simapp/export.go

amaury1093

Didn't go through everything yet, here are some first comments

proto/cosmos/staking/v1beta1/staking.proto

simapp/test_helpers.go

x/staking/types/historical_info.go

Use gRPC in GetCmdQueryValidators

clevinson

Few small stylistic things, and one question about an error handling. Pre-approving as I don't consider any of them blockers.

Massive lift @robert-zaremba! Thanks for pushing this through 🎉

And big kudos to team effort from @amaurymartiny & @blushi for helping with test debugging throughout :)

simapp/app.go

simapp/encoding.go

simapp/params/proto.go

x/staking/genesis.go

amaury1093

Overall lgtm 👍, small nits only

(I also changed the title to something more explicit)

simapp/test_helpers.go

x/staking/types/validator_test.go

x/staking/simulation/genesis_test.go

x/staking/types/validator_test.go

simapp/test_helpers.go

x/staking/simulation/genesis_test.go

x/staking/types/validator_test.go

Co-authored-by: Marie Gauthier <marie.gauthier63@gmail.com>

tac0turtle · 2020-10-23T14:29:56Z

x/gov/keeper/tally_test.go

@@ -410,7 +410,9 @@ func TestTallyJailedValidator(t *testing.T) {

 	_ = staking.EndBlocker(ctx, app.StakingKeeper)

-	app.StakingKeeper.Jail(ctx, sdk.ConsAddress(val2.GetConsPubKey().Address()))
+	consKey, err := val2.TmConsPubKey()


why does this need to be a tendermint key? It seems this PR intertwined Tendermint keys even deeper into the sdk..

tac0turtle · 2020-10-23T14:31:21Z

x/slashing/genesis.go

@@ -12,7 +12,11 @@ import (
 func InitGenesis(ctx sdk.Context, keeper keeper.Keeper, stakingKeeper types.StakingKeeper, data *types.GenesisState) {
 	stakingKeeper.IterateValidators(ctx,
 		func(index int64, validator stakingtypes.ValidatorI) bool {
-			keeper.AddPubkey(ctx, validator.GetConsPubKey())
+			consPk, err := validator.TmConsPubKey()


this doesnt need to be a tendermint key. I believe the only places that need to be tendermint keys is where it is going to Tendermint. Within the sdk you can use sdk's keys

Why? this is a consensus key, and it's coming from Tendermint. I'm not very familiar with the tendermint side. I assume that when we get a tendermint key then we should use it consistently as a tendermint key except when we serialize it in SDK objects.

@marbar3778 could you confirm?

Tendermint doesn't care how its handled. The reason you would want to handle it with your own key is to disentangle Tendermint from the inner workings of the sdk. You can use tendermint ed25519 keys here but then what is the point of having your own?

Will this PR be backported to stargate?

To be able to serialize it in SDK structures / messages.

* protobuf pubkey type update * wip2 * wip3 * solving types.NewValidator issues * remove bech32 from validator type assignment * update Validator interface * Changelog update * wip4 * update genutil * fix simapp & x/ibc/testing tests * update staking * changelog update * fix import cycle in tests * fix amino panic on TestValidatorMarshalUnmarshalJSON * fix TestValidatorMarshalUnmarshalJSON consensus_pubkey check * Add UnpackInterfaces to HistoricalInfo * fix TestHistoricalInfo * update todos * fix: Expecting ed25519.PubKey to implement proto.Message * fix linter issues * Fix migrate test * Update CHANGELOG.md Co-authored-by: Marie Gauthier <marie.gauthier63@gmail.com> * review comments * cosmetic changes * add UnpackInterfaces got GenesisRandomized test * Validator.Equal reuses Validator.MinEqual * fix test * use Validator.Equal in tests * Fix staking simulation TestRandomizedGenState * Remove TODO * use HistoricalInfo.Equal * use proto.Equal * rename Validator.GetConsPubKey to TmConsPubKey * prefer require.Equal over reflect.DeepEqual * SetHistoricalInfo using a pointer * Fix TestQueryDelegation test * Fix TestQueryValidators test * Fix TestSimulateMsgUnjail test * experiement with LegacyAmino instances * Register codecs in all simapp tests * Fix cli_test compilation problems * fix typo sdk -> std * fix typo * fix TestPlanStringer * Rename to MakeEncodingConfig * Remove RegisterCodecsTests * Use gRPC in GetCmdQueryValidators * Empty status * fix info log check * linter fixes * rename simapparams to simappparams * Update simapp/test_helpers.go Co-authored-by: Marie Gauthier <marie.gauthier63@gmail.com> * comments updates * use valAddr1 instead of sdk.ValAddress(pk1.Address().Bytes()) Co-authored-by: Cory Levinson <cjlevinson@gmail.com> Co-authored-by: Amaury Martiny <amaury.martiny@protonmail.com> Co-authored-by: Marie Gauthier <marie.gauthier63@gmail.com>

robert-zaremba requested review from aaronc, alessio and alexanderbez as code owners October 19, 2020 15:19

robert-zaremba marked this pull request as draft October 19, 2020 15:19

robert-zaremba added 6 commits October 19, 2020 21:42

protobuf pubkey type update

e098384

wip2

4999b5c

wip3

8f3645c

solving types.NewValidator issues

bdd82af

remove bech32 from validator type assignment

94047df

update Validator interface

324f31a

robert-zaremba force-pushed the robert/validator-pubkey branch from f23fb0c to 324f31a Compare October 19, 2020 19:43

robert-zaremba added 2 commits October 19, 2020 21:52

Changelog update

cc2d8a5

wip4

774bf73

clevinson reviewed Oct 19, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

robert-zaremba and others added 7 commits October 19, 2020 23:26

update genutil

ee2cad2

fix simapp & x/ibc/testing tests

fc02eab

update staking

51b2ab9

changelog update

05b8e73

fix import cycle in tests

3f139a1

fix amino panic on TestValidatorMarshalUnmarshalJSON

8734a4e

fix TestValidatorMarshalUnmarshalJSON consensus_pubkey check

dfa92d6

robert-zaremba added 2 commits October 20, 2020 02:18

Add UnpackInterfaces to HistoricalInfo

32a2278

fix TestHistoricalInfo

da990bf

robert-zaremba marked this pull request as ready for review October 20, 2020 00:38

robert-zaremba mentioned this pull request Oct 20, 2020

Remove bech32 PubKey support #7447

Closed

9 tasks

robert-zaremba added 2 commits October 20, 2020 02:52

update todos

003affd

fix: Expecting ed25519.PubKey to implement proto.Message

ff38afc

blushi suggested changes Oct 20, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

simapp/export.go Show resolved Hide resolved

amaury1093 reviewed Oct 20, 2020

View reviewed changes

proto/cosmos/staking/v1beta1/staking.proto Show resolved Hide resolved

simapp/test_helpers.go Show resolved Hide resolved

x/staking/types/historical_info.go Outdated Show resolved Hide resolved

x/staking/types/historical_info.go Outdated Show resolved Hide resolved

amaury1093 mentioned this pull request Oct 22, 2020

Use gRPC in GetCmdQueryValidators #7626

Merged

9 tasks

amaury1093 and others added 3 commits October 22, 2020 15:27

Empty status

1cae118

Merge pull request #7626 from cosmos/am-validator-pubkey

334253f

Use gRPC in GetCmdQueryValidators

fix info log check

54c85a5

robert-zaremba requested review from clevinson and amaury1093 October 22, 2020 21:31

linter fixes

9d0ca25

clevinson self-assigned this Oct 23, 2020

clevinson approved these changes Oct 23, 2020

View reviewed changes

simapp/app.go Outdated Show resolved Hide resolved

simapp/encoding.go Outdated Show resolved Hide resolved

simapp/params/proto.go Show resolved Hide resolved

x/staking/genesis.go Show resolved Hide resolved

rename simapparams to simappparams

2598a7f

amaury1093 changed the title ~~Robert/validator pubkey~~ Use any as validator pubkey Oct 23, 2020

amaury1093 approved these changes Oct 23, 2020

View reviewed changes

simapp/test_helpers.go Show resolved Hide resolved

x/staking/types/validator_test.go Outdated Show resolved Hide resolved

x/staking/simulation/genesis_test.go Outdated Show resolved Hide resolved

x/staking/types/validator_test.go Outdated Show resolved Hide resolved

blushi suggested changes Oct 23, 2020

View reviewed changes

simapp/test_helpers.go Outdated Show resolved Hide resolved

x/staking/simulation/genesis_test.go Outdated Show resolved Hide resolved

x/staking/types/validator_test.go Outdated Show resolved Hide resolved

robert-zaremba and others added 3 commits October 23, 2020 12:56

Update simapp/test_helpers.go

4e21ba4

Co-authored-by: Marie Gauthier <marie.gauthier63@gmail.com>

comments updates

a019c4d

use valAddr1 instead of sdk.ValAddress(pk1.Address().Bytes())

1c8b045

robert-zaremba requested a review from blushi October 23, 2020 11:13

blushi approved these changes Oct 23, 2020

View reviewed changes

robert-zaremba mentioned this pull request Oct 23, 2020

remove tendermint keys & toTMPubkey interface #7635

Closed

9 tasks

robert-zaremba added the A:automerge Automatically merge PR once all prerequisites pass. label Oct 23, 2020

Merge branch 'master' into robert/validator-pubkey

1fa3829

mergify bot merged commit 6bc8ff2 into master Oct 23, 2020

mergify bot deleted the robert/validator-pubkey branch October 23, 2020 12:07

tac0turtle reviewed Oct 23, 2020

View reviewed changes

robert-zaremba mentioned this pull request Oct 23, 2020

Refactore simapp.MakeEncodingConfig #7642

Closed

4 tasks

clevinson added the backport/0.40.x (Stargate) label Oct 28, 2020

clevinson mentioned this pull request Oct 29, 2020

[backports]: v0.40.0-rc2 Cherry-picks #7734

Closed

45 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use any as validator pubkey #7597

Use any as validator pubkey #7597

robert-zaremba commented Oct 19, 2020 •

edited

Loading

codecov bot commented Oct 19, 2020 •

edited

Loading

blushi left a comment •

edited

Loading

amaury1093 left a comment

clevinson left a comment

amaury1093 left a comment •

edited

Loading

tac0turtle Oct 23, 2020 •

edited

Loading

tac0turtle Oct 23, 2020

robert-zaremba Oct 23, 2020

tac0turtle Oct 23, 2020 •

edited

Loading

robert-zaremba Oct 23, 2020

Use any as validator pubkey #7597

Use any as validator pubkey #7597

Conversation

robert-zaremba commented Oct 19, 2020 • edited Loading

Description

codecov bot commented Oct 19, 2020 • edited Loading

Codecov Report

blushi left a comment • edited Loading

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

clevinson left a comment

Choose a reason for hiding this comment

amaury1093 left a comment • edited Loading

Choose a reason for hiding this comment

tac0turtle Oct 23, 2020 • edited Loading

Choose a reason for hiding this comment

tac0turtle Oct 23, 2020

Choose a reason for hiding this comment

robert-zaremba Oct 23, 2020

Choose a reason for hiding this comment

tac0turtle Oct 23, 2020 • edited Loading

Choose a reason for hiding this comment

robert-zaremba Oct 23, 2020

Choose a reason for hiding this comment

robert-zaremba commented Oct 19, 2020 •

edited

Loading

codecov bot commented Oct 19, 2020 •

edited

Loading

blushi left a comment •

edited

Loading

amaury1093 left a comment •

edited

Loading

tac0turtle Oct 23, 2020 •

edited

Loading

tac0turtle Oct 23, 2020 •

edited

Loading