fix: Do not call Remove during Walk

CHANGELOG.md

@@ -58,6 +58,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/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")
@@ -77,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")

x/gov/simulation/operations_test.go

@@ -183,7 +183,6 @@ func TestSimulateMsgSubmitLegacyProposal(t *testing.T) {
 		Hash:   app.LastCommitID().Hash,
 	})
 	require.NoError(t, err)
-
 	// execute operation
 	op := simulation.SimulateMsgSubmitLegacyProposal(suite.TxConfig, suite.AccountKeeper, suite.BankKeeper, suite.GovKeeper, MockWeightedProposals{3}.ContentSimulatorFn())
 	operationMsg, _, err := op(r, app.BaseApp, ctx, accounts, "")