From 2995ebaaf094afd38974409322ee77e141871164 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Mon, 25 Sep 2023 17:36:05 +0200 Subject: [PATCH 1/6] fix(gov): do not panic when proposal implementation not found --- x/gov/abci.go | 79 +++++++++++++++++++++++++++++++++++++++++++--- x/gov/abci_test.go | 33 +++++++++++++++++++ 2 files changed, 107 insertions(+), 5 deletions(-) diff --git a/x/gov/abci.go b/x/gov/abci.go index 8fbfb5c75613..96487f19fc6a 100644 --- 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,25 @@ 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) { + if err := deleteUnsupportedProposals(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 +94,21 @@ 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) { + if err := deleteUnsupportedProposals(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 +129,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 +265,42 @@ func safeExecuteHandler(ctx sdk.Context, msg sdk.Msg, handler baseapp.MsgService res, err = handler(ctx, msg) return } + +func deleteUnsupportedProposals( + logger log.Logger, + ctx sdk.Context, + keeper *keeper.Keeper, + proposal v1.Proposal, + errMsg string, +) error { + proposal.Status = v1.StatusFailed + proposal.FailedReason = 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.AttributeValueProposalDropped), + ), + ) + + 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 index 915efd2333f7..7371fbd56788 100644 --- a/x/gov/abci_test.go +++ b/x/gov/abci_test.go @@ -22,6 +22,39 @@ import ( stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" ) +func TestUnregisteredProposal(t *testing.T) { + suite := createTestSuite(t) + app := suite.App + ctx := app.BaseApp.NewContext(false) + + addrs := simtestutil.AddTestAddrs(suite.BankKeeper, suite.StakingKeeper, ctx, 10, valTokens) + + _, err := app.FinalizeBlock(&abci.RequestFinalizeBlock{ + Height: app.LastBlockHeight() + 1, + Hash: app.LastCommitID().Hash, + }) + require.NoError(t, err) + + // 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) +} + func TestTickExpiredDepositPeriod(t *testing.T) { suite := createTestSuite(t) app := suite.App From d376e6a8b562f413eae837eb9c2337cc103199ac Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Mon, 25 Sep 2023 21:54:02 +0200 Subject: [PATCH 2/6] updates --- x/gov/abci.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/x/gov/abci.go b/x/gov/abci.go index 96487f19fc6a..82d73453faa5 100644 --- a/x/gov/abci.go +++ b/x/gov/abci.go @@ -31,7 +31,7 @@ func EndBlocker(ctx sdk.Context, keeper *keeper.Keeper) error { // 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) { - if err := deleteUnsupportedProposals(logger, ctx, keeper, proposal, err.Error()); err != nil { + if err := failUnsupportedProposal(logger, ctx, keeper, proposal, err.Error()); err != nil { return false, err } @@ -98,7 +98,7 @@ func EndBlocker(ctx sdk.Context, keeper *keeper.Keeper) error { // 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) { - if err := deleteUnsupportedProposals(logger, ctx, keeper, proposal, err.Error()); err != nil { + if err := failUnsupportedProposal(logger, ctx, keeper, proposal, err.Error()); err != nil { return false, err } @@ -266,7 +266,8 @@ func safeExecuteHandler(ctx sdk.Context, msg sdk.Msg, handler baseapp.MsgService return } -func deleteUnsupportedProposals( +// failUnsupportedProposal fails a proposal that cannot be processed by gov +func failUnsupportedProposal( logger log.Logger, ctx sdk.Context, keeper *keeper.Keeper, @@ -274,7 +275,7 @@ func deleteUnsupportedProposals( errMsg string, ) error { proposal.Status = v1.StatusFailed - proposal.FailedReason = errMsg + 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 { @@ -289,7 +290,7 @@ func deleteUnsupportedProposals( sdk.NewEvent( types.EventTypeInactiveProposal, sdk.NewAttribute(types.AttributeKeyProposalID, fmt.Sprintf("%d", proposal.Id)), - sdk.NewAttribute(types.AttributeKeyProposalResult, types.AttributeValueProposalDropped), + sdk.NewAttribute(types.AttributeKeyProposalResult, types.AttributeValueProposalFailed), ), ) @@ -302,5 +303,4 @@ func deleteUnsupportedProposals( ) return nil - } From 9257e6735472813856913c9a77e556f534466721 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Mon, 25 Sep 2023 22:07:59 +0200 Subject: [PATCH 3/6] finish tests --- x/gov/abci_test.go | 46 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 36 insertions(+), 10 deletions(-) diff --git a/x/gov/abci_test.go b/x/gov/abci_test.go index 7371fbd56788..1565e94a2b8f 100644 --- a/x/gov/abci_test.go +++ b/x/gov/abci_test.go @@ -22,19 +22,11 @@ import ( stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" ) -func TestUnregisteredProposal(t *testing.T) { +func TestUnregisteredProposal_InactiveProposalFails(t *testing.T) { suite := createTestSuite(t) - app := suite.App - ctx := app.BaseApp.NewContext(false) - + ctx := suite.App.BaseApp.NewContext(false) addrs := simtestutil.AddTestAddrs(suite.BankKeeper, suite.StakingKeeper, ctx, 10, valTokens) - _, err := app.FinalizeBlock(&abci.RequestFinalizeBlock{ - Height: app.LastBlockHeight() + 1, - Hash: app.LastCommitID().Hash, - }) - require.NoError(t, err) - // manually set proposal in store startTime, endTime := time.Now().Add(-4*time.Hour), ctx.BlockHeader().Time proposal, err := v1.NewProposal([]sdk.Msg{ @@ -53,6 +45,40 @@ func TestUnregisteredProposal(t *testing.T) { 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) { From cc4ce8d5babdc77e4cc35586403e1b99bf8f3c2d Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Mon, 25 Sep 2023 22:10:27 +0200 Subject: [PATCH 4/6] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e639a38361a3..f3855b8b3583 100644 --- a/CHANGELOG.md +++ b/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. From e34ff2b5d99de5206b2adcadc00a0d8847b3c16a Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Mon, 25 Sep 2023 23:50:39 +0200 Subject: [PATCH 5/6] updates --- x/gov/abci.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/x/gov/abci.go b/x/gov/abci.go index 82d73453faa5..e54cdff6e561 100644 --- a/x/gov/abci.go +++ b/x/gov/abci.go @@ -31,6 +31,7 @@ func EndBlocker(ctx sdk.Context, keeper *keeper.Keeper) error { // 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 } @@ -98,6 +99,7 @@ func EndBlocker(ctx sdk.Context, keeper *keeper.Keeper) error { // 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 } From 22534af3784212cb0f5227d49bab5557a6933ea8 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Tue, 26 Sep 2023 08:13:50 +0200 Subject: [PATCH 6/6] feedback --- x/gov/abci.go | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/x/gov/abci.go b/x/gov/abci.go index e54cdff6e561..f43370348fd7 100644 --- a/x/gov/abci.go +++ b/x/gov/abci.go @@ -28,11 +28,11 @@ func EndBlocker(ctx sdk.Context, keeper *keeper.Keeper) 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 + // 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()); err != nil { + if err := failUnsupportedProposal(logger, ctx, keeper, proposal, err.Error(), false); err != nil { return false, err } @@ -96,11 +96,11 @@ func EndBlocker(ctx sdk.Context, keeper *keeper.Keeper) 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 + // 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()); err != nil { + if err := failUnsupportedProposal(logger, ctx, keeper, proposal, err.Error(), true); err != nil { return false, err } @@ -275,6 +275,7 @@ func failUnsupportedProposal( 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) @@ -288,9 +289,14 @@ func failUnsupportedProposal( return err } + eventType := types.EventTypeInactiveProposal + if active { + eventType = types.EventTypeActiveProposal + } + ctx.EventManager().EmitEvent( sdk.NewEvent( - types.EventTypeInactiveProposal, + eventType, sdk.NewAttribute(types.AttributeKeyProposalID, fmt.Sprintf("%d", proposal.Id)), sdk.NewAttribute(types.AttributeKeyProposalResult, types.AttributeValueProposalFailed), ),