From 41f922f68e65645717da5215cb3055f035a07cb2 Mon Sep 17 00:00:00 2001 From: Sergio Mena Date: Tue, 7 May 2024 19:37:50 +0200 Subject: [PATCH 1/5] Apply ConsensusParam update rules according to CometBFT spec --- x/consensus/keeper/keeper.go | 19 ++++- x/consensus/keeper/keeper_test.go | 125 +++++++++++++++++++++++++----- 2 files changed, 123 insertions(+), 21 deletions(-) diff --git a/x/consensus/keeper/keeper.go b/x/consensus/keeper/keeper.go index 52dfd6740f6c..f140848ec80b 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,25 @@ func (k Keeper) UpdateParams(ctx context.Context, msg *types.MsgUpdateParams) (* if err != nil { return nil, err } - if err := cmttypes.ConsensusParamsFromProto(consensusParams).ValidateBasic(); err != nil { + + sdkCtx := sdk.UnwrapSDKContext(ctx) + 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 := params.ValidateUpdate(&consensusParams, sdkCtx.BlockHeight()); 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 } diff --git a/x/consensus/keeper/keeper_test.go b/x/consensus/keeper/keeper_test.go index ce95158652bd..c349c8dbaffe 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) From f9b237cd147cb78b7b587032f8b2cad5232c65c8 Mon Sep 17 00:00:00 2001 From: Sergio Mena Date: Wed, 8 May 2024 16:27:41 +0200 Subject: [PATCH 2/5] Apply @tac0turtle suggestions from code review Co-authored-by: Marko --- x/consensus/keeper/keeper.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/x/consensus/keeper/keeper.go b/x/consensus/keeper/keeper.go index f140848ec80b..aa4b5e4ae5fd 100644 --- a/x/consensus/keeper/keeper.go +++ b/x/consensus/keeper/keeper.go @@ -74,7 +74,6 @@ func (k Keeper) UpdateParams(ctx context.Context, msg *types.MsgUpdateParams) (* return nil, err } - sdkCtx := sdk.UnwrapSDKContext(ctx) paramsProto, err := k.ParamsStore.Get(ctx) if err != nil { return nil, err @@ -87,7 +86,7 @@ func (k Keeper) UpdateParams(ctx context.Context, msg *types.MsgUpdateParams) (* return nil, err } - if err := params.ValidateUpdate(&consensusParams, sdkCtx.BlockHeight()); err != nil { + if err := params.ValidateUpdate(&consensusParams, k.HeaderService.HeaderInfo(ctx).Height); err != nil { return nil, err } From 37f38c62aa4a800f627fbc828715d8aaf660e49d Mon Sep 17 00:00:00 2001 From: marbar3778 Date: Tue, 14 May 2024 14:46:48 +0200 Subject: [PATCH 3/5] fix build --- x/consensus/keeper/keeper.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/x/consensus/keeper/keeper.go b/x/consensus/keeper/keeper.go index aa4b5e4ae5fd..c78fdb69d5b9 100644 --- a/x/consensus/keeper/keeper.go +++ b/x/consensus/keeper/keeper.go @@ -86,7 +86,9 @@ func (k Keeper) UpdateParams(ctx context.Context, msg *types.MsgUpdateParams) (* return nil, err } - if err := params.ValidateUpdate(&consensusParams, k.HeaderService.HeaderInfo(ctx).Height); err != nil { + sdkCtx := sdk.UnwrapSDKContext(ctx) + + if err := params.ValidateUpdate(&consensusParams, sdkCtx.HeaderInfo().Height); err != nil { return nil, err } From d2b74b92622648a9b75ac28465aeab536765fbfd Mon Sep 17 00:00:00 2001 From: sontrinh16 Date: Mon, 27 May 2024 16:35:11 +0700 Subject: [PATCH 4/5] fix test --- x/consensus/keeper/keeper.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/consensus/keeper/keeper.go b/x/consensus/keeper/keeper.go index c78fdb69d5b9..50b5b2d93f6c 100644 --- a/x/consensus/keeper/keeper.go +++ b/x/consensus/keeper/keeper.go @@ -88,7 +88,7 @@ func (k Keeper) UpdateParams(ctx context.Context, msg *types.MsgUpdateParams) (* sdkCtx := sdk.UnwrapSDKContext(ctx) - if err := params.ValidateUpdate(&consensusParams, sdkCtx.HeaderInfo().Height); err != nil { + if err := params.ValidateUpdate(&consensusParams, sdkCtx.BlockHeader().Height); err != nil { return nil, err } From e4a9fd2eceb45ecdffcea84dedd1504529cafcce Mon Sep 17 00:00:00 2001 From: marbar3778 Date: Tue, 28 May 2024 12:08:23 +0200 Subject: [PATCH 5/5] add changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7e280ccadb73..e44df6363971 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Improvements * (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. ### Bug Fixes