[STAB-19] Reduce the size of the critical section

This change makes a copy of `runTx` and does the following:
* moves transaction descoding and tx validation before lock acquisition
* moves creation of the response after releasing the lock
* removes support for the post handler (unused by dYdX) and extracts out the only code that is executed in `runMsgs` allowing us to avoid the creation of the `runMsgCtx` and its associated `MultiStore`
* removes `consumeBlockGas` since it is only executed during `deliverTx`

README.md

@@ -32,9 +32,10 @@ When a new version of CosmosSDK is published, we may want to adopt the changes i
 3. Push the new branch (i.e `dydx-fork-v0.47.0-alpha2`).
 4. Off of this new branch, create a new branch. (i.e `totoro/dydxCommits`)
 5. Cherry-pick each dydx-created commit from the current default branch, in order, on to the new `dydx-fork-$VERSION` branch (note: you may want to consider creating multiple PRs for this process if there are difficulties or merge conflicts). For example, `git cherry-pick <commit hash>`. You can verify the first commit by seeing the most recent commit sha for the `$VERSION` (i.e `v0.47.0-alpha2`) tag on the `cosmos/cosmos-sdk` repo, and taking the next commit from that sha.
-6. Open a PR to merge the second branch (`totoro/dydxCommits`) into the first (`dydx-fork-v0.47.0-alpha2`). Get approval, and merge. *DO NOT SQUASH AND MERGE. PLEASE REBASE AND MERGE*
-7. Update `dydxprotocol/v4` by following the steps in "Making Changes to the fork" above.
-8. Set `dydx-fork-$VERSION` as the [default branch](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-branches-in-your-repository/changing-the-default-branch) in this repository.
+6. The dYdX fork uses an optimized version of `runTx` for handling ABCI CheckTx messages. Ensure that the logic within `runCheckTxConcurrently` stays up to date with the `runTx` counterpart by diffing [baseapp.go#runTx](https://github.com/dydxprotocol/cosmos-sdk/blob/6477dcf5693422fef693432e29bd979709aa995d/baseapp/baseapp.go#L754) current version against the new version copying over relevant changes to [baseapp.go#runCheckTxConcurrently](https://github.com/dydxprotocol/cosmos-sdk/blob/6477dcf5693422fef693432e29bd979709aa995d/baseapp/baseapp.go#L619).
+7. Open a PR to merge the second branch (`totoro/dydxCommits`) into the first (`dydx-fork-v0.47.0-alpha2`). Get approval, and merge. *DO NOT SQUASH AND MERGE. PLEASE REBASE AND MERGE*
+8. Update `dydxprotocol/v4` by following the steps in "Making Changes to the fork" above.
+9. Set `dydx-fork-$VERSION` as the [default branch](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-branches-in-your-repository/changing-the-default-branch) in this repository.
 
 Note that the github CI workflows need permissioning. If they fail with a permissioning error, i.e `Resource not accessible through integration`, a Github admin will need to whitelist the workflow.
 

baseapp/abci.go

@@ -368,9 +368,6 @@ func (app *BaseApp) ApplySnapshotChunk(req *abci.RequestApplySnapshotChunk) (*ab
 // will contain relevant error information. Regardless of tx execution outcome,
 // the ResponseCheckTx will contain relevant gas execution context.
 func (app *BaseApp) CheckTx(req *abci.RequestCheckTx) (*abci.ResponseCheckTx, error) {
-	app.mtx.Lock()
-	defer app.mtx.Unlock()
-
 	var mode execMode
 
 	switch {
@@ -384,7 +381,7 @@ func (app *BaseApp) CheckTx(req *abci.RequestCheckTx) (*abci.ResponseCheckTx, er
 		return nil, fmt.Errorf("unknown RequestCheckTx type: %s", req.Type)
 	}
 
-	gInfo, result, anteEvents, err := app.runTx(mode, req.Tx)
+	gInfo, result, anteEvents, err := app.runCheckTxConcurrently(mode, req.Tx)
 	if err != nil {
 		return sdkerrors.ResponseCheckTxWithEvents(err, gInfo.GasWanted, gInfo.GasUsed, anteEvents, app.trace), nil
 	}

baseapp/baseapp.go

@@ -6,6 +6,7 @@ import (
 	"math"
 	"sort"
 	"strconv"
+	"strings"
 	"sync"
 
 	"github.com/cockroachdb/errors"
@@ -757,6 +758,144 @@ func (app *BaseApp) endBlock(ctx context.Context) (sdk.EndBlock, error) {
 	return endblock, nil
 }
 
+// runCheckTxConcurrently processes a transaction with either the checkTx or recheckTx modes, encoded transaction
+// bytes, and the decoded transaction itself. All state transitions occur through
+// a cached Context depending on the mode provided.
+//
+// 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) runCheckTxConcurrently(mode execMode, txBytes []byte) (gInfo sdk.GasInfo, result *sdk.Result, anteEvents []abci.Event, err error) {
+	if mode != execModeCheck && mode != execModeReCheck {
+		panic("runCheckTxConcurrently can only be invoked for CheckTx and RecheckTx.")
+	}
+
+	// We don't support the post handler specifically to avoid creating a branched MultiStore and since dYdX
+	// doesn't need support for it. Once support is necessary or when we are trying to upstream these changes
+	// we can guard creation of the MultiStore to only occur when the post handler is specified.
+	if app.postHandler != nil {
+		panic("CheckTx/RecheckTx does not support a post hander.")
+	}
+
+	// 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
+
+	tx, err := app.txDecoder(txBytes)
+	if err != nil {
+		return sdk.GasInfo{}, nil, nil, err
+	}
+
+	msgs := tx.GetMsgs()
+	if err := validateBasicTxMsgs(msgs); err != nil {
+		return sdk.GasInfo{}, nil, nil, err
+	}
+
+	for _, msg := range msgs {
+		handler := app.msgServiceRouter.Handler(msg)
+		if handler == nil {
+			return sdk.GasInfo{}, nil, nil, errorsmod.Wrapf(sdkerrors.ErrUnknownRequest, "no message handler found for %T", msg)
+		}
+	}
+
+	// Execute the critical section under lock.
+	//
+	// Note that careful consideration is needed in the block below to ensure that we don't redefine
+	// gInfo, result, anteEvents, priority, or err local variables. Also note that this function is
+	// embedded here to ensure that the lifetime of the mutex is limited to only this function allowing
+	// for the return values to be computed without holding the lock.
+	func() {
+		app.mtx.Lock()
+		defer app.mtx.Unlock()
+
+		ctx := app.getContextForTx(mode, txBytes)
+		ms := ctx.MultiStore()
+
+		defer func() {
+			if r := recover(); r != nil {
+				recoveryMW := newOutOfGasRecoveryMiddleware(gasWanted, ctx, app.runTxRecoveryMiddleware)
+				err, result = processRecovery(r, recoveryMW), nil
+			}
+
+			gInfo = sdk.GasInfo{GasWanted: gasWanted, GasUsed: ctx.GasMeter().GasConsumed()}
+		}()
+
+		if app.anteHandler != nil {
+			var (
+				anteCtx sdk.Context
+				msCache storetypes.CacheMultiStore
+				newCtx  sdk.Context
+			)
+
+			// Branch context before AnteHandler call in case it aborts.
+			// This is required for both CheckTx and DeliverTx.
+			// Ref: https://github.com/cosmos/cosmos-sdk/issues/2772
+			//
+			// NOTE: Alternatively, we could require that AnteHandler ensures that
+			// writes do not happen if aborted/failed.  This may have some
+			// performance benefits, but it'll be more difficult to get right.
+			anteCtx, msCache = app.cacheTxContext(ctx, txBytes)
+			anteCtx = anteCtx.WithEventManager(sdk.NewEventManager())
+			newCtx, err = app.anteHandler(anteCtx, tx, mode == execModeSimulate)
+
+			if !newCtx.IsZero() {
+				// At this point, newCtx.MultiStore() is a store branch, or something else
+				// replaced by the AnteHandler. We want the original multistore.
+				//
+				// Also, in the case of the tx aborting, we need to track gas consumed via
+				// the instantiated gas meter in the AnteHandler, so we update the context
+				// prior to returning.
+				ctx = newCtx.WithMultiStore(ms)
+			}
+
+			events := ctx.EventManager().Events()
+
+			// GasMeter expected to be set in AnteHandler
+			gasWanted = ctx.GasMeter().Limit()
+
+			if err != nil {
+				// Note that we set the outputs here and return from the critical function back into
+				// runCheckTxConcurrently which will check `err` and return immediately.
+				result = nil
+				anteEvents = nil
+				return
+			}
+
+			msCache.Write()
+			anteEvents = events.ToABCIEvents()
+		}
+
+		if mode == execModeCheck {
+			err = app.mempool.Insert(ctx, tx)
+			if err != nil {
+				result = nil
+				return
+			}
+		}
+	}()
+	if err != nil {
+		return gInfo, result, anteEvents, err
+	}
+
+	// Execute a stripped down version of runMsgs that is only used for CheckTx and RecheckTx
+	var msgResponses []*codectypes.Any
+	data, err := makeABCIData(msgResponses)
+	if err != nil {
+		return gInfo, result, anteEvents, errorsmod.Wrap(err, "failed to marshal tx data")
+	}
+
+	result = &sdk.Result{
+		Data: data,
+		// Use an empty logs slice and format it to maintain forward compatibility with changes done by the Cosmos SDK.
+		Log:          strings.TrimSpace(sdk.ABCIMessageLogs{}.String()),
+		Events:       sdk.EmptyEvents().ToABCIEvents(),
+		MsgResponses: msgResponses,
+	}
+
+	return gInfo, result, anteEvents, err
+}
+
 // runTx processes a transaction within a given execution mode, encoded transaction
 // bytes, and the decoded transaction itself. All state transitions occur through
 // a cached Context depending on the mode provided. State only gets persisted

baseapp/test_helpers.go

@@ -19,7 +19,7 @@ func (app *BaseApp) SimCheck(txEncoder sdk.TxEncoder, tx sdk.Tx) (sdk.GasInfo, *
 		return sdk.GasInfo{}, nil, errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "%s", err)
 	}
 
-	gasInfo, result, _, err := app.runTx(execModeCheck, bz)
+	gasInfo, result, _, err := app.runCheckTxConcurrently(execModeCheck, bz)
 	return gasInfo, result, err
 }