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

refactor(x/slashing): audit QA #21477

Merged
merged 2 commits into from
Sep 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down
1 change: 1 addition & 0 deletions x/slashing/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down
2 changes: 1 addition & 1 deletion x/slashing/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)`
Expand Down
2 changes: 1 addition & 1 deletion x/slashing/keeper/hooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 3 additions & 2 deletions x/slashing/keeper/infractions.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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

Expand All @@ -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
Expand Down
17 changes: 7 additions & 10 deletions x/slashing/keeper/signing_info.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down
6 changes: 3 additions & 3 deletions x/slashing/keeper/signing_info_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down Expand Up @@ -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)

Expand Down
8 changes: 4 additions & 4 deletions x/slashing/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,27 +85,27 @@ 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))

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())

Expand Down
2 changes: 1 addition & 1 deletion x/slashing/simulation/proposals.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion x/slashing/simulation/proposals_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading