refactor(x/slashing): audit x/slashing changes

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.
 

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

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")
 	}

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,

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)

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

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,

x/slashing/keeper/signing_info.go

@@ -23,16 +23,15 @@ 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 {
 		addr, err := k.sk.ConsensusAddressCodec().BytesToString(consAddr)
 		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

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,

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)

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

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