From 6a2329fc933847c5829ef698e4d709a10917b16f Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Mon, 8 Jan 2024 22:13:46 +0100 Subject: [PATCH] refactor(x/gov): log error when deposit refund error (#18976) --- x/gov/CHANGELOG.md | 1 + x/gov/abci.go | 14 ++++- x/gov/abci_test.go | 122 +++++++++++++++++----------------------- x/gov/keeper/deposit.go | 26 ++++----- x/gov/types/events.go | 16 +++--- 5 files changed, 88 insertions(+), 91 deletions(-) diff --git a/x/gov/CHANGELOG.md b/x/gov/CHANGELOG.md index a22fc538d9..c7f80a822a 100644 --- a/x/gov/CHANGELOG.md +++ b/x/gov/CHANGELOG.md @@ -34,6 +34,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Improvements +* [#18976](https://github.com/cosmos/cosmos-sdk/pull/18976) Log and send an event when a proposal deposit refund or burn has failed. * [#18856](https://github.com/cosmos/cosmos-sdk/pull/18856) Add `ProposalCancelMaxPeriod` for modifying how long a proposal can be cancelled after it has been submitted. * [#18445](https://github.com/cosmos/cosmos-sdk/pull/18445) Extend gov config. * [#18532](https://github.com/cosmos/cosmos-sdk/pull/18532) Repurpose `govcliutils.NormalizeProposalType` to work for gov v1 proposal types. diff --git a/x/gov/abci.go b/x/gov/abci.go index cc1fa97247..70074849a4 100644 --- a/x/gov/abci.go +++ b/x/gov/abci.go @@ -137,7 +137,19 @@ func EndBlocker(ctx sdk.Context, keeper *keeper.Keeper) error { err = keeper.RefundAndDeleteDeposits(ctx, proposal.Id) } if err != nil { - return false, err + // in case of an error, log it and emit an event + // we do not want to halt the chain if the refund/burn fails + // as it could happen due to a governance mistake (governance has let a proposal pass that sends gov funds that were from proposal deposits) + + keeper.Logger(ctx).Error("failed to refund or burn deposits", "error", err) + ctx.EventManager().EmitEvent( + sdk.NewEvent( + types.EventTypeProposalDeposit, + sdk.NewAttribute(types.AttributeKeyProposalID, fmt.Sprintf("%d", proposal.Id)), + sdk.NewAttribute(types.AttributeKeyProposalDepositError, "failed to refund or burn deposits"), + sdk.NewAttribute("error", err.Error()), + ), + ) } if err = keeper.ActiveProposalsQueue.Remove(ctx, collections.Join(*proposal.VotingEndTime, proposal.Id)); err != nil { diff --git a/x/gov/abci_test.go b/x/gov/abci_test.go index e6106fc1cc..ac62edbc64 100644 --- a/x/gov/abci_test.go +++ b/x/gov/abci_test.go @@ -41,8 +41,6 @@ func TestUnregisteredProposal_InactiveProposalFails(t *testing.T) { 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) @@ -71,8 +69,6 @@ func TestUnregisteredProposal_ActiveProposalFails(t *testing.T) { 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) @@ -89,8 +85,6 @@ func TestTickExpiredDepositPeriod(t *testing.T) { govMsgSvr := keeper.NewMsgServerImpl(suite.GovKeeper) - checkInactiveProposalsQueue(t, ctx, suite.GovKeeper) - newProposalMsg, err := v1.NewMsgSubmitProposal( []sdk.Msg{mkTestLegacyContent(t)}, sdk.Coins{sdk.NewInt64Coin(sdk.DefaultBondDenom, 100000)}, @@ -106,25 +100,17 @@ func TestTickExpiredDepositPeriod(t *testing.T) { require.NoError(t, err) require.NotNil(t, res) - checkInactiveProposalsQueue(t, ctx, suite.GovKeeper) - newHeader := ctx.HeaderInfo() newHeader.Time = ctx.HeaderInfo().Time.Add(time.Duration(1) * time.Second) ctx = ctx.WithHeaderInfo(newHeader) - checkInactiveProposalsQueue(t, ctx, suite.GovKeeper) - params, _ := suite.GovKeeper.Params.Get(ctx) newHeader = ctx.HeaderInfo() newHeader.Time = ctx.HeaderInfo().Time.Add(*params.MaxDepositPeriod) ctx = ctx.WithHeaderInfo(newHeader) - checkInactiveProposalsQueue(t, ctx, suite.GovKeeper) - err = gov.EndBlocker(ctx, suite.GovKeeper) require.NoError(t, err) - - checkInactiveProposalsQueue(t, ctx, suite.GovKeeper) } func TestTickMultipleExpiredDepositPeriod(t *testing.T) { @@ -134,8 +120,6 @@ func TestTickMultipleExpiredDepositPeriod(t *testing.T) { addrs := simtestutil.AddTestAddrs(suite.BankKeeper, suite.StakingKeeper, ctx, 10, valTokens) govMsgSvr := keeper.NewMsgServerImpl(suite.GovKeeper) - checkInactiveProposalsQueue(t, ctx, suite.GovKeeper) - newProposalMsg, err := v1.NewMsgSubmitProposal( []sdk.Msg{mkTestLegacyContent(t)}, sdk.Coins{sdk.NewInt64Coin(sdk.DefaultBondDenom, 100000)}, @@ -151,14 +135,10 @@ func TestTickMultipleExpiredDepositPeriod(t *testing.T) { require.NoError(t, err) require.NotNil(t, res) - checkInactiveProposalsQueue(t, ctx, suite.GovKeeper) - newHeader := ctx.HeaderInfo() newHeader.Time = ctx.HeaderInfo().Time.Add(time.Duration(2) * time.Second) ctx = ctx.WithHeaderInfo(newHeader) - checkInactiveProposalsQueue(t, ctx, suite.GovKeeper) - newProposalMsg2, err := v1.NewMsgSubmitProposal( []sdk.Msg{mkTestLegacyContent(t)}, sdk.Coins{sdk.NewInt64Coin(sdk.DefaultBondDenom, 100000)}, @@ -179,17 +159,12 @@ func TestTickMultipleExpiredDepositPeriod(t *testing.T) { newHeader.Time = ctx.HeaderInfo().Time.Add(*params.MaxDepositPeriod).Add(time.Duration(-1) * time.Second) ctx = ctx.WithHeaderInfo(newHeader) - checkInactiveProposalsQueue(t, ctx, suite.GovKeeper) require.NoError(t, gov.EndBlocker(ctx, suite.GovKeeper)) - checkInactiveProposalsQueue(t, ctx, suite.GovKeeper) newHeader = ctx.HeaderInfo() newHeader.Time = ctx.HeaderInfo().Time.Add(time.Duration(5) * time.Second) ctx = ctx.WithHeaderInfo(newHeader) - - checkInactiveProposalsQueue(t, ctx, suite.GovKeeper) require.NoError(t, gov.EndBlocker(ctx, suite.GovKeeper)) - checkInactiveProposalsQueue(t, ctx, suite.GovKeeper) } func TestTickPassedDepositPeriod(t *testing.T) { @@ -216,21 +191,68 @@ func TestTickPassedDepositPeriod(t *testing.T) { proposalID := res.ProposalId - checkInactiveProposalsQueue(t, ctx, suite.GovKeeper) - newHeader := ctx.HeaderInfo() newHeader.Time = ctx.HeaderInfo().Time.Add(time.Duration(1) * time.Second) ctx = ctx.WithHeaderInfo(newHeader) - checkInactiveProposalsQueue(t, ctx, suite.GovKeeper) - newDepositMsg := v1.NewMsgDeposit(addrs[1], proposalID, sdk.Coins{sdk.NewInt64Coin(sdk.DefaultBondDenom, 100000)}) res1, err := govMsgSvr.Deposit(ctx, newDepositMsg) require.NoError(t, err) require.NotNil(t, res1) +} - checkActiveProposalsQueue(t, ctx, suite.GovKeeper) +func TestProposalDepositRefundFailEndBlocker(t *testing.T) { + suite := createTestSuite(t) + app := suite.App + ctx := app.BaseApp.NewContext(false) + addrs := simtestutil.AddTestAddrs(suite.BankKeeper, suite.StakingKeeper, ctx, 10, valTokens) + govMsgSvr := keeper.NewMsgServerImpl(suite.GovKeeper) + + depositMultiplier := getDepositMultiplier(v1.ProposalType_PROPOSAL_TYPE_STANDARD) + proposalCoins := sdk.Coins{sdk.NewCoin(sdk.DefaultBondDenom, suite.StakingKeeper.TokensFromConsensusPower(ctx, 15*depositMultiplier))} + + // create a proposal that empties the gov module account + // which will cause the proposal deposit refund to fail + newProposalMsg, err := v1.NewMsgSubmitProposal( + []sdk.Msg{}, + proposalCoins, + addrs[0].String(), + "metadata", + "proposal", + "description of proposal", + v1.ProposalType_PROPOSAL_TYPE_STANDARD, + ) + require.NoError(t, err) + + res, err := govMsgSvr.SubmitProposal(ctx, newProposalMsg) + require.NoError(t, err) + require.NotNil(t, res) + + proposal, err := suite.GovKeeper.Proposals.Get(ctx, res.ProposalId) + require.NoError(t, err) + require.Equal(t, v1.StatusVotingPeriod, proposal.Status) + + proposalID := res.ProposalId + err = suite.GovKeeper.AddVote(ctx, proposalID, addrs[0], v1.NewNonSplitVoteOption(v1.OptionYes), "metadata") + require.NoError(t, err) + + // empty the gov module account before the proposal ends + err = suite.BankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, addrs[1], proposalCoins) + require.NoError(t, err) + + // fast forward to the end of the voting period + newHeader := ctx.HeaderInfo() + newHeader.Time = proposal.VotingEndTime.Add(time.Duration(100) * time.Second) + ctx = ctx.WithHeaderInfo(newHeader) + + err = gov.EndBlocker(ctx, suite.GovKeeper) + require.NoError(t, err) // no error, means does not halt the chain + + events := ctx.EventManager().Events() + attr, ok := events.GetAttributes(types.AttributeKeyProposalDepositError) + require.True(t, ok) + require.Contains(t, attr[0].Value, "failed to refund or burn deposits") } func TestTickPassedVotingPeriod(t *testing.T) { @@ -258,9 +280,6 @@ func TestTickPassedVotingPeriod(t *testing.T) { SortAddresses(addrs) govMsgSvr := keeper.NewMsgServerImpl(suite.GovKeeper) - checkInactiveProposalsQueue(t, ctx, suite.GovKeeper) - checkActiveProposalsQueue(t, ctx, suite.GovKeeper) - proposalCoins := sdk.Coins{sdk.NewCoin(sdk.DefaultBondDenom, suite.StakingKeeper.TokensFromConsensusPower(ctx, 5*depositMultiplier))} newProposalMsg, err := v1.NewMsgSubmitProposal([]sdk.Msg{mkTestLegacyContent(t)}, proposalCoins, addrs[0].String(), "", "Proposal", "description of proposal", tc.proposalType) require.NoError(t, err) @@ -291,9 +310,6 @@ func TestTickPassedVotingPeriod(t *testing.T) { newHeader.Time = ctx.HeaderInfo().Time.Add(*params.MaxDepositPeriod).Add(*votingPeriod) ctx = ctx.WithHeaderInfo(newHeader) - checkInactiveProposalsQueue(t, ctx, suite.GovKeeper) - checkActiveProposalsQueue(t, ctx, suite.GovKeeper) - proposal, err := suite.GovKeeper.Proposals.Get(ctx, res.ProposalId) require.NoError(t, err) require.Equal(t, v1.StatusVotingPeriod, proposal.Status) @@ -302,13 +318,10 @@ func TestTickPassedVotingPeriod(t *testing.T) { require.NoError(t, err) if tc.proposalType != v1.ProposalType_PROPOSAL_TYPE_EXPEDITED { - checkActiveProposalsQueue(t, ctx, suite.GovKeeper) return } // If expedited, it should be converted to a regular proposal instead. - checkActiveProposalsQueue(t, ctx, suite.GovKeeper) - proposal, err = suite.GovKeeper.Proposals.Get(ctx, res.ProposalId) require.Nil(t, err) require.Equal(t, v1.StatusVotingPeriod, proposal.Status) @@ -490,8 +503,6 @@ func TestExpeditedProposal_PassAndConversionToRegular(t *testing.T) { createValidators(t, stakingMsgSvr, ctx, []sdk.ValAddress{valAddr}, []int64{10}) _, err = suite.StakingKeeper.EndBlocker(ctx) require.NoError(t, err) - checkInactiveProposalsQueue(t, ctx, suite.GovKeeper) - checkActiveProposalsQueue(t, ctx, suite.GovKeeper) macc := suite.GovKeeper.GetGovernanceAccount(ctx) require.NotNil(t, macc) @@ -524,9 +535,6 @@ func TestExpeditedProposal_PassAndConversionToRegular(t *testing.T) { newHeader.Time = ctx.HeaderInfo().Time.Add(*params.MaxDepositPeriod).Add(*params.ExpeditedVotingPeriod) ctx = ctx.WithHeaderInfo(newHeader) - checkInactiveProposalsQueue(t, ctx, suite.GovKeeper) - checkActiveProposalsQueue(t, ctx, suite.GovKeeper) - proposal, err := suite.GovKeeper.Proposals.Get(ctx, res.ProposalId) require.Nil(t, err) require.Equal(t, v1.StatusVotingPeriod, proposal.Status) @@ -541,8 +549,6 @@ func TestExpeditedProposal_PassAndConversionToRegular(t *testing.T) { err = gov.EndBlocker(ctx, suite.GovKeeper) require.NoError(t, err) if tc.expeditedPasses { - checkActiveProposalsQueue(t, ctx, suite.GovKeeper) - proposal, err = suite.GovKeeper.Proposals.Get(ctx, res.ProposalId) require.Nil(t, err) @@ -562,7 +568,6 @@ func TestExpeditedProposal_PassAndConversionToRegular(t *testing.T) { } // Expedited proposal should be converted to a regular proposal instead. - checkActiveProposalsQueue(t, ctx, suite.GovKeeper) proposal, err = suite.GovKeeper.Proposals.Get(ctx, res.ProposalId) require.Nil(t, err) require.Equal(t, v1.StatusVotingPeriod, proposal.Status) @@ -583,9 +588,6 @@ func TestExpeditedProposal_PassAndConversionToRegular(t *testing.T) { newHeader.Time = ctx.HeaderInfo().Time.Add(*params.MaxDepositPeriod).Add(*params.VotingPeriod) ctx = ctx.WithHeaderInfo(newHeader) - checkInactiveProposalsQueue(t, ctx, suite.GovKeeper) - checkActiveProposalsQueue(t, ctx, suite.GovKeeper) - if tc.regularEventuallyPassing { // Validator votes YES, letting the converted regular proposal pass. err = suite.GovKeeper.AddVote(ctx, proposal.Id, addrs[0], v1.NewNonSplitVoteOption(v1.OptionYes), "metadata") @@ -602,8 +604,6 @@ func TestExpeditedProposal_PassAndConversionToRegular(t *testing.T) { submitterEventualBalance := suite.BankKeeper.GetAllBalances(ctx, addrs[0]) depositorEventualBalance := suite.BankKeeper.GetAllBalances(ctx, addrs[1]) - checkActiveProposalsQueue(t, ctx, suite.GovKeeper) - proposal, err = suite.GovKeeper.Proposals.Get(ctx, res.ProposalId) require.Nil(t, err) @@ -655,21 +655,3 @@ func getDepositMultiplier(proposalType v1.ProposalType) int64 { return 1 } } - -func checkActiveProposalsQueue(t *testing.T, ctx sdk.Context, k *keeper.Keeper) { - t.Helper() - err := k.ActiveProposalsQueue.Walk(ctx, collections.NewPrefixUntilPairRange[time.Time, uint64](ctx.HeaderInfo().Time), func(key collections.Pair[time.Time, uint64], value uint64) (stop bool, err error) { - return false, err - }) - - require.NoError(t, err) -} - -func checkInactiveProposalsQueue(t *testing.T, ctx sdk.Context, k *keeper.Keeper) { - t.Helper() - err := k.InactiveProposalsQueue.Walk(ctx, collections.NewPrefixUntilPairRange[time.Time, uint64](ctx.HeaderInfo().Time), func(key collections.Pair[time.Time, uint64], value uint64) (stop bool, err error) { - return false, err - }) - - require.NoError(t, err) -} diff --git a/x/gov/keeper/deposit.go b/x/gov/keeper/deposit.go index b0c6ed7926..be79f08263 100644 --- a/x/gov/keeper/deposit.go +++ b/x/gov/keeper/deposit.go @@ -48,6 +48,19 @@ func (k Keeper) DeleteAndBurnDeposits(ctx context.Context, proposalID uint64) er return k.bankKeeper.BurnCoins(ctx, k.authKeeper.GetModuleAddress(types.ModuleName), coinsToBurn) } +// RefundAndDeleteDeposits refunds and deletes all the deposits on a specific proposal. +func (k Keeper) RefundAndDeleteDeposits(ctx context.Context, proposalID uint64) error { + return k.IterateDeposits(ctx, proposalID, func(key collections.Pair[uint64, sdk.AccAddress], deposit v1.Deposit) (bool, error) { + depositor := key.K2() + err := k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, depositor, deposit.Amount) + if err != nil { + return false, err + } + err = k.Deposits.Remove(ctx, key) + return false, err + }) +} + // IterateDeposits iterates over all the proposals deposits and performs a callback function func (k Keeper) IterateDeposits(ctx context.Context, proposalID uint64, cb func(key collections.Pair[uint64, sdk.AccAddress], value v1.Deposit) (bool, error)) error { rng := collections.NewPrefixedPairRange[uint64, sdk.AccAddress](proposalID) @@ -265,19 +278,6 @@ func (k Keeper) ChargeDeposit(ctx context.Context, proposalID uint64, destAddres return nil } -// RefundAndDeleteDeposits refunds and deletes all the deposits on a specific proposal. -func (k Keeper) RefundAndDeleteDeposits(ctx context.Context, proposalID uint64) error { - return k.IterateDeposits(ctx, proposalID, func(key collections.Pair[uint64, sdk.AccAddress], deposit v1.Deposit) (bool, error) { - depositor := key.K2() - err := k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, depositor, deposit.Amount) - if err != nil { - return false, err - } - err = k.Deposits.Remove(ctx, key) - return false, err - }) -} - // validateInitialDeposit validates if initial deposit is greater than or equal to the minimum // required at the time of proposal submission. This threshold amount is determined by // the deposit parameters. Returns nil on success, error otherwise. diff --git a/x/gov/types/events.go b/x/gov/types/events.go index 7b8261bb8a..2ca0fa8dbf 100644 --- a/x/gov/types/events.go +++ b/x/gov/types/events.go @@ -9,13 +9,15 @@ const ( EventTypeActiveProposal = "active_proposal" EventTypeCancelProposal = "cancel_proposal" - AttributeKeyProposalResult = "proposal_result" - AttributeKeyVoter = "voter" - AttributeKeyOption = "option" - AttributeKeyProposalID = "proposal_id" - AttributeKeyProposalMessages = "proposal_messages" // Msg type_urls in the proposal - AttributeKeyVotingPeriodStart = "voting_period_start" - AttributeKeyProposalLog = "proposal_log" // log of proposal execution + AttributeKeyProposalResult = "proposal_result" + AttributeKeyVoter = "voter" + AttributeKeyOption = "option" + AttributeKeyProposalID = "proposal_id" + AttributeKeyProposalMessages = "proposal_messages" // Msg type_urls in the proposal + AttributeKeyVotingPeriodStart = "voting_period_start" + AttributeKeyProposalLog = "proposal_log" // log of proposal execution + AttributeKeyProposalDepositError = "proposal_deposit_error" // error on proposal deposit refund/burn + AttributeValueProposalDropped = "proposal_dropped" // didn't meet min deposit AttributeValueProposalPassed = "proposal_passed" // met vote quorum AttributeValueProposalRejected = "proposal_rejected" // didn't meet vote quorum