fix: should revert tx when block gas limit exceeded (#10770)

Closes: #10769 ## Description Solution: - create a `WithBranchedStore ` to handle state snapshot and revert - extract `ConsumeBlockGasMiddleware ` out from `RecoveryTxMiddleware`. - order the middlewares properly. --- ### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)
cosmos · Jan 11, 2022 · 17279fd · 17279fd
1 parent 9aef070
commit 17279fd
Show file tree

Hide file tree

Showing 8 changed files with 266 additions and 54 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -205,6 +205,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * [\#10422](https://github.com/cosmos/cosmos-sdk/pull/10422) and [\#10529](https://github.com/cosmos/cosmos-sdk/pull/10529) Add `MinCommissionRate` param to `x/staking` module.
 * [#10725](https://github.com/cosmos/cosmos-sdk/pull/10725) populate `ctx.ConsensusParams` for begin/end blockers.
 * [#10763](https://github.com/cosmos/cosmos-sdk/pull/10763) modify the fields in `TallyParams` to use `string` instead of `bytes`
+* [#10770](https://github.com/cosmos/cosmos-sdk/pull/10770) revert tx when block gas limit exceeded
 
  ### Deprecated
 

diff --git a/baseapp/baseapp_test.go b/baseapp/baseapp_test.go
@@ -137,6 +137,7 @@ func testTxHandler(options middleware.TxHandlerOptions, customTxHandlerMiddlewar
 		middleware.RecoveryTxMiddleware,
 		middleware.NewIndexEventsTxMiddleware(options.IndexEvents),
 		middleware.ValidateBasicMiddleware,
+		middleware.ConsumeBlockGasMiddleware,
 		CustomTxHandlerMiddleware(customTxHandlerMiddleware),
 	)
 }

diff --git a/x/auth/middleware/block_gas.go b/x/auth/middleware/block_gas.go
@@ -0,0 +1,53 @@
+package middleware
+
+import (
+	"context"
+
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
+	"github.com/cosmos/cosmos-sdk/types/tx"
+)
+
+type consumeBlockGasHandler struct {
+	next tx.Handler
+}
+
+// ConsumeBlockGasMiddleware check and consume block gas meter.
+func ConsumeBlockGasMiddleware(txh tx.Handler) tx.Handler {
+	return consumeBlockGasHandler{next: txh}
+}
+
+var _ tx.Handler = consumeBlockGasHandler{}
+
+// CheckTx implements tx.Handler.CheckTx method.
+func (cbgh consumeBlockGasHandler) CheckTx(ctx context.Context, req tx.Request, checkReq tx.RequestCheckTx) (res tx.Response, resCheckTx tx.ResponseCheckTx, err error) {
+	return cbgh.next.CheckTx(ctx, req, checkReq)
+}
+
+// DeliverTx implements tx.Handler.DeliverTx method.
+// Consume block gas meter, panic when block gas meter exceeded,
+// the panic should be caught by `RecoveryTxMiddleware`.
+func (cbgh consumeBlockGasHandler) DeliverTx(ctx context.Context, req tx.Request) (res tx.Response, err error) {
+	sdkCtx := sdk.UnwrapSDKContext(ctx)
+	// only run the tx if there is block gas remaining
+	if sdkCtx.BlockGasMeter().IsOutOfGas() {
+		err = sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "no block gas left to run tx")
+		return
+	}
+
+	// If BlockGasMeter() panics it will be caught by the `RecoveryTxMiddleware` and will
+	// return an error - in any case BlockGasMeter will consume gas past the limit.
+	defer func() {
+		sdkCtx.BlockGasMeter().ConsumeGas(
+			sdkCtx.GasMeter().GasConsumedToLimit(), "block gas meter",
+		)
+
+	}()
+
+	return cbgh.next.DeliverTx(ctx, req)
+}
+
+// SimulateTx implements tx.Handler.SimulateTx method.
+func (cbgh consumeBlockGasHandler) SimulateTx(ctx context.Context, req tx.Request) (res tx.Response, err error) {
+	return cbgh.next.SimulateTx(ctx, req)
+}
diff --git a/x/auth/middleware/branch_store.go b/x/auth/middleware/branch_store.go
@@ -0,0 +1,70 @@
+package middleware
+
+import (
+	"context"
+
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	"github.com/cosmos/cosmos-sdk/types/tx"
+	tmtypes "github.com/tendermint/tendermint/types"
+)
+
+type branchStoreHandler struct {
+	next tx.Handler
+}
+
+// WithBranchedStore creates a new MultiStore branch and commits the store if the downstream
+// returned no error. It cancels writes from the failed transactions.
+func WithBranchedStore(txh tx.Handler) tx.Handler {
+	return branchStoreHandler{next: txh}
+}
+
+// CheckTx implements tx.Handler.CheckTx method.
+// Do nothing during CheckTx.
+func (sh branchStoreHandler) CheckTx(ctx context.Context, req tx.Request, checkReq tx.RequestCheckTx) (tx.Response, tx.ResponseCheckTx, error) {
+	return sh.next.CheckTx(ctx, req, checkReq)
+}
+
+// DeliverTx implements tx.Handler.DeliverTx method.
+func (sh branchStoreHandler) DeliverTx(ctx context.Context, req tx.Request) (tx.Response, error) {
+	return branchAndRun(ctx, req, sh.next.DeliverTx)
+}
+
+// SimulateTx implements tx.Handler.SimulateTx method.
+func (sh branchStoreHandler) SimulateTx(ctx context.Context, req tx.Request) (tx.Response, error) {
+	return branchAndRun(ctx, req, sh.next.SimulateTx)
+}
+
+type nextFn func(ctx context.Context, req tx.Request) (tx.Response, error)
+
+// branchAndRun creates a new Context based on the existing Context with a MultiStore branch
+// in case message processing fails.
+func branchAndRun(ctx context.Context, req tx.Request, fn nextFn) (tx.Response, error) {
+	sdkCtx := sdk.UnwrapSDKContext(ctx)
+	runMsgCtx, branchedStore := branchStore(sdkCtx, tmtypes.Tx(req.TxBytes))
+
+	rsp, err := fn(sdk.WrapSDKContext(runMsgCtx), req)
+	if err == nil {
+		// commit storage iff no error
+		branchedStore.Write()
+	}
+
+	return rsp, err
+}
+
+// branchStore returns a new context based off of the provided context with
+// a branched multi-store.
+func branchStore(sdkCtx sdk.Context, tx tmtypes.Tx) (sdk.Context, sdk.CacheMultiStore) {
+	ms := sdkCtx.MultiStore()
+	msCache := ms.CacheMultiStore()
+	if msCache.TracingEnabled() {
+		msCache = msCache.SetTracingContext(
+			sdk.TraceContext(
+				map[string]interface{}{
+					"txHash": tx.Hash(),
+				},
+			),
+		).(sdk.CacheMultiStore)
+	}
+
+	return sdkCtx.WithMultiStore(msCache), msCache
+}
diff --git a/x/auth/middleware/branch_store_test.go b/x/auth/middleware/branch_store_test.go
@@ -0,0 +1,129 @@
+package middleware_test
+
+import (
+	"context"
+	"fmt"
+	"math"
+
+	cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
+	"github.com/cosmos/cosmos-sdk/simapp"
+	"github.com/cosmos/cosmos-sdk/testutil/testdata"
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
+	"github.com/cosmos/cosmos-sdk/types/tx"
+	"github.com/cosmos/cosmos-sdk/x/auth/middleware"
+	minttypes "github.com/cosmos/cosmos-sdk/x/mint/types"
+)
+
+var blockMaxGas = uint64(simapp.DefaultConsensusParams.Block.MaxGas)
+
+func (s *MWTestSuite) TestBranchStore() {
+	testcases := []struct {
+		name         string
+		gasToConsume uint64 // gas to consume in the msg execution
+		panicTx      bool   // panic explicitly in tx execution
+		expErr       bool
+	}{
+		{"less than block gas meter", 10, false, false},
+		{"more than block gas meter", blockMaxGas, false, true},
+		{"more than block gas meter", uint64(float64(blockMaxGas) * 1.2), false, true},
+		{"consume MaxUint64", math.MaxUint64, false, true},
+		{"consume block gas when paniced", 10, true, true},
+	}
+
+	for _, tc := range testcases {
+		s.Run(tc.name, func() {
+			ctx := s.SetupTest(true).WithBlockGasMeter(sdk.NewGasMeter(blockMaxGas)) // setup
+			txBuilder := s.clientCtx.TxConfig.NewTxBuilder()
+
+			// tx fee
+			feeCoin := sdk.NewCoin("atom", sdk.NewInt(150))
+			feeAmount := sdk.NewCoins(feeCoin)
+
+			// test account and fund
+			priv1, _, addr1 := testdata.KeyTestPubAddr()
+			err := s.app.BankKeeper.MintCoins(ctx, minttypes.ModuleName, feeAmount)
+			s.Require().NoError(err)
+			err = s.app.BankKeeper.SendCoinsFromModuleToAccount(ctx, minttypes.ModuleName, addr1, feeAmount)
+			s.Require().NoError(err)
+			s.Require().Equal(feeCoin.Amount, s.app.BankKeeper.GetBalance(ctx, addr1, feeCoin.Denom).Amount)
+			seq, _ := s.app.AccountKeeper.GetSequence(ctx, addr1)
+			s.Require().Equal(uint64(0), seq)
+
+			// testMsgTxHandler is a test txHandler that handles one single TestMsg,
+			// consumes the given `tc.gasToConsume`, and sets the bank store "ok" key to "ok".
+			var testMsgTxHandler = customTxHandler{func(ctx context.Context, req tx.Request) (tx.Response, error) {
+				msg, ok := req.Tx.GetMsgs()[0].(*testdata.TestMsg)
+				if !ok {
+					return tx.Response{}, fmt.Errorf("Wrong Msg type, expected %T, got %T", (*testdata.TestMsg)(nil), msg)
+				}
+
+				sdkCtx := sdk.UnwrapSDKContext(ctx)
+				sdkCtx.KVStore(s.app.GetKey("bank")).Set([]byte("ok"), []byte("ok"))
+				sdkCtx.GasMeter().ConsumeGas(tc.gasToConsume, "TestMsg")
+				if tc.panicTx {
+					panic("panic in tx execution")
+				}
+				return tx.Response{}, nil
+			}}
+
+			txHandler := middleware.ComposeMiddlewares(
+				testMsgTxHandler,
+				middleware.NewTxDecoderMiddleware(s.clientCtx.TxConfig.TxDecoder()),
+				middleware.GasTxMiddleware,
+				middleware.RecoveryTxMiddleware,
+				middleware.DeductFeeMiddleware(s.app.AccountKeeper, s.app.BankKeeper, s.app.FeeGrantKeeper),
+				middleware.IncrementSequenceMiddleware(s.app.AccountKeeper),
+				middleware.WithBranchedStore,
+				middleware.ConsumeBlockGasMiddleware,
+			)
+
+			// msg and signatures
+			msg := testdata.NewTestMsg(addr1)
+			var gasLimit uint64 = math.MaxUint64 // no limit on sdk.GasMeter
+			s.Require().NoError(txBuilder.SetMsgs(msg))
+			txBuilder.SetFeeAmount(feeAmount)
+			txBuilder.SetGasLimit(gasLimit)
+
+			privs, accNums, accSeqs := []cryptotypes.PrivKey{priv1}, []uint64{0}, []uint64{0}
+			testTx, _, err := s.createTestTx(txBuilder, privs, accNums, accSeqs, ctx.ChainID())
+			s.Require().NoError(err)
+
+			_, err = txHandler.DeliverTx(sdk.WrapSDKContext(ctx), tx.Request{Tx: testTx})
+
+			bankStore := ctx.KVStore(s.app.GetKey("bank"))
+			okValue := bankStore.Get([]byte("ok"))
+
+			if tc.expErr {
+				s.Require().Error(err)
+				if tc.panicTx {
+					s.Require().True(sdkerrors.IsOf(err, sdkerrors.ErrPanic))
+				} else {
+					s.Require().True(sdkerrors.IsOf(err, sdkerrors.ErrOutOfGas))
+				}
+				s.Require().Empty(okValue)
+			} else {
+				s.Require().NoError(err)
+				s.Require().Equal([]byte("ok"), okValue)
+			}
+			// block gas is always consumed
+			baseGas := uint64(24564) // baseGas is the gas consumed by middlewares
+			expGasConsumed := addUint64Saturating(tc.gasToConsume, baseGas)
+			s.Require().Equal(expGasConsumed, ctx.BlockGasMeter().GasConsumed())
+			// tx fee is always deducted
+			s.Require().Equal(int64(0), s.app.BankKeeper.GetBalance(ctx, addr1, feeCoin.Denom).Amount.Int64())
+			// sender's sequence is always increased
+			seq, err = s.app.AccountKeeper.GetSequence(ctx, addr1)
+			s.Require().NoError(err)
+			s.Require().Equal(uint64(1), seq)
+		})
+	}
+}
+
+func addUint64Saturating(a, b uint64) uint64 {
+	if math.MaxUint64-a < b {
+		return math.MaxUint64
+	}
+
+	return a + b
+}
diff --git a/x/auth/middleware/middleware.go b/x/auth/middleware/middleware.go
@@ -99,13 +99,23 @@ func NewDefaultTxHandler(options TxHandlerOptions) (tx.Handler, error) {
 		ConsumeTxSizeGasMiddleware(options.AccountKeeper),
 		// No gas should be consumed in any middleware above in a "post" handler part. See
 		// ComposeMiddlewares godoc for details.
+		// `DeductFeeMiddleware` and `IncrementSequenceMiddleware` should be put outside of `WithBranchedStore` middleware,
+		// so their storage writes are not discarded when tx fails.
 		DeductFeeMiddleware(options.AccountKeeper, options.BankKeeper, options.FeegrantKeeper),
 		TxPriorityMiddleware,
 		SetPubKeyMiddleware(options.AccountKeeper),
 		ValidateSigCountMiddleware(options.AccountKeeper),
 		SigGasConsumeMiddleware(options.AccountKeeper, sigGasConsumer),
 		SigVerificationMiddleware(options.AccountKeeper, options.SignModeHandler),
-		NewTipMiddleware(options.BankKeeper),
 		IncrementSequenceMiddleware(options.AccountKeeper),
+		// Creates a new MultiStore branch, discards downstream writes if the downstream returns error.
+		// These kinds of middlewares should be put under this:
+		// - Could return error after messages executed succesfully.
+		// - Storage writes should be discarded together when tx failed.
+		WithBranchedStore,
+		// Consume block gas. All middlewares whose gas consumption after their `next` handler
+		// should be accounted for, should go below this middleware.
+		ConsumeBlockGasMiddleware,
+		NewTipMiddleware(options.BankKeeper),
 	), nil
 }
diff --git a/x/auth/middleware/recovery.go b/x/auth/middleware/recovery.go
@@ -39,36 +39,13 @@ func (txh recoveryTxHandler) CheckTx(ctx context.Context, req tx.Request, checkR
 // DeliverTx implements tx.Handler.DeliverTx method.
 func (txh recoveryTxHandler) DeliverTx(ctx context.Context, req tx.Request) (res tx.Response, err error) {
 	sdkCtx := sdk.UnwrapSDKContext(ctx)
-	// only run the tx if there is block gas remaining
-	if sdkCtx.BlockGasMeter().IsOutOfGas() {
-		err = sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "no block gas left to run tx")
-		return
-	}
-
-	startingGas := sdkCtx.BlockGasMeter().GasConsumed()
-
 	// Panic recovery.
 	defer func() {
 		if r := recover(); r != nil {
 			err = handleRecovery(r, sdkCtx)
 		}
 	}()
 
-	// If BlockGasMeter() panics it will be caught by the above recover and will
-	// return an error - in any case BlockGasMeter will consume gas past the limit.
-	//
-	// NOTE: This must exist in a separate defer function for the above recovery
-	// to recover from this one.
-	defer func() {
-		sdkCtx.BlockGasMeter().ConsumeGas(
-			sdkCtx.GasMeter().GasConsumedToLimit(), "block gas meter",
-		)
-
-		if sdkCtx.BlockGasMeter().GasConsumed() < startingGas {
-			panic(sdk.ErrorGasOverflow{Descriptor: "tx gas summation"})
-		}
-	}()
-
 	return txh.next.DeliverTx(ctx, req)
 }
 

diff --git a/x/auth/middleware/run_msgs.go b/x/auth/middleware/run_msgs.go
@@ -2,11 +2,8 @@ package middleware
 
 import (
 	"context"
-	"fmt"
 	"strings"
 
-	"github.com/tendermint/tendermint/crypto/tmhash"
-
 	codectypes "github.com/cosmos/cosmos-sdk/codec/types"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
@@ -50,11 +47,6 @@ func (txh runMsgsTxHandler) SimulateTx(ctx context.Context, req tx.Request) (tx.
 // Handler does not exist for a given message route. Otherwise, a reference to a
 // Result is returned. The caller must not commit state if an error is returned.
 func (txh runMsgsTxHandler) runMsgs(sdkCtx sdk.Context, msgs []sdk.Msg, txBytes []byte) (tx.Response, error) {
-	// 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.
-	runMsgCtx, msCache := cacheTxContext(sdkCtx, txBytes)
-
 	// Attempt to execute all messages and only update state if all messages pass
 	// and we're in DeliverTx. Note, runMsgs will never return a reference to a
 	// Result if any single message fails or does not have a registered Handler.
@@ -72,7 +64,7 @@ func (txh runMsgsTxHandler) runMsgs(sdkCtx sdk.Context, msgs []sdk.Msg, txBytes
 
 		if handler := txh.msgServiceRouter.Handler(msg); handler != nil {
 			// ADR 031 request type routing
-			msgResult, err = handler(runMsgCtx, msg)
+			msgResult, err = handler(sdkCtx, msg)
 			eventMsgName = sdk.MsgTypeURL(msg)
 		} else if legacyMsg, ok := msg.(legacytx.LegacyMsg); ok {
 			// legacy sdk.Msg routing
@@ -116,31 +108,10 @@ func (txh runMsgsTxHandler) runMsgs(sdkCtx sdk.Context, msgs []sdk.Msg, txBytes
 		msgLogs = append(msgLogs, sdk.NewABCIMessageLog(uint32(i), msgResult.Log, msgEvents))
 	}
 
-	msCache.Write()
-
 	return tx.Response{
 		// GasInfo will be populated by the Gas middleware.
 		Log:          strings.TrimSpace(msgLogs.String()),
 		Events:       events.ToABCIEvents(),
 		MsgResponses: msgResponses,
 	}, nil
 }
-
-// cacheTxContext returns a new context based off of the provided context with
-// a branched multi-store.
-func cacheTxContext(sdkCtx sdk.Context, txBytes []byte) (sdk.Context, sdk.CacheMultiStore) {
-	ms := sdkCtx.MultiStore()
-	// TODO: https://github.com/cosmos/cosmos-sdk/issues/2824
-	msCache := ms.CacheMultiStore()
-	if msCache.TracingEnabled() {
-		msCache = msCache.SetTracingContext(
-			sdk.TraceContext(
-				map[string]interface{}{
-					"txHash": fmt.Sprintf("%X", tmhash.Sum(txBytes)),
-				},
-			),
-		).(sdk.CacheMultiStore)
-	}
-
-	return sdkCtx.WithMultiStore(msCache), msCache
-}