fix: Do not call Remove during Walk

CHANGELOG.md

@@ -56,6 +56,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### Bug Fixes
 
+* (x/gov) [#24044](https://github.com/cosmos/cosmos-sdk/pull/24044) Fix some places in which we call Remove inside a Walk (x/gov).
 * (baseapp) [#24042](https://github.com/cosmos/cosmos-sdk/pull/24042) Fixed a data race inside BaseApp.getContext, found by end-to-end (e2e) tests.
 * (client/server) [#24059](https://github.com/cosmos/cosmos-sdk/pull/24059) Consistently set viper prefix in client and server. It defaults for the binary name for both client and server.
 * (client/keys) [#24041](https://github.com/cosmos/cosmos-sdk/pull/24041) `keys delete` won't terminate when a key is not found, but will log the error.

x/gov/abci.go

@@ -24,35 +24,45 @@
 	// delete dead proposals from store and returns theirs deposits.
 	// A proposal is dead when it's inactive and didn't get enough deposit on time to get into voting phase.
 	rng := collections.NewPrefixUntilPairRange[time.Time, uint64](ctx.BlockTime())
-	err := keeper.InactiveProposalsQueue.Walk(ctx, rng, func(key collections.Pair[time.Time, uint64], _ uint64) (bool, error) {
-		proposal, err := keeper.Proposals.Get(ctx, key.K2())
+	iter, err := keeper.InactiveProposalsQueue.Iterate(ctx, rng)
+	if err != nil {
+		return err
+	}
+
+	inactiveProps, err := iter.KeyValues()
+	if err != nil {
+		return err
+	}
+
+	for _, prop := range inactiveProps {
+		proposal, err := keeper.Proposals.Get(ctx, prop.Key.K2())
 		if err != nil {
 			// if the proposal has an encoding error, this means it cannot be processed by x/gov
 			// this could be due to some types missing their registration
 			// instead of returning an error (i.e, halting the chain), we fail the proposal
 			if errors.Is(err, collections.ErrEncoding) {
-				proposal.Id = key.K2()
+				proposal.Id = prop.Key.K2()
 				if err := failUnsupportedProposal(logger, ctx, keeper, proposal, err.Error(), false); err != nil {
-					return false, err
+					return err
 				}
 
 				if err = keeper.DeleteProposal(ctx, proposal.Id); err != nil {
-					return false, err
+					return err
 				}
 
-				return false, nil
+				return nil
 			}
 
-			return false, err
+			return err
 		}
 
 		if err = keeper.DeleteProposal(ctx, proposal.Id); err != nil {
-			return false, err
+			return err
 		}
 
 		params, err := keeper.Params.Get(ctx)
 		if err != nil {
-			return false, err
+			return err
 		}
 		if !params.BurnProposalDepositPrevote {
 			err = keeper.RefundAndDeleteDeposits(ctx, proposal.Id) // refund deposit if proposal got removed without getting 100% of the proposal
@@ -61,7 +71,7 @@
 		}
 
 		if err != nil {
-			return false, err
+			return err
 		}
 
 		// called when proposal become inactive
@@ -89,42 +99,47 @@
 			"min_deposit", sdk.NewCoins(proposal.GetMinDepositFromParams(params)...).String(),
 			"total_deposit", sdk.NewCoins(proposal.TotalDeposit...).String(),
 		)
+	}
+
+	// fetch active proposals whose voting periods have ended (are passed the block time)
+	rng = collections.NewPrefixUntilPairRange[time.Time, uint64](ctx.BlockTime())
+	iter, err = keeper.ActiveProposalsQueue.Iterate(ctx, rng)
+	if err != nil {
+		return err
+	}
 
-		return false, nil
-	})
+	activeProps, err := iter.KeyValues()
 	if err != nil {
 		return err
 	}
 
-	// fetch active proposals whose voting periods have ended (are passed the block time)
-	rng = collections.NewPrefixUntilPairRange[time.Time, uint64](ctx.BlockTime())
-	err = keeper.ActiveProposalsQueue.Walk(ctx, rng, func(key collections.Pair[time.Time, uint64], _ uint64) (bool, error) {
-		proposal, err := keeper.Proposals.Get(ctx, key.K2())
+	for _, prop := range activeProps {
+		proposal, err := keeper.Proposals.Get(ctx, prop.Key.K2())
 		if err != nil {
 			// if the proposal has an encoding error, this means it cannot be processed by x/gov
 			// this could be due to some types missing their registration
 			// instead of returning an error (i.e, halting the chain), we fail the proposal
 			if errors.Is(err, collections.ErrEncoding) {
-				proposal.Id = key.K2()
+				proposal.Id = prop.Key.K2()
 				if err := failUnsupportedProposal(logger, ctx, keeper, proposal, err.Error(), true); err != nil {
-					return false, err
+					return err
 				}
 
 				if err = keeper.ActiveProposalsQueue.Remove(ctx, collections.Join(*proposal.VotingEndTime, proposal.Id)); err != nil {
-					return false, err
+					return err
 				}
 
-				return false, nil
+				return nil
 			}
 
-			return false, err
+			return err
 		}
 
 		var tagValue, logMsg string
 
 		passes, burnDeposits, tallyResults, err := keeper.Tally(ctx, proposal)
 		if err != nil {
-			return false, err
+			return err
 		}
 
 		// If an expedited proposal fails, we do not want to update
@@ -138,12 +153,12 @@
 				err = keeper.RefundAndDeleteDeposits(ctx, proposal.Id)
 			}
 			if err != nil {
-				return false, err
+				return err
 			}
 		}
 
 		if err = keeper.ActiveProposalsQueue.Remove(ctx, collections.Join(*proposal.VotingEndTime, proposal.Id)); err != nil {
-			return false, err
+			return err
 		}
 
 		switch {
@@ -207,14 +222,14 @@
 			proposal.Expedited = false
 			params, err := keeper.Params.Get(ctx)
 			if err != nil {
-				return false, err
+				return err
 			}
 			endTime := proposal.VotingStartTime.Add(*params.VotingPeriod)
 			proposal.VotingEndTime = &endTime
 
 			err = keeper.ActiveProposalsQueue.Set(ctx, collections.Join(*proposal.VotingEndTime, proposal.Id), proposal.Id)
 			if err != nil {
-				return false, err
+				return err
 			}
 
 			tagValue = types.AttributeValueExpeditedProposalRejected
@@ -230,7 +245,7 @@
 
 		err = keeper.SetProposal(ctx, proposal)
 		if err != nil {
-			return false, err
+			return err
 		}
 
 		// when proposal become active
@@ -259,11 +274,6 @@
 				sdk.NewAttribute(types.AttributeKeyProposalLog, logMsg),
 			),
 		)
-
-		return false, nil
-	})
-	if err != nil {
-		return err
 	}
 	return nil
 }

x/gov/abci_test.go

@@ -87,10 +87,11 @@ func TestTickExpiredDepositPeriod(t *testing.T) {
 	ctx := app.BaseApp.NewContext(false)
 	addrs := simtestutil.AddTestAddrs(suite.BankKeeper, suite.StakingKeeper, ctx, 10, valTokens)
 
-	app.FinalizeBlock(&abci.RequestFinalizeBlock{
+	_, err := app.FinalizeBlock(&abci.RequestFinalizeBlock{
 		Height: app.LastBlockHeight() + 1,
 		Hash:   app.LastCommitID().Hash,
 	})
+	require.NoError(t, err)
 
 	govMsgSvr := keeper.NewMsgServerImpl(suite.GovKeeper)
 
@@ -138,10 +139,11 @@ func TestTickMultipleExpiredDepositPeriod(t *testing.T) {
 	ctx := app.BaseApp.NewContext(false)
 	addrs := simtestutil.AddTestAddrs(suite.BankKeeper, suite.StakingKeeper, ctx, 10, valTokens)
 
-	app.FinalizeBlock(&abci.RequestFinalizeBlock{
+	_, err := app.FinalizeBlock(&abci.RequestFinalizeBlock{
 		Height: app.LastBlockHeight() + 1,
 		Hash:   app.LastCommitID().Hash,
 	})
+	require.NoError(t, err)
 
 	govMsgSvr := keeper.NewMsgServerImpl(suite.GovKeeper)
 
@@ -209,10 +211,11 @@ func TestTickPassedDepositPeriod(t *testing.T) {
 	ctx := app.BaseApp.NewContext(false)
 	addrs := simtestutil.AddTestAddrs(suite.BankKeeper, suite.StakingKeeper, ctx, 10, valTokens)
 
-	app.FinalizeBlock(&abci.RequestFinalizeBlock{
+	_, err := app.FinalizeBlock(&abci.RequestFinalizeBlock{
 		Height: app.LastBlockHeight() + 1,
 		Hash:   app.LastCommitID().Hash,
 	})
+	require.NoError(t, err)
 
 	govMsgSvr := keeper.NewMsgServerImpl(suite.GovKeeper)
 
@@ -274,10 +277,11 @@ func TestTickPassedVotingPeriod(t *testing.T) {
 
 			SortAddresses(addrs)
 
-			app.FinalizeBlock(&abci.RequestFinalizeBlock{
+			_, err := app.FinalizeBlock(&abci.RequestFinalizeBlock{
 				Height: app.LastBlockHeight() + 1,
 				Hash:   app.LastCommitID().Hash,
 			})
+			require.NoError(t, err)
 
 			govMsgSvr := keeper.NewMsgServerImpl(suite.GovKeeper)
 
@@ -368,16 +372,18 @@ func TestProposalPassedEndblocker(t *testing.T) {
 			govMsgSvr := keeper.NewMsgServerImpl(suite.GovKeeper)
 			stakingMsgSvr := stakingkeeper.NewMsgServerImpl(suite.StakingKeeper)
 
-			app.FinalizeBlock(&abci.RequestFinalizeBlock{
+			_, err := app.FinalizeBlock(&abci.RequestFinalizeBlock{
 				Height: app.LastBlockHeight() + 1,
 				Hash:   app.LastCommitID().Hash,
 			})
+			require.NoError(t, err)
 
 			valAddr := sdk.ValAddress(addrs[0])
 			proposer := addrs[0]
 
 			createValidators(t, stakingMsgSvr, ctx, []sdk.ValAddress{valAddr}, []int64{10})
-			suite.StakingKeeper.EndBlocker(ctx)
+			_, err = suite.StakingKeeper.EndBlocker(ctx)
+			require.NoError(t, err)
 
 			macc := suite.GovKeeper.GetGovernanceAccount(ctx)
 			require.NotNil(t, macc)
@@ -408,7 +414,8 @@ func TestProposalPassedEndblocker(t *testing.T) {
 			newHeader.Time = ctx.BlockHeader().Time.Add(*params.MaxDepositPeriod).Add(*params.VotingPeriod)
 			ctx = ctx.WithBlockHeader(newHeader)
 
-			gov.EndBlocker(ctx, suite.GovKeeper)
+			err = gov.EndBlocker(ctx, suite.GovKeeper)
+			require.NoError(t, err)
 
 			macc = suite.GovKeeper.GetGovernanceAccount(ctx)
 			require.NotNil(t, macc)
@@ -427,16 +434,18 @@ func TestEndBlockerProposalHandlerFailed(t *testing.T) {
 
 	stakingMsgSvr := stakingkeeper.NewMsgServerImpl(suite.StakingKeeper)
 
-	app.FinalizeBlock(&abci.RequestFinalizeBlock{
+	_, err := app.FinalizeBlock(&abci.RequestFinalizeBlock{
 		Height: app.LastBlockHeight() + 1,
 		Hash:   app.LastCommitID().Hash,
 	})
+	require.NoError(t, err)
 
 	valAddr := sdk.ValAddress(addrs[0])
 	proposer := addrs[0]
 
 	createValidators(t, stakingMsgSvr, ctx, []sdk.ValAddress{valAddr}, []int64{10})
-	suite.StakingKeeper.EndBlocker(ctx)
+	_, err = suite.StakingKeeper.EndBlocker(ctx)
+	require.NoError(t, err)
 
 	msg := banktypes.NewMsgSend(authtypes.NewModuleAddress(types.ModuleName), addrs[0], sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, math.NewInt(100000))))
 	proposal, err := suite.GovKeeper.SubmitProposal(ctx, []sdk.Msg{msg}, "", "title", "summary", proposer, false)
@@ -459,7 +468,8 @@ func TestEndBlockerProposalHandlerFailed(t *testing.T) {
 	ctx = ctx.WithBlockHeader(newHeader)
 
 	// validate that the proposal fails/has been rejected
-	gov.EndBlocker(ctx, suite.GovKeeper)
+	err = gov.EndBlocker(ctx, suite.GovKeeper)
+	require.NoError(t, err)
 
 	// check proposal events
 	events := ctx.EventManager().Events()
@@ -511,17 +521,19 @@ func TestExpeditedProposal_PassAndConversionToRegular(t *testing.T) {
 			govMsgSvr := keeper.NewMsgServerImpl(suite.GovKeeper)
 			stakingMsgSvr := stakingkeeper.NewMsgServerImpl(suite.StakingKeeper)
 
-			app.FinalizeBlock(&abci.RequestFinalizeBlock{
+			_, err = app.FinalizeBlock(&abci.RequestFinalizeBlock{
 				Height: app.LastBlockHeight() + 1,
 				Hash:   app.LastCommitID().Hash,
 			})
+			require.NoError(t, err)
 
 			valAddr := sdk.ValAddress(addrs[0])
 			proposer := addrs[0]
 
 			// Create a validator so that able to vote on proposal.
 			createValidators(t, stakingMsgSvr, ctx, []sdk.ValAddress{valAddr}, []int64{10})
-			suite.StakingKeeper.EndBlocker(ctx)
+			_, err = suite.StakingKeeper.EndBlocker(ctx)
+			require.NoError(t, err)
 
 			checkInactiveProposalsQueue(t, ctx, suite.GovKeeper)
 			checkActiveProposalsQueue(t, ctx, suite.GovKeeper)
@@ -571,7 +583,8 @@ func TestExpeditedProposal_PassAndConversionToRegular(t *testing.T) {
 			}
 
 			// Here the expedited proposal is converted to regular after expiry.
-			gov.EndBlocker(ctx, suite.GovKeeper)
+			err = gov.EndBlocker(ctx, suite.GovKeeper)
+			require.NoError(t, err)
 
 			if tc.expeditedPasses {
 				checkActiveProposalsQueue(t, ctx, suite.GovKeeper)
@@ -626,7 +639,8 @@ func TestExpeditedProposal_PassAndConversionToRegular(t *testing.T) {
 			}
 
 			// Here we validate the converted regular proposal
-			gov.EndBlocker(ctx, suite.GovKeeper)
+			err = gov.EndBlocker(ctx, suite.GovKeeper)
+			require.NoError(t, err)
 
 			macc = suite.GovKeeper.GetGovernanceAccount(ctx)
 			require.NotNil(t, macc)
@@ -661,6 +675,7 @@ func TestExpeditedProposal_PassAndConversionToRegular(t *testing.T) {
 }
 
 func createValidators(t *testing.T, stakingMsgSvr stakingtypes.MsgServer, ctx sdk.Context, addrs []sdk.ValAddress, powerAmt []int64) {
+	t.Helper()
 	require.True(t, len(addrs) <= len(pubkeys), "Not enough pubkeys specified at top of file.")
 
 	for i := 0; i < len(addrs); i++ {
@@ -688,6 +703,7 @@ func getDepositMultiplier(expedited bool) int64 {
 }
 
 func checkActiveProposalsQueue(t *testing.T, ctx sdk.Context, k *keeper.Keeper) {
+	t.Helper()
 	err := k.ActiveProposalsQueue.Walk(ctx, collections.NewPrefixUntilPairRange[time.Time, uint64](ctx.BlockTime()), func(key collections.Pair[time.Time, uint64], value uint64) (stop bool, err error) {
 		return false, err
 	})
@@ -696,6 +712,7 @@ func checkActiveProposalsQueue(t *testing.T, ctx sdk.Context, k *keeper.Keeper)
 }
 
 func checkInactiveProposalsQueue(t *testing.T, ctx sdk.Context, k *keeper.Keeper) {
+	t.Helper()
 	err := k.InactiveProposalsQueue.Walk(ctx, collections.NewPrefixUntilPairRange[time.Time, uint64](ctx.BlockTime()), func(key collections.Pair[time.Time, uint64], value uint64) (stop bool, err error) {
 		return false, err
 	})

x/gov/client/cli/prompt_test.go

@@ -47,7 +47,8 @@ func TestPromptIntegerOverflow(t *testing.T) {
 
 			fin, fw := readline.NewFillableStdin(os.Stdin)
 			readline.Stdin = fin
-			fw.Write([]byte(overflowStr + "\n"))
+			_, err := fw.Write([]byte(overflowStr + "\n"))
+			require.NoError(t, err)
 
 			v, err := cli.Prompt(st{}, "")
 			assert.Equal(t, st{}, v, "expected a value of zero")
@@ -69,7 +70,6 @@ func TestPromptParseInteger(t *testing.T) {
 	}
 
 	for _, tc := range values {
-		tc := tc
 		t.Run(tc.in, func(t *testing.T) {
 			origStdin := readline.Stdin
 			defer func() {
@@ -78,7 +78,8 @@ func TestPromptParseInteger(t *testing.T) {
 
 			fin, fw := readline.NewFillableStdin(os.Stdin)
 			readline.Stdin = fin
-			fw.Write([]byte(tc.in + "\n"))
+			_, err := fw.Write([]byte(tc.in + "\n"))
+			assert.NoError(t, err)
 
 			v, err := cli.Prompt(st{}, "")
 			assert.Nil(t, err, "expected a nil error")