From 570ab64762abe9bdf0260ac2850686178a6863a4 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Tue, 12 Dec 2023 22:00:13 +0100 Subject: [PATCH] feat(x/gov): better gov genesis validation (#18707) --- CHANGELOG.md | 1 + x/gov/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 2931c51fdf0a..9be34195299a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Improvements +* (x/gov) [#18707](https://github.com/cosmos/cosmos-sdk/pull/18707) Improve genesis validation. * (x/bank) [#18636](https://github.com/cosmos/cosmos-sdk/pull/18636) `SendCoinsFromModuleToAccount`, `SendCoinsFromModuleToModule`, `SendCoinsFromAccountToModule`, `DelegateCoinsFromAccountToModule`, `UndelegateCoinsFromModuleToAccount`, `MintCoins` and `BurnCoins` methods now returns an error instead of panicking if any module accounts does not exist or unauthorized. * (x/distribution) [#18636](https://github.com/cosmos/cosmos-sdk/pull/18636) `CalculateDelegationRewards` and `DelegationTotalRewards` methods no longer panics on any sanity checks and instead returns appropriate errors. * (x/slashing) [#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. diff --git a/x/gov/go.mod b/x/gov/go.mod index 93472feb1357..174399cf3ef2 100644 --- a/x/gov/go.mod +++ b/x/gov/go.mod @@ -152,7 +152,7 @@ require ( golang.org/x/crypto v0.16.0 // indirect golang.org/x/mod v0.14.0 // indirect golang.org/x/net v0.19.0 // indirect - golang.org/x/sync v0.5.0 // indirect + golang.org/x/sync v0.5.0 golang.org/x/sys v0.15.0 // indirect golang.org/x/term v0.15.0 // indirect golang.org/x/text v0.14.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 74f8e5eb7fda..9d0e42a23b34 100644 --- a/x/gov/types/v1/genesis_test.go +++ b/x/gov/types/v1/genesis_test.go @@ -25,7 +25,7 @@ func TestValidateGenesis(t *testing.T) { testCases := []struct { name string genesisState func() *v1.GenesisState - expErr bool + expErrMsg string }{ { name: "valid", @@ -38,7 +38,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", @@ -49,9 +49,9 @@ func TestValidateGenesis(t *testing.T) { Amount: sdkmath.NewInt(-100), }} - return v1.NewGenesisState(0, params1) + return v1.NewGenesisState(v1.DefaultStartingProposalID, params1) }, - expErr: true, + expErrMsg: "invalid minimum deposit", }, { name: "invalid max deposit period", @@ -59,9 +59,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", @@ -69,9 +69,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", @@ -79,9 +79,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", @@ -89,9 +89,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\"", }, } @@ -99,8 +176,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) }