From 496cd0de9b0ffad9dce8002f4ca153c9ba2da7bd Mon Sep 17 00:00:00 2001 From: Akhil Kumar P <36399231+akhilkumarpilli@users.noreply.github.com> Date: Mon, 2 Sep 2024 16:43:52 +0530 Subject: [PATCH] refactor(x/slashing): audit QA (#21477) --- CHANGELOG.md | 1 - x/slashing/CHANGELOG.md | 1 + x/slashing/README.md | 2 +- x/slashing/keeper/hooks.go | 2 +- x/slashing/keeper/infractions.go | 5 +++-- x/slashing/keeper/signing_info.go | 17 +++++++---------- x/slashing/keeper/signing_info_test.go | 6 +++--- x/slashing/module.go | 8 ++++---- x/slashing/simulation/proposals.go | 2 +- x/slashing/simulation/proposals_test.go | 2 +- 10 files changed, 22 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 136f78edeace..76d1d784c43f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -162,7 +162,6 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i * (x/gov/testutil) [#17986](https://github.com/cosmos/cosmos-sdk/pull/18036) `MsgDeposit` has been removed because of AutoCLI migration. * (x/staking/testutil) [#17986](https://github.com/cosmos/cosmos-sdk/pull/17986) `MsgRedelegateExec`, `MsgUnbondExec` has been removed because of AutoCLI migration. * (x/group) [#17937](https://github.com/cosmos/cosmos-sdk/pull/17937) Groups module was moved to its own go.mod `cosmossdk.io/x/group` -* (x/slashing) [#18115](https://github.com/cosmos/cosmos-sdk/pull/18115) `NewValidatorSigningInfo` takes strings instead of `sdk.AccAddress` * (x/gov) [#18197](https://github.com/cosmos/cosmos-sdk/pull/18197) Gov module was moved to its own go.mod `cosmossdk.io/x/gov` * (x/distribution) [#18199](https://github.com/cosmos/cosmos-sdk/pull/18199) Distribution module was moved to its own go.mod `cosmossdk.io/x/distribution` * (x/slashing) [#18201](https://github.com/cosmos/cosmos-sdk/pull/18201) Slashing module was moved to its own go.mod `cosmossdk.io/x/slashing` diff --git a/x/slashing/CHANGELOG.md b/x/slashing/CHANGELOG.md index d4951e5a0e07..ecf0f2ef8e5f 100644 --- a/x/slashing/CHANGELOG.md +++ b/x/slashing/CHANGELOG.md @@ -39,6 +39,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * [#20026](https://github.com/cosmos/cosmos-sdk/pull/20026) Removal of the Address.String() method and related changes: * `Migrate` now takes a `ValidatorAddressCodec` as argument. * `Migrator` has a new field of `ValidatorAddressCodec` type. +* [#18115](https://github.com/cosmos/cosmos-sdk/pull/18115) `NewValidatorSigningInfo` takes strings instead of `sdk.AccAddress`. * [#16441](https://github.com/cosmos/cosmos-sdk/pull/16441) Params state is migrated to collections. `GetParams` has been removed. * [#17023](https://github.com/cosmos/cosmos-sdk/pull/17023) Use collections for `ValidatorSigningInfo`: * remove `Keeper`: `SetValidatorSigningInfo`, `GetValidatorSigningInfo`, `IterateValidatorSigningInfos` diff --git a/x/slashing/README.md b/x/slashing/README.md index 004b112f42ce..c6da11e0ce2d 100644 --- a/x/slashing/README.md +++ b/x/slashing/README.md @@ -148,7 +148,7 @@ https://github.com/cosmos/cosmos-sdk/blob/release/v0.52.x/x/slashing/proto/cosmo ### Params -The slashing module stores it's params in state with the prefix of `0x00`, +The slashing module stores its params in state with the prefix of `0x00`, it can be updated with governance or the address with authority. * Params: `0x00 | ProtocolBuffer(Params)` diff --git a/x/slashing/keeper/hooks.go b/x/slashing/keeper/hooks.go index 2bd081a3d867..42eacfc5ee46 100644 --- a/x/slashing/keeper/hooks.go +++ b/x/slashing/keeper/hooks.go @@ -98,7 +98,7 @@ func (h Hooks) AfterUnbondingInitiated(_ context.Context, _ uint64) error { return nil } -// AfterConsensusPubKeyUpdate triggers the functions to rotate the signing-infos also sets address pubkey relation. +// AfterConsensusPubKeyUpdate handles the rotation of signing info and updates the address-pubkey relation after a consensus key update. func (h Hooks) AfterConsensusPubKeyUpdate(ctx context.Context, oldPubKey, newPubKey cryptotypes.PubKey, _ sdk.Coin) error { if err := h.k.performConsensusPubKeyUpdate(ctx, oldPubKey, newPubKey); err != nil { return err diff --git a/x/slashing/keeper/infractions.go b/x/slashing/keeper/infractions.go index e1712c3f84f6..997f854dae7d 100644 --- a/x/slashing/keeper/infractions.go +++ b/x/slashing/keeper/infractions.go @@ -13,7 +13,7 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" ) -// HandleValidatorSignature handles a validator signature, must be called once per validator per block. +// HandleValidatorSignature handles a validator signature, must be called once per validator for each block. func (k Keeper) HandleValidatorSignature(ctx context.Context, addr cryptotypes.Address, power int64, signed comet.BlockIDFlag) error { params, err := k.Params.Get(ctx) if err != nil { @@ -22,6 +22,7 @@ func (k Keeper) HandleValidatorSignature(ctx context.Context, addr cryptotypes.A return k.HandleValidatorSignatureWithParams(ctx, params, addr, power, signed) } +// HandleValidatorSignature handles a validator signature with the provided slashing module params. func (k Keeper) HandleValidatorSignatureWithParams(ctx context.Context, params types.Params, addr cryptotypes.Address, power int64, signed comet.BlockIDFlag) error { height := k.HeaderService.HeaderInfo(ctx).Height @@ -38,7 +39,7 @@ func (k Keeper) HandleValidatorSignatureWithParams(ctx context.Context, params t return nil } - // read the cons address again because validator may've rotated it's key + // read the cons address again because validator may've rotated its key valConsAddr, err := val.GetConsAddr() if err != nil { return err diff --git a/x/slashing/keeper/signing_info.go b/x/slashing/keeper/signing_info.go index 64e4b7421066..8270df52e217 100644 --- a/x/slashing/keeper/signing_info.go +++ b/x/slashing/keeper/signing_info.go @@ -83,8 +83,8 @@ func (k Keeper) SetMissedBlockBitmapChunk(ctx context.Context, addr sdk.ConsAddr return k.ValidatorMissedBlockBitmap.Set(ctx, collections.Join(addr.Bytes(), uint64(chunkIndex)), chunk) } -// getPreviousConsKey checks if the key rotated, returns the old consKey to get the missed blocks -// because missed blocks are still pointing to the old key +// getPreviousConsKey returns the old consensus key if it has rotated, +// allowing retrieval of missed blocks associated with the old key. func (k Keeper) getPreviousConsKey(ctx context.Context, addr sdk.ConsAddress) (sdk.ConsAddress, error) { oldPk, err := k.sk.ValidatorIdentifier(ctx, addr) if err != nil { @@ -105,8 +105,7 @@ func (k Keeper) getPreviousConsKey(ctx context.Context, addr sdk.ConsAddress) (s // IndexOffset modulo SignedBlocksWindow. This index is used to fetch the chunk // in the bitmap and the relative bit in that chunk. func (k Keeper) GetMissedBlockBitmapValue(ctx context.Context, addr sdk.ConsAddress, index int64) (bool, error) { - // check the key rotated, if rotated use the returned consKey to get the missed blocks - // because missed blocks are still pointing to the old key + // get the old consensus key if it has rotated, allowing retrieval of missed blocks associated with the old key addr, err := k.getPreviousConsKey(ctx, addr) if err != nil { return false, err @@ -141,8 +140,7 @@ func (k Keeper) GetMissedBlockBitmapValue(ctx context.Context, addr sdk.ConsAddr // index is used to fetch the chunk in the bitmap and the relative bit in that // chunk. func (k Keeper) SetMissedBlockBitmapValue(ctx context.Context, addr sdk.ConsAddress, index int64, missed bool) error { - // check the key rotated, if rotated use the returned consKey to get the missed blocks - // because missed blocks are still pointing to the old key + // get the old consensus key if it has rotated, allowing retrieval of missed blocks associated with the old key addr, err := k.getPreviousConsKey(ctx, addr) if err != nil { return err @@ -181,8 +179,7 @@ func (k Keeper) SetMissedBlockBitmapValue(ctx context.Context, addr sdk.ConsAddr // DeleteMissedBlockBitmap removes a validator's missed block bitmap from state. func (k Keeper) DeleteMissedBlockBitmap(ctx context.Context, addr sdk.ConsAddress) error { - // check the key rotated, if rotated use the returned consKey to delete the missed blocks - // because missed blocks are still pointing to the old key + // get the old consensus key if it has rotated, allowing retrieval of missed blocks associated with the old key addr, err := k.getPreviousConsKey(ctx, addr) if err != nil { return err @@ -239,8 +236,8 @@ func (k Keeper) GetValidatorMissedBlocks(ctx context.Context, addr sdk.ConsAddre return missedBlocks, err } -// performConsensusPubKeyUpdate updates cons address to its pub key relation -// Updates signing info, missed blocks (removes old one, and sets new one) +// performConsensusPubKeyUpdate updates the consensus address-pubkey relation +// and refreshes the signing info by replacing the old key with the new one. func (k Keeper) performConsensusPubKeyUpdate(ctx context.Context, oldPubKey, newPubKey cryptotypes.PubKey) error { // Connect new consensus address with PubKey if err := k.AddrPubkeyRelation.Set(ctx, newPubKey.Address(), newPubKey); err != nil { diff --git a/x/slashing/keeper/signing_info_test.go b/x/slashing/keeper/signing_info_test.go index 288cdfcb4b2f..8de1e0b83cd7 100644 --- a/x/slashing/keeper/signing_info_test.go +++ b/x/slashing/keeper/signing_info_test.go @@ -101,7 +101,7 @@ func (s *KeeperTestSuite) TestValidatorMissedBlockBitmap_SmallWindow() { require.NoError(err) require.Len(missedBlocks, int(params.SignedBlocksWindow)-1) - // if the validator rotated it's key there will be different consKeys and a mapping will be added in the state. + // if the validator rotated its key, there will be different consKeys and a mapping will be added in the state consAddr1 := sdk.ConsAddress("addr1_______________") s.stakingKeeper.EXPECT().ValidatorIdentifier(gomock.Any(), consAddr1).Return(consAddr, nil).AnyTimes() @@ -147,12 +147,12 @@ func (s *KeeperTestSuite) TestPerformConsensusPubKeyUpdate() { require.NoError(err) require.Equal(savedPubKey, pks[1]) - // check validator SigningInfo is set properly to new consensus pubkey + // check validator's SigningInfo is set properly with new consensus pubkey signingInfo, err := slashingKeeper.ValidatorSigningInfo.Get(ctx, newConsAddr) require.NoError(err) require.Equal(signingInfo, newInfo) - // missed blocks maps to old cons key only since there is a identifier added to get the missed blocks using the new cons key. + // missed blocks map corresponds only to the old cons key, as there is an identifier added to get the missed blocks using the new cons key missedBlocks, err := slashingKeeper.GetValidatorMissedBlocks(ctx, oldConsAddr) require.NoError(err) diff --git a/x/slashing/module.go b/x/slashing/module.go index c8aabf12bad2..b5f018101602 100644 --- a/x/slashing/module.go +++ b/x/slashing/module.go @@ -85,19 +85,19 @@ func (AppModule) RegisterLegacyAminoCodec(cdc legacy.Amino) { types.RegisterLegacyAminoCodec(cdc) } -// RegisterInterfaces registers the module's interface types +// RegisterInterfaces registers the slashing module's interface types func (AppModule) RegisterInterfaces(registrar registry.InterfaceRegistrar) { types.RegisterInterfaces(registrar) } -// RegisterGRPCGatewayRoutes registers the gRPC Gateway routes for the slashig module. +// RegisterGRPCGatewayRoutes registers the gRPC Gateway routes for the slashing module. func (AppModule) RegisterGRPCGatewayRoutes(clientCtx client.Context, mux *gwruntime.ServeMux) { if err := types.RegisterQueryHandlerClient(context.Background(), mux, types.NewQueryClient(clientCtx)); err != nil { panic(err) } } -// RegisterServices registers module services. +// RegisterServices registers slashing module's services. func (am AppModule) RegisterServices(registrar grpc.ServiceRegistrar) error { types.RegisterMsgServer(registrar, keeper.NewMsgServerImpl(am.keeper)) types.RegisterQueryServer(registrar, keeper.NewQuerier(am.keeper)) @@ -105,7 +105,7 @@ func (am AppModule) RegisterServices(registrar grpc.ServiceRegistrar) error { return nil } -// RegisterMigrations registers module migrations. +// RegisterMigrations registers slashing module's migrations. func (am AppModule) RegisterMigrations(mr appmodule.MigrationRegistrar) error { m := keeper.NewMigrator(am.keeper, am.stakingKeeper.ValidatorAddressCodec()) diff --git a/x/slashing/simulation/proposals.go b/x/slashing/simulation/proposals.go index d625360de80f..a0a382509e79 100644 --- a/x/slashing/simulation/proposals.go +++ b/x/slashing/simulation/proposals.go @@ -36,7 +36,7 @@ func ProposalMsgs() []simtypes.WeightedProposalMsg { // SimulateMsgUpdateParams returns a random MsgUpdateParams func SimulateMsgUpdateParams(_ context.Context, r *rand.Rand, _ []simtypes.Account, ac coreaddress.Codec) (sdk.Msg, error) { // use the default gov module account address as authority - var authority sdk.AccAddress = address.Module("gov") + var authority sdk.AccAddress = address.Module(types.GovModuleName) authorityAddr, err := ac.BytesToString(authority) if err != nil { diff --git a/x/slashing/simulation/proposals_test.go b/x/slashing/simulation/proposals_test.go index f741f9ff689b..f8ce0f5fec79 100644 --- a/x/slashing/simulation/proposals_test.go +++ b/x/slashing/simulation/proposals_test.go @@ -40,7 +40,7 @@ func TestProposalMsgs(t *testing.T) { msgUpdateParams, ok := msg.(*types.MsgUpdateParams) assert.Assert(t, ok) - moduleAddr, err := ac.BytesToString(address.Module("gov")) + moduleAddr, err := ac.BytesToString(address.Module(types.GovModuleName)) assert.NilError(t, err) assert.Equal(t, moduleAddr, msgUpdateParams.Authority)