fix(gov): fail proposal when implementation not found (backport #17873)…

… (#17882) Co-authored-by: Julien Robert <julien@rbrt.fr>
cosmos · Sep 26, 2023 · ab6a1e6 · ab6a1e6
1 parent cd9f0a6
commit ab6a1e6
Show file tree

Hide file tree

Showing 3 changed files with 145 additions and 5 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -38,6 +38,10 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ## [Unreleased]
 
+### Bug Fixes
+
+* (x/gov) [#17873](https://github.com/cosmos/cosmos-sdk/pull/17873) Fail any inactive and active proposals that cannot be decoded.
+
 ## [v0.50.0-rc.1](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.50.0-rc.1) - 2023-09-25
 
 ### Features

diff --git a/x/gov/abci.go b/x/gov/abci.go
@@ -1,10 +1,12 @@
 package gov
 
 import (
+	"errors"
 	"fmt"
 	"time"
 
 	"cosmossdk.io/collections"
+	"cosmossdk.io/log"
 
 	"github.com/cosmos/cosmos-sdk/baseapp"
 	"github.com/cosmos/cosmos-sdk/telemetry"
@@ -25,10 +27,26 @@ func EndBlocker(ctx sdk.Context, keeper *keeper.Keeper) error {
 	err := keeper.InactiveProposalsQueue.Walk(ctx, rng, func(key collections.Pair[time.Time, uint64], _ uint64) (bool, error) {
 		proposal, err := keeper.Proposals.Get(ctx, 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()
+				if err := failUnsupportedProposal(logger, ctx, keeper, proposal, err.Error(), false); err != nil {
+					return false, err
+				}
+
+				if err = keeper.DeleteProposal(ctx, proposal.Id); err != nil {
+					return false, err
+				}
+
+				return false, nil
+			}
+
 			return false, err
 		}
-		err = keeper.DeleteProposal(ctx, proposal.Id)
-		if err != nil {
+
+		if err = keeper.DeleteProposal(ctx, proposal.Id); err != nil {
 			return false, err
 		}
 
@@ -77,6 +95,22 @@ func EndBlocker(ctx sdk.Context, keeper *keeper.Keeper) error {
 	err = keeper.ActiveProposalsQueue.Walk(ctx, rng, func(key collections.Pair[time.Time, uint64], _ uint64) (bool, error) {
 		proposal, err := keeper.Proposals.Get(ctx, 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()
+				if err := failUnsupportedProposal(logger, ctx, keeper, proposal, err.Error(), true); err != nil {
+					return false, err
+				}
+
+				if err = keeper.ActiveProposalsQueue.Remove(ctx, collections.Join(*proposal.VotingEndTime, proposal.Id)); err != nil {
+					return false, err
+				}
+
+				return false, nil
+			}
+
 			return false, err
 		}
 
@@ -97,14 +131,12 @@ func EndBlocker(ctx sdk.Context, keeper *keeper.Keeper) error {
 			} else {
 				err = keeper.RefundAndDeleteDeposits(ctx, proposal.Id)
 			}
-
 			if err != nil {
 				return false, err
 			}
 		}
 
-		err = keeper.ActiveProposalsQueue.Remove(ctx, collections.Join(*proposal.VotingEndTime, proposal.Id))
-		if err != nil {
+		if err = keeper.ActiveProposalsQueue.Remove(ctx, collections.Join(*proposal.VotingEndTime, proposal.Id)); err != nil {
 			return false, err
 		}
 
@@ -235,3 +267,48 @@ func safeExecuteHandler(ctx sdk.Context, msg sdk.Msg, handler baseapp.MsgService
 	res, err = handler(ctx, msg)
 	return
 }
+
+// failUnsupportedProposal fails a proposal that cannot be processed by gov
+func failUnsupportedProposal(
+	logger log.Logger,
+	ctx sdk.Context,
+	keeper *keeper.Keeper,
+	proposal v1.Proposal,
+	errMsg string,
+	active bool,
+) error {
+	proposal.Status = v1.StatusFailed
+	proposal.FailedReason = fmt.Sprintf("proposal failed because it cannot be processed by gov: %s", errMsg)
+	proposal.Messages = nil // clear out the messages
+
+	if err := keeper.SetProposal(ctx, proposal); err != nil {
+		return err
+	}
+
+	if err := keeper.RefundAndDeleteDeposits(ctx, proposal.Id); err != nil {
+		return err
+	}
+
+	eventType := types.EventTypeInactiveProposal
+	if active {
+		eventType = types.EventTypeActiveProposal
+	}
+
+	ctx.EventManager().EmitEvent(
+		sdk.NewEvent(
+			eventType,
+			sdk.NewAttribute(types.AttributeKeyProposalID, fmt.Sprintf("%d", proposal.Id)),
+			sdk.NewAttribute(types.AttributeKeyProposalResult, types.AttributeValueProposalFailed),
+		),
+	)
+
+	logger.Info(
+		"proposal failed to decode; deleted",
+		"proposal", proposal.Id,
+		"expedited", proposal.Expedited,
+		"title", proposal.Title,
+		"results", errMsg,
+	)
+
+	return nil
+}
diff --git a/x/gov/abci_test.go b/x/gov/abci_test.go
@@ -22,6 +22,65 @@ import (
 	stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
 )
 
+func TestUnregisteredProposal_InactiveProposalFails(t *testing.T) {
+	suite := createTestSuite(t)
+	ctx := suite.App.BaseApp.NewContext(false)
+	addrs := simtestutil.AddTestAddrs(suite.BankKeeper, suite.StakingKeeper, ctx, 10, valTokens)
+
+	// manually set proposal in store
+	startTime, endTime := time.Now().Add(-4*time.Hour), ctx.BlockHeader().Time
+	proposal, err := v1.NewProposal([]sdk.Msg{
+		&v1.Proposal{}, // invalid proposal message
+	}, 1, startTime, startTime, "", "Unsupported proposal", "Unsupported proposal", addrs[0], false)
+	require.NoError(t, err)
+
+	err = suite.GovKeeper.SetProposal(ctx, proposal)
+	require.NoError(t, err)
+
+	// manually set proposal in inactive proposal queue
+	err = suite.GovKeeper.InactiveProposalsQueue.Set(ctx, collections.Join(endTime, proposal.Id), proposal.Id)
+	require.NoError(t, err)
+
+	checkInactiveProposalsQueue(t, ctx, suite.GovKeeper)
+
+	err = gov.EndBlocker(ctx, suite.GovKeeper)
+	require.NoError(t, err)
+
+	_, err = suite.GovKeeper.Proposals.Get(ctx, proposal.Id)
+	require.Error(t, err, collections.ErrNotFound)
+}
+
+func TestUnregisteredProposal_ActiveProposalFails(t *testing.T) {
+	suite := createTestSuite(t)
+	ctx := suite.App.BaseApp.NewContext(false)
+	addrs := simtestutil.AddTestAddrs(suite.BankKeeper, suite.StakingKeeper, ctx, 10, valTokens)
+
+	// manually set proposal in store
+	startTime, endTime := time.Now().Add(-4*time.Hour), ctx.BlockHeader().Time
+	proposal, err := v1.NewProposal([]sdk.Msg{
+		&v1.Proposal{}, // invalid proposal message
+	}, 1, startTime, startTime, "", "Unsupported proposal", "Unsupported proposal", addrs[0], false)
+	require.NoError(t, err)
+	proposal.Status = v1.StatusVotingPeriod
+	proposal.VotingEndTime = &endTime
+
+	err = suite.GovKeeper.SetProposal(ctx, proposal)
+	require.NoError(t, err)
+
+	// manually set proposal in active proposal queue
+	err = suite.GovKeeper.ActiveProposalsQueue.Set(ctx, collections.Join(endTime, proposal.Id), proposal.Id)
+	require.NoError(t, err)
+
+	checkActiveProposalsQueue(t, ctx, suite.GovKeeper)
+
+	err = gov.EndBlocker(ctx, suite.GovKeeper)
+	require.NoError(t, err)
+
+	p, err := suite.GovKeeper.Proposals.Get(ctx, proposal.Id)
+	require.NoError(t, err)
+	require.Equal(t, v1.StatusFailed, p.Status)
+}
+
 func TestTickExpiredDepositPeriod(t *testing.T) {
 	suite := createTestSuite(t)
 	app := suite.App