Bound concurrent CheckTx on the producer ingest path - #4182
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
PR SummaryMedium Risk Overview
Reviewed by Cursor Bugbot for commit 8b53310. Bugbot is set up for automated code reviews on this repo. Configure here. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## giga-1 #4182 +/- ##
========================================
Coverage 65.55% 65.55%
========================================
Files 2081 2078 -3
Lines 157460 157160 -300
========================================
- Hits 103222 103034 -188
+ Misses 54097 53985 -112
Partials 141 141
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bounding CheckTx with a ctx-cancellable semaphore is the right shape and the concurrency tests are solid, but making the CheckTx path observe ctx cancellation for the first time breaks broadcast_tx_async on autobahn validators, which hands TryInsertTx a request-scoped context from a detached goroutine. The new permit wait is also invisible to the producer metrics the PR proposes to validate against.
Findings: 1 blocking | 3 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion] The permit wait is unmeasured, which undercuts the validation plan in the PR description.
metrics.PhaseCheckTx.Enter()andObserveCheckTxare now both entered afterAcquirereturns, soinFlight{phase="check_tx"}saturates atMaxConcurrentCheckTxandcheckTxLatencyexcludes queueing time — waiters sit in no phase at all, and ingest queue depth (the signal that shows whether the cap is set too low) disappears from the dashboards. Adding aPhaseCheckTxWaitgauge plus a wait-latency histogram around theAcquirecall would keep the existing phase gauges exhaustive and make the new backpressure visible. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
sei-tendermint/internal/rpc/core/mempool.go:67(BroadcastTxAsync) spawnsgo func() { _, _ = v.TryInsertTx(ctx, req.Tx) }()with the JSON-RPC handler's request context (hreq.Context()inrpc/jsonrpc/server/http_json_handler.go:68), which net/http cancels as soon as the handler returns. Even before this PR that already silently dropped the tx wheneverwaitForCapacitywas reached (full mempool), and it also violates the module's "don't spawn goroutines with plain go func" convention.
|
@seidroid review |
There was a problem hiding this comment.
The permit-around-CheckTx shape is correct — the permit is released before the mempool lock, so admission/nonce semantics are untouched — and both findings from the previous review are genuinely fixed (BroadcastTxAsync now detaches with context.WithoutCancel, and the wait has its own check_tx_wait phase plus histogram). Two non-blocking notes: the async ingest queue behind the cap is now unbounded and unsheddable, and the new config key has no wiring/validation test.
Findings: 0 blocking | 2 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] No test covers the new config key end to end.
sei-tendermint/node/setup_test.goasserts themax_txs_per_second→producer.Configpass-through (TestBuildGigaConfig_EnabledWithValidators / _NoneMaxTxsPerSecond) but nothing assertsmax_concurrent_check_txreachesProducer.MaxConcurrentCheckTx, and the newValidatebound (0 and > MaxInt32 rejected) is untested, so a dropped field inbuildValidatorGigaConfigwould silently fall back to the GOMAXPROCS/2 default. The producer-side unit test (TestConfig_MaxConcurrentCheckTxDefault) only covers the in-package resolution. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| // checkTx runs the app CheckTx for tx, holding one of cfg.MaxConcurrentCheckTx permits | ||
| // for the duration of the call. Waiting for a permit is cancelled with ctx. | ||
| func (s *State) checkTx(ctx context.Context, tx tmtypes.Tx) (*abci.ResponseCheckTxV2, error) { | ||
| release, err := s.acquireCheckTxPermit(ctx) |
There was a problem hiding this comment.
[suggestion] The permit wait turns TryInsertTx from fail-fast into an unbounded, now-unsheddable queue. TryInsertTx is documented as "returns error if mempool is full" and BroadcastTxAsync relies on that to be best-effort and non-blocking; with the permit acquired before the mempool state is consulted, every async broadcast parks here first, and since that goroutine is now handed context.WithoutCancel(ctx) nothing sheds it — not a client disconnect, not node shutdown.
Concretely: once arrival rate exceeds MaxConcurrentCheckTx / checkTxLatency (the cap halves the previous service rate, so this threshold is now lower), waiters accumulate one goroutine plus the tx bytes per broadcast with no bound, and the RPC has already returned 200. The CPU starvation this PR fixes is real, but the trade is currently unbounded memory growth instead, with no inserts{result=...} signal distinguishing it — only in_flight{phase="check_tx_wait"} drifting upward.
Bounding the queue on the non-waiting path would keep the fail-fast contract: e.g. a TryAcquire (or a max-waiters count) that returns errMempoolFull for waitIfFull == false, so overload surfaces as inserts{result="full"} and the goroutine exits instead of parking indefinitely. InsertTx (sync broadcast_tx / eth_sendRawTransaction) can keep blocking, since there the caller's ctx still provides backpressure.
Superseded: latest AI review found no blocking issues.
2 participants
On autobahn-e2e load tests the validator that receives the load-generator traffic runs at ~28 of 32 Ps while its peers idle at 4-7, and its certify/execute stages fall hundreds of blocks behind receive. Every broadcast_tx RPC call runs the app CheckTx (including ecrecover) on its own goroutine via producer.State.insertTx, so under load there is nothing stopping ingest from taking every P and starving the consensus, data and execute loops.
This change bounds that work. producer.Config gains MaxConcurrentCheckTx (default max(1, GOMAXPROCS/2)), and State.insertTx now takes a permit from a utils.Semaphore of that size around CheckTxSafe, releasing it before the mempool Watch lock is taken. Waiting for a permit is ctx-cancellable, so cancelled waiters return without consuming a slot. Admission, ordering and nonce semantics are unchanged: the permit only covers the CheckTx call. The knob is exposed in the autobahn JSON config as max_concurrent_check_tx (absent means the default, 0 is rejected by Validate). The permit wait is exposed as its own insert phase (in_flight{phase="check_tx_wait"}) plus a check_tx_wait_latency histogram, so queue depth behind the cap is visible alongside the existing CheckTx metrics. BroadcastTxAsync still spawns its own goroutine per call; since the insert path now observes ctx cancellation while waiting for a permit, that goroutine passes context.WithoutCancel of the request ctx so an async broadcast is not dropped once the HTTP handler returns. Replacing the goroutine with a scope owned by the RPC environment remains out of scope here.
To validate on a brandon-autobahn run under the same load: pod CPU of the ingesting validator should drop from ~28 cores to roughly half of GOMAXPROCS plus consensus overhead, executed TPS should hold (or improve, since execute is no longer starved), and next_block{stage="certify"} - next_block{stage="receive"} should stay near 1 instead of drifting into the hundreds. If executed TPS drops with the lag fixed, raise max_concurrent_check_tx in the autobahn config to trade back some ingest parallelism.