fix(gov): fail proposal when implementation not found

CHANGELOG.md

@@ -62,6 +62,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### Bug Fixes
 
+* (x/gov) [#17873](https://github.com/cosmos/cosmos-sdk/pull/17873) Fail any inactive and active proposals whose messages cannot be decoded.
 * (baseapp) [#17769](https://github.com/cosmos/cosmos-sdk/pull/17769) Ensure we respect block size constraints in the `DefaultProposalHandler`'s `PrepareProposal` handler when a nil or no-op mempool is used. We provide a `TxSelector` type to assist in making transaction selection generalized. We also fix a comparison bug in tx selection when `req.maxTxBytes` is reached.
 * (types) [#16583](https://github.com/cosmos/cosmos-sdk/pull/16583), [#17372](https://github.com/cosmos/cosmos-sdk/pull/17372), [#17421](https://github.com/cosmos/cosmos-sdk/pull/17421), [#17713](https://github.com/cosmos/cosmos-sdk/pull/17713) Introduce `PreBlock`, which executes in `FinalizeBlock` before `BeginBlock`. It allows the application to modify consensus parameters and have access to VE state. Note, `PreFinalizeBlockHook` is replaced by`PreBlocker`.
 * (baseapp) [#17518](https://github.com/cosmos/cosmos-sdk/pull/17518) Utilizing voting power from vote extensions (CometBFT) instead of the current bonded tokens (x/staking) to determine if a set of vote extensions are valid.

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 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()); 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 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()); 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,42 @@ 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,
+) 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
+	}
+
+	ctx.EventManager().EmitEvent(
+		sdk.NewEvent(
+			types.EventTypeInactiveProposal,
+			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
+}

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