From 358b09aa8b9088af8ebd00b0c4efdec33c4a5e74 Mon Sep 17 00:00:00 2001 From: "Masih H. Derkani" Date: Tue, 19 May 2026 15:13:41 +0100 Subject: [PATCH] Tidy app gas accounting loop and tests - Remove `nonzeroTxsCnt`: declared and incremented but never read. - Remove a stale "Skipping" inline comment that no longer describes the branch it sits next to. - Reduce allocations and optimise for fast path on decode failure. --- app/app.go | 98 ++++++--------- app/check_total_block_gas_test.go | 192 ++++++++++++++---------------- 2 files changed, 129 insertions(+), 161 deletions(-) diff --git a/app/app.go b/app/app.go index bad9997209..8befd0c6a8 100644 --- a/app/app.go +++ b/app/app.go @@ -2695,22 +2695,22 @@ func RegisterSwaggerAPI(rtr *mux.Router) { // checkTotalBlockGas checks that the block gas limit is not exceeded by our best estimate of // the total gas by the txs in the block. The gas of a tx is either the gas estimate if it's an EVM tx, // or the gas wanted if it's a Cosmos tx. typedTxs must align with proposal order (nil = decode failure). -func (app *App) checkTotalBlockGas(ctx sdk.Context, typedTxs []sdk.Tx) (result bool) { +func (app *App) checkTotalBlockGas(ctx sdk.Context, typedTxs []sdk.Tx) (_result bool) { defer func() { if r := recover(); r != nil { logger.Error("panic recovered in checkTotalBlockGas", "panic", r) - result = false // Reject proposal if panic occurs + _result = false } }() - totalGas, totalGasWanted := uint64(0), uint64(0) - nonzeroTxsCnt := 0 + var totalGas, totalGasWanted uint64 for _, decodedTx := range typedTxs { if decodedTx == nil { - // such tx will not be processed and thus won't consume gas. Skipping - continue + return false } + isEVM, evmErr := evmante.IsEVMMessage(decodedTx) + // MsgEVMTransaction cannot be gasless under IsTxGasless (only oracle vote / MsgAssociate). // Skip keeper-backed IsTxGasless for valid single-message EVM txs; still run it when the tx // is not EVM or EVM classification failed (e.g. multi-msg with an EVM message). @@ -2719,11 +2719,11 @@ func (app *App) checkTotalBlockGas(ctx sdk.Context, typedTxs []sdk.Tx) (result b isGasless, err := antedecorators.IsTxGasless(decodedTx, ctx, app.OracleKeeper, &app.EvmKeeper) if err != nil { if strings.Contains(err.Error(), "panic in IsTxGasless") { - // This is a unexpected panic, reject the entire proposal + // Unexpected panic: reject the entire proposal. logger.Error("malicious transaction detected in gasless check", "err", err) return false } - // Other business logic errors (like duplicate votes) - continue processing but tx is not gasless + // Business-logic errors (e.g. duplicate votes): keep going, tx is treated as non-gasless. logger.Info("transaction failed gasless check but not malicious", "err", err) continue } @@ -2731,11 +2731,14 @@ func (app *App) checkTotalBlockGas(ctx sdk.Context, typedTxs []sdk.Tx) (result b continue } } - // Check whether it's associate tx - gasWanted := uint64(0) + + // EVM classification failed (e.g. multi-msg containing an EVM message); such a tx won't be + // processed and so contributes no gas to the block. if evmErr != nil { continue } + + var gasWanted uint64 if isEVM { msg := evmtypes.MustGetEVMTransactionMessage(decodedTx) if msg.IsAssociateTx() { @@ -2746,43 +2749,34 @@ func (app *App) checkTotalBlockGas(ctx sdk.Context, typedTxs []sdk.Tx) (result b } else { feeTx, ok := decodedTx.(sdk.FeeTx) if !ok { - // such tx will not be processed and thus won't consume gas. Skipping + // Non-fee tx won't be processed and thus won't consume gas. Skipping. continue } - - // Check for overflow before adding gasWanted = feeTx.GetGas() } - if int64(gasWanted) < 0 || int64(totalGas) > math.MaxInt64-int64(gasWanted) { // nolint:gosec + // Overflow guards: gasWanted must fit in int64, and adding it to either accumulator + // must not wrap uint64. + if int64(gasWanted) < 0 || //nolint:gosec + totalGasWanted > math.MaxUint64-gasWanted || + totalGas > math.MaxUint64-gasWanted { return false } - if gasWanted > 0 { - nonzeroTxsCnt++ - } - totalGasWanted += gasWanted - // If the gas estimate is set and at least 21k (the minimum gas needed for an EVM tx) - // and less than or equal to the tx gas limit, use the gas estimate. Otherwise, use gasWanted. - useEstimate := false - if decodedTx.GetGasEstimate() >= MinGasEVMTx { - if decodedTx.GetGasEstimate() <= gasWanted { - useEstimate = true - } - } - if useEstimate { - totalGas += decodedTx.GetGasEstimate() + // Prefer the gas estimate when it's a valid EVM estimate (>= MinGasEVMTx) and not + // inflated above gasWanted; otherwise charge full gasWanted. + if est := decodedTx.GetGasEstimate(); est >= MinGasEVMTx && est <= gasWanted { + totalGas += est } else { totalGas += gasWanted } - if totalGasWanted > uint64(ctx.ConsensusParams().Block.MaxGasWanted) { //nolint:gosec - return false - } - - if totalGas > uint64(ctx.ConsensusParams().Block.MaxGas) { //nolint:gosec + blockParams := ctx.ConsensusParams().Block + maxGasWanted := uint64(blockParams.MaxGasWanted) //nolint:gosec + maxGas := uint64(blockParams.MaxGas) //nolint:gosec + if totalGasWanted > maxGasWanted || totalGas > maxGas { return false } } @@ -2790,50 +2784,36 @@ func (app *App) checkTotalBlockGas(ctx sdk.Context, typedTxs []sdk.Tx) (result b return true } +// isExpectedGaslessMetricsError reports whether err is the well-known oracle +// duplicate-vote error that we deliberately tolerate when collecting +// gasless-tx metrics. errors.Is handles properly-wrapped chains; the substring +// fallback covers chains that lost sentinel identity via %s/%v wrapping. func isExpectedGaslessMetricsError(err error) bool { if err == nil { return false } - if errors.Is(err, oracletypes.ErrAggregateVoteExist) { return true } - - // Some wrapped error chains can lose sentinel identity while preserving - // the canonical oracle error text. return strings.Contains(err.Error(), oracletypes.ErrAggregateVoteExist.Error()) } -// couldBeGaslessTransaction performs a fast heuristic check to identify potentially -// gasless transactions, avoiding expensive keeper queries for performance. -// -// Returns true if the transaction COULD be gasless (needs expensive check). -// Returns false only if DEFINITELY not gasless. -// False negatives are unacceptable as they cause incorrect gas metrics. +// couldBeGaslessTransaction is a fast heuristic that returns true when tx +// might be gasless and a full IsTxGasless keeper check is therefore worth +// running. It MUST be a conservative over-approximation: returning false for +// a tx that is actually gasless would cause its gas to be counted against +// the block limit, producing incorrect gas accounting (and in the worst case +// rejecting an otherwise-valid block). func (app *App) couldBeGaslessTransaction(tx sdk.Tx) bool { if tx == nil { return false } - - msgs := tx.GetMsgs() - if len(msgs) == 0 { - // Empty transactions are definitely not gasless - return false - } - - // Check if ANY message could potentially be gasless - for _, msg := range msgs { + for _, msg := range tx.GetMsgs() { switch msg.(type) { - case *evmtypes.MsgAssociate: - // Associate txs can be gasless, so we need to check - return true - case *oracletypes.MsgAggregateExchangeRateVote: - // Oracle vote txs can be gasless, so we need to check + case *evmtypes.MsgAssociate, *oracletypes.MsgAggregateExchangeRateVote: return true } } - - // If none of the messages are known gasless types, it's definitely not gasless return false } diff --git a/app/check_total_block_gas_test.go b/app/check_total_block_gas_test.go index c511163767..5cef31435e 100644 --- a/app/check_total_block_gas_test.go +++ b/app/check_total_block_gas_test.go @@ -21,62 +21,87 @@ import ( "github.com/stretchr/testify/require" ) -func testCheckTotalBlockGasCtx(t *testing.T, a *App) sdk.Context { - t.Helper() - cp := &tmproto.ConsensusParams{ +// blockGasParams builds a ConsensusParams with only the block-gas fields populated. +func blockGasParams(maxGas, maxGasWanted int64) *tmproto.ConsensusParams { + return &tmproto.ConsensusParams{ Block: &tmproto.BlockParams{ - MaxGas: 1_000_000, - MaxGasWanted: 1_000_000, + MaxGas: maxGas, + MaxGasWanted: maxGasWanted, }, } +} + +// newBlockGasCtx returns a fresh App context with the given block-gas caps. +func newBlockGasCtx(t *testing.T, a *App, maxGas, maxGasWanted int64) sdk.Context { + t.Helper() return a.NewContext(false, tmproto.Header{ Height: 2, ChainID: "sei-test", Time: time.Now().UTC(), - }).WithConsensusParams(cp) + }).WithConsensusParams(blockGasParams(maxGas, maxGasWanted)) +} + +// encodeDecodeTx round-trips a tx through the App's encoder/decoder, matching what +// proposal processing sees. +func encodeDecodeTx(t *testing.T, a *App, tx sdk.Tx) sdk.Tx { + t.Helper() + raw, err := a.GetTxConfig().TxEncoder()(tx) + require.NoError(t, err) + decoded, err := a.GetTxConfig().TxDecoder()(raw) + require.NoError(t, err) + return decoded +} + +// buildCosmosTx wraps a single Cosmos msg in a tx with the given gas limit and round-trips it. +func buildCosmosTx(t *testing.T, a *App, msg sdk.Msg, gasLimit uint64) sdk.Tx { + t.Helper() + txb := a.GetTxConfig().NewTxBuilder() + require.NoError(t, txb.SetMsgs(msg)) + txb.SetGasLimit(gasLimit) + return encodeDecodeTx(t, a, txb.GetTx()) } -func buildSignedLegacyEVMTx(t *testing.T, a *App, nonce uint64, gasWanted, gasEstimate uint64) sdk.Tx { +// buildSignedLegacyEVMTx builds a signed legacy EVM tx wrapped as MsgEVMTransaction. +// Pass gasEstimate=0 to leave the estimate unset. +func buildSignedLegacyEVMTx(t *testing.T, a *App, nonce, gasWanted, gasEstimate uint64) sdk.Tx { t.Helper() privKey := secp256k1.GenPrivKey() key, err := crypto.HexToECDSA(hex.EncodeToString(privKey.Bytes())) require.NoError(t, err) + to := common.HexToAddress("0x1111111111111111111111111111111111111111") - txData := ethtypes.LegacyTx{ + ethCfg := evmtypes.DefaultChainConfig().EthereumConfig(big.NewInt(config.DefaultChainID)) + signer := ethtypes.MakeSigner(ethCfg, big.NewInt(1), 123) + + signedTx, err := ethtypes.SignTx(ethtypes.NewTx(ðtypes.LegacyTx{ Nonce: nonce, GasPrice: big.NewInt(10), Gas: gasWanted, To: &to, Value: big.NewInt(1), - } - chainCfg := evmtypes.DefaultChainConfig() - ethCfg := chainCfg.EthereumConfig(big.NewInt(config.DefaultChainID)) - signer := ethtypes.MakeSigner(ethCfg, big.NewInt(1), uint64(123)) - signedTx, err := ethtypes.SignTx(ethtypes.NewTx(&txData), signer, key) + }), signer, key) require.NoError(t, err) + ethtxdata, err := ethtx.NewTxDataFromTx(signedTx) require.NoError(t, err) msg, err := evmtypes.NewMsgEVMTransaction(ethtxdata) require.NoError(t, err) + txb := a.GetTxConfig().NewTxBuilder() require.NoError(t, txb.SetMsgs(msg)) txb.SetGasEstimate(gasEstimate) - tx, err := a.GetTxConfig().TxEncoder()(txb.GetTx()) - require.NoError(t, err) - decoded, err := a.GetTxConfig().TxDecoder()(tx) - require.NoError(t, err) - return decoded + return encodeDecodeTx(t, a, txb.GetTx()) } // TestCheckTotalBlockGas_MultipleEVMUnderLimit covers the fast path where valid // single-message EVM txs skip IsTxGasless but still accumulate gas toward the block cap. func TestCheckTotalBlockGas_MultipleEVMUnderLimit(t *testing.T) { a := Setup(t, false, false, false) - ctx := testCheckTotalBlockGasCtx(t, a) + ctx := newBlockGasCtx(t, a, 1_000_000, 1_000_000) - var txs []sdk.Tx - for i := 0; i < 3; i++ { - txs = append(txs, buildSignedLegacyEVMTx(t, a, uint64(i+1), 21000, 0)) + txs := make([]sdk.Tx, 0, 3) + for i := uint64(1); i <= 3; i++ { + txs = append(txs, buildSignedLegacyEVMTx(t, a, i, 21000, 0)) } require.True(t, a.checkTotalBlockGas(ctx, txs)) } @@ -85,17 +110,7 @@ func TestCheckTotalBlockGas_MultipleEVMUnderLimit(t *testing.T) { // when cumulative gas wanted exceeds MaxGas (no gas estimate path). func TestCheckTotalBlockGas_MultipleEVMExceedsMaxGas(t *testing.T) { a := Setup(t, false, false, false) - cp := &tmproto.ConsensusParams{ - Block: &tmproto.BlockParams{ - MaxGas: 50_000, - MaxGasWanted: 1_000_000, - }, - } - ctx := a.NewContext(false, tmproto.Header{ - Height: 2, - ChainID: "sei-test", - Time: time.Now().UTC(), - }).WithConsensusParams(cp) + ctx := newBlockGasCtx(t, a, 50_000, 1_000_000) txs := []sdk.Tx{ buildSignedLegacyEVMTx(t, a, 1, 30_000, 0), @@ -104,105 +119,78 @@ func TestCheckTotalBlockGas_MultipleEVMExceedsMaxGas(t *testing.T) { require.False(t, a.checkTotalBlockGas(ctx, txs)) } +// TestCheckTotalBlockGas_GasEstimatePreferredOverGasWanted exercises the branch where a +// valid gas estimate (>= MinGasEVMTx and <= gasWanted) is charged instead of full gasWanted. +// With MaxGas=80_000, three txs at gasWanted=100_000 would clearly exceed if counted by +// gasWanted (300_000) but fit comfortably when counted by estimate (63_000). +func TestCheckTotalBlockGas_GasEstimatePreferredOverGasWanted(t *testing.T) { + a := Setup(t, false, false, false) + ctx := newBlockGasCtx(t, a, 80_000, 1_000_000) + + txs := make([]sdk.Tx, 0, 3) + for i := uint64(1); i <= 3; i++ { + txs = append(txs, buildSignedLegacyEVMTx(t, a, i, 100_000, 21_000)) + } + require.True(t, a.checkTotalBlockGas(ctx, txs)) +} + // TestCheckTotalBlockGas_CosmosBankSendWithoutGaslessTypes exercises txs that are not // EVM and not oracle/associate: couldBeGaslessTransaction is false so IsTxGasless is skipped. func TestCheckTotalBlockGas_CosmosBankSendWithoutGaslessTypes(t *testing.T) { a := Setup(t, false, false, false) - ctx := testCheckTotalBlockGasCtx(t, a) + ctx := newBlockGasCtx(t, a, 1_000_000, 1_000_000) - fromAddr := sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()) - toAddr := sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()) - txb := a.GetTxConfig().NewTxBuilder() - require.NoError(t, txb.SetMsgs(&banktypes.MsgSend{ - FromAddress: fromAddr.String(), - ToAddress: toAddr.String(), + send := &banktypes.MsgSend{ + FromAddress: sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String(), + ToAddress: sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String(), Amount: sdk.NewCoins(sdk.NewInt64Coin("usei", 1)), - })) - txb.SetGasLimit(80_000) - raw, err := a.GetTxConfig().TxEncoder()(txb.GetTx()) - require.NoError(t, err) - decoded, err := a.GetTxConfig().TxDecoder()(raw) - require.NoError(t, err) - - require.True(t, a.checkTotalBlockGas(ctx, []sdk.Tx{decoded})) + } + require.True(t, a.checkTotalBlockGas(ctx, []sdk.Tx{buildCosmosTx(t, a, send, 80_000)})) } -// TestCheckTotalBlockGas_NilDecodedTxSkipped ensures nil entries (decode failures) are ignored. -func TestCheckTotalBlockGas_NilDecodedTxSkipped(t *testing.T) { +// TestCheckTotalBlockGas_NilDecodedTx exercises the nil-entry branch of the accounting loop. +func TestCheckTotalBlockGas_NilDecodedTx(t *testing.T) { a := Setup(t, false, false, false) - ctx := testCheckTotalBlockGasCtx(t, a) + ctx := newBlockGasCtx(t, a, 1_000_000, 1_000_000) evmTx := buildSignedLegacyEVMTx(t, a, 1, 21000, 0) - require.True(t, a.checkTotalBlockGas(ctx, []sdk.Tx{nil, evmTx})) + require.False(t, a.checkTotalBlockGas(ctx, []sdk.Tx{nil, evmTx})) } // TestCheckTotalBlockGas_AssociateTxIsGasless verifies that a MsgAssociate from an -// unassociated address is excluded from block gas accounting (treated as gasless). +// unassociated address is excluded from block gas accounting. MaxGas is set below the +// tx's gas limit so the test fails iff the tx is incorrectly counted. func TestCheckTotalBlockGas_AssociateTxIsGasless(t *testing.T) { a := Setup(t, false, false, false) - cp := &tmproto.ConsensusParams{ - Block: &tmproto.BlockParams{ - MaxGas: 100, - MaxGasWanted: 1_000_000, - }, - } - ctx := a.NewContext(false, tmproto.Header{ - Height: 2, - ChainID: "sei-test", - Time: time.Now().UTC(), - }).WithConsensusParams(cp) + ctx := newBlockGasCtx(t, a, 100, 1_000_000) - sender := sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()) - txb := a.GetTxConfig().NewTxBuilder() - require.NoError(t, txb.SetMsgs(&evmtypes.MsgAssociate{ - Sender: sender.String(), + msg := &evmtypes.MsgAssociate{ + Sender: sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String(), CustomMessage: "test", - })) - txb.SetGasLimit(1_000) // exceeds MaxGas=100 if counted - raw, err := a.GetTxConfig().TxEncoder()(txb.GetTx()) - require.NoError(t, err) - decoded, err := a.GetTxConfig().TxDecoder()(raw) - require.NoError(t, err) - - // MsgAssociate for an unassociated address is gasless: not counted toward block gas - require.True(t, a.checkTotalBlockGas(ctx, []sdk.Tx{decoded})) + } + tx := buildCosmosTx(t, a, msg, 1_000) // 1_000 > MaxGas=100 if counted + require.True(t, a.checkTotalBlockGas(ctx, []sdk.Tx{tx})) } // TestCheckTotalBlockGas_OracleVoteIsGasless verifies that a valid oracle aggregate vote // from a bonded validator with no prior vote is excluded from block gas accounting. func TestCheckTotalBlockGas_OracleVoteIsGasless(t *testing.T) { - tm := time.Now().UTC() valPub := secp256k1.GenPrivKey().PubKey() - testWrapper := NewTestWrapper(t, tm, valPub, false) + tw := NewTestWrapper(t, time.Now().UTC(), valPub, false) - // Promote validator to Bonded so ValidateFeeder succeeds + // Promote the validator to Bonded so ValidateFeeder succeeds. valAddr := sdk.ValAddress(valPub.Address()) - val, found := testWrapper.App.StakingKeeper.GetValidator(testWrapper.Ctx, valAddr) + val, found := tw.App.StakingKeeper.GetValidator(tw.Ctx, valAddr) require.True(t, found) - val = val.UpdateStatus(stakingtypes.Bonded) - testWrapper.App.StakingKeeper.SetValidator(testWrapper.Ctx, val) + tw.App.StakingKeeper.SetValidator(tw.Ctx, val.UpdateStatus(stakingtypes.Bonded)) - // Self-feeder oracle vote; no prior aggregate vote exists in fresh state - oracleMsg := &oracletypes.MsgAggregateExchangeRateVote{ + // Self-feeder oracle vote; no prior aggregate vote exists in fresh state. + vote := &oracletypes.MsgAggregateExchangeRateVote{ ExchangeRates: "1.2uatom", Feeder: sdk.AccAddress(valAddr).String(), Validator: valAddr.String(), } - txb := testWrapper.App.GetTxConfig().NewTxBuilder() - require.NoError(t, txb.SetMsgs(oracleMsg)) - txb.SetGasLimit(1_000) // exceeds MaxGas=100 if counted - raw, err := testWrapper.App.GetTxConfig().TxEncoder()(txb.GetTx()) - require.NoError(t, err) - decoded, err := testWrapper.App.GetTxConfig().TxDecoder()(raw) - require.NoError(t, err) - - cp := &tmproto.ConsensusParams{ - Block: &tmproto.BlockParams{ - MaxGas: 100, - MaxGasWanted: 1_000_000, - }, - } - ctx := testWrapper.Ctx.WithConsensusParams(cp) + tx := buildCosmosTx(t, tw.App, vote, 1_000) // 1_000 > MaxGas=100 if counted - // Oracle vote is gasless: not counted toward block gas - require.True(t, testWrapper.App.checkTotalBlockGas(ctx, []sdk.Tx{decoded})) + ctx := tw.Ctx.WithConsensusParams(blockGasParams(100, 1_000_000)) + require.True(t, tw.App.checkTotalBlockGas(ctx, []sdk.Tx{tx})) }