refactor(baseapp): handle cases for PostHandler in runTx

CHANGELOG.md

@@ -147,6 +147,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### State Machine Breaking
 
+* (baseapp) [#14593](https://github.com/cosmos/cosmos-sdk/pull/14593) Refactor `runTx` to handle `PostHandler` cases.
 * (baseapp, x/auth/posthandler) [#13940](https://github.com/cosmos/cosmos-sdk/pull/13940) Update `PostHandler` to receive the `runTx` success boolean.
 * (x/group) [#13742](https://github.com/cosmos/cosmos-sdk/pull/13742) Migrate group policy account from module accounts to base account.
 * (codec) [#13307](https://github.com/cosmos/cosmos-sdk/pull/13307) Register all modules' `Msg`s with group's ModuleCdc so that Amino sign bytes are correctly generated.

baseapp/baseapp.go

@@ -601,12 +601,17 @@ func (app *BaseApp) cacheTxContext(ctx sdk.Context, txBytes []byte) (sdk.Context
 // Note, gas execution info is always returned. A reference to a Result is
 // returned if the tx does not run out of gas and if all the messages are valid
 // and execute successfully. An error is returned otherwise.
-func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, result *sdk.Result, anteEvents []abci.Event, priority int64, err error) {
+func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, result *sdk.Result, events []abci.Event, priority int64, err error) {
 	// NOTE: GasWanted should be returned by the AnteHandler. GasUsed is
 	// determined by the GasMeter. We need access to the context to get the gas
 	// meter, so we initialize upfront.
 	var gasWanted uint64
 
+	var (
+		anteEvents []abci.Event
+		postEvents []abci.Event
+	)
+
 	ctx := app.getContextForTx(mode, txBytes)
 	ms := ctx.MultiStore()
 
@@ -692,23 +697,25 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, re
 		}
 
 		priority = ctx.Priority()
-		msCache.Write()
 		anteEvents = events.ToABCIEvents()
+		msCache.Write()
 	}
 
-	if mode == runTxModeCheck {
+	// insert or remove the transaction from the mempool
+	switch mode {
+	case runTxModeCheck:
 		err = app.mempool.Insert(ctx, tx)
-		if err != nil {
-			return gInfo, nil, anteEvents, priority, err
-		}
-	} else if mode == runTxModeDeliver {
+	case runTxModeDeliver:
 		err = app.mempool.Remove(tx)
 		if err != nil && !errors.Is(err, mempool.ErrTxNotFound) {
-			return gInfo, nil, anteEvents, priority,
-				fmt.Errorf("failed to remove tx from mempool: %w", err)
+			err = fmt.Errorf("failed to remove tx from mempool: %w", err)
 		}
 	}
 
+	if err != nil {
+		return gInfo, nil, events, priority, err
+	}
+
 	// Create a new Context based off of the existing Context with a MultiStore branch
 	// in case message processing fails. At this point, the MultiStore
 	// is a branch of a branch.
@@ -719,39 +726,85 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, re
 	// Result if any single message fails or does not have a registered Handler.
 	result, err = app.runMsgs(runMsgCtx, msgs, mode)
 
-	if err == nil {
+	// Case 1: the msg errors and the post handler is not set.
+	if err != nil && app.postHandler == nil {
+		return gInfo, nil, anteEvents, priority, err
+	}
 
-		// Run optional postHandlers.
-		//
-		// Note: If the postHandler fails, we also revert the runMsgs state.
-		if app.postHandler != nil {
-			// The runMsgCtx context currently contains events emitted by the ante handler.
-			// We clear this to correctly order events without duplicates.
-			// Note that the state is still preserved.
-			postCtx := runMsgCtx.WithEventManager(sdk.NewEventManager())
-
-			newCtx, err := app.postHandler(postCtx, tx, mode == runTxModeSimulate, err == nil)
-			if err != nil {
-				return gInfo, nil, anteEvents, priority, err
-			}
+	// Case 2: tx errors and the post handler is set. Run PostHandler and revert state from runMsgs
+	if err != nil && app.postHandler != nil {
+		// Run optional postHandlers with a context branched off the ante handler ctx
+		postCtx, postCache := app.cacheTxContext(ctx, txBytes)
 
-			result.Events = append(result.Events, newCtx.EventManager().ABCIEvents()...)
+		newCtx, err := app.postHandler(postCtx, tx, mode == runTxModeSimulate, err == nil)
+		if err != nil {
+			// return result in case the pointer has been modified by the post decorators
+			return gInfo, result, events, priority, err
 		}
 
 		if mode == runTxModeDeliver {
 			// When block gas exceeds, it'll panic and won't commit the cached store.
 			consumeBlockGas()
 
-			msCache.Write()
+			postCache.Write()
 		}
 
 		if len(anteEvents) > 0 && (mode == runTxModeDeliver || mode == runTxModeSimulate) {
 			// append the events in the order of occurrence
-			result.Events = append(anteEvents, result.Events...)
+			postEvents = newCtx.EventManager().ABCIEvents()
+			events = make([]abci.Event, len(anteEvents)+len(postEvents))
+			copy(events[:len(anteEvents)], anteEvents)
+			copy(events[len(anteEvents):], postEvents)
+			result.Events = append(result.Events, events...)
+		} else {
+			events = make([]abci.Event, len(postEvents))
+			copy(events, postEvents)
+			result.Events = append(result.Events, postEvents...)
 		}
+
+		return gInfo, result, events, priority, err
+	}
+
+	// Case 3: tx successful and post handler is set. Run Post Handler with runMsgCtx so that the state from runMsgs is persisted
+	if app.postHandler != nil {
+		newCtx, err := app.postHandler(runMsgCtx, tx, mode == runTxModeSimulate, err == nil)
+		if err != nil {
+			return gInfo, nil, nil, priority, err
+		}
+
+		postEvents = newCtx.EventManager().Events().ToABCIEvents()
+		result.Events = append(result.Events, postEvents...)
+
+		if len(anteEvents) > 0 {
+			events = make([]abci.Event, len(anteEvents)+len(postEvents))
+			copy(events[:len(anteEvents)], anteEvents)
+			copy(events[len(anteEvents):], postEvents)
+		} else {
+			events = make([]abci.Event, len(postEvents))
+			copy(events, postEvents)
+		}
+	}
+
+	// Case 4: tx successful and post handler is not set.
+
+	if mode == runTxModeDeliver {
+		// When block gas exceeds, it'll panic and won't commit the cached store.
+		consumeBlockGas()
+
+		msCache.Write()
+	}
+
+	if len(anteEvents) > 0 && (mode == runTxModeDeliver || mode == runTxModeSimulate) {
+		// append the events in the order of occurrence:
+		// 	1. AnteHandler events
+		// 	2. Transaction Result events
+		// 	3. PostHandler events
+		result.Events = append(anteEvents, result.Events...)
+
+		copy(events, result.Events)
 	}
 
-	return gInfo, result, anteEvents, priority, err
+	return gInfo, result, events, priority, err
 }
 
 // runMsgs iterates through a list of messages and executes them with the provided

baseapp/baseapp_test.go

@@ -3,9 +3,14 @@ package baseapp_test
 import (
 	"fmt"
 	"math/rand"
+	"os"
 	"testing"
 	"time"
 
+	"cosmossdk.io/depinject"
+
+	"github.com/cosmos/cosmos-sdk/runtime"
+
 	abci "github.com/cometbft/cometbft/abci/types"
 	"github.com/cometbft/cometbft/libs/log"
 	tmproto "github.com/cometbft/cometbft/proto/tendermint/types"
@@ -656,3 +661,167 @@ func TestLoadVersionPruning(t *testing.T) {
 	require.Nil(t, err)
 	testLoadVersionHelper(t, app, int64(7), lastCommitID)
 }
+
+func TestBaseAppPostHandler(t *testing.T) {
+	testCases := []struct {
+		name     string
+		testFunc func()
+	}{
+		{
+			"success case 1 - The msg errors and the PostHandler is not set",
+			func() {
+				// Setup baseapp.
+				var (
+					appBuilder *runtime.AppBuilder
+					cdc        codec.ProtoCodecMarshaler
+				)
+				err := depinject.Inject(makeMinimalConfig(), &appBuilder, &cdc)
+				require.NoError(t, err)
+
+				testCtx := testutil.DefaultContextWithDB(t, capKey1, storetypes.NewTransientStoreKey("transient_test"))
+
+				app := appBuilder.Build(log.NewTMLogger(log.NewSyncWriter(os.Stdout)), testCtx.DB, nil)
+				app.SetCMS(testCtx.CMS)
+				baseapptestutil.RegisterInterfaces(cdc.InterfaceRegistry())
+
+				// patch in TxConfig instead of using an output from x/auth/tx
+				txConfig := authtx.NewTxConfig(cdc, authtx.DefaultSignModes)
+				// set the TxDecoder in the BaseApp for minimal tx simulations
+				app.SetTxDecoder(txConfig.TxDecoder())
+
+				app.InitChain(abci.RequestInitChain{})
+
+				deliverKey := []byte("deliver-key")
+				baseapptestutil.RegisterCounterServer(app.MsgServiceRouter(), CounterServerImpl{t, capKey1, deliverKey})
+
+				header := tmproto.Header{Height: int64(1)}
+				app.BeginBlock(abci.RequestBeginBlock{Header: header})
+
+				tx := newTxCounter(t, txConfig, 1, 0)
+				tx = setFailOnHandler(txConfig, tx, true)
+				txBytes, err := txConfig.TxEncoder()(tx)
+				require.NoError(t, err)
+
+				r := app.CheckTx(abci.RequestCheckTx{Tx: txBytes})
+				require.True(t, r.IsOK(), fmt.Sprintf("%v", r))
+
+				res := app.DeliverTx(abci.RequestDeliverTx{Tx: txBytes})
+				require.False(t, res.IsOK(), fmt.Sprintf("%v", res))
+
+				events := res.GetEvents()
+				require.Len(t, events, 0, "Should not contain any events as the post handler is not set")
+
+				app.EndBlock(abci.RequestEndBlock{})
+				app.Commit()
+			},
+		},
+		{
+			"success case 2 - The msg errors and the PostHandler is set",
+			func() {
+				postKey := []byte("post-key")
+				anteKey := []byte("ante-key")
+				// Setup baseapp.
+				var (
+					appBuilder *runtime.AppBuilder
+					cdc        codec.ProtoCodecMarshaler
+				)
+				err := depinject.Inject(makeMinimalConfig(), &appBuilder, &cdc)
+				require.NoError(t, err)
+
+				testCtx := testutil.DefaultContextWithDB(t, capKey1, storetypes.NewTransientStoreKey("transient_test"))
+
+				app := appBuilder.Build(log.NewTMLogger(log.NewSyncWriter(os.Stdout)), testCtx.DB, nil)
+				app.SetCMS(testCtx.CMS)
+				baseapptestutil.RegisterInterfaces(cdc.InterfaceRegistry())
+
+				// patch in TxConfig instead of using an output from x/auth/tx
+				txConfig := authtx.NewTxConfig(cdc, authtx.DefaultSignModes)
+				// set the TxDecoder in the BaseApp for minimal tx simulations
+				app.SetTxDecoder(txConfig.TxDecoder())
+
+				app.InitChain(abci.RequestInitChain{})
+
+				deliverKey := []byte("deliver-key")
+				baseapptestutil.RegisterCounterServer(app.MsgServiceRouter(), CounterServerImpl{t, capKey1, deliverKey})
+
+				app.SetAnteHandler(anteHandlerTxTest(t, capKey1, anteKey))
+				app.SetPostHandler(testTxPostHandler(t, capKey1, postKey))
+				header := tmproto.Header{Height: int64(1)}
+				app.BeginBlock(abci.RequestBeginBlock{Header: header})
+
+				tx := newTxCounter(t, txConfig, 0, 0)
+				tx = setFailOnHandler(txConfig, tx, true)
+				txBytes, err := txConfig.TxEncoder()(tx)
+				require.NoError(t, err)
+
+				r := app.CheckTx(abci.RequestCheckTx{Tx: txBytes})
+				require.True(t, r.IsOK(), fmt.Sprintf("%v", r))
+
+				res := app.DeliverTx(abci.RequestDeliverTx{Tx: txBytes})
+				require.False(t, res.IsOK(), fmt.Sprintf("%v", res))
+
+				events := res.GetEvents()
+				require.Len(t, events, 3, "Contains the AnteHandler and the PostHandler")
+
+				app.EndBlock(abci.RequestEndBlock{})
+				app.Commit()
+			},
+		},
+		{
+			"success case 3 - Run Post Handler with runMsgCtx so that the state from runMsgs is persisted",
+			func() {
+				postKey := []byte("post-key")
+				// Setup baseapp.
+				var (
+					appBuilder *runtime.AppBuilder
+					cdc        codec.ProtoCodecMarshaler
+				)
+				err := depinject.Inject(makeMinimalConfig(), &appBuilder, &cdc)
+				require.NoError(t, err)
+
+				testCtx := testutil.DefaultContextWithDB(t, capKey1, storetypes.NewTransientStoreKey("transient_test"))
+
+				app := appBuilder.Build(log.NewTMLogger(log.NewSyncWriter(os.Stdout)), testCtx.DB, nil)
+				app.SetCMS(testCtx.CMS)
+				baseapptestutil.RegisterInterfaces(cdc.InterfaceRegistry())
+
+				// patch in TxConfig instead of using an output from x/auth/tx
+				txConfig := authtx.NewTxConfig(cdc, authtx.DefaultSignModes)
+				// set the TxDecoder in the BaseApp for minimal tx simulations
+				app.SetTxDecoder(txConfig.TxDecoder())
+
+				app.InitChain(abci.RequestInitChain{})
+
+				deliverKey := []byte("deliver-key")
+				baseapptestutil.RegisterCounterServer(app.MsgServiceRouter(), CounterServerImpl{t, capKey1, deliverKey})
+
+				app.SetPostHandler(testTxPostHandler(t, capKey1, postKey))
+				header := tmproto.Header{Height: int64(1)}
+				app.BeginBlock(abci.RequestBeginBlock{Header: header})
+
+				tx := newTxCounter(t, txConfig, 0, 0)
+				txBytes, err := txConfig.TxEncoder()(tx)
+				require.NoError(t, err)
+
+				r := app.CheckTx(abci.RequestCheckTx{Tx: txBytes})
+				require.True(t, r.IsOK(), fmt.Sprintf("%v", r))
+
+				res := app.DeliverTx(abci.RequestDeliverTx{Tx: txBytes})
+				require.True(t, res.IsOK(), fmt.Sprintf("%v", res))
+
+				events := res.GetEvents()
+				require.Len(t, events, 3, "should contain ante handler, message type and counter events respectively")
+
+				require.NoError(t, err)
+				app.EndBlock(abci.RequestEndBlock{})
+				app.Commit()
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			tc.testFunc()
+		})
+	}
+}

baseapp/utils_test.go

@@ -232,6 +232,20 @@ func anteHandlerTxTest(t *testing.T, capKey storetypes.StoreKey, storeKey []byte
 	}
 }
 
+func testTxPostHandler(t *testing.T, capKey storetypes.StoreKey, storeKey []byte) sdk.PostHandler {
+	return func(ctx sdk.Context, tx sdk.Tx, simulate, success bool) (sdk.Context, error) {
+		counter, _ := parseTxMemo(t, tx)
+
+		ctx.EventManager().EmitEvents(
+			counterEvent("post_handler", counter),
+		)
+
+		ctx = ctx.WithPriority(testTxPriority)
+
+		return ctx, nil
+	}
+}
+
 func incrementingCounter(t *testing.T, store storetypes.KVStore, counterKey []byte, counter int64) (*sdk.Result, error) {
 	storedCounter := getIntFromStore(t, store, counterKey)
 	require.Equal(t, storedCounter, counter)