From e29139fe836bb5490059ad89ced41ba4d15bfc16 Mon Sep 17 00:00:00 2001 From: Alexander Peters Date: Fri, 4 Apr 2025 15:57:08 +0200 Subject: [PATCH] fix(baseapp): Fix post handler error always results in code 1 (#24261) Co-authored-by: Alex | Interchain Labs Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com> --- CHANGELOG.md | 1 + baseapp/baseapp.go | 7 ++++- baseapp/baseapp_test.go | 63 +++++++++++++++++++++++++++++++++++++++++ baseapp/utils_test.go | 11 +++++++ 4 files changed, 81 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cd60b380fe..a6e21d2cad 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index 1e735d5aa5..f4d9983039 100644 --- a/baseapp/baseapp.go +++ b/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 diff --git a/baseapp/baseapp_test.go b/baseapp/baseapp_test.go index 8edce7c0c6..2813dfc3ea 100644 --- a/baseapp/baseapp_test.go +++ b/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 diff --git a/baseapp/utils_test.go b/baseapp/utils_test.go index 6a280a7552..d2f9ce50d0 100644 --- a/baseapp/utils_test.go +++ b/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(