diff --git a/CHANGELOG.md b/CHANGELOG.md index afc58d5440df..d95c57d12546 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (store) [#10218](https://github.com/cosmos/cosmos-sdk/pull/10218) Charge gas even when there are no entries while seeking. * (store) [#10247](https://github.com/cosmos/cosmos-sdk/pull/10247) Charge gas for the key length in gas meter. * (x/gov) [\#10740](https://github.com/cosmos/cosmos-sdk/pull/10740) Increase maximum proposal description size from 5k characters to 10k characters. +* [#10814](https://github.com/cosmos/cosmos-sdk/pull/10814) revert tx when block gas limit exceeded. ### API Breaking Changes diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index 8f6a18be5c03..d9981ec5f8ad 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -815,11 +815,6 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, re return gInfo, nil, nil, sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "no block gas left to run tx") } - var startingGas uint64 - if mode == runTxModeDeliver { - startingGas = ctx.BlockGasMeter().GasConsumed() - } - defer func() { if r := recover(); r != nil { recoveryMW := newOutOfGasRecoveryMiddleware(gasWanted, ctx, app.runTxRecoveryMiddleware) @@ -831,10 +826,8 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, re }() blockGasConsumed := false - - // consumeBlockGas makes sure block gas is consumed at most once. It must - // happen after tx processing, and must be executed even if tx processing - // fails. Hence, it's execution is deferred. + // consumeBlockGas makes sure block gas is consumed at most once. It must happen after + // tx processing, and must be execute even if tx processing fails. Hence we use trick with `defer` consumeBlockGas := func() { if !blockGasConsumed { blockGasConsumed = true @@ -847,10 +840,9 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, re // 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: consumeBlockGas must exist in a separate defer function from the - // general deferred recovery function to recover from consumeBlockGas as it'll - // be executed first (deferred statements are executed as stack). - if mode == execModeFinalize { + // NOTE: This must exist in a separate defer function for the above recovery + // to recover from this one. + if mode == runTxModeDeliver { defer consumeBlockGas() } @@ -915,19 +907,12 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, re // 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. - reflectMsgs, err := tx.GetReflectMessages() - if err == nil { - result, err = app.runMsgs(runMsgCtx, msgs, reflectMsgs, mode) - } + result, err = app.runMsgs(runMsgCtx, msgs, mode) + if err == nil && mode == runTxModeDeliver { + // When block gas exceeds, it'll panic and won't commit the cached store. + consumeBlockGas() - if mode == execModeSimulate { - for _, msg := range msgs { - nestedErr := app.simulateNestedMessages(ctx, msg) - if nestedErr != nil { - return gInfo, nil, anteEvents, nestedErr - } - } - } + msCache.Write() if len(anteEvents) > 0 { // append the events in the order of occurrence diff --git a/baseapp/block_gas_test.go b/baseapp/block_gas_test.go new file mode 100644 index 000000000000..9a1be3a5f48f --- /dev/null +++ b/baseapp/block_gas_test.go @@ -0,0 +1,200 @@ +package baseapp_test + +import ( + "encoding/json" + "fmt" + "math" + "testing" + + "github.com/stretchr/testify/require" + abci "github.com/tendermint/tendermint/abci/types" + "github.com/tendermint/tendermint/libs/log" + tmproto "github.com/tendermint/tendermint/proto/tendermint/types" + dbm "github.com/tendermint/tm-db" + + "github.com/cosmos/cosmos-sdk/baseapp" + "github.com/cosmos/cosmos-sdk/client" + "github.com/cosmos/cosmos-sdk/client/tx" + 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" + txtypes "github.com/cosmos/cosmos-sdk/types/tx" + "github.com/cosmos/cosmos-sdk/types/tx/signing" + xauthsigning "github.com/cosmos/cosmos-sdk/x/auth/signing" + banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" + minttypes "github.com/cosmos/cosmos-sdk/x/mint/types" +) + +var blockMaxGas = uint64(simapp.DefaultConsensusParams.Block.MaxGas) + +func TestBaseApp_BlockGas(t *testing.T) { + 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 MaxGasWanted", txtypes.MaxGasWanted, false, true}, + {"consume block gas when paniced", 10, true, true}, + } + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + var app *simapp.SimApp + routerOpt := func(bapp *baseapp.BaseApp) { + route := (&testdata.TestMsg{}).Route() + bapp.Router().AddRoute(sdk.NewRoute(route, func(ctx sdk.Context, msg sdk.Msg) (*sdk.Result, error) { + _, ok := msg.(*testdata.TestMsg) + if !ok { + return &sdk.Result{}, fmt.Errorf("Wrong Msg type, expected %T, got %T", (*testdata.TestMsg)(nil), msg) + } + ctx.KVStore(app.GetKey(banktypes.ModuleName)).Set([]byte("ok"), []byte("ok")) + ctx.GasMeter().ConsumeGas(tc.gasToConsume, "TestMsg") + if tc.panicTx { + panic("panic in tx execution") + } + return &sdk.Result{}, nil + })) + } + encCfg := simapp.MakeTestEncodingConfig() + encCfg.Amino.RegisterConcrete(&testdata.TestMsg{}, "testdata.TestMsg", nil) + encCfg.InterfaceRegistry.RegisterImplementations((*sdk.Msg)(nil), + &testdata.TestMsg{}, + ) + app = simapp.NewSimApp(log.NewNopLogger(), dbm.NewMemDB(), nil, true, map[int64]bool{}, "", 0, encCfg, simapp.EmptyAppOptions{}, routerOpt) + genState := simapp.NewDefaultGenesisState(encCfg.Marshaler) + stateBytes, err := json.MarshalIndent(genState, "", " ") + require.NoError(t, err) + app.InitChain(abci.RequestInitChain{ + Validators: []abci.ValidatorUpdate{}, + ConsensusParams: simapp.DefaultConsensusParams, + AppStateBytes: stateBytes, + }) + + ctx := app.NewContext(false, tmproto.Header{}) + + // tx fee + feeCoin := sdk.NewCoin("atom", sdk.NewInt(150)) + feeAmount := sdk.NewCoins(feeCoin) + + // test account and fund + priv1, _, addr1 := testdata.KeyTestPubAddr() + err = app.BankKeeper.MintCoins(ctx, minttypes.ModuleName, feeAmount) + require.NoError(t, err) + err = app.BankKeeper.SendCoinsFromModuleToAccount(ctx, minttypes.ModuleName, addr1, feeAmount) + require.NoError(t, err) + require.Equal(t, feeCoin.Amount, app.BankKeeper.GetBalance(ctx, addr1, feeCoin.Denom).Amount) + seq, _ := app.AccountKeeper.GetSequence(ctx, addr1) + require.Equal(t, uint64(0), seq) + + // msg and signatures + msg := testdata.NewTestMsg(addr1) + + txBuilder := encCfg.TxConfig.NewTxBuilder() + require.NoError(t, txBuilder.SetMsgs(msg)) + txBuilder.SetFeeAmount(feeAmount) + txBuilder.SetGasLimit(txtypes.MaxGasWanted) // tx validation checks that gasLimit can't be bigger than this + + privs, accNums, accSeqs := []cryptotypes.PrivKey{priv1}, []uint64{6}, []uint64{0} + _, txBytes, err := createTestTx(encCfg.TxConfig, txBuilder, privs, accNums, accSeqs, ctx.ChainID()) + require.NoError(t, err) + + app.BeginBlock(abci.RequestBeginBlock{Header: tmproto.Header{Height: 1}}) + rsp := app.DeliverTx(abci.RequestDeliverTx{Tx: txBytes}) + + // check result + ctx = app.GetContextForDeliverTx(txBytes) + okValue := ctx.KVStore(app.GetKey(banktypes.ModuleName)).Get([]byte("ok")) + + if tc.expErr { + if tc.panicTx { + require.Equal(t, sdkerrors.ErrPanic.ABCICode(), rsp.Code) + } else { + require.Equal(t, sdkerrors.ErrOutOfGas.ABCICode(), rsp.Code) + } + require.Empty(t, okValue) + } else { + require.Equal(t, uint32(0), rsp.Code) + require.Equal(t, []byte("ok"), okValue) + } + // check block gas is always consumed + baseGas := uint64(59142) // baseGas is the gas consumed before tx msg + expGasConsumed := addUint64Saturating(tc.gasToConsume, baseGas) + if expGasConsumed > txtypes.MaxGasWanted { + // capped by gasLimit + expGasConsumed = txtypes.MaxGasWanted + } + require.Equal(t, expGasConsumed, ctx.BlockGasMeter().GasConsumed()) + // tx fee is always deducted + require.Equal(t, int64(0), app.BankKeeper.GetBalance(ctx, addr1, feeCoin.Denom).Amount.Int64()) + // sender's sequence is always increased + seq, err = app.AccountKeeper.GetSequence(ctx, addr1) + require.NoError(t, err) + require.Equal(t, uint64(1), seq) + }) + } +} + +func createTestTx(txConfig client.TxConfig, txBuilder client.TxBuilder, privs []cryptotypes.PrivKey, accNums []uint64, accSeqs []uint64, chainID string) (xauthsigning.Tx, []byte, error) { + // First round: we gather all the signer infos. We use the "set empty + // signature" hack to do that. + var sigsV2 []signing.SignatureV2 + for i, priv := range privs { + sigV2 := signing.SignatureV2{ + PubKey: priv.PubKey(), + Data: &signing.SingleSignatureData{ + SignMode: txConfig.SignModeHandler().DefaultMode(), + Signature: nil, + }, + Sequence: accSeqs[i], + } + + sigsV2 = append(sigsV2, sigV2) + } + err := txBuilder.SetSignatures(sigsV2...) + if err != nil { + return nil, nil, err + } + + // Second round: all signer infos are set, so each signer can sign. + sigsV2 = []signing.SignatureV2{} + for i, priv := range privs { + signerData := xauthsigning.SignerData{ + ChainID: chainID, + AccountNumber: accNums[i], + Sequence: accSeqs[i], + } + sigV2, err := tx.SignWithPrivKey( + txConfig.SignModeHandler().DefaultMode(), signerData, + txBuilder, priv, txConfig, accSeqs[i]) + if err != nil { + return nil, nil, err + } + + sigsV2 = append(sigsV2, sigV2) + } + err = txBuilder.SetSignatures(sigsV2...) + if err != nil { + return nil, nil, err + } + + txBytes, err := txConfig.TxEncoder()(txBuilder.GetTx()) + if err != nil { + return nil, nil, err + } + + return txBuilder.GetTx(), txBytes, nil +} + +func addUint64Saturating(a, b uint64) uint64 { + if math.MaxUint64-a < b { + return math.MaxUint64 + } + + return a + b +} diff --git a/baseapp/test_helpers.go b/baseapp/test_helpers.go index 879a2d34f552..36ce59708be3 100644 --- a/baseapp/test_helpers.go +++ b/baseapp/test_helpers.go @@ -70,3 +70,7 @@ func (app *BaseApp) GetContextForFinalizeBlock(txBytes []byte) sdk.Context { func (app *BaseApp) GetContextForCheckTx(txBytes []byte) sdk.Context { return app.getContextForTx(execModeCheck, txBytes) } + +func (app *BaseApp) GetContextForDeliverTx(txBytes []byte) sdk.Context { + return app.getContextForTx(runTxModeDeliver, txBytes) +}