feat(x/gov): better gov genesis validation (backport #18707) (#18712)

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

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

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{}

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,58 +46,136 @@ 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",
 			genesisState: func() *v1.GenesisState {
 				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",
 			genesisState: func() *v1.GenesisState {
 				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",
 			genesisState: func() *v1.GenesisState {
 				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",
 			genesisState: func() *v1.GenesisState {
 				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\"",
 		},
 	}
 
 	for _, tc := range testCases {
 		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)
 			}