[codex] add evm-only executor load test harness - #3658
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
bd57e4c to
7c6c44c
Compare
8e28929 to
4ec8da5
Compare
7c6c44c to
6f9547a
Compare
8cc2094 to
a65459e
Compare
6f9547a to
31f16a8
Compare
fc79ac5 to
e312a84
Compare
31f16a8 to
9cb00d6
Compare
e312a84 to
63b8662
Compare
9cb00d6 to
35864cf
Compare
63b8662 to
6a6c193
Compare
35864cf to
3eaa2ab
Compare
6a6c193 to
ef82dfd
Compare
3eaa2ab to
54456cf
Compare
ef82dfd to
2d0570c
Compare
54456cf to
6037fbb
Compare
2d0570c to
fa7c755
Compare
6037fbb to
f72a746
Compare
fa7c755 to
a9debf4
Compare
PR SummaryLow Risk Overview Transaction generation leans on Also bumps Reviewed by Cursor Bugbot for commit 201f2e2. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Adds a well-structured standalone evmonly-loadtest harness (prepare/execute pipeline, result sinks, metrics) with good test coverage. Two robustness issues surfaced by Codex are confirmed but both are confined to this dev/benchmark tool, so neither blocks merge.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
cursor-review.mdis empty (Cursor produced no output) andREVIEW_GUIDELINES.mdis empty/missing, so this review proceeded without repo-specific guidelines or a Cursor second opinion.- No direct test exercises the streaming (
runStreaming) path or the enqueue-under-writer-failure path; the confirmed sink race would be a good target for a focused test. - Minor: in
resultSinks.StoreBlockResultthe fallback (non-BlockResultSink) path does not callrelease()ifStoreChangeSet/StoreReceiptserrors, leaking the pooled result. Harmless today because the discard sink never errors, but worth guarding with a deferred release. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| return err | ||
| } | ||
| select { | ||
| case s.records <- record: |
There was a problem hiding this comment.
[suggestion] Race between this non-blocking send and the writer's failure path. enqueue checks getErr() above, then sends here; meanwhile run() can fail a write, call setErr + releaseQueuedResults, and return. A record that lands in the buffer after the drain but after run() has exited is never written and never released, leaking a pooled BlockResult slot (Codex's high finding — confirmed). Consider selecting on s.done in this first (and the buffered) send, or re-checking getErr()/draining, so a record enqueued after the writer stops is released. In practice the next StoreBlockResult returns the stored error and execution unwinds, so a full hang is unlikely, but the leak/latent race is real.
| } | ||
|
|
||
| func newAppendRLPFile(path string, bufferSize int, syncOnWrite bool) (*appendRLPFile, error) { | ||
| file, err := os.OpenFile(path, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, 0o644) |
There was a problem hiding this comment.
[suggestion] O_CREATE|O_TRUNC silently truncates pre-existing changesets.rlp/receipts.rlp, and cleanup later os.Removes them — so pointing --persist-dir at a directory containing prior output destroys those files (Codex's medium finding — confirmed). Since this is a load-test tool and the docs already recommend a fresh mktemp -d, this is acceptable, but consider O_EXCL (or rejecting a dir that already contains these files) to avoid clobbering user data by accident.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
83967c0 to
205f54c
Compare
There was a problem hiding this comment.
Beyond the inline finding, a candidate issue was also investigated and ruled out this run: the staking precompile's delegate/redelegate/createValidator paths appearing to skip the MaxVotingPowerRatio cap — verified this is not a real bug (the cap is enforced elsewhere in the flow).
Extended reasoning...
This run added one new inline finding (a nit about occWorkerPool serializing OCC execution across --workers in the standalone evmonly-loadtest harness) plus a ruled-out candidate (staking precompile MaxVotingPowerRatio cap). Given this PR's scope (300 files, including consensus-sensitive app.go giga-fallback changes, EVM RPC hardening, and config-management changes) and that prior runs already flagged unresolved issues (lint failures, an ERC20 fixture bytecode inversion, an async sink release race, and a prepareBlocks error-masking bug), this PR clearly still requires human review; this note is purely to record what else was checked this run, not a new verdict.
205f54c to
beef917
Compare
ed80600 to
4bc9274
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4bc9274. Configure here.
| if err := group.Wait(); err != nil { | ||
| return nil, err | ||
| } | ||
| return prebuilt, nil |
There was a problem hiding this comment.
Cancel during prebuild continues run
Medium Severity
prebuildBlockRequests treats cancellation as success and returns a partially filled slice. Unbuilt slots stay as zero-value envelopes (number=0), so SIGINT/SIGTERM during prebuild can still enter the execute phase and fail with confusing prepared-block ordering errors instead of stopping cleanly.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 4bc9274. Configure here.
| s.metrics.recordSinkEnqueueWait(time.Since(startedAt)) | ||
| } | ||
| return ctx.Err() | ||
| } |
There was a problem hiding this comment.
Sink enqueue ignores prior write errors
Medium Severity
After the async file writer records a persistence failure, enqueue can still accept more records and return success whenever the queue has space. Those later records are only released and never written, so StoreBlockResult reports success for outputs that were dropped.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 4bc9274. Configure here.
| func (s *generatedState) GetCode(addr common.Address) []byte { | ||
| if s.frozen.Load() { | ||
| return cloneBytes(s.code[addr]) | ||
| } | ||
| s.mu.RLock() | ||
| defer s.mu.RUnlock() | ||
| return cloneBytes(s.code[addr]) | ||
| } |
There was a problem hiding this comment.
🟡 generatedState.GetCode always clones the code slice on every call, even in the frozen (post-genesis) branch, whereas the sibling GetBalance deliberately returns the shared pointer once frozen (documented as safe since consumers must copy before mutation). Since code is immutable after Freeze() (SetCode is blocked by requireMutable), GetCode's frozen branch can return s.code[addr] directly like GetBalance does, removing a per-call allocation+copy of full contract bytecode from the hot path of every contract-call tx in the erc20-transfer and snapshot-revert workloads.
Extended reasoning...
The bug: In giga/evmonly/cmd/evmonly-loadtest/state.go, generatedState.GetBalance has two branches: a frozen branch that returns the shared *big.Int pointer directly (with a comment explicitly documenting that this is intentional — Frozen reads return shared, non-owned pointers; StateReader consumers must copy before mutation), and a mutable branch that defensively copies with new(big.Int).Set(balance). GetCode, right next to it, does not follow this pattern: both its frozen and mutable branches call cloneBytes(s.code[addr]), allocating and copying the full runtime bytecode on every single call regardless of whether the state has been frozen.
Why the frozen clone is unnecessary: SetCode is the only writer of s.code, and it begins with s.requireMutable(), which panics if s.frozen has been set via Freeze(). In the loadtest harness's actual usage (pipeline.go's runPrebuilt), all contract code is written by SetCode during block-request construction, and state.Freeze() is called once immediately after prebuilding completes, before any executor worker starts reading. From that point forward the underlying []byte slice in s.code[addr] can never be mutated again — requireMutable guarantees it. This is exactly the same invariant that already justifies GetBalance's frozen fast path, and the giga/evmonly/README.md StateReader contract ("Values returned by GetBalance and GetCode must remain stable while being read; the executor treats them as immutable and copies them into transaction-local state") explicitly extends the same immutability guarantee to GetCode. Executor call sites bear this out: state_db.go's ingest path does Code: cloneBytes(s.source.GetCode(addr)) itself (so a second clone inside GetCode is redundant), and occ.go's only other caller does a read-only bytes.Equal comparison.
Why nothing currently prevents this from being fixed: There's no correctness reason for the clone once frozen — it's purely defensive copying left over from (presumably) not distinguishing the frozen/mutable cases the way GetBalance already does one line above it in the same file.
Impact: This is a dev-only calibration/load-testing harness (package main, giga/evmonly/cmd/evmonly-loadtest), so there's no correctness or production risk. The cost is a real but modest one: every contract-call transaction in the erc20-transfer and snapshot-revert workloads calls GetCode on the executor's hot path, and each such call allocates and copys the full runtime bytecode (tens to low hundreds of bytes for the synthetic contracts here, but the pattern would matter more for larger contracts) instead of reusing the already-immutable shared slice — extra garbage and copy time in a tool whose entire purpose is to measure the executor's own allocation/throughput behavior.
Step-by-step proof:
runPrebuiltbuilds all blocks up front for thesnapshot-revertworkload;NewSnapshotRevertWorkloadcallsstate.SetCode(cfg.SnapshotRevertContract, snapshotRevertOuterRuntimeCode)andstate.SetCode(cfg.SnapshotRevertHelper, snapshotRevertHelperRuntimeCode), each internally cloning the passed-in code intos.code[addr].runPrebuiltthen callsstate.Freeze(), settings.frozenand permanently blocking any furtherSetCode(viarequireMutable's panic).- The executor pipeline starts; every executed transaction that calls the contract triggers a StateDB code load, which calls
generatedState.GetCode(addr). GetCode's current implementation takes theif s.frozen.Load()branch but still executescloneBytes(s.code[addr]), allocating a new[]byteand copying every byte of the runtime code — on every single call, for every single transaction, for the life of the run.- Because
s.code[addr]cannot change after step 2 (guaranteed byrequireMutable), returnings.code[addr]directly here would be exactly as safe asGetBalance's existing frozen branch (line 47-52 in the diff) already is for balances — no caller can observe a mutation because none is possible, and the sameStateReadercontract documented in the README already commits to treating this return value as immutable.
Fix: Mirror GetBalance's pattern — in the frozen branch, return s.code[addr] directly (falling back to nil for a missing key, matching cloneBytes's existing nil-for-empty behavior); keep the cloneBytes call only in the mutable (pre-freeze) branch.
This was independently flagged as a nit by reviewer seidroid in the PR timeline (state.go:83) using the same GetBalance-vs-GetCode comparison.
🔬 also observed by seidroid
| func (p *profileSession) Close() error { | ||
| if p == nil { | ||
| return nil | ||
| } | ||
| var errs []error | ||
| if p.traceActive { | ||
| trace.Stop() | ||
| p.traceActive = false | ||
| } | ||
| if p.cpuActive { | ||
| pprof.StopCPUProfile() | ||
| p.cpuActive = false | ||
| } | ||
| if p.cpuFile != nil { | ||
| errs = append(errs, p.cpuFile.Close()) | ||
| p.cpuFile = nil | ||
| } | ||
| if p.traceFile != nil { | ||
| errs = append(errs, p.traceFile.Close()) | ||
| p.traceFile = nil | ||
| } | ||
| if p.heapPath != "" && !p.heapComplete { | ||
| errs = append(errs, writeHeapProfile(p.heapPath)) | ||
| p.heapComplete = true | ||
| } | ||
| return errors.Join(errs...) | ||
| } |
There was a problem hiding this comment.
🟡 startProfiles reuses session.Close() as its cleanup path when CPU/trace profile setup fails after another profile file was already opened (profiles.go:34,42,47). Close() (profiles.go:87-90) unconditionally writes the heap profile if --heap-profile is set, forcing a GC and truncating/overwriting that file via O_CREATE|O_TRUNC even though the run never started, and the caller discards the Close() error so this happens silently. Fix by only writing the heap profile from the normal end-of-run path (finishProfiles), not from setup-failure cleanup.
Extended reasoning...
The bug
startProfiles (giga/evmonly/cmd/evmonly-loadtest/profiles.go:22-53) builds up a profileSession incrementally: it opens --cpu-profile, starts CPU profiling, then opens --trace-profile and starts the runtime trace. If any step after the first file is opened fails — e.g. CPU profiling starts fine but createProfileFile for the trace path fails, or trace.Start itself fails — the function calls session.Close() as its cleanup path (lines 34, 42, 47) and returns the original setup error.
Close() (lines 66-92) is the same method used for the normal end-of-run teardown. It unconditionally does:
if p.heapPath != "" && !p.heapComplete {
errs = append(errs, writeHeapProfile(p.heapPath))
p.heapComplete = true
}There is nothing in this branch that distinguishes "the run finished and we should snapshot the heap" from "setup failed and we are just releasing already-opened file handles." writeHeapProfile forces a runtime.GC() and opens the --heap-profile path with os.O_CREATE|os.O_TRUNC (via createProfileFile, line 59) — so any pre-existing file at that path is truncated and overwritten with a near-empty, startup-time heap snapshot.
Trigger path
evmonly-loadtest --cpu-profile=ok.out --trace-profile=/no/perm/trace.out --heap-profile=heap.out ...:
cfg.cpuProfile != ""→createProfileFilesucceeds,pprof.StartCPUProfilesucceeds,session.cpuActive = true.cfg.traceProfile != ""→createProfileFile("/no/perm/trace.out")fails (bad path/permissions).startProfilescalls_ = session.Close()and returns the create-file error.- Inside
Close(),p.heapPath == "heap.out"andp.heapComplete == false, sowriteHeapProfile("heap.out")runs — GC forced,heap.outtruncated and replaced with a startup-time snapshot. - The caller (
runPrebuilt, viastartProfiles(cfg)at the top of the function) receives the setup error and returns immediately — the harness never executes a single block.
Why nothing else catches it
The error from session.Close() is explicitly discarded with _ = session.Close() in the failure branches, so there is no log line or exit code signaling that a heap file was just written. heapComplete only exists to make Close() idempotent (so a later call from finishProfiles does not write twice) — it is not a guard against calling Close() before execution ever started.
Impact
This is a standalone dev/calibration harness (giga/evmonly/cmd/evmonly-loadtest), not app or consensus code, so nothing here affects a production path. The impact is limited to: (a) a user re-running the harness pointed at the same --heap-profile path loses whatever heap snapshot a prior successful run had produced, with no warning, and (b) the on-disk artifact after a failed setup is a misleading near-empty startup snapshot rather than being absent or clearly marked as invalid.
Fix
The heap write belongs only to the normal end-of-run path (finishProfiles, called after the pipeline actually runs), not to the setup-failure cleanup path inside startProfiles. A simple fix is to give Close() a mode flag (or split it into closeFiles() used by setup-failure cleanup and the full Close() used by finishProfiles), so the heap profile is only ever written once execution has actually started/finished.
3 participants
Summary
Adds a standalone
evmonly-loadtestcommand that feeds generated EVM-only blocks into the EVM-only executor with generated genesis state, configurable result sinks, and Prometheus/stdout throughput metrics.The executor owns the result-sink boundary.
giga/evmonlyexposesResultSink,BlockResultSink, andWithResultSink, and executor completion invokes the sink for produced outputs before returning. The loadtest harness implements discard/file sink modes through those interfaces.The executor can also use a bounded reusable
BlockResultpool viaBlockResultPoolSize. Pooled results are reference-counted: callers release their returned result withBlockResult.Release(), and async sinks retain/release throughBlockResultSinkafter changesets and receipts are no longer referenced. The loadtest harness enables this by default with--result-pool-size=0, sized for executor workers plus in-flight async sink records; negative disables result pooling.This branch also pipelines stateless sender recovery ahead of execution:
Executor.PrepareBlockdecodes raw tx RLP and recovers senders intoPreparedBlock.Executor.ExecutePreparedBlockexecutes an already prepared block.ExecuteBlockremains the convenience prepare-then-execute path.evmonly-loadtestnow has--prepare-workers; raw blocks flow through parallel prepare workers into an ordered prepared-block queue consumed by the single external executor worker.The harness supports optimistic no-overlap native transfers and ERC20 transfers. It defaults to unique senders and recipients, supports
--prebuild-blocks, and includes a lightweight async file sink via--result-sink=file --persist-dir=<dir>. Persistence records are append-only RLP records for changesets and receipts, and the sink removes files on normal completion, execution errors, andSIGINT/SIGTERM.Metrics include block input/prepared/finished throughput, prepared tx/s, executed tx/s, gas/s, OCC attempts/fallbacks/conflicts, result-sink queue depth, enqueue wait, write time, bytes written, and record counts.
sink_enqueue_waitis the backpressure signal: if it rises, executor workers are blocked on persistence queue capacity.Validation
go test ./giga/evmonly/...DIR=$(mktemp -d)first andremove it with
rmdir "$DIR"after the command succeeds.go run ./giga/evmonly/cmd/evmonly-loadtest \ --metrics-addr= \ --report-interval=0 \ --prebuild-blocks \ --blocks=30 \ --txs-per-block=1000 \ --builders=8 \ --prepare-workers=8 \ --workers=1 \ --executor-workers=12 \ --gas-price-wei=0 \ --min-gas-price-wei=0 \ --queue-size=64 \ --result-sink=file \ --persist-dir="$DIR"Observed locally with pooled async file persistence:
218,638 TPS,prepare_errors=0,errors=0,sink_enqueue_wait=0s, and after closesink_written=60.go run ./giga/evmonly/cmd/evmonly-loadtest \ --metrics-addr= \ --report-interval=0 \ --prebuild-blocks \ --blocks=30 \ --txs-per-block=1000 \ --builders=8 \ --prepare-workers=8 \ --workers=1 \ --executor-workers=12 \ --workload=erc20-transfer \ --gas-price-wei=0 \ --min-gas-price-wei=0 \ --queue-size=64 \ --result-sink=file \ --persist-dir="$DIR"Observed locally with pooled async file persistence:
202,486 TPS,prepare_errors=0,errors=0,sink_enqueue_wait=0s, and after closesink_written=60.EC2 Persistent Runs
Run on commit
fa7c7556busing a temporaryc8i.48xlargeinus-east-1a, SMT disabled withCoreCount=96,ThreadsPerCore=1, Go1.25.6,GOMAXPROCS=96,GOGC=400, one external executor worker, prebuilt raw blocks, unique senders/recipients, zero gas price/min gas price, default result pooling, and--result-sink=file. The temporary instance, key pair, and security group were deleted after collecting logs. Logs were copied locally to/tmp/evmonly-pool-20260702210816/logs.Native transfer tuned command:
GOMAXPROCS=96 GOGC=400 /tmp/evmonly-loadtest \ --metrics-addr= \ --blocks=1000 \ --txs-per-block=5000 \ --prebuild-blocks \ --builders=96 \ --prepare-workers=48 \ --workers=1 \ --executor-workers=40 \ --gas-price-wei=0 \ --min-gas-price-wei=0 \ --report-interval=5s \ --queue-size=64 \ --result-sink=file \ --persist-dir="$DIR"Observed:
ERC20 transfer tuned command:
GOMAXPROCS=96 GOGC=400 /tmp/evmonly-loadtest \ --metrics-addr= \ --blocks=1000 \ --txs-per-block=5000 \ --prebuild-blocks \ --builders=96 \ --prepare-workers=48 \ --workers=1 \ --executor-workers=48 \ --workload=erc20-transfer \ --gas-price-wei=0 \ --min-gas-price-wei=0 \ --report-interval=5s \ --queue-size=64 \ --result-sink=file \ --persist-dir="$DIR"Observed:
Tuning notes from the same EC2 instance:
result-pool-sizeblocks. Commita38d56ec6fixes that.nativeStateDBscratch reuse was tested and removed. It raised CPU use but lowered EC2 throughput to about166k TPS, likely from map-clearing/retained-heap costs.--prepare-workers=24became sender-recovery limited; increasing to48recovered throughput.--prepare-workers=48 --executor-workers=40,185.6k TPSover 2.5M tx. The full 5M-tx confirmation reached196.7k TPS.--prepare-workers=48 --executor-workers=48,173.5k TPSover 2.5M tx. The full 5M-tx confirmation reached181.0k TPS.sink_enqueue_wait=0s.Historical Non-Persistent 199.3k Repro
The earlier 199.3k EC2 benchmark was run before persistent sink and prepared pipeline changes, from commit
4ec8da52c, on ac8i.48xlargeinus-east-1awith SMT disabled, Go1.25.6, one external executor worker, prebuilt blocks, unique senders/recipients, and zero gas price/min gas price.Observed:
EC2 Worker Pool / Pinning Follow-up
A follow-up on temporary commit
63b8662b4bc90936175181b1fff6347c502fb03ftested persistent OCC workers with and without Linux worker pinning on the samec8i.48xlargeshape. The persistent OCC worker pool stayed in this branch; the worker pinning flags/files were removed afterward because they did not improve throughput or CPU utilization.Observed 5M-tx persistent-sink results:
186,607 TPS,sink_enqueue_wait=0s,Percent of CPU: 2404%.186,683 TPS,sink_enqueue_wait=0s,Percent of CPU: 2283%.170,501 TPS,sink_enqueue_wait=0s,Percent of CPU: 2003%.165,290 TPS,sink_enqueue_wait=0s,Percent of CPU: 2009%.Conclusion: the persistent OCC pool removes per-block worker creation overhead, but thread pinning was flat for native transfer and worse for ERC20. CPU remained around 20-24 effective cores on a 96-core instance, so the current branch keeps the simpler unpinned worker-pool model.