diff --git a/app/app.go b/app/app.go index 3225851286..24175130c6 100644 --- a/app/app.go +++ b/app/app.go @@ -17,6 +17,7 @@ import ( "runtime/debug" "strings" "sync" + "sync/atomic" "time" "github.com/ethereum/go-ethereum/common" @@ -2508,6 +2509,20 @@ func RegisterSwaggerAPI(rtr *mux.Router) { rtr.PathPrefix("/swagger/").Handler(http.StripPrefix("/swagger/", staticServer)) } +// txGasResult holds the per-tx gas classification outcome for checkTotalBlockGas. +type txGasResult struct { + gasWanted uint64 + gasContrib uint64 // effective gas: estimate when valid (>= MinGasEVMTx and <= gasWanted), else gasWanted + skip bool // tx contributes no gas (nil, gasless, associate, decode error) + malicious bool // panic inside IsTxGasless — reject the entire proposal +} + +// txGasClassifyJob is one unit of work for the checkTotalBlockGas worker pool. +type txGasClassifyJob struct { + idx int + tx sdk.Tx +} + // 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). @@ -2515,97 +2530,152 @@ func (app *App) checkTotalBlockGas(ctx sdk.Context, typedTxs []sdk.Tx) (result b 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 - for _, decodedTx := range typedTxs { - if decodedTx == nil { - // such tx will not be processed and thus won't consume gas. Skipping + results := make([]txGasResult, len(typedTxs)) + + // ctx (CacheMultiStore) is not goroutine-safe; IsTxGasless is the only caller that + // reads from it. EVM txs skip that path entirely, so ctxMu is uncontested on + // typical EVM-heavy blocks. + var ctxMu sync.Mutex + + const maxConcurrent = 32 + jobs := make([]txGasClassifyJob, 0, len(typedTxs)) + for i, tx := range typedTxs { + if tx == nil { + results[i].skip = true continue } - 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). - skipGaslessCheck := evmErr == nil && isEVM - if !skipGaslessCheck && app.couldBeGaslessTransaction(decodedTx) { - 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 - 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 - logger.Info("transaction failed gasless check but not malicious", "err", err) - continue - } - if isGasless { - continue - } + jobs = append(jobs, txGasClassifyJob{idx: i, tx: tx}) + } + if len(jobs) > 0 { + var maliciousSeen atomic.Bool + nWorkers := maxConcurrent + if nWorkers > len(jobs) { + nWorkers = len(jobs) } - // Check whether it's associate tx - gasWanted := uint64(0) - if evmErr != nil { - continue + jobCh := make(chan txGasClassifyJob, len(jobs)) + for _, job := range jobs { + jobCh <- job } - if isEVM { - msg := evmtypes.MustGetEVMTransactionMessage(decodedTx) - if msg.IsAssociateTx() { - continue - } - etx, _ := msg.AsTransaction() - gasWanted = etx.Gas() - } else { - feeTx, ok := decodedTx.(sdk.FeeTx) - if !ok { - // such tx will not be processed and thus won't consume gas. Skipping - continue - } - - // Check for overflow before adding - gasWanted = feeTx.GetGas() + close(jobCh) + var wg sync.WaitGroup + wg.Add(nWorkers) + for range nWorkers { + go func() { + defer wg.Done() + for job := range jobCh { + if maliciousSeen.Load() { + break + } + r := app.classifyTxForGas(ctx, job.tx, &ctxMu) + results[job.idx] = r + if r.malicious { + maliciousSeen.Store(true) + } + } + }() } - - if int64(gasWanted) < 0 || int64(totalGas) > math.MaxInt64-int64(gasWanted) { // nolint:gosec + wg.Wait() + if maliciousSeen.Load() { return false } + } - if gasWanted > 0 { - nonzeroTxsCnt++ + // Serial phase: malicious check + overflow-safe accumulation + limit enforcement. + totalGas, totalGasWanted := uint64(0), uint64(0) + for _, r := range results { + if r.malicious { + return false } - - 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 r.skip { + continue } - if useEstimate { - totalGas += decodedTx.GetGasEstimate() - } else { - totalGas += gasWanted + if int64(r.gasWanted) < 0 || int64(totalGas) > math.MaxInt64-int64(r.gasWanted) { //nolint:gosec + return false } - + totalGasWanted += r.gasWanted + totalGas += r.gasContrib if totalGasWanted > uint64(ctx.ConsensusParams().Block.MaxGasWanted) { //nolint:gosec return false } - if totalGas > uint64(ctx.ConsensusParams().Block.MaxGas) { //nolint:gosec return false } } - return true } +// classifyTxForGas computes the gas contribution of a single tx for checkTotalBlockGas. +// Safe to call concurrently: each invocation reads a fixed ctx snapshot with no writes, so +// results are order-independent. ctxMu serializes IsTxGasless calls because sdk.Context +// (CacheMultiStore) is not goroutine-safe even for reads. +func (app *App) classifyTxForGas(ctx sdk.Context, tx sdk.Tx, ctxMu *sync.Mutex) (res txGasResult) { + // Worker goroutines are not covered by the recover in checkTotalBlockGas (different goroutine). + // Catch panics here so a malformed tx (e.g. MustGetEVMTransactionMessage on bad shape) rejects + // the proposal instead of crashing the process. + defer func() { + if r := recover(); r != nil { + logger.Error("panic recovered in classifyTxForGas", "panic", r) + res = txGasResult{malicious: true} + } + }() + isEVM, evmErr := evmante.IsEVMMessage(tx) + // 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). + skipGaslessCheck := evmErr == nil && isEVM + + if !skipGaslessCheck && app.couldBeGaslessTransaction(tx) { + ctxMu.Lock() + isGasless, err := func() (bool, error) { + defer ctxMu.Unlock() + return antedecorators.IsTxGasless(tx, ctx, app.OracleKeeper, &app.EvmKeeper) + }() + if err != nil { + if strings.Contains(err.Error(), "panic in IsTxGasless") { + logger.Error("malicious transaction detected in gasless check", "err", err) + return txGasResult{malicious: true} + } + logger.Info("transaction failed gasless check but not malicious", "err", err) + return txGasResult{skip: true} + } + if isGasless { + return txGasResult{skip: true} + } + } + + if evmErr != nil { + return txGasResult{skip: true} + } + + var gasWanted uint64 + if isEVM { + msg := evmtypes.MustGetEVMTransactionMessage(tx) + if msg.IsAssociateTx() { + return txGasResult{skip: true} + } + etx, _ := msg.AsTransaction() + gasWanted = etx.Gas() + } else { + feeTx, ok := tx.(sdk.FeeTx) + if !ok { + return txGasResult{skip: true} + } + gasWanted = feeTx.GetGas() + } + + // Use the gas estimate if it's set and within the tx gas limit; otherwise fall back to gasWanted. + gasContrib := gasWanted + if est := tx.GetGasEstimate(); est >= MinGasEVMTx && est <= gasWanted { + gasContrib = est + } + + return txGasResult{gasWanted: gasWanted, gasContrib: gasContrib} +} + func isExpectedGaslessMetricsError(err error) bool { if err == nil { return false diff --git a/app/check_total_block_gas_test.go b/app/check_total_block_gas_test.go index c511163767..c2a0f9fa20 100644 --- a/app/check_total_block_gas_test.go +++ b/app/check_total_block_gas_test.go @@ -167,6 +167,166 @@ func TestCheckTotalBlockGas_AssociateTxIsGasless(t *testing.T) { require.True(t, a.checkTotalBlockGas(ctx, []sdk.Tx{decoded})) } +// TestCheckTotalBlockGas_GasEstimateUsedWhenValid verifies that a gas estimate in the valid +// range [MinGasEVMTx, gasWanted] is used as gasContrib instead of gasWanted. +func TestCheckTotalBlockGas_GasEstimateUsedWhenValid(t *testing.T) { + a := Setup(t, false, false, false) + cp := &tmproto.ConsensusParams{ + Block: &tmproto.BlockParams{ + MaxGas: 25_000, // between estimate (22_000) and gasWanted (30_000) + MaxGasWanted: 1_000_000, + }, + } + ctx := a.NewContext(false, tmproto.Header{ + Height: 2, + ChainID: "sei-test", + Time: time.Now().UTC(), + }).WithConsensusParams(cp) + + // estimate=22_000 is valid (>= MinGasEVMTx=21_000 and <= gasWanted=30_000). + // With estimate: totalGas=22_000 <= MaxGas=25_000 → true. + // Without estimate: totalGas=30_000 > MaxGas=25_000 → false. + tx := buildSignedLegacyEVMTx(t, a, 1, 30_000, 22_000) + require.True(t, a.checkTotalBlockGas(ctx, []sdk.Tx{tx})) +} + +// TestCheckTotalBlockGas_GasEstimateBelowMinFallsBack verifies that an estimate below +// MinGasEVMTx is ignored and gasWanted is used instead. +func TestCheckTotalBlockGas_GasEstimateBelowMinFallsBack(t *testing.T) { + a := Setup(t, false, false, false) + cp := &tmproto.ConsensusParams{ + Block: &tmproto.BlockParams{ + MaxGas: 25_000, + MaxGasWanted: 1_000_000, + }, + } + ctx := a.NewContext(false, tmproto.Header{ + Height: 2, + ChainID: "sei-test", + Time: time.Now().UTC(), + }).WithConsensusParams(cp) + + // estimate=1_000 < MinGasEVMTx=21_000 → invalid, falls back to gasWanted=30_000. + // totalGas=30_000 > MaxGas=25_000 → false. + tx := buildSignedLegacyEVMTx(t, a, 1, 30_000, 1_000) + require.False(t, a.checkTotalBlockGas(ctx, []sdk.Tx{tx})) +} + +// TestCheckTotalBlockGas_GasEstimateAboveGasWantedFallsBack verifies that an estimate +// exceeding gasWanted is ignored and gasWanted is used instead. +func TestCheckTotalBlockGas_GasEstimateAboveGasWantedFallsBack(t *testing.T) { + a := Setup(t, false, false, false) + cp := &tmproto.ConsensusParams{ + Block: &tmproto.BlockParams{ + MaxGas: 25_000, + MaxGasWanted: 1_000_000, + }, + } + ctx := a.NewContext(false, tmproto.Header{ + Height: 2, + ChainID: "sei-test", + Time: time.Now().UTC(), + }).WithConsensusParams(cp) + + // estimate=35_000 > gasWanted=30_000 → invalid, falls back to gasWanted=30_000. + // totalGas=30_000 > MaxGas=25_000 → false. + tx := buildSignedLegacyEVMTx(t, a, 1, 30_000, 35_000) + require.False(t, a.checkTotalBlockGas(ctx, []sdk.Tx{tx})) +} + +// TestCheckTotalBlockGas_MaxGasWantedExceeded verifies that exceeding MaxGasWanted (while +// staying under MaxGas via a valid estimate) rejects the proposal. +func TestCheckTotalBlockGas_MaxGasWantedExceeded(t *testing.T) { + a := Setup(t, false, false, false) + cp := &tmproto.ConsensusParams{ + Block: &tmproto.BlockParams{ + MaxGas: 1_000_000, + MaxGasWanted: 15_000, // below a single EVM tx's gasWanted + }, + } + ctx := a.NewContext(false, tmproto.Header{ + Height: 2, + ChainID: "sei-test", + Time: time.Now().UTC(), + }).WithConsensusParams(cp) + + // gasWanted=21_000 > MaxGasWanted=15_000 → false regardless of MaxGas. + tx := buildSignedLegacyEVMTx(t, a, 1, 21_000, 0) + require.False(t, a.checkTotalBlockGas(ctx, []sdk.Tx{tx})) +} + +// TestCheckTotalBlockGas_ManyConcurrentTxsUnderLimit exercises the semaphore path +// (> maxConcurrent=32 goroutines) and verifies the result is correct when under the limit. +func TestCheckTotalBlockGas_ManyConcurrentTxsUnderLimit(t *testing.T) { + const numTxs = 50 + a := Setup(t, false, false, false) + cp := &tmproto.ConsensusParams{ + Block: &tmproto.BlockParams{ + MaxGas: numTxs * 21_000, + MaxGasWanted: numTxs * 21_000, + }, + } + ctx := a.NewContext(false, tmproto.Header{ + Height: 2, + ChainID: "sei-test", + Time: time.Now().UTC(), + }).WithConsensusParams(cp) + + txs := make([]sdk.Tx, numTxs) + for i := range txs { + txs[i] = buildSignedLegacyEVMTx(t, a, uint64(i+1), 21_000, 0) + } + require.True(t, a.checkTotalBlockGas(ctx, txs)) +} + +// TestCheckTotalBlockGas_ManyConcurrentTxsExceedsLimit verifies gas limit enforcement still +// works correctly when more than maxConcurrent=32 goroutines are spawned. +func TestCheckTotalBlockGas_ManyConcurrentTxsExceedsLimit(t *testing.T) { + const numTxs = 50 + a := Setup(t, false, false, false) + cp := &tmproto.ConsensusParams{ + Block: &tmproto.BlockParams{ + MaxGas: (numTxs - 1) * 21_000, // one tx over the limit + MaxGasWanted: numTxs * 21_000, + }, + } + ctx := a.NewContext(false, tmproto.Header{ + Height: 2, + ChainID: "sei-test", + Time: time.Now().UTC(), + }).WithConsensusParams(cp) + + txs := make([]sdk.Tx, numTxs) + for i := range txs { + txs[i] = buildSignedLegacyEVMTx(t, a, uint64(i+1), 21_000, 0) + } + require.False(t, a.checkTotalBlockGas(ctx, txs)) +} + +// TestCheckTotalBlockGas_NilsInterspersed verifies nil entries at arbitrary positions are skipped +// and do not affect the gas total for surrounding valid txs. +func TestCheckTotalBlockGas_NilsInterspersed(t *testing.T) { + a := Setup(t, false, false, false) + ctx := testCheckTotalBlockGasCtx(t, a) + + txs := []sdk.Tx{ + nil, + buildSignedLegacyEVMTx(t, a, 1, 21_000, 0), + nil, + buildSignedLegacyEVMTx(t, a, 2, 21_000, 0), + nil, + } + require.True(t, a.checkTotalBlockGas(ctx, txs)) +} + +// TestCheckTotalBlockGas_EmptyBlock verifies nil and empty tx slices are accepted. +func TestCheckTotalBlockGas_EmptyBlock(t *testing.T) { + a := Setup(t, false, false, false) + ctx := testCheckTotalBlockGasCtx(t, a) + require.True(t, a.checkTotalBlockGas(ctx, nil)) + require.True(t, a.checkTotalBlockGas(ctx, []sdk.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) {