From 1bc7ff528c2186befe3716a7716502bf7ee272cf Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Tue, 13 Aug 2024 22:14:05 +0200 Subject: [PATCH] refactor(x/slashing): audit x/slashing changes (backport #21270) (#21279) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Julián Toledano --- x/slashing/CHANGELOG.md | 2 +- x/slashing/abci.go | 6 +++--- x/slashing/keeper/grpc_query.go | 4 ++-- x/slashing/keeper/grpc_query_test.go | 8 +++----- x/slashing/keeper/infractions.go | 8 ++++---- x/slashing/keeper/keeper.go | 12 ++++-------- x/slashing/keeper/keeper_test.go | 4 ++-- x/slashing/keeper/signing_info.go | 5 ++--- x/slashing/keeper/signing_info_test.go | 6 +++--- x/slashing/migrations/v4/migrate.go | 12 ++++++------ x/slashing/migrations/v4/migrate_test.go | 2 +- x/slashing/simulation/genesis_test.go | 2 -- 12 files changed, 31 insertions(+), 40 deletions(-) diff --git a/x/slashing/CHANGELOG.md b/x/slashing/CHANGELOG.md index 787d759973..d4951e5a0e 100644 --- a/x/slashing/CHANGELOG.md +++ b/x/slashing/CHANGELOG.md @@ -29,7 +29,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Improvements -* [#19458](https://github.com/cosmos/cosmos-sdk/pull/19458) Avoid writing SignInfo's for validator's who did not miss a block. (Every BeginBlock) +* [#19458](https://github.com/cosmos/cosmos-sdk/pull/19458) Avoid writing SignInfo's for validators who did not miss a block. (Every BeginBlock) * [#18959](https://github.com/cosmos/cosmos-sdk/pull/18959) Avoid deserialization of parameters with every validator lookup * [#18636](https://github.com/cosmos/cosmos-sdk/pull/18636) `JailUntil` and `Tombstone` methods no longer panics if the signing info does not exist for the validator but instead returns error. diff --git a/x/slashing/abci.go b/x/slashing/abci.go index 66cf3feff6..e99b6acea5 100644 --- a/x/slashing/abci.go +++ b/x/slashing/abci.go @@ -15,9 +15,9 @@ import ( func BeginBlocker(ctx context.Context, k keeper.Keeper, cometService comet.Service) error { defer telemetry.ModuleMeasureSince(types.ModuleName, telemetry.Now(), telemetry.MetricKeyBeginBlocker) - // Iterate over all the validators which *should* have signed this block - // store whether or not they have actually signed it and slash/unbond any - // which have missed too many blocks in a row (downtime slashing) + // Retrieve CometBFT info, then iterate through all validator votes + // from the last commit. For each vote, handle the validator's signature, potentially + // slashing or unbonding validators who have missed too many blocks. params, err := k.Params.Get(ctx) if err != nil { return err diff --git a/x/slashing/keeper/grpc_query.go b/x/slashing/keeper/grpc_query.go index fa3167168a..9ac25cf625 100644 --- a/x/slashing/keeper/grpc_query.go +++ b/x/slashing/keeper/grpc_query.go @@ -34,7 +34,7 @@ func (k Querier) Params(ctx context.Context, req *types.QueryParamsRequest) (*ty } // SigningInfo returns signing-info of a specific validator. -func (k Keeper) SigningInfo(ctx context.Context, req *types.QuerySigningInfoRequest) (*types.QuerySigningInfoResponse, error) { +func (k Querier) SigningInfo(ctx context.Context, req *types.QuerySigningInfoRequest) (*types.QuerySigningInfoResponse, error) { if req == nil { return nil, status.Errorf(codes.InvalidArgument, "empty request") } @@ -57,7 +57,7 @@ func (k Keeper) SigningInfo(ctx context.Context, req *types.QuerySigningInfoRequ } // SigningInfos returns signing-infos of all validators. -func (k Keeper) SigningInfos(ctx context.Context, req *types.QuerySigningInfosRequest) (*types.QuerySigningInfosResponse, error) { +func (k Querier) SigningInfos(ctx context.Context, req *types.QuerySigningInfosRequest) (*types.QuerySigningInfosResponse, error) { if req == nil { return nil, status.Errorf(codes.InvalidArgument, "empty request") } diff --git a/x/slashing/keeper/grpc_query_test.go b/x/slashing/keeper/grpc_query_test.go index 89afc2ae7f..84acf9a32e 100644 --- a/x/slashing/keeper/grpc_query_test.go +++ b/x/slashing/keeper/grpc_query_test.go @@ -45,10 +45,8 @@ func (s *KeeperTestSuite) TestGRPCSigningInfo() { info, err := keeper.ValidatorSigningInfo.Get(ctx, consAddr) require.NoError(err) - consAddrStr, err := s.stakingKeeper.ConsensusAddressCodec().BytesToString(consAddr) - require.NoError(err) infoResp, err = queryClient.SigningInfo(gocontext.Background(), - &slashingtypes.QuerySigningInfoRequest{ConsAddress: consAddrStr}) + &slashingtypes.QuerySigningInfoRequest{ConsAddress: consStr}) require.NoError(err) require.Equal(info, infoResp.ValSigningInfo) } @@ -58,10 +56,10 @@ func (s *KeeperTestSuite) TestGRPCSigningInfos() { require := s.Require() // set two validator signing information - consAddr1 := sdk.ConsAddress(sdk.AccAddress([]byte("addr1_______________"))) + consAddr1 := sdk.ConsAddress("addr1_______________") consStr1, err := s.stakingKeeper.ConsensusAddressCodec().BytesToString(consAddr1) require.NoError(err) - consAddr2 := sdk.ConsAddress(sdk.AccAddress([]byte("addr2_______________"))) + consAddr2 := sdk.ConsAddress("addr2_______________") signingInfo := slashingtypes.NewValidatorSigningInfo( consStr1, 0, diff --git a/x/slashing/keeper/infractions.go b/x/slashing/keeper/infractions.go index df190f43c5..e1712c3f84 100644 --- a/x/slashing/keeper/infractions.go +++ b/x/slashing/keeper/infractions.go @@ -28,12 +28,12 @@ func (k Keeper) HandleValidatorSignatureWithParams(ctx context.Context, params t // fetch the validator public key consAddr := sdk.ConsAddress(addr) - // don't update missed blocks when validator's jailed val, err := k.sk.ValidatorByConsAddr(ctx, consAddr) if err != nil { return err } + // don't update missed blocks when validator's jailed if val.IsJailed() { return nil } @@ -142,11 +142,11 @@ func (k Keeper) HandleValidatorSignatureWithParams(ctx context.Context, params t } if validator != nil && !validator.IsJailed() { // Downtime confirmed: slash and jail the validator - // We need to retrieve the stake distribution which signed the block, so we subtract ValidatorUpdateDelay from the evidence height, + // We need to retrieve the stake distribution that signed the block. To do this, we subtract ValidatorUpdateDelay from the evidence height, // and subtract an additional 1 since this is the LastCommit. - // Note that this *can* result in a negative "distributionHeight" up to -ValidatorUpdateDelay-1, + // Note that this *can* result in a negative "distributionHeight" of up to -ValidatorUpdateDelay-1, // i.e. at the end of the pre-genesis block (none) = at the beginning of the genesis block. - // That's fine since this is just used to filter unbonding delegations & redelegations. + // This is acceptable since it's only used to filter unbonding delegations & redelegations. distributionHeight := height - sdk.ValidatorUpdateDelay - 1 slashFractionDowntime, err := k.SlashFractionDowntime(ctx) diff --git a/x/slashing/keeper/keeper.go b/x/slashing/keeper/keeper.go index 1aa8c1f8c5..8ea641ca4b 100644 --- a/x/slashing/keeper/keeper.go +++ b/x/slashing/keeper/keeper.go @@ -90,13 +90,13 @@ func (k Keeper) GetPubkey(ctx context.Context, a cryptotypes.Address) (cryptotyp } // Slash attempts to slash a validator. The slash is delegated to the staking -// module to make the necessary validator changes. It specifies no intraction reason. +// module to make the necessary validator changes. It specifies no infraction reason. func (k Keeper) Slash(ctx context.Context, consAddr sdk.ConsAddress, fraction sdkmath.LegacyDec, power, distributionHeight int64) error { return k.SlashWithInfractionReason(ctx, consAddr, fraction, power, distributionHeight, st.Infraction_INFRACTION_UNSPECIFIED) } // SlashWithInfractionReason attempts to slash a validator. The slash is delegated to the staking -// module to make the necessary validator changes. It specifies an intraction reason. +// module to make the necessary validator changes. It specifies an infraction reason. func (k Keeper) SlashWithInfractionReason(ctx context.Context, consAddr sdk.ConsAddress, fraction sdkmath.LegacyDec, power, distributionHeight int64, infraction st.Infraction) error { coinsBurned, err := k.sk.SlashWithInfractionReason(ctx, consAddr, distributionHeight, power, fraction, infraction) if err != nil { @@ -137,11 +137,7 @@ func (k Keeper) Jail(ctx context.Context, consAddr sdk.ConsAddress) error { return err } - if err := k.EventService.EventManager(ctx).EmitKV( + return k.EventService.EventManager(ctx).EmitKV( types.EventTypeSlash, - event.NewAttribute(types.AttributeKeyJailed, consStr), - ); err != nil { - return err - } - return nil + event.NewAttribute(types.AttributeKeyJailed, consStr)) } diff --git a/x/slashing/keeper/keeper_test.go b/x/slashing/keeper/keeper_test.go index 37878d830c..5223244db9 100644 --- a/x/slashing/keeper/keeper_test.go +++ b/x/slashing/keeper/keeper_test.go @@ -29,7 +29,7 @@ import ( moduletestutil "github.com/cosmos/cosmos-sdk/types/module/testutil" ) -var consAddr = sdk.ConsAddress(sdk.AccAddress([]byte("addr1_______________"))) +var consAddr = sdk.ConsAddress("addr1_______________") type KeeperTestSuite struct { suite.Suite @@ -153,7 +153,7 @@ func validatorMissedBlockBitmapKey(v sdk.ConsAddress, chunkIndex int64) []byte { func (s *KeeperTestSuite) TestValidatorMissedBlockBMMigrationToColls() { s.SetupTest() - consAddr := sdk.ConsAddress(sdk.AccAddress([]byte("addr1_______________"))) + consAddr := sdk.ConsAddress("addr1_______________") index := int64(0) err := sdktestutil.DiffCollectionsMigration( s.ctx, diff --git a/x/slashing/keeper/signing_info.go b/x/slashing/keeper/signing_info.go index d4ab17a355..352590e24e 100644 --- a/x/slashing/keeper/signing_info.go +++ b/x/slashing/keeper/signing_info.go @@ -23,8 +23,7 @@ func (k Keeper) HasValidatorSigningInfo(ctx context.Context, consAddr sdk.ConsAd return err == nil && has } -// JailUntil attempts to set a validator's JailedUntil attribute in its signing -// info. +// JailUntil attempts to set a validator's JailedUntil attribute in its signing info. func (k Keeper) JailUntil(ctx context.Context, consAddr sdk.ConsAddress, jailTime time.Time) error { signInfo, err := k.ValidatorSigningInfo.Get(ctx, consAddr) if err != nil { @@ -32,7 +31,7 @@ func (k Keeper) JailUntil(ctx context.Context, consAddr sdk.ConsAddress, jailTim if err != nil { return types.ErrNoSigningInfoFound.Wrapf("could not convert consensus address to string. Error: %s", err.Error()) } - return errorsmod.Wrap(err, fmt.Sprintf("cannot jail validator with consensus address %s that does not have any signing information", addr)) + return types.ErrNoSigningInfoFound.Wrapf(fmt.Sprintf("cannot jail validator with consensus address %s that does not have any signing information", addr)) } signInfo.JailedUntil = jailTime diff --git a/x/slashing/keeper/signing_info_test.go b/x/slashing/keeper/signing_info_test.go index b616bc9e6c..288cdfcb4b 100644 --- a/x/slashing/keeper/signing_info_test.go +++ b/x/slashing/keeper/signing_info_test.go @@ -102,7 +102,7 @@ func (s *KeeperTestSuite) TestValidatorMissedBlockBitmap_SmallWindow() { 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. - consAddr1 := sdk.ConsAddress(sdk.AccAddress([]byte("addr1_______________"))) + consAddr1 := sdk.ConsAddress("addr1_______________") s.stakingKeeper.EXPECT().ValidatorIdentifier(gomock.Any(), consAddr1).Return(consAddr, nil).AnyTimes() missedBlocks, err = keeper.GetValidatorMissedBlocks(ctx, consAddr1) @@ -121,11 +121,11 @@ func (s *KeeperTestSuite) TestPerformConsensusPubKeyUpdate() { oldConsAddr := sdk.ConsAddress(pks[0].Address()) newConsAddr := sdk.ConsAddress(pks[1].Address()) - consAddr, err := s.stakingKeeper.ConsensusAddressCodec().BytesToString(newConsAddr) + consStrAddr, err := s.stakingKeeper.ConsensusAddressCodec().BytesToString(newConsAddr) s.Require().NoError(err) newInfo := slashingtypes.NewValidatorSigningInfo( - consAddr, + consStrAddr, int64(4), time.Unix(2, 0).UTC(), false, diff --git a/x/slashing/migrations/v4/migrate.go b/x/slashing/migrations/v4/migrate.go index 331068c328..454995300d 100644 --- a/x/slashing/migrations/v4/migrate.go +++ b/x/slashing/migrations/v4/migrate.go @@ -63,7 +63,7 @@ func Migrate(ctx context.Context, cdc codec.BinaryCodec, store storetypes.KVStor } func iterateValidatorSigningInfos( - ctx context.Context, + _ context.Context, cdc codec.BinaryCodec, store storetypes.KVStore, cb func(address sdk.ConsAddress, info types.ValidatorSigningInfo) (stop bool), @@ -72,18 +72,18 @@ func iterateValidatorSigningInfos( defer iter.Close() for ; iter.Valid(); iter.Next() { - address := ValidatorSigningInfoAddress(iter.Key()) + addr := ValidatorSigningInfoAddress(iter.Key()) var info types.ValidatorSigningInfo cdc.MustUnmarshal(iter.Value(), &info) - if cb(address, info) { + if cb(addr, info) { break } } } func iterateValidatorMissedBlockBitArray( - ctx context.Context, + _ context.Context, cdc codec.BinaryCodec, store storetypes.KVStore, addr sdk.ConsAddress, @@ -120,7 +120,7 @@ func GetValidatorMissedBlocks( return missedBlocks } -func deleteValidatorMissedBlockBitArray(ctx context.Context, store storetypes.KVStore, addr sdk.ConsAddress) { +func deleteValidatorMissedBlockBitArray(_ context.Context, store storetypes.KVStore, addr sdk.ConsAddress) { iter := storetypes.KVStorePrefixIterator(store, validatorMissedBlockBitArrayPrefixKey(addr)) defer iter.Close() @@ -129,7 +129,7 @@ func deleteValidatorMissedBlockBitArray(ctx context.Context, store storetypes.KV } } -func setMissedBlockBitmapValue(ctx context.Context, store storetypes.KVStore, addr sdk.ConsAddress, index int64, missed bool) error { +func setMissedBlockBitmapValue(_ context.Context, store storetypes.KVStore, addr sdk.ConsAddress, index int64, missed bool) error { // get the chunk or "word" in the logical bitmap chunkIndex := index / MissedBlockBitmapChunkSize key := ValidatorMissedBlockBitmapKey(addr, chunkIndex) diff --git a/x/slashing/migrations/v4/migrate_test.go b/x/slashing/migrations/v4/migrate_test.go index 84fe795446..94b39e492f 100644 --- a/x/slashing/migrations/v4/migrate_test.go +++ b/x/slashing/migrations/v4/migrate_test.go @@ -19,7 +19,7 @@ import ( moduletestutil "github.com/cosmos/cosmos-sdk/types/module/testutil" ) -var consAddr = sdk.ConsAddress(sdk.AccAddress([]byte("addr1_______________"))) +var consAddr = sdk.ConsAddress("addr1_______________") func TestMigrate(t *testing.T) { cdc := moduletestutil.MakeTestEncodingConfig(codectestutil.CodecOptions{}, slashing.AppModule{}).Codec diff --git a/x/slashing/simulation/genesis_test.go b/x/slashing/simulation/genesis_test.go index 1748645ec2..c4626a7e20 100644 --- a/x/slashing/simulation/genesis_test.go +++ b/x/slashing/simulation/genesis_test.go @@ -83,8 +83,6 @@ func TestRandomizedGenState1(t *testing.T) { } for _, tt := range tests { - tt := tt - require.Panicsf(t, func() { simulation.RandomizedGenState(&tt.simState) }, tt.panicMsg) } }