fix(baseapp): Fix post handler error always results in code 1 (#24261)

Co-authored-by: Alex | Interchain Labs <alex@interchainlabs.io>
Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>

CHANGELOG.md

@@ -75,6 +75,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### Bug Fixes
 
+* (baseapp) [24261](https://github.com/cosmos/cosmos-sdk/pull/24261) Fix post handler error always results in code 1
 * (server) [#24068](https://github.com/cosmos/cosmos-sdk/pull/24068) Allow align block header with skip check header in grpc server.
 * (x/gov) [#24044](https://github.com/cosmos/cosmos-sdk/pull/24044) Fix some places in which we call Remove inside a Walk (x/gov).
 * (baseapp) [#24042](https://github.com/cosmos/cosmos-sdk/pull/24042) Fixed a data race inside BaseApp.getContext, found by end-to-end (e2e) tests.

baseapp/baseapp.go

@@ -980,7 +980,12 @@ func (app *BaseApp) runTx(mode execMode, txBytes []byte, tx sdk.Tx) (gInfo sdk.G
 
 		newCtx, errPostHandler := app.postHandler(postCtx, tx, mode == execModeSimulate, err == nil)
 		if errPostHandler != nil {
-			return gInfo, nil, anteEvents, errors.Join(err, errPostHandler)
+			if err == nil {
+				// when the msg was handled successfully, return the post handler error only
+				return gInfo, nil, anteEvents, errPostHandler
+			}
+			// otherwise append to the msg error so that we keep the original error code for better user experience
+			return gInfo, nil, anteEvents, errorsmod.Wrapf(err, "postHandler: %s", errPostHandler)
 		}
 
 		// we don't want runTx to panic if runMsgs has failed earlier

baseapp/baseapp_test.go

@@ -12,6 +12,7 @@ import (
 	abci "github.com/cometbft/cometbft/abci/types"
 	cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"
 	dbm "github.com/cosmos/cosmos-db"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	errorsmod "cosmossdk.io/errors"
@@ -32,6 +33,7 @@ import (
 	"github.com/cosmos/cosmos-sdk/testutil"
 	"github.com/cosmos/cosmos-sdk/testutil/testdata"
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
 	authtx "github.com/cosmos/cosmos-sdk/x/auth/tx"
 )
 
@@ -690,6 +692,67 @@ func TestBaseAppPostHandler(t *testing.T) {
 	require.NotContains(t, suite.logBuffer.String(), "panic recovered in runTx")
 }
 
+func TestBaseAppPostHandlerErrorHandling(t *testing.T) {
+	specs := map[string]struct {
+		msgHandlerErr  error
+		postHandlerErr error
+		expCode        uint32
+		expLog         string
+	}{
+		"msg handler ok, post ok": {
+			expLog:  "",
+			expCode: 0,
+		},
+		"msg handler fails, post ok": {
+			msgHandlerErr: sdkerrors.ErrUnknownRequest.Wrap("my svc error"),
+			expCode:       sdkerrors.ErrUnknownRequest.ABCICode(),
+			expLog:        "failed to execute message; message index: 0: my svc error: unknown request",
+		},
+		"msg handler ok, post fails": {
+			postHandlerErr: sdkerrors.ErrInsufficientFunds.Wrap("my post handler error"),
+			expCode:        sdkerrors.ErrInsufficientFunds.ABCICode(),
+			expLog:         "my post handler error: insufficient funds",
+		},
+		"both fail": {
+			msgHandlerErr:  sdkerrors.ErrUnknownRequest.Wrap("my svc error"),
+			postHandlerErr: sdkerrors.ErrInsufficientFunds.Wrap("my post handler error"),
+			expCode:        sdkerrors.ErrUnknownRequest.ABCICode(),
+			expLog:         "postHandler: my post handler error: insufficient funds: failed to execute message; message index: 0: my svc error: unknown request",
+		},
+	}
+	for name, spec := range specs {
+		t.Run(name, func(t *testing.T) {
+			anteOpt := func(bapp *baseapp.BaseApp) {
+				bapp.SetPostHandler(func(ctx sdk.Context, tx sdk.Tx, simulate, success bool) (newCtx sdk.Context, err error) {
+					return ctx, spec.postHandlerErr
+				})
+			}
+			suite := NewBaseAppSuite(t, anteOpt)
+			csMock := mockCounterServer{
+				incrementCounterFn: func(ctx context.Context, counter *baseapptestutil.MsgCounter) (*baseapptestutil.MsgCreateCounterResponse, error) {
+					return &baseapptestutil.MsgCreateCounterResponse{}, spec.msgHandlerErr
+				},
+			}
+			baseapptestutil.RegisterCounterServer(suite.baseApp.MsgServiceRouter(), csMock)
+
+			_, err := suite.baseApp.InitChain(&abci.RequestInitChain{
+				ConsensusParams: &cmtproto.ConsensusParams{},
+			})
+			require.NoError(t, err)
+
+			txBytes, err := suite.txConfig.TxEncoder()(newTxCounter(t, suite.txConfig, 0, 0))
+			require.NoError(t, err)
+
+			// when
+			res, err := suite.baseApp.FinalizeBlock(&abci.RequestFinalizeBlock{Height: 1, Txs: [][]byte{txBytes}})
+			// then
+			require.NoError(t, err)
+			assert.Equal(t, spec.expCode, res.TxResults[0].Code)
+			assert.Equal(t, spec.expLog, res.TxResults[0].Log)
+		})
+	}
+}
+
 // Test and ensure that invalid block heights always cause errors.
 // See issues:
 // - https://github.com/cosmos/cosmos-sdk/issues/11220

baseapp/utils_test.go

@@ -133,6 +133,17 @@ func (m CounterServerImplGasMeterOnly) IncrementCounter(ctx context.Context, msg
 	return &baseapptestutil.MsgCreateCounterResponse{}, nil
 }
 
+type mockCounterServer struct {
+	incrementCounterFn func(context.Context, *baseapptestutil.MsgCounter) (*baseapptestutil.MsgCreateCounterResponse, error)
+}
+
+func (m mockCounterServer) IncrementCounter(ctx context.Context, req *baseapptestutil.MsgCounter) (*baseapptestutil.MsgCreateCounterResponse, error) {
+	if m.incrementCounterFn == nil {
+		panic("not expected to be called")
+	}
+	return m.incrementCounterFn(ctx, req)
+}
+
 type NoopCounterServerImpl struct{}
 
 func (m NoopCounterServerImpl) IncrementCounter(