Skip to content

perf(app): parallelize per-tx gas classification in checkTotalBlockGas#3399

Open
amir-deris wants to merge 10 commits into
mainfrom
amir/plt-324-concurrent-block-gas-calculation
Open

perf(app): parallelize per-tx gas classification in checkTotalBlockGas#3399
amir-deris wants to merge 10 commits into
mainfrom
amir/plt-324-concurrent-block-gas-calculation

Conversation

@amir-deris
Copy link
Copy Markdown
Contributor

@amir-deris amir-deris commented May 6, 2026

Summary

checkTotalBlockGas iterates every tx in a proposed block to compute total gas. Each tx's classification (EVM check, gasless check, gas extraction) is independent, making the loop a natural fan-out target.

Changes

  • checkTotalBlockGas — replaced the serial loop with a two-phase design:
    • Phase 1 (parallel): up to 32 concurrent goroutines, one per tx, each calling classifyTxForGas. Results are written into a pre-allocated []txGasResult slice indexed by position (no lock needed on the slice).
    • Phase 2 (serial): single pass over results for malicious-tx detection, overflow-safe accumulation, and MaxGasWanted/MaxGas limit enforcement.
  • classifyTxForGas — extracted helper that owns all per-tx logic. EVM txs (the vast majority) run fully unlocked. The non-EVM IsTxGasless path takes a ctxMu mutex because sdk.Context wraps a CacheMultiStore that is not goroutine-safe.
  • txGasResult — small struct (gasWanted, gasContrib, skip, malicious) that carries each tx's outcome between phases.
  • Dropped nonzeroTxsCnt — it was declared and incremented but never read.

Safety

  • No data races: each goroutine owns its results[i] slot; ctxMu serializes the only shared-state reader (IsTxGasless).
  • On typical EVM-heavy blocks ctxMu is uncontested — oracle-vote txs are the only non-EVM path that acquires it.
  • Semantics of the overflow check and gas-limit enforcement are unchanged; they moved to the serial accumulation phase where the running total is available.

Improvements

TBC


Note

Medium Risk
Touches consensus-critical proposal validation logic and introduces concurrency (goroutines, shared context locking), which could cause subtle race/ordering issues if incorrect despite added guards and tests.

Overview
Parallelizes checkTotalBlockGas per-tx classification by fanning out tx inspection to a bounded worker pool (max 32) and then performing a serial accumulation/enforcement pass against MaxGasWanted/MaxGas.

Adds txGasResult/txGasClassifyJob and extracts classifyTxForGas to encapsulate per-tx gasless/EVM/associate handling, including per-worker panic recovery and an atomic "malicious" short-circuit; uses a mutex to serialize IsTxGasless due to non-goroutine-safe sdk.Context.

Expands tests to cover gas-estimate selection/fallback, MaxGasWanted enforcement independent of MaxGas, behavior with many txs (>32), and nil/empty tx slices.

Reviewed by Cursor Bugbot for commit edf945e. Bugbot is set up for automated code reviews on this repo. Configure here.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 6, 2026

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedMay 11, 2026, 12:27 AM

@amir-deris amir-deris changed the title Added concurrent gas calculation for tx in the block perf(app): parallelize per-tx gas classification in checkTotalBlockGas May 6, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 6, 2026

Codecov Report

❌ Patch coverage is 69.13580% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.07%. Comparing base (0543e0e) to head (edf945e).

Files with missing lines Patch % Lines
app/app.go 69.13% 15 Missing and 10 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3399      +/-   ##
==========================================
- Coverage   59.24%   59.07%   -0.17%     
==========================================
  Files        2110     2105       -5     
  Lines      174149   173179     -970     
==========================================
- Hits       103175   102311     -864     
+ Misses      62041    61978      -63     
+ Partials     8933     8890      -43     
Flag Coverage Δ
sei-chain-pr 50.29% <69.13%> (?)
sei-db 70.41% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
app/app.go 68.47% <69.13%> (-1.22%) ⬇️

... and 72 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@amir-deris amir-deris requested a review from codchen May 6, 2026 18:13
@amir-deris amir-deris requested review from bdchatham and masih May 7, 2026 18:18
Copy link
Copy Markdown
Collaborator

@masih masih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice; thank you @amir-deris for picking this work up so quickly. this is impactful!

The current implementation works so technically no hard blocker.
However, I have left a bunch of comments that are worth addressing which should result in a more optimal solution.

🍻

Comment thread app/app.go
var ctxMu sync.Mutex
var wg sync.WaitGroup

const maxConcurrent = 32
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document the rationale for 32; if there is none please either paremeterise it in config or make it a factor of CPU cores with capped min and max.

Comment thread app/app.go Outdated
}
sem <- struct{}{}
wg.Add(1)
go func() {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you want here is a worker pool, right?

This implementation works, but: it creates a goroutine per tx. The idea is to use a pool of workers across which work is distributed. Specially since the max degree of concurrency is fixed.

Using the worker pool pattern here means we will have less allocations and the chances are if the work completes fast we might need to instantiate less than 32 concurrent goroutines (ignoring the obvious gain from goroutine reuse at max concurency)

Comment thread app/app.go
totalGas, totalGasWanted := uint64(0), uint64(0)
for _, r := range results {
if r.malicious {
return false
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why proceed with the remaining concurrent processing if a malicious result is found?

Have a look at err group. that primitive SDK type allows you to then return an error and as a result cancel all remaining concurrent work, which would make a more effective optimisation given that the point of this PR is exactly that.

Comment thread app/app.go Outdated
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 016d8f7. Configure here.

Comment thread app/app.go Outdated
Comment thread app/app.go Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants