fix: correctly propagate msg errors in gov

[johnletey]

Description

When upgrading our chain to v0.46.5, our governance unit tests were failing. After doing some further investigation, we realised that proposals with invalid messages no longer had the status StatusFailed and instead had the status StatusPassed. This PR fixes this issue by correctly propagating message errors after #13728.

cc @damiannolan @mbreithecker

---

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

• included the correct type prefix in the PR title
• added ! to the type prefix if API or client breaking change
• targeted the correct branch (see PR Targeting)
• provided a link to the relevant issue or specification
• followed the guidelines for building modules
• included the necessary unit and integration tests
• added a changelog entry to CHANGELOG.md
• included comments for documenting Go code
• updated the relevant documentation or specification
• reviewed "Files changed" and left comments if necessary
• confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

• confirmed the correct type prefix in the PR title
• confirmed ! in the type prefix if API or client breaking change
• confirmed all author checklist items have been addressed
• reviewed state machine logic
• reviewed API design and naming
• reviewed documentation is accurate
• reviewed tests and test coverage
• manually tested (if applicable)

[johnletey]

This should be backported to v0.46.x, and in theory this error might also be in x/group after #13808.

[mbreithecker]

Another thing to add:
Even if the message execution fails it gets flushed to the KV-Store:
https://github.com/cosmos/cosmos-sdk/blob/main/x/gov/abci.go#L92

This could potentially lead to inconsistent state.

[julienrbrt]

Could you maybe add a test and a changelog too?

[codecov]

Codecov Report

Merging #13918 (f866c66) into main (34dcca7) will decrease coverage by 0.33%.
The diff coverage is 100.00%.

❗ Current head f866c66 differs from pull request most recent head 84843be. Consider uploading reports for the commit 84843be to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13918      +/-   ##
==========================================
- Coverage   56.90%   56.57%   -0.34%     
==========================================
  Files         641      638       -3     
  Lines       55828    54912     -916     
==========================================
- Hits        31769    31066     -703     
+ Misses      21481    21321     -160     
+ Partials     2578     2525      -53     

Impacted Files	Coverage Δ
x/gov/abci.go	90.47% <100.00%> (+0.18%)	⬆️
x/distribution/simulation/operations.go	80.64% <0.00%> (-9.68%)	⬇️
x/group/keeper/keeper.go	56.07% <0.00%> (-0.40%)	⬇️
math/dec.go
math/uint.go
math/int.go
crypto/keys/internal/ecdsa/privkey.go	83.01% <0.00%> (+1.88%)	⬆️

[johnletey]

@julienrbrt Happy to add a test for this - what would be a message to check if it fails execution?

A majority of modules still use legacy proposal types 😄

[julienrbrt]

@julienrbrt Happy to add a test for this - what would be a message to check if it fails execution?

A majority of modules still use legacy proposal types smile

I think it is a matter of fixing TestEndBlockerProposalHandlerFailed that does not seem to work, and neither check anything.

[johnletey]

@julienrbrt Tried to update the TestEndBlockerProposalHandlerFailed, but it looks like the proposal is executed just fine. Would you (or someone else on the team) mind taking a look and updating it? I think something is going wrong when setting the contextKeyBadProposal value.

[julienrbrt]

@julienrbrt Tried to update the TestEndBlockerProposalHandlerFailed, but it looks like the proposal is executed just fine. Would you (or someone else on the team) mind taking a look and updating it? I think something is going wrong when setting the contextKeyBadProposal value.

Sure, I will have a look!

[julienrbrt]

@johnletey

Can you update the test to this:

func TestEndBlockerProposalHandlerFailed(t *testing.T) {
	suite := createTestSuite(t)
	app := suite.App
	ctx := app.BaseApp.NewContext(false, tmproto.Header{})
	addrs := simtestutil.AddTestAddrs(suite.BankKeeper, suite.StakingKeeper, ctx, 1, valTokens)

	SortAddresses(addrs)

	stakingMsgSvr := stakingkeeper.NewMsgServerImpl(suite.StakingKeeper)
	header := tmproto.Header{Height: app.LastBlockHeight() + 1}
	app.BeginBlock(abci.RequestBeginBlock{Header: header})

	valAddr := sdk.ValAddress(addrs[0])

	createValidators(t, stakingMsgSvr, ctx, []sdk.ValAddress{valAddr}, []int64{10})
	staking.EndBlocker(ctx, suite.StakingKeeper)

	msg := banktypes.NewMsgSend(authtypes.NewModuleAddress(govtypes.ModuleName), addrs[0], sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100000))))
	proposal, err := suite.GovKeeper.SubmitProposal(ctx, []sdk.Msg{msg}, "")
	require.NoError(t, err)

	proposalCoins := sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, suite.StakingKeeper.TokensFromConsensusPower(ctx, 10)))
	newDepositMsg := v1.NewMsgDeposit(addrs[0], proposal.Id, proposalCoins)

	govMsgSvr := keeper.NewMsgServerImpl(suite.GovKeeper)
	res, err := govMsgSvr.Deposit(sdk.WrapSDKContext(ctx), newDepositMsg)
	require.NoError(t, err)
	require.NotNil(t, res)

	err = suite.GovKeeper.AddVote(ctx, proposal.Id, addrs[0], v1.NewNonSplitVoteOption(v1.OptionYes), "")
	require.NoError(t, err)

	newHeader := ctx.BlockHeader()
	newHeader.Time = ctx.BlockHeader().Time.Add(*suite.GovKeeper.GetParams(ctx).MaxDepositPeriod).Add(*suite.GovKeeper.GetParams(ctx).VotingPeriod)
	ctx = ctx.WithBlockHeader(newHeader)

	// validate that the proposal fails/has been rejected
	gov.EndBlocker(ctx, suite.GovKeeper)

	proposal, ok := suite.GovKeeper.GetProposal(ctx, proposal.Id)
	require.True(t, ok)
	require.Equal(t, v1.StatusFailed, proposal.Status)
}

Then you can delete that contextKeyBadProposal that has no use in common_test.go

[julienrbrt]

pre-approving, pending the addition of the test mentioned here: #13918 (comment)

[johnletey]

@julienrbrt Thanks for taking a look at the tests. I just pushed! It's good to go now 👍🏻

[julienrbrt]

@julienrbrt Thanks for taking a look at the tests. I just pushed! It's good to go now 👍🏻

Thanks! Small nit, can you delete contextKeyBadProposal that isn't used anywhere then?