From 3c13bc03cdb12f38c3a7aa66995df663c360e17d Mon Sep 17 00:00:00 2001 From: Facundo Medica <14063057+facundomedica@users.noreply.github.com> Date: Wed, 19 Jun 2024 17:52:47 +0200 Subject: [PATCH] fix(x/staking): Check existing pubkeys when creating a Validator + refactors (#20713) Co-authored-by: Hieu Vu <72878483+hieuvubk@users.noreply.github.com> Co-authored-by: Ezequiel Raynaudo --- .../staking/keeper/msg_server_test.go | 4 +- x/staking/README.md | 8 +-- x/staking/keeper/cons_pubkey.go | 25 ++++--- x/staking/keeper/keeper.go | 18 ++--- x/staking/keeper/msg_server.go | 71 +++++++++++++------ x/staking/keeper/msg_server_test.go | 21 +++++- x/staking/keeper/validator.go | 4 +- x/staking/simulation/operations.go | 12 ++++ x/staking/types/errors.go | 5 +- x/staking/types/keys.go | 4 +- 10 files changed, 112 insertions(+), 60 deletions(-) diff --git a/tests/integration/staking/keeper/msg_server_test.go b/tests/integration/staking/keeper/msg_server_test.go index 33653fe4a5..f5e4accc00 100644 --- a/tests/integration/staking/keeper/msg_server_test.go +++ b/tests/integration/staking/keeper/msg_server_test.go @@ -264,7 +264,7 @@ func TestRotateConsPubKey(t *testing.T) { validator: validators[0].GetOperator(), newPubKey: validators[1].ConsensusPubkey.GetCachedValue().(cryptotypes.PubKey), pass: false, - expErrMsg: "consensus pubkey is already used for a validator", + expErrMsg: "validator already exist for this pubkey; must use new validator pubkey", }, { name: "consensus pubkey rotation limit check", @@ -314,7 +314,6 @@ func TestRotateConsPubKey(t *testing.T) { // this shouldn't remove the existing keys from waiting queue since unbonding time isn't reached _, err = stakingKeeper.EndBlocker(ctx) assert.NilError(t, err) - // stakingKeeper.UpdateAllMaturedConsKeyRotatedKeys(ctx, ctx.BlockHeader().Time) msg, err = types.NewMsgRotateConsPubKey( validators[3].GetOperator(), @@ -331,7 +330,6 @@ func TestRotateConsPubKey(t *testing.T) { // this should remove keys from waiting queue since unbonding time is reached _, err = stakingKeeper.EndBlocker(newCtx) assert.NilError(t, err) - // stakingKeeper.UpdateAllMaturedConsKeyRotatedKeys(newCtx, newCtx.BlockHeader().Time) return newCtx }, diff --git a/x/staking/README.md b/x/staking/README.md index 7e1c50b808..e1f7574019 100644 --- a/x/staking/README.md +++ b/x/staking/README.md @@ -278,9 +278,9 @@ ValidatorConsensusKeyRotationRecordQueueKey: `103 | format(time) -> ProtocolBuff ValidatorConsensusKeyRotationRecordIndexKey:`104 | valAddr | format(time) -> ProtocolBuffer([]Byte{})` -OldToNewConsKeyMap:`105 | byte(oldConsKey) -> byte(newConsKey)` +OldToNewConsAddrMap:`105 | byte(oldConsAddr) -> byte(newConsAddr)` -ConsKeyToValidatorIdentifierMap:`106 | byte(newConsKey) -> byte(initialConsKey)` +ConsAddrToValidatorIdentifierMap:`106 | byte(newConsAddr) -> byte(initialConsAddr)` `ConsPubKeyRotationHistory` is used for querying the rotations of a validator @@ -290,9 +290,9 @@ ConsKeyToValidatorIdentifierMap:`106 | byte(newConsKey) -> byte(initialConsKey)` A `ConsPubKeyRotationHistory` object is created every time a consensus pubkey rotation occurs. -An entry is added in `OldToNewConsKeyMap` collection for every rotation (Note: this is to handle the evidences when submitted with old cons key). +An entry is added in `OldToNewConsAddrMap` collection for every rotation (Note: this is to handle the evidences when submitted with old cons key). -An entry is added in `ConsKeyToValidatorIdentifierMap` collection for every rotation, this entry is to block the rotation if the validator is rotating to the cons key which is involved in the history. +An entry is added in `ConsAddrToValidatorIdentifierMap` collection for every rotation, this entry is to block the rotation if the validator is rotating to the cons key which is involved in the history. To prevent the spam: diff --git a/x/staking/keeper/cons_pubkey.go b/x/staking/keeper/cons_pubkey.go index 6914e9bbab..0f91710525 100644 --- a/x/staking/keeper/cons_pubkey.go +++ b/x/staking/keeper/cons_pubkey.go @@ -42,7 +42,7 @@ func (k Keeper) setConsPubKeyRotationHistory( } for _, r := range allRotations { if r.NewConsPubkey.Compare(newPubKey) == 0 { - return types.ErrConsensusPubKeyAlreadyUsedForValidator + return types.ErrValidatorPubKeyExists } } @@ -105,37 +105,36 @@ func (k Keeper) updateToNewPubkey(ctx context.Context, val types.Validator, oldP } // sets a map: oldConsKey -> newConsKey - if err := k.OldToNewConsKeyMap.Set(ctx, oldPk.Address(), newPk.Address()); err != nil { + if err := k.OldToNewConsAddrMap.Set(ctx, oldPk.Address(), newPk.Address()); err != nil { return err } // sets a map: newConsKey -> initialConsKey - if err := k.setConsKeyToValidatorIdentifierMap(ctx, sdk.ConsAddress(oldPk.Address()), sdk.ConsAddress(newPk.Address())); err != nil { + if err := k.setConsAddrToValidatorIdentifierMap(ctx, sdk.ConsAddress(oldPk.Address()), sdk.ConsAddress(newPk.Address())); err != nil { return err } return k.Hooks().AfterConsensusPubKeyUpdate(ctx, oldPk, newPk, fee) } -// setConsKeyToValidatorIdentifierMap adds an entry in the state with the current consKey to the initial consKey of the validator. -// it tries to find the oldPk if there is a entry already present in the state -func (k Keeper) setConsKeyToValidatorIdentifierMap(ctx context.Context, oldPk, newPk sdk.ConsAddress) error { - pk, err := k.ConsKeyToValidatorIdentifierMap.Get(ctx, oldPk) +// setConsAddrToValidatorIdentifierMap adds an entry in the state with the current consAddr to the initial consAddr of the validator. +// It first tries to find the validatorIdentifier if there is a entry already present in the state. +func (k Keeper) setConsAddrToValidatorIdentifierMap(ctx context.Context, oldConsAddr, newConsAddr sdk.ConsAddress) error { + validatorIdentifier, err := k.ConsAddrToValidatorIdentifierMap.Get(ctx, oldConsAddr) if err != nil && !errors.Is(err, collections.ErrNotFound) { return err } - if pk != nil { - oldPk = pk + if validatorIdentifier != nil { + oldConsAddr = validatorIdentifier } - - return k.ConsKeyToValidatorIdentifierMap.Set(ctx, newPk, oldPk) + return k.ConsAddrToValidatorIdentifierMap.Set(ctx, newConsAddr, oldConsAddr) } // ValidatorIdentifier maps the new cons key to previous cons key (which is the address before the rotation). -// (that is: newConsKey -> oldConsKey) +// (that is: newConsAddr -> oldConsAddr) func (k Keeper) ValidatorIdentifier(ctx context.Context, newPk sdk.ConsAddress) (sdk.ConsAddress, error) { - pk, err := k.ConsKeyToValidatorIdentifierMap.Get(ctx, newPk) + pk, err := k.ConsAddrToValidatorIdentifierMap.Get(ctx, newPk) if err != nil && !errors.Is(err, collections.ErrNotFound) { return nil, err } diff --git a/x/staking/keeper/keeper.go b/x/staking/keeper/keeper.go index b14db987c2..c9b632f0a9 100644 --- a/x/staking/keeper/keeper.go +++ b/x/staking/keeper/keeper.go @@ -122,10 +122,10 @@ type Keeper struct { ValidatorConsensusKeyRotationRecordIndexKey collections.KeySet[collections.Pair[[]byte, time.Time]] // ValidatorConsensusKeyRotationRecordQueue: this key is used to set the unbonding period time on each rotation ValidatorConsensusKeyRotationRecordQueue collections.Map[time.Time, types.ValAddrsOfRotatedConsKeys] - // ConsKeyToValidatorIdentifierMap: maps the new cons key to the initial cons key - ConsKeyToValidatorIdentifierMap collections.Map[[]byte, []byte] - // OldToNewConsKeyMap: maps the old cons key to the new cons key - OldToNewConsKeyMap collections.Map[[]byte, []byte] + // ConsAddrToValidatorIdentifierMap: maps the new cons addr to the initial cons addr + ConsAddrToValidatorIdentifierMap collections.Map[[]byte, []byte] + // OldToNewConsAddrMap: maps the old cons addr to the new cons addr + OldToNewConsAddrMap collections.Map[[]byte, []byte] // ValidatorConsPubKeyRotationHistory: consPubkey rotation history by validator // A index is being added with key `BlockConsPubKeyRotationHistory`: consPubkey rotation history by height RotationHistory *collections.IndexedMap[collections.Pair[[]byte, uint64], types.ConsPubKeyRotationHistory, rotationHistoryIndexes] @@ -280,16 +280,16 @@ func NewKeeper( ), // key format is: 105 | consAddr - ConsKeyToValidatorIdentifierMap: collections.NewMap( - sb, types.ConsKeyToValidatorIdentifierMapPrefix, - "new_to_old_cons_key_map", + ConsAddrToValidatorIdentifierMap: collections.NewMap( + sb, types.ConsAddrToValidatorIdentifierMapPrefix, + "new_to_old_cons_addr_map", collections.BytesKey, collections.BytesValue, ), // key format is: 106 | consAddr - OldToNewConsKeyMap: collections.NewMap( - sb, types.OldToNewConsKeyMap, + OldToNewConsAddrMap: collections.NewMap( + sb, types.OldToNewConsAddrMap, "old_to_new_cons_key_map", collections.BytesKey, collections.BytesValue, diff --git a/x/staking/keeper/msg_server.go b/x/staking/keeper/msg_server.go index 6c2543a244..d53cd07b55 100644 --- a/x/staking/keeper/msg_server.go +++ b/x/staking/keeper/msg_server.go @@ -1,6 +1,7 @@ package keeper import ( + "bytes" "context" "errors" "fmt" @@ -89,8 +90,9 @@ func (k msgServer) CreateValidator(ctx context.Context, msg *types.MsgCreateVali } } - if _, err := k.GetValidatorByConsAddr(ctx, sdk.GetConsAddress(pk)); err == nil { - return nil, types.ErrValidatorPubKeyExists + err = k.checkConsKeyAlreadyUsed(ctx, pk) + if err != nil { + return nil, err } bondDenom, err := k.BondDenom(ctx) @@ -649,40 +651,26 @@ func (k msgServer) RotateConsPubKey(ctx context.Context, msg *types.MsgRotateCon return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidType, "expecting cryptotypes.PubKey, got %T", cv) } - // check cons key is already present in the key rotation history. - rotatedTo, err := k.ConsKeyToValidatorIdentifierMap.Get(ctx, pk.Address()) - if err != nil && !errors.Is(err, collections.ErrNotFound) { + err = k.checkConsKeyAlreadyUsed(ctx, pk) + if err != nil { return nil, err } - if rotatedTo != nil { - return nil, errorsmod.Wrap(sdkerrors.ErrInvalidAddress, - "the new public key is already present in rotation history, please try with a different one") - } - - newConsAddr := sdk.ConsAddress(pk.Address()) - - // checks if NewPubKey is not duplicated on ValidatorsByConsAddr - _, err = k.Keeper.ValidatorByConsAddr(ctx, newConsAddr) - if err == nil { - return nil, types.ErrConsensusPubKeyAlreadyUsedForValidator - } - valAddr, err := k.validatorAddressCodec.StringToBytes(msg.ValidatorAddress) if err != nil { return nil, err } - validator2, err := k.Keeper.GetValidator(ctx, valAddr) + validator, err := k.Keeper.GetValidator(ctx, valAddr) if err != nil { return nil, err } - if validator2.GetOperator() == "" { + if validator.GetOperator() == "" { return nil, types.ErrNoValidatorFound } - if status := validator2.GetStatus(); status != sdk.Bonded { + if status := validator.GetStatus(); status != sdk.Bonded { return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidType, "validator status is not bonded, got %x", status) } @@ -709,7 +697,7 @@ func (k msgServer) RotateConsPubKey(ctx context.Context, msg *types.MsgRotateCon err = k.setConsPubKeyRotationHistory( ctx, valAddr, - validator2.ConsensusPubkey, + validator.ConsensusPubkey, msg.NewPubkey, params.KeyRotationFee, ) @@ -719,3 +707,42 @@ func (k msgServer) RotateConsPubKey(ctx context.Context, msg *types.MsgRotateCon return res, nil } + +// checkConsKeyAlreadyUsed returns an error if the consensus public key is already used, +// in ConsAddrToValidatorIdentifierMap, OldToNewConsAddrMap, or in the current block (RotationHistory). +func (k msgServer) checkConsKeyAlreadyUsed(ctx context.Context, newConsPubKey cryptotypes.PubKey) error { + newConsAddr := sdk.ConsAddress(newConsPubKey.Address()) + rotatedTo, err := k.ConsAddrToValidatorIdentifierMap.Get(ctx, newConsAddr) + if err != nil && !errors.Is(err, collections.ErrNotFound) { + return err + } + + if rotatedTo != nil { + return sdkerrors.ErrInvalidAddress.Wrap( + "public key was already used") + } + + // check in the current block + rotationHistory, err := k.GetBlockConsPubKeyRotationHistory(ctx) + if err != nil { + return err + } + + for _, rotation := range rotationHistory { + cachedValue := rotation.NewConsPubkey.GetCachedValue() + if cachedValue == nil { + return sdkerrors.ErrInvalidAddress.Wrap("new public key is nil") + } + if bytes.Equal(cachedValue.(cryptotypes.PubKey).Address(), newConsAddr) { + return sdkerrors.ErrInvalidAddress.Wrap("public key was already used") + } + } + + // checks if NewPubKey is not duplicated on ValidatorsByConsAddr + _, err = k.Keeper.ValidatorByConsAddr(ctx, newConsAddr) + if err == nil { + return types.ErrValidatorPubKeyExists + } + + return nil +} diff --git a/x/staking/keeper/msg_server_test.go b/x/staking/keeper/msg_server_test.go index 14163c6bcf..1f67108a6a 100644 --- a/x/staking/keeper/msg_server_test.go +++ b/x/staking/keeper/msg_server_test.go @@ -1205,6 +1205,9 @@ func (s *KeeperTestSuite) TestConsKeyRotn() { existingPubkey, ok := validators[1].ConsensusPubkey.GetCachedValue().(cryptotypes.PubKey) s.Require().True(ok) + validator0PubKey, ok := validators[0].ConsensusPubkey.GetCachedValue().(cryptotypes.PubKey) + s.Require().True(ok) + bondedPool := authtypes.NewEmptyModuleAccount(types.BondedPoolName) accountKeeper.EXPECT().GetModuleAccount(gomock.Any(), types.BondedPoolName).Return(bondedPool).AnyTimes() bankKeeper.EXPECT().GetBalance(gomock.Any(), bondedPool.GetAddress(), sdk.DefaultBondDenom).Return(sdk.NewInt64Coin(sdk.DefaultBondDenom, 1000000)).AnyTimes() @@ -1235,7 +1238,7 @@ func (s *KeeperTestSuite) TestConsKeyRotn() { name: "pubkey already associated with another validator", malleate: func() sdk.Context { return ctx }, isErr: true, - errMsg: "consensus pubkey is already used for a validator", + errMsg: "validator already exist for this pubkey; must use new validator pubkey", newPubKey: existingPubkey, validator: validators[0].GetOperator(), }, @@ -1350,6 +1353,20 @@ func (s *KeeperTestSuite) TestConsKeyRotn() { errMsg: "exceeding maximum consensus pubkey rotations within unbonding period", validator: validators[5].GetOperator(), }, + { + name: "try using the old pubkey of another validator that rotated", + malleate: func() sdk.Context { + val, err := stakingKeeper.ValidatorAddressCodec().StringToBytes(validators[0].GetOperator()) + s.Require().NoError(err) + + bankKeeper.EXPECT().SendCoinsFromAccountToModule(gomock.Any(), sdk.AccAddress(val), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + return ctx + }, + isErr: true, + errMsg: "validator already exist for this pubkey; must use new validator pubkey", + newPubKey: validator0PubKey, + validator: validators[2].GetOperator(), + }, } for _, tc := range testCases { @@ -1403,5 +1420,5 @@ func (s *KeeperTestSuite) TestConsKeyRotationInSameBlock() { s.Require().NoError(err) _, err = msgServer.RotateConsPubKey(ctx, req) - s.Require().ErrorContains(err, "consensus pubkey is already used for a validator") + s.Require().ErrorContains(err, "public key was already used") } diff --git a/x/staking/keeper/validator.go b/x/staking/keeper/validator.go index 892b204da0..908cff4d8e 100644 --- a/x/staking/keeper/validator.go +++ b/x/staking/keeper/validator.go @@ -38,12 +38,12 @@ func (k Keeper) GetValidator(ctx context.Context, addr sdk.ValAddress) (validato func (k Keeper) GetValidatorByConsAddr(ctx context.Context, consAddr sdk.ConsAddress) (validator types.Validator, err error) { opAddr, err := k.ValidatorByConsensusAddress.Get(ctx, consAddr) if err != nil { - // if the validator not found try to find it in the map of `OldToNewConsKeyMap` because validator may've rotated it's key. + // if the validator not found try to find it in the map of `OldToNewConsAddrMap` because validator may've rotated it's key. if !errors.Is(err, collections.ErrNotFound) { return types.Validator{}, err } - newConsAddr, err := k.OldToNewConsKeyMap.Get(ctx, consAddr.Bytes()) + newConsAddr, err := k.OldToNewConsAddrMap.Get(ctx, consAddr.Bytes()) if err != nil { if errors.Is(err, collections.ErrNotFound) { return types.Validator{}, types.ErrNoValidatorFound diff --git a/x/staking/simulation/operations.go b/x/staking/simulation/operations.go index 5be48161ca..7b3eecbbb6 100644 --- a/x/staking/simulation/operations.go +++ b/x/staking/simulation/operations.go @@ -192,11 +192,23 @@ func SimulateMsgCreateValidator( if err != nil { return simtypes.NoOpMsg(types.ModuleName, msgType, "unable to generate validator address"), nil, err } + msg, err := types.NewMsgCreateValidator(addr, simAccount.ConsKey.PubKey(), selfDelegation, description, commission, math.OneInt()) if err != nil { return simtypes.NoOpMsg(types.ModuleName, sdk.MsgTypeURL(msg), "unable to create CreateValidator message"), nil, err } + // check if there's another key rotation for this same key in the same block + allRotations, err := k.GetBlockConsPubKeyRotationHistory(ctx) + if err != nil { + return simtypes.NoOpMsg(types.ModuleName, msgType, "cannot get block cons key rotation history"), nil, err + } + for _, r := range allRotations { + if r.NewConsPubkey.Compare(msg.Pubkey) == 0 { + return simtypes.NoOpMsg(types.ModuleName, msgType, "cons key already used in this block"), nil, nil + } + } + txCtx := simulation.OperationInput{ R: r, App: app, diff --git a/x/staking/types/errors.go b/x/staking/types/errors.go index d8e91e196b..c2bf04f4a7 100644 --- a/x/staking/types/errors.go +++ b/x/staking/types/errors.go @@ -49,7 +49,6 @@ var ( ErrNoUnbondingType = errors.Register(ModuleName, 45, "unbonding type not found") // consensus key errors - ErrConsensusPubKeyAlreadyUsedForValidator = errors.Register(ModuleName, 46, "consensus pubkey is already used for a validator") - ErrExceedingMaxConsPubKeyRotations = errors.Register(ModuleName, 47, "exceeding maximum consensus pubkey rotations within unbonding period") - ErrConsensusPubKeyLenInvalid = errors.Register(ModuleName, 48, "consensus pubkey len is invalid") + ErrExceedingMaxConsPubKeyRotations = errors.Register(ModuleName, 46, "exceeding maximum consensus pubkey rotations within unbonding period") + ErrConsensusPubKeyLenInvalid = errors.Register(ModuleName, 47, "consensus pubkey len is invalid") ) diff --git a/x/staking/types/keys.go b/x/staking/types/keys.go index e957eab9de..8e445584b4 100644 --- a/x/staking/types/keys.go +++ b/x/staking/types/keys.go @@ -66,8 +66,8 @@ var ( BlockConsPubKeyRotationHistoryKey = collections.NewPrefix(102) // prefix for consPubkey rotation history by height ValidatorConsensusKeyRotationRecordQueueKey = collections.NewPrefix(103) // this key is used to set the unbonding period time on each rotation ValidatorConsensusKeyRotationRecordIndexKey = collections.NewPrefix(104) // this key is used to restrict the validator next rotation within waiting (unbonding) period - ConsKeyToValidatorIdentifierMapPrefix = collections.NewPrefix(105) // prefix for rotated cons address to new cons address - OldToNewConsKeyMap = collections.NewPrefix(106) // prefix for rotated cons address to new cons address + ConsAddrToValidatorIdentifierMapPrefix = collections.NewPrefix(105) // prefix for rotated cons address to new cons address + OldToNewConsAddrMap = collections.NewPrefix(106) // prefix for rotated cons address to new cons address ) // Reserved kvstore keys