From 2809bf14672f700ba6f86578fbe077d4826f4224 Mon Sep 17 00:00:00 2001 From: Amir Deris Date: Wed, 6 May 2026 09:19:08 -0700 Subject: [PATCH 1/5] Added concurrent gas calculation for tx in the block --- app/app.go | 162 +++++++++++++++++++++++++++++++---------------------- 1 file changed, 94 insertions(+), 68 deletions(-) diff --git a/app/app.go b/app/app.go index 6bcef5afab..8c9c0aa18a 100644 --- a/app/app.go +++ b/app/app.go @@ -2447,6 +2447,14 @@ 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 +} + // 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). @@ -2454,95 +2462,113 @@ 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 + var wg sync.WaitGroup + + const maxConcurrent = 32 + sem := make(chan struct{}, maxConcurrent) + + 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 - } + sem <- struct{}{} + wg.Add(1) + go func() { + defer wg.Done() + defer func() { <-sem }() + results[i] = app.classifyTxForGas(ctx, tx, &ctxMu) + }() + } + wg.Wait() + + // Serial phase: malicious check + overflow-safe accumulation + limit enforcement. + totalGas, totalGasWanted := uint64(0), uint64(0) + for _, r := range results { + if r.malicious { + return false } - // Check whether it's associate tx - gasWanted := uint64(0) - if evmErr != nil { + if r.skip { continue } - 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() + if int64(r.gasWanted) < 0 || int64(totalGas) > math.MaxInt64-int64(r.gasWanted) { //nolint:gosec + return false } - - if int64(gasWanted) < 0 || int64(totalGas) > math.MaxInt64-int64(gasWanted) { // nolint:gosec + totalGasWanted += r.gasWanted + totalGas += r.gasContrib + if totalGasWanted > uint64(ctx.ConsensusParams().Block.MaxGasWanted) { //nolint:gosec return false } - - if gasWanted > 0 { - nonzeroTxsCnt++ + if totalGas > uint64(ctx.ConsensusParams().Block.MaxGas) { //nolint:gosec + return false } + } + return true +} - 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 +// classifyTxForGas computes the gas contribution of a single tx for checkTotalBlockGas. +// ctxMu must guard any call to IsTxGasless since sdk.Context is not goroutine-safe. +func (app *App) classifyTxForGas(ctx sdk.Context, tx sdk.Tx, ctxMu *sync.Mutex) txGasResult { + 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 := antedecorators.IsTxGasless(tx, ctx, app.OracleKeeper, &app.EvmKeeper) + ctxMu.Unlock() + 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 useEstimate { - totalGas += decodedTx.GetGasEstimate() - } else { - totalGas += gasWanted + if isGasless { + return txGasResult{skip: true} } + } - if totalGasWanted > uint64(ctx.ConsensusParams().Block.MaxGasWanted) { //nolint:gosec - return false - } + if evmErr != nil { + return txGasResult{skip: true} + } - if totalGas > uint64(ctx.ConsensusParams().Block.MaxGas) { //nolint:gosec - return false + 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() } - return true + // 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 { From 753e1bb17dfb50f23a2fd292fad691ae3d640289 Mon Sep 17 00:00:00 2001 From: Amir Deris Date: Wed, 6 May 2026 09:41:28 -0700 Subject: [PATCH 2/5] Added comments, fixed panic issue in go routines --- app/app.go | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/app/app.go b/app/app.go index 8c9c0aa18a..aefd15dbec 100644 --- a/app/app.go +++ b/app/app.go @@ -2517,8 +2517,19 @@ func (app *App) checkTotalBlockGas(ctx sdk.Context, typedTxs []sdk.Tx) (result b } // classifyTxForGas computes the gas contribution of a single tx for checkTotalBlockGas. -// ctxMu must guard any call to IsTxGasless since sdk.Context is not goroutine-safe. -func (app *App) classifyTxForGas(ctx sdk.Context, tx sdk.Tx, ctxMu *sync.Mutex) txGasResult { +// 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 From b3d58a19b9c93ba1c1c81257c74b940ac3058705 Mon Sep 17 00:00:00 2001 From: Amir Deris Date: Thu, 7 May 2026 10:29:48 -0700 Subject: [PATCH 3/5] Added test coverage --- app/check_total_block_gas_test.go | 160 ++++++++++++++++++++++++++++++ 1 file changed, 160 insertions(+) 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) { From 016d8f7736d8162ed976976df8634a84dcf8433a Mon Sep 17 00:00:00 2001 From: Amir Deris Date: Fri, 8 May 2026 18:07:50 -0700 Subject: [PATCH 4/5] Addressing some pr feedback --- app/app.go | 55 ++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 43 insertions(+), 12 deletions(-) diff --git a/app/app.go b/app/app.go index bd7537b33c..43e976b31e 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" @@ -2516,6 +2517,12 @@ type txGasResult struct { 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). @@ -2533,25 +2540,49 @@ func (app *App) checkTotalBlockGas(ctx sdk.Context, typedTxs []sdk.Tx) (result b // reads from it. EVM txs skip that path entirely, so ctxMu is uncontested on // typical EVM-heavy blocks. var ctxMu sync.Mutex - var wg sync.WaitGroup const maxConcurrent = 32 - sem := make(chan struct{}, maxConcurrent) - + jobs := make([]txGasClassifyJob, 0, len(typedTxs)) for i, tx := range typedTxs { if tx == nil { results[i].skip = true continue } - sem <- struct{}{} - wg.Add(1) - go func() { - defer wg.Done() - defer func() { <-sem }() - results[i] = app.classifyTxForGas(ctx, tx, &ctxMu) - }() + 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) + } + jobCh := make(chan txGasClassifyJob) + 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) + } + } + }() + } + for _, job := range jobs { + jobCh <- job + } + close(jobCh) + wg.Wait() + if maliciousSeen.Load() { + return false + } } - wg.Wait() // Serial phase: malicious check + overflow-safe accumulation + limit enforcement. totalGas, totalGasWanted := uint64(0), uint64(0) @@ -2599,8 +2630,8 @@ func (app *App) classifyTxForGas(ctx sdk.Context, tx sdk.Tx, ctxMu *sync.Mutex) if !skipGaslessCheck && app.couldBeGaslessTransaction(tx) { ctxMu.Lock() + defer ctxMu.Unlock() isGasless, err := antedecorators.IsTxGasless(tx, ctx, app.OracleKeeper, &app.EvmKeeper) - ctxMu.Unlock() if err != nil { if strings.Contains(err.Error(), "panic in IsTxGasless") { logger.Error("malicious transaction detected in gasless check", "err", err) From edf945e15cae1e690b2f2e9ec77b987fc820efe2 Mon Sep 17 00:00:00 2001 From: Amir Deris Date: Sun, 10 May 2026 17:26:45 -0700 Subject: [PATCH 5/5] Addressing Cursor bot feedbacks --- app/app.go | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/app/app.go b/app/app.go index 43e976b31e..24175130c6 100644 --- a/app/app.go +++ b/app/app.go @@ -2556,7 +2556,11 @@ func (app *App) checkTotalBlockGas(ctx sdk.Context, typedTxs []sdk.Tx) (result b if nWorkers > len(jobs) { nWorkers = len(jobs) } - jobCh := make(chan txGasClassifyJob) + jobCh := make(chan txGasClassifyJob, len(jobs)) + for _, job := range jobs { + jobCh <- job + } + close(jobCh) var wg sync.WaitGroup wg.Add(nWorkers) for range nWorkers { @@ -2574,10 +2578,6 @@ func (app *App) checkTotalBlockGas(ctx sdk.Context, typedTxs []sdk.Tx) (result b } }() } - for _, job := range jobs { - jobCh <- job - } - close(jobCh) wg.Wait() if maliciousSeen.Load() { return false @@ -2630,8 +2630,10 @@ func (app *App) classifyTxForGas(ctx sdk.Context, tx sdk.Tx, ctxMu *sync.Mutex) if !skipGaslessCheck && app.couldBeGaslessTransaction(tx) { ctxMu.Lock() - defer ctxMu.Unlock() - isGasless, err := antedecorators.IsTxGasless(tx, ctx, app.OracleKeeper, &app.EvmKeeper) + 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)