From 17279fdf300a125ce62e58ca8594795e13143f29 Mon Sep 17 00:00:00 2001 From: yihuang Date: Tue, 11 Jan 2022 18:21:01 +0800 Subject: [PATCH] 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) --- CHANGELOG.md | 1 + baseapp/baseapp_test.go | 1 + x/auth/middleware/block_gas.go | 53 ++++++++++ x/auth/middleware/branch_store.go | 70 ++++++++++++++ x/auth/middleware/branch_store_test.go | 129 +++++++++++++++++++++++++ x/auth/middleware/middleware.go | 12 ++- x/auth/middleware/recovery.go | 23 ----- x/auth/middleware/run_msgs.go | 31 +----- 8 files changed, 266 insertions(+), 54 deletions(-) create mode 100644 x/auth/middleware/block_gas.go create mode 100644 x/auth/middleware/branch_store.go create mode 100644 x/auth/middleware/branch_store_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 02eafc1703b2..27a12dfd9ab2 100644 --- 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 index 85e9933e9320..bb3d614ece08 100644 --- 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 new file mode 100644 index 000000000000..bfd67f92ecbe --- /dev/null +++ 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 new file mode 100644 index 000000000000..236d288c122b --- /dev/null +++ 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 new file mode 100644 index 000000000000..ea675492c1b5 --- /dev/null +++ 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 index 80a70765dc85..87b1216363fb 100644 --- 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 index 1d9cd4db7cdd..563389d85fc5 100644 --- a/x/auth/middleware/recovery.go +++ b/x/auth/middleware/recovery.go @@ -39,14 +39,6 @@ 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 { @@ -54,21 +46,6 @@ func (txh recoveryTxHandler) DeliverTx(ctx context.Context, req tx.Request) (res } }() - // 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 index c90d222a907b..d333e5918995 100644 --- 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,8 +108,6 @@ 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()), @@ -125,22 +115,3 @@ func (txh runMsgsTxHandler) runMsgs(sdkCtx sdk.Context, msgs []sdk.Msg, txBytes 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 -}