fix(x/consensus): harden consensus params proposal

CHANGELOG.md

@@ -42,6 +42,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 * (debug) [#20328](https://github.com/cosmos/cosmos-sdk/pull/20328) Add consensus address for debug cmd.
 * (runtime) [#20264](https://github.com/cosmos/cosmos-sdk/pull/20264) Expose grpc query router via depinject.
+* (x/consensus) [#20381](https://github.com/cosmos/cosmos-sdk/pull/20381) Use Comet utility for consensus module consensus param updates.
 * (client) [#20356](https://github.com/cosmos/cosmos-sdk/pull/20356) Overwrite client context when available in `SetCmdClientContext`.
 
 ### Bug Fixes

x/consensus/keeper/keeper.go

@@ -14,6 +14,7 @@ import (
 	"cosmossdk.io/errors"
 
 	"github.com/cosmos/cosmos-sdk/codec"
+	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/cosmos-sdk/x/consensus/exported"
 	"github.com/cosmos/cosmos-sdk/x/consensus/types"
 	govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
@@ -72,11 +73,26 @@ func (k Keeper) UpdateParams(ctx context.Context, msg *types.MsgUpdateParams) (*
 	if err != nil {
 		return nil, err
 	}
-	if err := cmttypes.ConsensusParamsFromProto(consensusParams).ValidateBasic(); err != nil {
+
+	paramsProto, err := k.ParamsStore.Get(ctx)
+	if err != nil {
+		return nil, err
+	}
+	params := cmttypes.ConsensusParamsFromProto(paramsProto)
+
+	nextParams := params.Update(&consensusParams)
+
+	if err := nextParams.ValidateBasic(); err != nil {
+		return nil, err
+	}
+
+	sdkCtx := sdk.UnwrapSDKContext(ctx)
+
+	if err := params.ValidateUpdate(&consensusParams, sdkCtx.BlockHeader().Height); err != nil {
 		return nil, err
 	}
 
-	if err := k.ParamsStore.Set(ctx, consensusParams); err != nil {
+	if err := k.ParamsStore.Set(ctx, nextParams.ToProto()); err != nil {
 		return nil, err
 	}
 

x/consensus/keeper/keeper_test.go

@@ -30,7 +30,8 @@ type KeeperTestSuite struct {
 func (s *KeeperTestSuite) SetupTest() {
 	key := storetypes.NewKVStoreKey(consensusparamkeeper.StoreKey)
 	testCtx := testutil.DefaultContextWithDB(s.T(), key, storetypes.NewTransientStoreKey("transient_test"))
-	ctx := testCtx.Ctx.WithBlockHeader(cmtproto.Header{})
+	header := cmtproto.Header{Height: 5}
+	ctx := testCtx.Ctx.WithBlockHeader(header)
 	encCfg := moduletestutil.MakeTestEncodingConfig()
 	storeService := runtime.NewKVStoreService(key)
 
@@ -43,14 +44,23 @@ func (s *KeeperTestSuite) SetupTest() {
 	queryHelper := baseapp.NewQueryServerTestHelper(ctx, encCfg.InterfaceRegistry)
 	types.RegisterQueryServer(queryHelper, keeper)
 	s.queryClient = types.NewQueryClient(queryHelper)
+	err := s.consensusParamsKeeper.ParamsStore.Set(ctx, cmttypes.DefaultConsensusParams().ToProto())
+	s.Require().NoError(err)
 }
 
 func TestKeeperTestSuite(t *testing.T) {
 	suite.Run(t, new(KeeperTestSuite))
 }
 
 func (s *KeeperTestSuite) TestGRPCQueryConsensusParams() {
-	defaultConsensusParams := cmttypes.DefaultConsensusParams().ToProto()
+	// Create ConsensusParams with modified fields
+	modifiedConsensusParams := cmttypes.DefaultConsensusParams().ToProto()
+	modifiedConsensusParams.Block.MaxBytes++
+	modifiedConsensusParams.Block.MaxGas = 100
+	modifiedConsensusParams.Evidence.MaxAgeDuration++
+	modifiedConsensusParams.Evidence.MaxAgeNumBlocks++
+	modifiedConsensusParams.Evidence.MaxBytes++
+	modifiedConsensusParams.Validator.PubKeyTypes = []string{cmttypes.ABCIPubKeyTypeSecp256k1}
 
 	testCases := []struct {
 		msg      string
@@ -65,18 +75,22 @@ func (s *KeeperTestSuite) TestGRPCQueryConsensusParams() {
 			func() {
 				input := &types.MsgUpdateParams{
 					Authority: s.consensusParamsKeeper.GetAuthority(),
-					Block:     defaultConsensusParams.Block,
-					Validator: defaultConsensusParams.Validator,
-					Evidence:  defaultConsensusParams.Evidence,
+					Block:     modifiedConsensusParams.Block,
+					Validator: modifiedConsensusParams.Validator,
+					Evidence:  modifiedConsensusParams.Evidence,
 				}
-				s.consensusParamsKeeper.UpdateParams(s.ctx, input)
+				_, err := s.consensusParamsKeeper.UpdateParams(s.ctx, input)
+				s.Require().NoError(err)
 			},
 			types.QueryParamsResponse{
 				Params: &cmtproto.ConsensusParams{
-					Block:     defaultConsensusParams.Block,
-					Validator: defaultConsensusParams.Validator,
-					Evidence:  defaultConsensusParams.Evidence,
-					Version:   defaultConsensusParams.Version,
+					Block:     modifiedConsensusParams.Block,
+					Validator: modifiedConsensusParams.Validator,
+					Evidence:  modifiedConsensusParams.Evidence,
+					Version:   modifiedConsensusParams.Version,
+					Abci: &cmtproto.ABCIParams{
+						VoteExtensionsEnableHeight: 0,
+					},
 				},
 			},
 			true,
@@ -87,21 +101,22 @@ func (s *KeeperTestSuite) TestGRPCQueryConsensusParams() {
 			func() {
 				input := &types.MsgUpdateParams{
 					Authority: s.consensusParamsKeeper.GetAuthority(),
-					Block:     defaultConsensusParams.Block,
-					Validator: defaultConsensusParams.Validator,
-					Evidence:  defaultConsensusParams.Evidence,
+					Block:     modifiedConsensusParams.Block,
+					Validator: modifiedConsensusParams.Validator,
+					Evidence:  modifiedConsensusParams.Evidence,
 					Abci: &cmtproto.ABCIParams{
 						VoteExtensionsEnableHeight: 1234,
 					},
 				}
-				s.consensusParamsKeeper.UpdateParams(s.ctx, input)
+				_, err := s.consensusParamsKeeper.UpdateParams(s.ctx, input)
+				s.Require().NoError(err)
 			},
 			types.QueryParamsResponse{
 				Params: &cmtproto.ConsensusParams{
-					Block:     defaultConsensusParams.Block,
-					Validator: defaultConsensusParams.Validator,
-					Evidence:  defaultConsensusParams.Evidence,
-					Version:   defaultConsensusParams.Version,
+					Block:     modifiedConsensusParams.Block,
+					Validator: modifiedConsensusParams.Validator,
+					Evidence:  modifiedConsensusParams.Evidence,
+					Version:   modifiedConsensusParams.Version,
 					Abci: &cmtproto.ABCIParams{
 						VoteExtensionsEnableHeight: 1234,
 					},
@@ -204,6 +219,76 @@ func (s *KeeperTestSuite) TestUpdateParams() {
 			expErr:    true,
 			expErrMsg: "all parameters must be present",
 		},
+		{
+			name: "valid ABCI update",
+			input: &types.MsgUpdateParams{
+				Authority: s.consensusParamsKeeper.GetAuthority(),
+				Block:     defaultConsensusParams.Block,
+				Validator: defaultConsensusParams.Validator,
+				Evidence:  defaultConsensusParams.Evidence,
+				Abci: &cmtproto.ABCIParams{
+					VoteExtensionsEnableHeight: 1235,
+				},
+			},
+			expErr:    false,
+			expErrMsg: "",
+		},
+		{
+			name: "noop ABCI update",
+			input: &types.MsgUpdateParams{
+				Authority: s.consensusParamsKeeper.GetAuthority(),
+				Block:     defaultConsensusParams.Block,
+				Validator: defaultConsensusParams.Validator,
+				Evidence:  defaultConsensusParams.Evidence,
+				Abci: &cmtproto.ABCIParams{
+					VoteExtensionsEnableHeight: 1235,
+				},
+			},
+			expErr:    false,
+			expErrMsg: "",
+		},
+		{
+			name: "valid ABCI clear",
+			input: &types.MsgUpdateParams{
+				Authority: s.consensusParamsKeeper.GetAuthority(),
+				Block:     defaultConsensusParams.Block,
+				Validator: defaultConsensusParams.Validator,
+				Evidence:  defaultConsensusParams.Evidence,
+				Abci: &cmtproto.ABCIParams{
+					VoteExtensionsEnableHeight: 0,
+				},
+			},
+			expErr:    false,
+			expErrMsg: "",
+		},
+		{
+			name: "invalid ABCI update - current height",
+			input: &types.MsgUpdateParams{
+				Authority: s.consensusParamsKeeper.GetAuthority(),
+				Block:     defaultConsensusParams.Block,
+				Validator: defaultConsensusParams.Validator,
+				Evidence:  defaultConsensusParams.Evidence,
+				Abci: &cmtproto.ABCIParams{
+					VoteExtensionsEnableHeight: 5,
+				},
+			},
+			expErr:    true,
+			expErrMsg: "vote extensions cannot be updated to a past or current height",
+		},
+		{
+			name: "invalid ABCI update - past height",
+			input: &types.MsgUpdateParams{
+				Authority: s.consensusParamsKeeper.GetAuthority(),
+				Block:     defaultConsensusParams.Block,
+				Validator: defaultConsensusParams.Validator,
+				Evidence:  defaultConsensusParams.Evidence,
+				Abci: &cmtproto.ABCIParams{
+					VoteExtensionsEnableHeight: 4,
+				},
+			},
+			expErr:    true,
+			expErrMsg: "vote extensions cannot be updated to a past or current height",
+		},
 	}
 
 	for _, tc := range testCases {
@@ -220,7 +305,9 @@ func (s *KeeperTestSuite) TestUpdateParams() {
 				res, err := s.consensusParamsKeeper.Params(s.ctx, &types.QueryParamsRequest{})
 				s.Require().NoError(err)
 
-				s.Require().Equal(tc.input.Abci, res.Params.Abci)
+				if tc.input.Abci != nil {
+					s.Require().Equal(tc.input.Abci, res.Params.Abci)
+				}
 				s.Require().Equal(tc.input.Block, res.Params.Block)
 				s.Require().Equal(tc.input.Evidence, res.Params.Evidence)
 				s.Require().Equal(tc.input.Validator, res.Params.Validator)