From 3c7a090caa0bbc49a099442e307326432df78f44 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Wed, 13 Dec 2023 07:51:33 +0100 Subject: [PATCH] feat(x/gov): better gov genesis validation (backport #18707) (#18712) --- CHANGELOG.md | 1 + go.mod | 2 +- x/gov/types/v1/genesis.go | 73 ++++++++++++++++++++++- x/gov/types/v1/genesis_test.go | 104 ++++++++++++++++++++++++++++----- 4 files changed, 164 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7fce80dadb19..9195332f0f9a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Improvements +* (x/gov) [#18707](https://github.com/cosmos/cosmos-sdk/pull/18707) Improve genesis validation. * (server) [#18478](https://github.com/cosmos/cosmos-sdk/pull/18478) Add command flag to disable colored logs. ### Bug Fixes diff --git a/go.mod b/go.mod index 9a1a1cbf88f1..7bd3b692d31c 100644 --- a/go.mod +++ b/go.mod @@ -58,6 +58,7 @@ require ( github.com/tidwall/btree v1.6.0 golang.org/x/crypto v0.14.0 golang.org/x/exp v0.0.0-20230711153332-06a737ee72cb + golang.org/x/sync v0.3.0 google.golang.org/genproto/googleapis/api v0.0.0-20231002182017-d307bd883b97 google.golang.org/grpc v1.58.3 google.golang.org/protobuf v1.31.0 @@ -164,7 +165,6 @@ require ( go.opencensus.io v0.24.0 // indirect golang.org/x/net v0.17.0 // indirect golang.org/x/oauth2 v0.10.0 // indirect - golang.org/x/sync v0.3.0 // indirect golang.org/x/sys v0.13.0 // indirect golang.org/x/term v0.13.0 // indirect golang.org/x/text v0.13.0 // indirect diff --git a/x/gov/types/v1/genesis.go b/x/gov/types/v1/genesis.go index ef44bd481627..0065eb9ea965 100644 --- a/x/gov/types/v1/genesis.go +++ b/x/gov/types/v1/genesis.go @@ -2,6 +2,9 @@ package v1 import ( "errors" + "fmt" + + "golang.org/x/sync/errgroup" "github.com/cosmos/cosmos-sdk/codec/types" ) @@ -27,13 +30,79 @@ func (data GenesisState) Empty() bool { return data.StartingProposalId == 0 || data.Params == nil } -// ValidateGenesis checks if parameters are within valid ranges +// ValidateGenesis checks if gov genesis state is valid ranges +// It checks if params are in valid ranges +// It also makes sure that the provided proposal IDs are unique and +// that there are no duplicate deposit or vote records and no vote or deposits for non-existent proposals func ValidateGenesis(data *GenesisState) error { if data.StartingProposalId == 0 { return errors.New("starting proposal id must be greater than 0") } - return data.Params.ValidateBasic() + var errGroup errgroup.Group + + // weed out duplicate proposals + proposalIds := make(map[uint64]struct{}) + for _, p := range data.Proposals { + if _, ok := proposalIds[p.Id]; ok { + return fmt.Errorf("duplicate proposal id: %d", p.Id) + } + + proposalIds[p.Id] = struct{}{} + } + + // weed out duplicate deposits + errGroup.Go(func() error { + type depositKey struct { + ProposalId uint64 + Depositor string + } + depositIds := make(map[depositKey]struct{}) + for _, d := range data.Deposits { + if _, ok := proposalIds[d.ProposalId]; !ok { + return fmt.Errorf("deposit %v has non-existent proposal id: %d", d, d.ProposalId) + } + + dk := depositKey{d.ProposalId, d.Depositor} + if _, ok := depositIds[dk]; ok { + return fmt.Errorf("duplicate deposit: %v", d) + } + + depositIds[dk] = struct{}{} + } + + return nil + }) + + // weed out duplicate votes + errGroup.Go(func() error { + type voteKey struct { + ProposalId uint64 + Voter string + } + voteIds := make(map[voteKey]struct{}) + for _, v := range data.Votes { + if _, ok := proposalIds[v.ProposalId]; !ok { + return fmt.Errorf("vote %v has non-existent proposal id: %d", v, v.ProposalId) + } + + vk := voteKey{v.ProposalId, v.Voter} + if _, ok := voteIds[vk]; ok { + return fmt.Errorf("duplicate vote: %v", v) + } + + voteIds[vk] = struct{}{} + } + + return nil + }) + + // verify params + errGroup.Go(func() error { + return data.Params.ValidateBasic() + }) + + return errGroup.Wait() } var _ types.UnpackInterfacesMessage = GenesisState{} diff --git a/x/gov/types/v1/genesis_test.go b/x/gov/types/v1/genesis_test.go index 1d74e2ded392..f8f3ab45413e 100644 --- a/x/gov/types/v1/genesis_test.go +++ b/x/gov/types/v1/genesis_test.go @@ -22,7 +22,7 @@ func TestValidateGenesis(t *testing.T) { testCases := []struct { name string genesisState func() *v1.GenesisState - expErr bool + expErrMsg string }{ { name: "valid", @@ -35,7 +35,7 @@ func TestValidateGenesis(t *testing.T) { genesisState: func() *v1.GenesisState { return v1.NewGenesisState(0, params) }, - expErr: true, + expErrMsg: "starting proposal id must be greater than 0", }, { name: "invalid min deposit", @@ -46,9 +46,9 @@ func TestValidateGenesis(t *testing.T) { Amount: sdk.NewInt(-100), }} - return v1.NewGenesisState(0, params1) + return v1.NewGenesisState(v1.DefaultStartingProposalID, params1) }, - expErr: true, + expErrMsg: "invalid minimum deposit", }, { name: "invalid max deposit period", @@ -56,9 +56,9 @@ func TestValidateGenesis(t *testing.T) { params1 := params params1.MaxDepositPeriod = nil - return v1.NewGenesisState(0, params1) + return v1.NewGenesisState(v1.DefaultStartingProposalID, params1) }, - expErr: true, + expErrMsg: "maximum deposit period must not be nil", }, { name: "invalid quorum", @@ -66,9 +66,9 @@ func TestValidateGenesis(t *testing.T) { params1 := params params1.Quorum = "2" - return v1.NewGenesisState(0, params1) + return v1.NewGenesisState(v1.DefaultStartingProposalID, params1) }, - expErr: true, + expErrMsg: "quorom too large", }, { name: "invalid threshold", @@ -76,9 +76,9 @@ func TestValidateGenesis(t *testing.T) { params1 := params params1.Threshold = "2" - return v1.NewGenesisState(0, params1) + return v1.NewGenesisState(v1.DefaultStartingProposalID, params1) }, - expErr: true, + expErrMsg: "vote threshold too large", }, { name: "invalid veto threshold", @@ -86,9 +86,86 @@ func TestValidateGenesis(t *testing.T) { params1 := params params1.VetoThreshold = "2" - return v1.NewGenesisState(0, params1) + return v1.NewGenesisState(v1.DefaultStartingProposalID, params1) }, - expErr: true, + expErrMsg: "veto threshold too large", + }, + { + name: "duplicate proposals", + genesisState: func() *v1.GenesisState { + state := v1.NewGenesisState(v1.DefaultStartingProposalID, params) + state.Proposals = append(state.Proposals, &v1.Proposal{Id: 1}) + state.Proposals = append(state.Proposals, &v1.Proposal{Id: 1}) + + return state + }, + expErrMsg: "duplicate proposal id: 1", + }, + { + name: "duplicate votes", + genesisState: func() *v1.GenesisState { + state := v1.NewGenesisState(v1.DefaultStartingProposalID, params) + state.Proposals = append(state.Proposals, &v1.Proposal{Id: 1}) + state.Votes = append(state.Votes, + &v1.Vote{ + ProposalId: 1, + Voter: "voter", + }, + &v1.Vote{ + ProposalId: 1, + Voter: "voter", + }) + + return state + }, + expErrMsg: "duplicate vote", + }, + { + name: "duplicate deposits", + genesisState: func() *v1.GenesisState { + state := v1.NewGenesisState(v1.DefaultStartingProposalID, params) + state.Proposals = append(state.Proposals, &v1.Proposal{Id: 1}) + state.Deposits = append(state.Deposits, + &v1.Deposit{ + ProposalId: 1, + Depositor: "depositor", + }, + &v1.Deposit{ + ProposalId: 1, + Depositor: "depositor", + }) + + return state + }, + expErrMsg: "duplicate deposit: proposal_id:1 depositor:\"depositor\"", + }, + { + name: "non-existent proposal id in votes", + genesisState: func() *v1.GenesisState { + state := v1.NewGenesisState(v1.DefaultStartingProposalID, params) + state.Votes = append(state.Votes, + &v1.Vote{ + ProposalId: 1, + Voter: "voter", + }) + + return state + }, + expErrMsg: "vote proposal_id:1 voter:\"voter\" has non-existent proposal id: 1", + }, + { + name: "non-existent proposal id in deposits", + genesisState: func() *v1.GenesisState { + state := v1.NewGenesisState(v1.DefaultStartingProposalID, params) + state.Deposits = append(state.Deposits, + &v1.Deposit{ + ProposalId: 1, + Depositor: "depositor", + }) + + return state + }, + expErrMsg: "deposit proposal_id:1 depositor:\"depositor\"", }, } @@ -96,8 +173,9 @@ func TestValidateGenesis(t *testing.T) { tc := tc t.Run(tc.name, func(t *testing.T) { err := v1.ValidateGenesis(tc.genesisState()) - if tc.expErr { + if tc.expErrMsg != "" { require.Error(t, err) + require.ErrorContains(t, err, tc.expErrMsg) } else { require.NoError(t, err) }