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)
This commit is contained in:
parent
9aef070625
commit
17279fdf30
|
@ -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
|
||||
|
||||
|
|
|
@ -137,6 +137,7 @@ func testTxHandler(options middleware.TxHandlerOptions, customTxHandlerMiddlewar
|
|||
middleware.RecoveryTxMiddleware,
|
||||
middleware.NewIndexEventsTxMiddleware(options.IndexEvents),
|
||||
middleware.ValidateBasicMiddleware,
|
||||
middleware.ConsumeBlockGasMiddleware,
|
||||
CustomTxHandlerMiddleware(customTxHandlerMiddleware),
|
||||
)
|
||||
}
|
||||
|
|
|
@ -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)
|
||||
}
|
|
@ -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
|
||||
}
|
|
@ -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
|
||||
}
|
|
@ -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
|
||||
}
|
||||
|
|
|
@ -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)
|
||||
}
|
||||
|
||||
|
|
|
@ -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
|
||||
}
|
||||
|
|
Loading…
Reference in New Issue