-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: revert tx when block gas limit exceeded (backport: #10770) #10814
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -586,11 +586,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) | ||
|
@@ -600,20 +595,25 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, re | |
gInfo = sdk.GasInfo{GasWanted: gasWanted, GasUsed: ctx.GasMeter().GasConsumed()} | ||
}() | ||
|
||
blockGasConsumed := false | ||
// consumeBlockGas makes sure block gas is consumed at most once. | ||
yihuang marked this conversation as resolved.
Show resolved
Hide resolved
|
||
consumeBlockGas := func() { | ||
if !blockGasConsumed { | ||
blockGasConsumed = true | ||
ctx.BlockGasMeter().ConsumeGas( | ||
ctx.GasMeter().GasConsumedToLimit(), "block gas meter", | ||
) | ||
} | ||
} | ||
|
||
// 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() { | ||
if mode == runTxModeDeliver { | ||
amaury1093 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
ctx.BlockGasMeter().ConsumeGas( | ||
ctx.GasMeter().GasConsumedToLimit(), "block gas meter", | ||
) | ||
|
||
if ctx.BlockGasMeter().GasConsumed() < startingGas { | ||
panic(sdk.ErrorGasOverflow{Descriptor: "tx gas summation"}) | ||
} | ||
consumeBlockGas() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we remove this defer func? AFAIK, there's no gas consumption after runMsgs. So if we put There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need defer function to consume block gas when panic happens I think. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, let's maybe keep it for now |
||
} | ||
}() | ||
|
||
|
@@ -677,6 +677,9 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, re | |
// Result if any single message fails or does not have a registered Handler. | ||
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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need to call it here? It's already called in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's the main purpose of this PR, call it here so if block gas limit exceeded, it'll panic and won't execute the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. makes sense. Thanks! |
||
|
||
msCache.Write() | ||
|
||
if len(anteEvents) > 0 { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,201 @@ | ||
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 { | ||
require.NotEqual(t, uint32(0), rsp.Code) | ||
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 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to check overflow since it's already handled in
ConsumeGas
.