From d180df817efcc1b4ad1d5b146209e13d115dfa1f Mon Sep 17 00:00:00 2001 From: Marko Date: Tue, 28 May 2024 12:25:56 +0200 Subject: [PATCH] fix(x/consensus): harden consensus params proposal (#20381) Co-authored-by: Sergio Mena Co-authored-by: sontrinh16 --- CHANGELOG.md | 1 + x/consensus/keeper/keeper.go | 20 ++++- x/consensus/keeper/keeper_test.go | 125 +++++++++++++++++++++++++----- 3 files changed, 125 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0b67b0766d..cc483a7590 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/x/consensus/keeper/keeper.go b/x/consensus/keeper/keeper.go index 52dfd6740f..50b5b2d93f 100644 --- a/x/consensus/keeper/keeper.go +++ b/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 } - if err := k.ParamsStore.Set(ctx, consensusParams); err != nil { + sdkCtx := sdk.UnwrapSDKContext(ctx) + + if err := params.ValidateUpdate(&consensusParams, sdkCtx.BlockHeader().Height); err != nil { + return nil, err + } + + if err := k.ParamsStore.Set(ctx, nextParams.ToProto()); err != nil { return nil, err } diff --git a/x/consensus/keeper/keeper_test.go b/x/consensus/keeper/keeper_test.go index ce95158652..c349c8dbaf 100644 --- a/x/consensus/keeper/keeper_test.go +++ b/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,6 +44,8 @@ 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) { @@ -50,7 +53,14 @@ func TestKeeperTestSuite(t *testing.T) { } 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)