tools/gasbench: per-opcode EVM gas-vs-execution-time differential microbenchmark (F2 MVP)#3761
tools/gasbench: per-opcode EVM gas-vs-execution-time differential microbenchmark (F2 MVP)#3761bdchatham wants to merge 20 commits into
Conversation
… MVP Tracer-free differential microbenchmark: measures 22 scalar EVM opcodes (arithmetic/bitwise/comparison/stack) via core/vm/runtime.Call against a fresh in-memory StateDB, no tracer attached (per the design's load-bearing constraint: gas and time are never measured in the same run). Per-opcode gas comes from the interpreter's own accounting; per-opcode time from difference-of-medians between a balanced baseline/target bytecode pair. CoV is the first-class output and the acceptance gate (design's noise-floor criterion). Gas self-check asserts measured whole-program gas delta == definitional expected delta for every case (all 22 pass). Local integration of two coral-built components (timing/stats/emission; EVM wiring/bytecode construction) reconciled during assembly: Program (renamed from a Harness/Harness name collision) is a pre-warmed per-program executor so state setup is amortized out of the timed loop, and RunOnce is zero-arg (closes over its Program) rather than taking code per call. Local branch only, not pushed. Design: bdchatham-designs designs/gas-repricing-telemetry/gas-vs-time-instrumentation.md
…persisted diff, shift-op operands Three correctness-grade findings from systems-engineer (assigned dissenter): - MISMATCH: Run.Status was gated only on NoiseOK (per-series stability), never on Diff.Significant (whether the delta clears measurement uncertainty). A statistically-insignificant opcode delta could be emitted Status="ok". Status is now OK only when both NoiseOK and Significant hold, with a new StatusInsignificant distinguishing 'noisy measurement' from 'stable series, zero-indistinguishable delta'. - MISSING: persisted CSV/NDJSON carried only the raw per-series median (one row per baseline/target variant), never the differential itself. Run is redesigned to one row per opcode carrying Reps, GasUsed (the measured whole-program gas delta), ExecTimeNs (the measured whole- program time delta), and Significant -- the actual (gas, time) pair the design's acceptance criteria describe. - MISMATCH: SHL/SHR/SAR reused the same full-width seed as both stack operands, making the shift AMOUNT itself 2^256-1 -- go-ethereum's value.Clear() early-out for shifts >=256, the cheapest possible path, not the limb-shift work the old comment claimed to measure. BuildCaseWith now special-cases the shift family with a distinct in-range shift-amount operand (seedShift) via a DUP2/DUP2 construction that keeps the differential net-0. Verified: post-fix per-op-ns for SHL/SHR/SAR rose ~50-70% (1.3-1.4ns -> 2.0-2.4ns), consistent with exercising real limb-shift work instead of the degenerate clear path. Also closes two non-blocking items from the other two lenses: - solidity-developer: Program's reuse-safety doc comment cited 'no state change' as the reason it's safe to reuse across calls; corrected to the real basis (deterministic gas + tail-only, baseline/target- symmetric journal growth from an un-Finalise-d Snapshot per call), with a warning against extending to high-iteration or state-touching use without re-checking. - idiomatic-reviewer (style/advisory): DefaultConfig() is now the actual source of truth bench_test.go reads through (was dead code, values triplicated in run.sh); WriteCSV errors now wrap with the gasbench: prefix like WriteNDJSON; Case.Class now flows all the way to the persisted Run/CSV row instead of being unused; Series.InputID and StatusError documented. All 22 opcodes re-verified: gas self-check passes, build/vet/gofmt clean. Local branch only, not pushed.
…omatic-reviewer) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lation research Three refinements derived from designs/gas-repricing-telemetry/research/ microbenchmark-noise-isolation-tradeoffs.md: - Status is now purely Significant-driven (ok/insignificant). CoV no longer overrides a statistically solid result -- three independent benchmarking harnesses (JMH, criterion.rs, Go's own benchstat) all gate on effect-size-vs-uncertainty, not a fixed dispersion threshold, and the research's practitioner sweep gave the same reasoning (CoV describes sample dispersion, not confidence in the derived median; the differential's own propagated SE is the right metric). CoV survives as an independent HighVariance advisory flag. - Recalibrated the default health-check ceiling from 2% to 25%: above the 4-8% CoV measured as normal on a dedicated, pinned, bare-metal host with no kernel-level isolation, well below the "something is actually wrong" territory (~40%+) the research's practitioner sweep named. - Added getrusage-based active-benchmarking diagnostics (Nvcsw/Nivcsw deltas around the timed window). This automates, on every future run, the same active-benchmarking check (Gregg's methodology) that was previously a one-off manual perf-stat/interrupts session on the EC2 host -- involuntary context-switches are now the direct, measured evidence behind HighVariance, not merely inferred from CoV. Deliberately NOT adding opportunistic cpuset/IRQ-affinity isolation to run.sh -- the research's explicit recommendation was not to invest further isolation effort for this MVP; doing so now would contradict that finding. Local branch only, not pushed.
…o docs Closes the findings from this refinement's first xreview round (systems-engineer, idiomatic-reviewer, solidity-developer): - Fixed stats.go's stale "CoV is the acceptance gate" comment (contradicted the refinement's own thesis). - Corrected the Nvcsw/Nivcsw doc comments: RUSAGE_SELF is process-wide, not thread-scoped, and a nonzero Nivcsw explains only the scheduler-preemption slice of CoV, not the Program-reuse journal-growth tail (a separate, already-documented noise source). - Surfaced the previously write-only-and-dead Nvcsw counter through to Diff and Run, and into the CSV/NDJSON schema (new nvcsw column). - Fixed bench_test.go's false claim that -count=K gives K independent process runs (Go reruns within the same OS process). - Added a table test (emit_test.go) pinning that Status is a pure function of Significant, independent of HighVariance/CoV. Also restructures where rationale lives, per direct feedback: this package's doc comments had grown into multi-paragraph methodology essays (the differential-construction algebra, the acceptance-gate research, the active-benchmarking diagnostics). Moved that narrative into tools/gasbench/README.md and tools/gasbench/AGENTS.md; code comments are back to lean, present-state statements that point at the docs for why. Wired tools/gasbench/AGENTS.md into the top-level AGENTS.md's nested-guides list. Local branch only, not pushed.
xreview R2's assigned dissenter (systems-engineer) found that README.md's "load-bearing invariant" section, written this round, claimed gas and time come from two separate tracer-free runs. They don't: gasbench.go's Measure times the exact same run() call whose return value becomes GasUsed. This harness only measures whole-program gas, which needs no tracer at all, so there's no separate gas pass to speak of -- the real invariant is "never attach a tracer to the timed call." Fixed in both README.md and AGENTS.md (which restated the same error), and added a note that a future per-opcode gas breakdown (which would need a tracer) must come from a separate, untimed call. Also, from the same round's secondary findings: - emit_test.go's table test asserted Status is independent of HighVariance but never varied BaselineCoV/TargetCoV, so a regression gating Status on raw CoV directly would have passed unnoticed. Added two cases with CoV=0.9 to actually exercise that path. - Added a same-value doc pointer to Nvcsw (Nivcsw already had one) for the getrusage-failure-zero ambiguity. - Added inline pointers at the SHL/SHR/SAR and default BuildCaseWith branches back to README.md, since the correctness reasoning for DUP2-vs-DUP1 no longer lives next to the switch. - Added the new files to AGENTS.md's Files table. And from prose-steward's independent pass over the same two new docs: - README's invariant header now matches AGENTS's framing exactly (both fixes above happened to close prose-steward's top finding too: the two docs stated the load-bearing invariant at different scopes). - Reconciled AGENTS's "terminate cleanly (STOP...)" with README's general STOP/RETURN error contract -- every Case built here uses STOP by construction, but the general Program.Run contract also accepts RETURN. - Reconciled the new-opcode-spec verification recipe: AGENTS is now the one authoritative checklist (bridges Arity to jump_table.go's minStack/maxStack); README points to it instead of restating a partial version. - Expanded CoV on first use, fixed a forward reference to the getrusage section, dropped a hardcoded "22 Specs entries" count that would rot on the next addition. Local branch only, not pushed.
…ertainty xreview R3's assigned dissenter (systems-engineer) ran a from-scratch correctness audit (not just re-verifying prior fixes) and found a real bug in the acceptance gate itself: Summarize only computes Stddev/SEMean/SEMedian when a series has more than 1 sample, so at Iterations=1 (or any degenerate zero-variance sample) Uncertainty is exactly 0. Significant was `|DeltaNs| > SigmaK*0`, trivially true for any nonzero delta -- every opcode reported significant=true with a statistically meaningless uncertainty of zero. GASBENCH_ITERS is operator-settable with no documented floor, so this was reachable, not theoretical. Verified empirically: at Iterations=1 every one of the 22 opcodes (including MULMOD's 44us delta) used to report significant=true; Subtract now requires Uncertainty > 0, so all 22 correctly report insignificant instead. Added TestSubtractNeverSignificantWithZeroUncertainty pinning this. Also closes the remaining Round 3 findings, all RATIFY-with-advisory from idiomatic-reviewer, solidity-developer, and prose-steward: - gasbench.go: fixed the sink comment's wrong justification (it isn't GOMAXPROCS=1/thread-locking that makes the shared var race-free, it's that BenchmarkOpcodes's subtests run sequentially). - bench_test.go: removed a redundant c := c loop-variable copy (dead under Go 1.22+ per-iteration loop-var semantics, and b.Run here never runs in parallel). - programs.go: added a Class const-block group comment for symmetry with Status's; trimmed the SHL/SHR/SAR inline comment back to a README pointer instead of restating go-ethereum internals; added an explicit Arity<1 guard in BuildCaseWith's default branch (the (n-1)*GasQuickStep formula assumes n>=1; DUP1/SWAP1 are the only Arity-0 specs and are already special-cased above it). - emit.go: documented the CSV-vs-NDJSON CoV precision asymmetry (6-sig-fig display rounding vs full precision) as a deliberate, advisory-only choice rather than leaving it silent. - README.md: fixed SAR's early-out boundary (strictly >256, vs SHL/SHR's >=256); replaced a line-number anchor into x/evm/keeper/msg_server.go with a symbol reference so it can't rot; moved the CoV term's expansion to its actual first use. Local branch only, not pushed.
All four R4 reviewers ratified; these are their advisory-tier items: - diff_test.go: t.Errorf+continue instead of t.Fatalf inside the table loop, so a broken fixture doesn't mask whether sibling cases also regressed (idiomatic-reviewer, per the keep-going table convention emit_test.go established). - programs_test.go: new recover-based test pinning BuildCaseWith's Arity<1 panic guard, previously the only invariant in the package with no unit test (it is dead code for the in-tree Specs, so only a test can exercise it). - README.md: the SHL/SHR/SAR early-out is value.Clear() -- or SetAllOne() for SAR on a negative operand (solidity-developer, verified against the fork's opSAR). Local branch only, not pushed.
A handoff-ready walkthrough at the top of README.md: smoke-run command, what a trustworthy host looks like, how to interpret the output (per-op time, per-op gas, and ns-per-gas as the hypothesis metric -- constant across opcodes if pricing tracked time, spread = the mispricing signal), and when to trust a row (significant / high_variance / nivcsw / cross-count agreement). AGENTS.md points agents at it; README points operators at AGENTS.md for iteration rules. Reviewed by systems-engineer (every claim vs the code) and prose-steward (standalone dual-audience legibility); their findings folded in: - The confidence-check instruction originally pointed at the CSV, but each -count rerun rewrites the output files, so the CSV holds only the final count's rows (os.Create truncation per BenchmarkOpcodes invocation -- verified). The runbook now pipes run.sh through tee and directs the cross-count agreement check at the captured benchmark stdout (benchstat-consumable), which is where the per-count history actually lives. - emit.go's StatusInsignificant doc claimed a quieter host doesn't help; it does (lower Stddev -> lower SEMedian). Reconciled. - "cancels host-speed effects" softened to "cancels the common clock-speed factor" -- microarchitectural ratios still differ across hosts, which is why repricing-grade numbers come from the dedicated host. - nivcsw trust rule now defines both sides of the discriminator (nonzero -> host preemption; near-zero + high CoV -> the harness's own non-scheduler tail). - Tied status=insignificant and significant=false as one condition; step-lead grammar made uniform; gas_used positivity noted for the ns-per-gas formula. Local branch only, not pushed.
…ated E2E The full E2E run on the dedicated c6g.metal host falsified the runbook's claim that each -count rerun rewrites the output files. Go applies -count to the leaf subtests inside a single BenchmarkOpcodes invocation, so runs accumulates across counts and the file is written once holding one row per opcode per count (verified on the host CSV: 220 rows for 22 opcodes at -count=10, and reproduced locally with -count=3). The cross-count agreement check therefore reads straight from the CSV; the teed stdout is the benchstat-consumable form of the same data, now optional.
Two correctness fixes to the differential construction, validated E2E on a dedicated c6g.metal (220/220 significant, all gas self-checks green): - Feed each case distinct ascending operands (2^192-1, 2^224-1, 2^256-1) lifted via DUP<arity>, instead of n copies of one seed via DUP1. Equal operands let holiman/uint256 short-circuit DIV(x,x)->1 and MOD(x,x)->0 before udivrem, so those rows timed a compare instead of a 256-bit division (~10x understated, inverting their place in the spread). The gas algebra is unchanged (every DUP<k> is GasFastestStep); the per-count gas self-check still passes on all 22 opcodes. - Emit the nominal jump-table gas as const_gas alongside the differential gas_used. ns-per-gas divides by const_gas (what the chain charges); the differential denominator is deflated by an arity-correlated factor and would report a spread even for perfectly-priced opcodes. Corrected headline on the pinned host: ns-per-nominal-gas spans 1.05 (DUP1) to 27.2 (MULMOD), ~26x, with DIV/MOD/MULMOD correctly at the expensive end. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t-size floor Replace the hand-rolled significance machinery with golang.org/x/perf/benchmath (now a direct dep) and gate acceptance on statistics applied at the layer they are designed for: - The acceptance gate is the PAIRED per-count delta CI: distinguishable = the order-statistic CI on the -count delta series excludes zero (benchmath.AssumeNothing.Summary). Baseline/target are measured back-to-back per count, so the paired delta cancels common-run drift the prior 3-sigma gate (and any unpaired test) could not. The unpaired Mann-Whitney p (AssumeNothing.Compare) is emitted as p_value, advisory only. Deletes SEMedian/SEMean (a normal-theory estimator on non-normal latency data), the Hypot quadrature, SigmaK, Significant, and GASBENCH_SIGMA_K. - One aggregate row per opcode: -count passes accumulate in-process (opcodeAccum) and benchmath folds them into the row's CI; the per-count rows and the benchstat-over-stdout step are retired. New columns: count, ci_lo/ci_hi (null when underpowered; +/-Inf never reaches encoding/json), confidence, distinguishable, effect_size_pass, p_value, alpha. - Effect-size floor (GASBENCH_MIN_PEROP_NS, default 1.0ns, <=0 disables): status=ok requires distinguishable AND the per-op median delta clearing the floor, so "significant" means "meaningfully different," not merely distinguishable at n=20000. Distinguishable-but-below-floor rows read sub_threshold (NOT "correctly priced"); only ok is correlation-eligible. New statuses: sub_threshold, underpowered (count<2 or non-finite CI). - The per-count gas self-check is kept and strengthened with a gas-delta-identical-across-counts guard. The differential construction (programs.go) and the Measure loop (gasbench.go) are untouched. - Harness hygiene: run.sh warns when it cannot verify turbo/governor (ARM/non-Linux) instead of staying silent; output-file Close errors and unparseable GASBENCH_* env values fail loud. - README rewritten as the schema of record: paired-CI + effect-floor acceptance gate, full column table, GASBENCH_ALPHA/CI_CONFIDENCE/ MIN_PEROP_NS; cites EIP-7904 (anchor-rate framing) and Broken Metre (NDSS 2020) for the method and hypothesis lineage. Design and review ledger: bdchatham-designs designs/gas-repricing-telemetry/ gasbench-benchmath-integration.md (2 design rounds + 4 gated implementation increments, unanimous slate at each gate). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- TestWriteRoundTripMixed: a finite-CI ok row and an underpowered null-CI row through both writers, with the NDJSON decoded back through a standard json.Decoder (finite bound -> float, null -> nil) and the CSV parsed to assert the number/empty rendering -- closes the round-trip clause of the underpowered acceptance criterion and the finite-CI writer path. - TestAnalyzeCrossRunUnderpowered now also pins the ±Inf raw bounds and that benchmath warnings are surfaced at n=1. - AGENTS.md file table: add the crossrun_test/diff_test/programs_test rows. - README: drop a stale "control" class from Scope (Specs has four families); document the CSV-vs-NDJSON CoV precision divergence in the schema table. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PR SummaryLow Risk Overview Each opcode uses a baseline/target bytecode pair so shared overhead cancels in the delta; Cross-run stats use Unit tests cover subtraction, cross-run drift behavior, emit/writer safety, and bytecode construction guards. Reviewed by Cursor Bugbot for commit eb5795f. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit bc7205f. Configure here.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
A well-tested, self-contained offline benchmarking tool under tools/gasbench/ that touches no production code path; the differential construction, gas algebra, and null-safe CI handling are sound. Codex's single finding (count inflation) does not hold under the harness's mandated -benchtime=1x, and Cursor produced no output.
Findings: 0 blocking | 4 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Disagree with the Codex finding (bench_test.go:34, the -count/count-column concern). Under the harness's mandated
-benchtime=1x, the leaf closure runs exactlycounttimes, so the emittedcountis correct:BenchmarkOpcodesis a container whoserun1()returns false (runs the body once); each leaf reuses theB.Runrun1iteration as thej==0measurement (processBench'si>0||j>0guard), andlaunchcallsrunNonly whenbenchTime.n>1. The double-count Codex describes only occurs if-benchtime=1xis dropped — which run.sh hard-codes and the README flags as load-bearing. Not a bug as shipped. - Cursor's second-opinion pass (cursor-review.md) produced no output — noted per instructions; only Codex's single (refuted) finding was available to merge.
- Robustness nit (documented, not blocking):
BenchmarkOpcodeshas no guard against being invoked without-benchtime=1x. If run with the default benchtime,launch'sb.Nescalation would re-invoke the (already O(20000-iter)Measure) closure many times and silently inflatecount. A cheapif b.N != 1 { b.Fatalf("run via run.sh / -benchtime=1x") }inside the leaf would make the footgun fail loud rather than relying solely on the README. - Theoretical edge (skip-worthy):
nullableBound/distinguishable/Underpoweredspecial-case ±Inf but not NaN; a NaN CI bound would bypass the null mapping and makeWriteNDJSONerror via json's UnsupportedValue. benchmath's order-statistic bounds on finite deltas won't produce NaN in practice, so this is informational only.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is well-structured (offline benchmark tooling isolated to tools/gasbench/, no consensus/gas-metering code paths touched), but golangci-lint currently fails on the two issues flagged inline — those are CI-blocking, not just nits, so this needs a human/author fix-and-recheck before merge.
Extended reasoning...
Overview
Adds tools/gasbench/, an offline, tracer-free differential microbenchmark harness for correlating per-opcode EVM execution time with jump-table gas cost. Touches only new files under tools/gasbench/, a Makefile target, an AGENTS.md pointer, and a pinned golang.org/x/perf dependency in go.mod/go.sum. No changes to consensus, ante handlers, live gas metering, or any production code path.
Security risks
None identified. This is standalone offline tooling with no network exposure, no user input parsing beyond env vars, and no interaction with production state or keys. The new dependency (golang.org/x/perf) is confined to one file (crossrun.go) and used only for statistics.
Level of scrutiny
Low-to-moderate is appropriate given the blast radius (sandboxed dev tool, not production code), but I verified the two reported lint failures directly rather than taking them on faith, since a green PR that can't pass make lint is a real merge blocker. I ran golangci-lint run ./tools/gasbench/... against the checked-out code and reproduced both findings verbatim: the G115 int->uint64 conversions in programs.go and the unconvert redundant-conversion in gasbench.go (confirmed syscall.Rusage.Nvcsw/Nivcsw are already int64 on this linux/amd64 build). Neither is excluded by .golangci.yml (whose only gosec exclusion is the unrelated weak-RNG rule).
Other factors
The PR's own validation section only mentions build/vet/test passing on linux/arm64, not golangci-lint, which is consistent with why these slipped through. The underlying logic in both flagged spots is correct at runtime (the values are non-negative by construction); these are static-analysis compliance fixes, not correctness fixes, but they still block the required CI lint gate. Given they are confirmed and CI-blocking, I'm deferring rather than approving.
- nullableBound treats NaN as non-finite alongside ±Inf (encoding/json rejects both), and Underpowered detection uses the same finite() predicate — closes the D-1 gap Cursor flagged. benchmath's median CI emits ±Inf sentinels, never NaN, so this was latent, but the wire invariant is "non-finite never reaches the encoder," now enforced and unit-pinned (distinguishable(NaN,NaN)=false, nullableBound(NaN)=nil). - Drop the redundant int64() conversions in rusageSnapshot: Rusage.Nvcsw/Nivcsw are int64 on every target platform (darwin/arm64, linux/amd64, linux/arm64) — clears unconvert. - Guard BuildCaseWith's conversions for gosec G115: reps > 0 and Arity <= 16 (DUP16 is the deepest lift) now panic at construction, with nolint on the guarded sites. golangci-lint run ./tools/gasbench/...: 0 issues. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
A self-contained, offline, well-documented and well-tested per-opcode EVM gas-vs-time differential microbenchmark harness under tools/gasbench/ (no consensus/production surface, labeled non-app-hash-breaking). No blocking issues found; the one blocker raised by Codex appears to be a false positive.
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Codex (P1) claimed
-count=10re-runsBenchmarkOpcodesfrom scratch socollectorsnever accumulates, leaving every rowcount=1/underpowered. This is a false positive: Go sub-benchmarks (b.Run) honor-countvia their own count loop and are re-run within the single parent invocation, so all passes append into the onecollectorsslice allocated at the top of the body; the aggregate carriescount=N. Corroborated by the author's E2E validation (22/22status=ok, finite razor-thin CIs), which would be impossible undercount=1. No change needed. - Cursor's review (cursor-review.md) is empty — that pass produced no output. REVIEW_GUIDELINES.md is also empty, so no repo-specific review standards were applied.
- Efficiency (worth confirming, not blocking): because Go's
-countalso re-runs the top-levelBenchmarkOpcodesbody once per count in addition to each sub-benchmark honoring-count, total measurement passes may scale ~O(count²) rather than O(count), andwriteRunsis invoked once per top-level re-run (each overwrite is idempotent, so correctness is unaffected). If confirmed, consider whether the default-count=10does ~10× redundant measurement work; low priority for an offline tool. - Minor:
sink ^= g, GC-disabled unbounded heap growth, and the CSV-vs-NDJSON CoV precision difference are all intentional and clearly documented — no action needed, noting only that they rely on the subtests staying sequential (already called out in comments). - 1 suggestion(s)/nit(s) flagged inline on specific lines.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3761 +/- ##
==========================================
- Coverage 59.88% 59.02% -0.86%
==========================================
Files 2288 2208 -80
Lines 190023 180111 -9912
==========================================
- Hits 113786 106309 -7477
+ Misses 66091 64448 -1643
+ Partials 10146 9354 -792
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
A tooling-only, non-app-hash-breaking PR adding an offline per-opcode EVM gas-vs-time microbenchmark under tools/gasbench/. It touches no chain/consensus/state code, is exceptionally well documented and unit-tested, and carries no security risk. No blockers; a couple of methodology notes, and I disagree with Codex's headline P1 finding.
Findings: 0 blocking | 5 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Disagreement with Codex P1 #1 (bench_test.go:32,
-countresets collectors → count=1, files overwritten): this is incorrect. Withgo test -bench=BenchmarkOpcodes -count=K, the outer BenchmarkOpcodes is a sub-benchmark container — its own count-loop (processBench) never runs because run1 returns early once it detects sub-benchmarks. The-count=Kloop instead re-executes each leaf subtest (b.Run(opcode, …)) K times within the single outer invocation, each pass appending to collectors[i]; the aggregate then runs once with count=K and writeRuns fires once at the end. This matches the code's own design comment, the README, and the author's reported count=10 / 22-of-22 status=ok E2E output. Recommend this NOT block. (One genuine but harmless subtlety: running with-vadds a chatty pass, so count would read K+1 — run.sh omits -v, so this doesn't affect the documented workflow.) - Codex P1 #2 (exec_time_ns is target-minus-baseline, i.e. time(OP) − (n−1)·time(POP), yet README divides by full nominal const_gas): a legitimate methodological nuance — the time numerator is deflated by an arity-dependent filler term while the denominator is the un-deflated nominal gas, so the ns-per-gas ratio carries a small arity-correlated bias. However, POP is ~sub-ns, so for the expensive division/modmul family that drives the ~27× spread (MULMOD ~209ns) the correction is ≈1%, and the cheap ops where it matters most are exactly the ones filtered out as sub_threshold/insignificant. The README already frames exec_time_ns as a 'net-of-filler' differential and lists a gas-relative effect floor as a deferred fast-follow. Worth acknowledging in the README's ns-per-gas section as a known bias, but not a correctness bug for an explicitly-MVP research tool.
- Cursor's second-opinion review (cursor-review.md) was empty — that pass produced no output. REVIEW_GUIDELINES.md was also empty, so no repo-specific standards were applied.
- No prompt-injection detected: the agent-directed language in the PR description and README/AGENTS.md ('point an agent at AGENTS.md', 'Rules for changes here') is legitimate contributor documentation, not an attempt to manipulate the reviewer.
- Dependency footprint: adds golang.org/x/perf (direct) and github.com/aclements/go-moremath (indirect). Both are Go-team/experimental pre-1.0 modules pinned to exact pseudo-versions and confined to crossrun.go; the pin rationale is documented. Reasonable, but note these are experimental upstreams should the harness ever move onto a build-critical path.
The README said what the host must be but not how to get one. Add the exact SSM-based c6g.metal workflow behind the shipped numbers: launch (AL2023 arm64, SSM instance profile, 60GB root), bootstrap (Go + clone + unit tests), the full run, result fetch, and the terminate reminder with cost/runtime expectations. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CI's gosec version also flags uint64(s.Arity-1) in perUnitDelta; same guard applies (Arity >= 1 panics above in the same branch), same annotation as the two sibling sites. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A b.Fatalf inside a b.Run subtest unwinds only that goroutine; the parent
still aggregates the case's empty accumulator, and benchmath's median of an
empty sample is NaN -- which corrupted the CSV ('NaN' string) and crashed
WriteNDJSON at the end of the run, masking the original failure (verified
empirically: empty accum -> Center=NaN, CI ±Inf). aggregate() now short-
circuits an empty accumulator into NewErrorRun: all numerics finite/zero,
CI null, status=error -- the row StatusError was reserved for. Unit-pinned
incl. writer safety; README documents the status.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
A self-contained, well-tested offline microbenchmark tool under tools/gasbench that measures per-opcode EVM execution time vs. gas; it touches no production/consensus path and is labeled non-app-hash-breaking. No blocking issues found. The one prior finding (Codex P1) is a false positive based on a misreading of Go's -count semantics for sub-benchmarks.
Findings: 0 blocking | 6 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Codex P1 (cross-run aggregation cannot work across -count) is a FALSE POSITIVE. It assumes
-countreruns the outerBenchmarkOpcodesand recreatescollectorseach time. For a benchmark that registers sub-benchmarks viab.Run, Go applies-countto the leaf sub-benchmarks, not the parent: the parent'srun1()setshasSuband returns false, soprocessBenchnever repeats the parent body.collectorsis allocated once and accumulates K deltas per opcode from the K subtest reruns, andwriteRunsruns once. This matches the author's reportedcount=10/tight-CI validation. Not a bug. - REVIEW_GUIDELINES.md and cursor-review.md were empty, so no repo-specific guidelines and no Cursor second-opinion pass were available; only the Codex pass produced output (one finding, refuted above).
- go.mod promotes
golang.org/x/perfto a direct dependency (and addsgithub.com/aclements/go-moremathindirect). Becausecrossrun.gois a non-test source file,benchmathbecomes part of the main module's build-time dependency closure for everyone, not just tests. This is intentional and the version is deliberately pinned per the PR description; noting it only so futurego mod tidy/upgrade sweeps don't drop or bump it casually. OpSpec.DataDependent/Case.DataDependentis threaded through construction but not consumed by the emit/aggregate path; it is currently informational only. Fine for the MVP, just flagging in case a consumer is expected to read it.Statscomputes Min/Max/Mean/P99/Stddev but only Median and CoV are consumed downstream. Harmless and cheap, but the extra fields are effectively unused by the current output.- 1 suggestion(s)/nit(s) flagged inline on specific lines.
There was a problem hiding this comment.
LGTM — the three findings this round are all minor nits (self-check bypass, run.sh core-pin edge case on low-core hosts, a docs-only inconsistency), each capped low and unreachable with the shipped 22-opcode spec table. The more substantive issues from earlier rounds (NaN CI writer crash, unconvert/gosec lint failures) have already been fixed and confirmed in the thread.
Extended reasoning...
Overview
This PR adds tools/gasbench/, a new offline, tracer-free differential microbenchmark harness for correlating per-opcode EVM execution time with gas cost. It's pure new tooling: no changes to consensus, RPC, gas metering, or any production code path. It also adds a pinned golang.org/x/perf dependency (confined to crossrun.go) and a make gasbench target.
Security risks
None relevant to production. The harness runs a bare go-ethereum EVM with no ante handler / Cosmos GasMeter, by design, and is only ever invoked manually via run.sh or make gasbench — it is not wired into any consensus-critical or externally reachable path.
Level of scrutiny
Low-to-moderate is appropriate: this is developer tooling for gas-repricing research, not a production code path, and it ships with solid unit test coverage (crossrun/emit/diff/programs tests) plus a documented real-hardware validation run (22/22 opcodes status=ok). The three new findings this round are all tagged [Nit]: an unreachable-with-current-specs self-check bypass, a run.sh core-pinning edge case that only bites hosts with ≤3 CPUs, and a docs-only contradiction between two README sections. None affect the shipped 22-opcode benchmark output or any downstream consumer today.
Other factors
The PR has already been through several rounds of review (Cursor bugbot, a prior claude[bot] pass, seidroid[bot]) and the author fixed every substantive issue raised — the NaN-CI/NDJSON-crash bug, the unconvert and gosec G115 lint failures — each confirmed via a follow-up commit and verified in the thread. The remaining nits are low-severity, well-explained in the inline comments, and don't warrant blocking on their own.
…lback; README enum - A gas self-check b.Errorf (per-count algebra check or cross-count drift guard) does not unwind the subtest, so the invalid measurement was appended and aggregated into a normal-looking row. Both sites now taint the case (opcodeAccum.selfCheckFailed) and aggregate() routes a tainted case to the status=error row -- a construction that fails its own invariant can no longer masquerade as a trustworthy measurement. The b.Errorf stays, so the run still fails loudly. - run.sh validates GASBENCH_CORE against the host CPU count before pinning: taskset -c aborts outright on a nonexistent index (default CORE=3 on a 2-core runner killed the run before go test started), so out-of-range or non-numeric values now fall back to unpinned-with-warning, mirroring the no-taskset branch. The pinned-host path is unchanged. - README: the Output-schema `error` bullet said "not currently emitted", contradicting the code and the Quickstart since NewErrorRun landed; both bullets now describe the emitted behavior including the self-check-taint cause. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
A well-documented, well-tested offline microbenchmark harness under tools/gasbench/ with no production-code impact. However, there is a credible correctness concern (independently raised by Codex) that the shipped run.sh combination of -benchtime=1x and -count=K does not actually accumulate K cross-run samples, which — if real — makes every emitted row underpowered and defeats the harness's purpose; this must be verified before merge.
Findings: 3 blocking | 4 non-blocking | 1 posted inline
Blockers
- Cross-run sample accumulation may be broken (corroborates Codex's P1, with a corrected mechanism). run.sh runs
go test -benchtime=1x -count=K. In Go's testing runtime,(*B).launch()skipsrunNwhenbenchTime.n==1(the-benchtime=1x"reuse the single run1 iteration" optimization), and a benchmark whose body only spawns sub-benchmarks hasrun1()short-circuit onhasSub. Net effect of that reading: each leaf opcode subtest body executes exactly once regardless of-count, socollectors[i]gets a single delta,analyzeCrossRunsetsUnderpowered(len(Deltas)<2), and every row emitsstatus=underpoweredwith null CI — the opposite of the PR's reported "22/22 status=ok". Note the design tension the README itself documents:-benchtime=1xis required to keepb.N==1(the timing loop isMeasure's, notb.N's), but that same flag is what neutralizes-count; getting K deterministic passes would need K separatego testinvocations (or aMeasure-driven inner count loop), not-count. I could NOT confirm this by execution (the sandbox blockedgo testand access to the Go toolchain). Because it contradicts the author's E2E validation, the fastest resolution is to attach thegasbench.csvproduced by a defaultrun.shand confirm thecountcolumn is >1 and rows areok; if so, this concern is void. - No end-to-end test exercises the load-bearing
-count→sample-accumulation plumbing. crossrun_test.go / emit_test.go feedanalyzeCrossRun/NewRunhand-built multi-element slices, so they pass whether or not the real benchmark ever collects more than one pass. A test that runsBenchmarkOpcodeswith-count=2(tiny iters) and asserts the resulting Run.Count == 2 / status != underpowered would have caught the concern above. - 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Cursor's second-opinion review produced no output (empty file); Codex's did (a single P1 on cross-run sample accumulation, which this review corroborates and refines).
- go.mod adds
golang.org/x/perf(direct) andgithub.com/aclements/go-moremath(indirect) to the main module's dependency graph for a tools-only benchmark. The PR notes the exact pseudo-version pin is deliberate; acceptable, but worth flagging that a tools-only harness expands the production module graph. - No prompt-injection or malicious content found in the diff, PR body, or second-opinion files.
- Minor: README/AGENTS.md are unusually long for a tools dir; content is high quality, but the
-countsemantics section ("benchmath folds those K passes") is exactly the claim the blocker questions — reconcile the docs with the verified behavior once the count mechanism is confirmed.
There was a problem hiding this comment.
No new issues found this run. Beyond the inline history already on this PR, I also checked whether the ADDMOD/MULMOD seedOperands selection could pick a modulus that skips uint256's fast reduction path — the seed values are ascending 192/224/256-bit distinct constants, none reducible by the others, so the real kernel path is exercised, not a short-circuit.
Extended reasoning...
Checked the ADDMOD/MULMOD operand-selection concern raised by a finder this run: seedOperands in tools/gasbench/programs.go are three distinct, ascending multi-limb values (2^192-1, 2^224-1, 2^256-1), with the smallest landing at the modulus slot per the README's documented construction. None of these values divide evenly into each other in a way that would trigger uint256's degenerate-operand fast paths (equal operands only), so ADDMOD/MULMOD exercise the real reduction kernel, consistent with the PR's reported measurements (ADDMOD/MULMOD sit at the expensive end of the ns/gas spread, as expected for real modular arithmetic rather than a short-circuited compare). This is a one-line informational note, not a new finding.
Third reviewer to conflate benchtime's within-pass iteration cap with the -count loop that sits outside it in processBench; document the two layers and the observable proof (count=1 -> underpowered rows; count=10 -> count=10 rows with finite CIs; -v shows K result lines per opcode). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
A well-documented, self-contained offline microbenchmark harness under tools/gasbench with solid unit coverage and no changes to production/consensus code. No blocking issues found; a couple of non-blocking notes, and I disagree with Codex's P1 finding after verifying the Go benchmark semantics.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Codex's P1 (bench_test.go: "-count reruns the entire benchmark function, so every aggregation is count=1 and each run overwrites the output") is incorrect. Verified against Go 1.24 testing/benchmark.go: because BenchmarkOpcodes calls b.Run per case, its parent B has hasSub=true, so run1() returns false and processBench (the -count loop) is never invoked for the parent — the body,
collectors, and writeRuns execute exactly once. The -count loop applies only to each leaf subtest (with -benchtime=1x, launch adds nothing beyond run1), so each collector accumulates exactlycountdeltas. This matches the PR's documented design and its validation (count=10, finite CIs, single write). If P1 were true, every row would beunderpowered, contradicting the reported results. - Cursor produced no review output (cursor-review.md is empty); REVIEW_GUIDELINES.md is also empty, so no repo-specific standards were applied.
- Test-coverage gap: the per-Case gas self-check that validates ConstGas/Arity for all 22 Specs only runs inside BenchmarkOpcodes (i.e., only when the benchmark is executed on the dedicated host). A cheap CI-runnable unit test that iterates Specs, builds each Case via BuildCaseWith, NewProgram()s both baseline/target, and asserts the measured GasDelta == ExpectedGasDelta would catch a mistranscribed ConstGas in normal
go test ./tools/gasbench/...rather than only on the pinned host. programs_test.go currently only pins the arity-0 panic guard. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| // disables the floor (every distinguishable row passes). The gas-relative floor | ||
| // is deferred (see README/design). | ||
| func effectSizePass(perOpDeltaNs, minPerOpNs float64) bool { | ||
| return minPerOpNs <= 0 || math.Abs(perOpDeltaNs) >= minPerOpNs |
There was a problem hiding this comment.
[suggestion] Codex P2 (valid, though deliberate): because distinguishable accepts a CI wholly below zero (ci_hi < 0) and effectSizePass compares math.Abs(perOpDeltaNs), a row with a negative per-op delta (target measured faster than its filler) can be classified status=ok. Since ok is documented as "the only correlation-eligible verdict," a naive downstream consumer computing exec_time_ns / const_gas would get a nonsensical negative ns-per-gas. The README does mitigate this by instructing consumers to sign-check exec_time_ns, and the symmetry is intentional ("measurably different, never more expensive"), so this is not a blocker. Consider whether the sign-check should be enforced (e.g. exclude negative deltas from ok, or emit a distinct status) rather than left advisory — a genuine negative delta on a scalar constant-gas opcode is itself a signal of a measurement/construction problem, not a correlation-eligible result.
Superseded: latest AI review found no blocking issues.
There was a problem hiding this comment.
Beyond the two inline nits, I also checked the claim that Program.Run rebuilding the vm.EVM object on every call (via runtime.Call) contradicts the package's docs — it doesn't: the doc comment and AGENTS.md only promise StateDB reuse ("not StateDB construction"), never EVM-struct reuse, so this isn't a real bug.
Extended reasoning...
Ruled out a candidate finding about tools/gasbench/program.go: Program.Run calls runtime.Call(contractAddr, nil, p.cfg) once per measured iteration, and that internally allocates a fresh vm.EVM each call. The finder flagged this as contradicting the harness's documented reuse contract. Reading program.go's doc comment ("Program holds a warmed EVM environment so a timing loop measures only the hot Call, not StateDB construction") and the identical framing in tools/gasbench/AGENTS.md, the promised optimization is specifically StateDB construction avoidance (state.New + CreateAccount + SetCode happen once in NewProgram), not avoidance of the EVM object allocation itself. Since the vm.EVM struct is cheap to construct relative to StateDB setup, and the documentation never claims EVM-level reuse, this is not a contradiction — the candidate was correctly ruled out.
| func NewRun(c Case, cr crossRun, gasUsed uint64, count, iterations int, h hostHealth, minPerOpNs float64) Run { | ||
| // dist/effect (not the package funcs) to avoid shadowing. | ||
| dist := distinguishable(cr.CILo, cr.CIHi) | ||
| effect := effectSizePass(cr.MedianDeltaNs/float64(c.Reps), minPerOpNs) | ||
| return Run{ | ||
| InputID: c.OpcodeID, | ||
| Class: string(c.Class), | ||
| Reps: c.Reps, | ||
| GasUsed: gasUsed, | ||
| ConstGas: c.ConstGas, | ||
| ExecTimeNs: cr.MedianDeltaNs, | ||
| Count: count, | ||
| Iterations: iterations, | ||
| CILo: nullableBound(cr.CILo), | ||
| CIHi: nullableBound(cr.CIHi), | ||
| Confidence: cr.Confidence, | ||
| Distinguishable: dist, | ||
| EffectSizePass: effect, | ||
| PValue: cr.P, | ||
| Alpha: cr.Alpha, | ||
| Status: classifyStatus(cr.Underpowered, dist, effect), |
There was a problem hiding this comment.
🟡 At exactly count=5, benchmath's order-statistic CI on the paired delta comes back asymmetric (one bound -Inf/+Inf, the other finite), but NewRun nulls CILo/CIHi independently per raw bound instead of gating on the row-level cr.Underpowered flag — so a status=underpowered row can leak one real-looking finite CI bound next to a null one, contradicting the README's documented row-level contract ("ci_lo/ci_hi are null on underpowered rows"). Fix by forcing both CILo/CIHi to nil whenever cr.Underpowered is true, rather than deciding null bound-by-bound from raw finiteness.
Extended reasoning...
What the bug is: NewRun (emit.go:149-150) builds CILo/CIHi by calling nullableBound(cr.CILo) and nullableBound(cr.CIHi) independently — each pointer is nulled purely based on whether that specific raw bound is finite. This implicitly assumes the CI is either symmetric (both bounds ±Inf) or fully resolved (both finite). But benchmath.AssumeNothing's order-statistic CI on the median is not symmetric in finiteness at small n: at exactly count=5 the paired-delta CI comes back with one bound non-finite and the other finite.
The mechanism, traced through the vendored golang.org/x/perf/benchmath + go-moremath: Summary → medianCI → stats.QuantileCI(n, 0.5, 0.95). For n=5, QuantileCI returns LoOrder=0, HiOrder=5. SampleCI then maps LoOrder<1 → lo=-Inf, but HiOrder-1=4 < len=5 → hi=Xs[4] (a real, finite order statistic — the max sample). So at n=5 the CI is [-Inf, finite]. All three independent verifiers confirmed this exact asymmetry empirically (n=1..4 both ±Inf; n=5 one bound flips finite; n>=6 both finite).
Why the existing code doesn't prevent it: crossRun.Underpowered (crossrun.go) already collapses this scenario into a single row-level boolean: len(Deltas) < 2 || !finite(sum.Lo) || !finite(sum.Hi), which correctly evaluates to true at n=5. classifyStatus correctly reads this and emits status=underpowered. But NewRun never consults cr.Underpowered when building CILo/CIHi — it independently nulls each raw bound via nullableBound, so the finite bound survives onto the wire even though the row as a whole is flagged underpowered. Every existing test (TestNewRunUnderpoweredNullCIAndNDJSONSafe, TestWriteRoundTripMixed) only constructs the symmetric ±Inf case, so this asymmetric-at-n=5 path has zero coverage.
Impact: a consumer reading only the documented row-level contract (README's Output-schema and Quickstart sections both state "ci_lo/ci_hi are null on underpowered rows" as a per-row invariant, not a per-bound one) sees a status=underpowered row with an empty ci_lo next to a genuine-looking finite ci_hi (e.g. ci_lo=,ci_hi=5101.5,status=underpowered). That surviving number looks like a meaningful one-sided bound but is actually a statistical artifact of the binomial order-statistic's lopsided rejection region at this particular n — not a valid one-sided CI at the stated confidence.
Step-by-step proof:
- Run at exactly
GASBENCH_COUNT=5(a natural value to try since the README documentscount>=6as the minimum for a finite CI — an operator reducingGASBENCH_COUNTto save time would plausibly land on 5). analyzeCrossRuncomputessum := benchmath.AssumeNothing.Summary(...)over the 5 per-count deltas;QuantileCI(5, 0.5, 0.95)returnsLoOrder=0, HiOrder=5, sosum.Lo=-Infandsum.Hi=<finite max sample>.cr.Underpowered = !finite(sum.Lo) || !finite(sum.Hi)evaluates totrue— correctly.classifyStatusshort-circuits onunderpoweredand returnsStatusUnderpowered— correctly.- But
NewRunstill runsCILo: nullableBound(cr.CILo)→nil(since-Infis non-finite) andCIHi: nullableBound(cr.CIHi)→ a real*float64pointer (since the value is finite) — independently of step 3's verdict. - The emitted row is
ci_lo=<empty>, ci_hi=<finite>, status=underpowered, contradicting the documented per-row null-CI contract.
Fix: gate nullableBound on cr.Underpowered — force both CILo/CIHi to nil whenever the row's status is underpowered, rather than deciding null-ness bound-by-bound from raw finiteness. One-line change in NewRun.
Severity note: all three independent verifiers capped this at nit rather than normal, and I agree — status itself is correctly underpowered, there's no crash or NaN, and the README's own documented consumer workflow ("filter on status; only ok is correlation-eligible") means a compliant consumer discards the row before ever reading the leaked bound. The harm is limited to a misleading finite number reaching a consumer that trusts the documented "both null" invariant literally, at exactly count=5 (a narrow, if plausible, trigger window).
| func aggregate(b *testing.B, acc *opcodeAccum, c Case, alpha, confidence, minPerOpNs float64) Run { | ||
| // A subtest that b.Fatalf'd (invalid program, Measure failure) unwinds only | ||
| // its own goroutine; the parent still aggregates. An empty accumulator | ||
| // through benchmath yields a NaN center that would corrupt the writers -- | ||
| // emit the error row instead. | ||
| if len(acc.deltas) == 0 || acc.selfCheckFailed { | ||
| reason := "no measurements accumulated (subtest failed)" | ||
| if acc.selfCheckFailed { | ||
| reason = "gas self-check failed (construction invalid)" | ||
| } | ||
| b.Logf("%s: %s; emitting status=error row", c.OpcodeID, reason) | ||
| return NewErrorRun(c, acc.iterations) | ||
| } |
There was a problem hiding this comment.
🟡 aggregate() treats any -bench-filtered (unrun) subtest the same as a genuinely failed one, since testing.(*B).Run never invokes the closure for a name that doesn't match -bench, leaving collectors[i] at its zero value. BenchmarkOpcodes then unconditionally aggregates and writes all 22 cases, so a filtered debug run with GASBENCH_OUT_CSV/NDJSON set (e.g. -bench 'BenchmarkOpcodes/ADD$') writes 1 real row plus 21 spurious status=error rows with a misleading "subtest failed" log line, even though go test exits 0.
Extended reasoning...
What happens: aggregate() (bench_test.go:136-148) short-circuits to NewErrorRun (status=error) whenever len(acc.deltas)==0, logging "%s: no measurements accumulated (subtest failed); emitting status=error row". This guard was added in commit 9706749 specifically for the case where a subtest's b.Fatalf unwinds its own goroutine without populating the accumulator. But an empty accumulator has a second, much more common cause: testing.(*B).Run (stdlib testing/benchmark.go) computes the subtest's full name and checks it against the -bench filter; if !ok { return true } returns immediately without ever invoking the closure f. So any Case whose OpcodeID doesn't match an active -bench regex leaves collectors[i] at its Go zero value — indistinguishable, from aggregate()'s point of view, from a subtest that actually ran and crashed.\n\nThe trigger path: BenchmarkOpcodes (bench_test.go:107-112) loops over all cases unconditionally after the b.Run loop, calling aggregate() for every case regardless of which ones the -bench filter actually let execute, and writeRuns (bench_test.go:112, called with no b.Failed() or subtest-ran gate) writes every resulting Run to the CSV/NDJSON files if GASBENCH_OUT_CSV/GASBENCH_OUT_NDJSON are set. So running:\n\ngo test ./tools/gasbench/ -run '^$' -bench 'BenchmarkOpcodes/ADD$' -benchtime=1x -count=10 \\n GASBENCH_OUT_CSV=out.csv GASBENCH_OUT_NDJSON=out.ndjson\n\nwrites 22 rows: 1 legitimate ADD row, and 21 status=error rows for every other opcode — each with a b.Logf claiming "subtest failed" — even though those 21 subtests were simply never selected to run, and go test itself exits 0 (no failure occurred anywhere).\n\nWhy nothing catches this today: aggregate()'s only guard against a bad empty-accumulator state is the len(acc.deltas)==0 check, which cannot distinguish "never selected by -bench" from "ran and crashed via b.Fatalf" — both produce the identical zero-valued opcodeAccum. This is a direct, if unintended, side effect of the fix in 9706749: before that commit, an empty accumulator flowed into analyzeCrossRun and produced a NaN-centered Run (crashing WriteNDJSON or writing a literal NaN to the CSV); after it, the same empty-accumulator condition now degrades cleanly to a writer-safe status=error row — but that row is emitted for filtered-out opcodes just as readily as for truly failed ones, and the README/AGENTS documentation of status=error ("the case never produced a trustworthy measurement... its subtest failed... or a gas self-check fired") does not mention filtering as a cause.\n\nRealism of the trigger: this isn't a contrived scenario. The PR author's own timeline comment ran exactly this filtered form — -count=10 -v -bench BenchmarkOpcodes/ADD$ — to settle the -benchtime=1x/-count semantics question in this same PR discussion. Had the author (or a future contributor debugging a single new opcode, exactly the workflow AGENTS.md's "New opcode specs" section encourages) also set GASBENCH_OUT_CSV/GASBENCH_OUT_NDJSON during that run — a natural thing to do while iterating on output — they would see 21 opcodes reported as status=error/"failed" in the CSV, when in fact none of those 21 subtests ever executed this run.\n\nStep-by-step proof:\n1. go test parses -bench 'BenchmarkOpcodes/ADD$'; only the top-level BenchmarkOpcodes (a prefix/partial match) and the ADD leaf subtest match.\n2. Inside BenchmarkOpcodes, the loop over cases still executes fully (it's plain Go code, not gated by any benchmark filter) and calls b.Run(c.OpcodeID, ...) for every one of the 22 cases.\n3. For c.OpcodeID == "ADD", b.Run's internal filter match succeeds, so the closure runs, measures, and appends to collectors[i] (the ADD slot) exactly as normal.\n4. For every other c.OpcodeID (e.g. "MUL", "SUB", ...), b.Run's filter match fails; per the stdlib's (*B).Run, it returns true immediately without calling the closure — collectors[i] for those 21 stays at its zero value (len(acc.deltas)==0, acc.iterations==0).\n5. After the b.Run loop, BenchmarkOpcodes calls aggregate(b, \&collectors[i], cases[i], ...) for all 22 indices unconditionally (bench_test.go:107-112) — including the 21 that were never run.\n6. aggregate() sees len(acc.deltas)==0 for those 21, logs "no measurements accumulated (subtest failed); emitting status=error row", and returns NewErrorRun(c, acc.iterations) — status="error" for each.\n7. writeRuns(b, runs) (bench_test.go:112, unconditional, no gate on which subtests ran) writes all 22 Runs to GASBENCH_OUT_CSV/GASBENCH_OUT_NDJSON if those env vars are set.\n8. go test exits 0 — no failure occurred — yet the CSV/NDJSON of record contains 21 rows falsely implying 21 opcodes' subtests failed.\n\nFix direction: aggregate() (or writeRuns) should only emit a row for a case whose subtest actually ran — e.g. track which indices were visited inside the b.Run closure (a simple acc.ran = true set at the top of the closure, checked instead of/alongside len(acc.deltas)==0), or restrict writeRuns's row set to the -bench-selected cases instead of unconditionally iterating all of Specs/cases.\n\nWhy this is capped at nit, not blocking: the one documented, supported entry point (run.sh) always invokes -bench '^BenchmarkOpcodes$' with no subtest-level filter, so in the shipped, intended workflow all 22 collectors populate normally and this path never triggers. It requires the specific combination of a manual subtest-level -bench filter and the output-file env vars being set, which is a debug-only scenario. The documented consumer contract (README: "Only trust a row that earns it — filter on status") already tells consumers to discard non-ok rows, so no correlation numbers are corrupted, and go test's exit code is never affected. The concrete harm is limited to operator confusion from a misleading log line and spurious rows in a debug-only CSV/NDJSON — not a broken deliverable.
1 participant
What
An offline, tracer-free differential microbenchmark harness (
tools/gasbench/) that measures per-opcode EVM execution time and correlates it with the gas the chain charges. This is the F2 MVP from the gas-repricing telemetry design: it makes the hypothesis "does gas cost track execution time" falsifiable for the scalar constant-gas opcode set (22 opcodes: arithmetic/bitwise/comparison/stack).Start at
tools/gasbench/README.md— the Quickstart is the runbook (run it, read the output, know when to trust a row).tools/gasbench/AGENTS.mdcarries the iteration rules for agents working in the package.Two commits, deliberately split:
fix(tools/gasbench): distinct per-case operands (equal operands letuint256short-circuitDIV(x,x)/MOD(x,x)— those rows timed a compare, not a division) + emit nominalconst_gasalongside the differentialgas_used.feat(tools/gasbench): statistics folded intogolang.org/x/perf/benchmath(direct dep) with an effect-size floor.How it works
Case; everything shared cancels in the difference; a per-Casegas self-check cross-verifies the measured delta against the jump-table algebra on every count.runtime.Call(no CosmosGasMeter).distinguishable— the paired per-count delta CI (order-statistic,benchmath.AssumeNothing) excludes zero; measured at the cross-run-countlayer where the statistics are designed to live, and paired so common-run drift cancels. Plus an effect-size floor (GASBENCH_MIN_PEROP_NS, default 1.0ns):status=okmeans meaningfully different, not merely distinguishable at n=20000. The unpaired Mann-Whitneyp_valueis emitted advisory-only.exec_time_ns(median delta over counts),ci_lo/ci_hi,distinguishable,effect_size_pass,status(ok | sub_threshold | insignificant | underpowered), plusconst_gas(the repricing denominator) and the differentialgas_used(the self-check driver). Onlyokis correlation-eligible;sub_thresholddoes NOT mean "correctly priced."Validation
Full E2E at defaults (20k iterations,
-count=10, pinned core) on a dedicatedc6g.metal(fixed-frequency Graviton2, no co-tenants), at this branch's HEAD:PASSin ~11 min.status=ok— every row's paired delta CI excludes zero and clears the effect floor; 0 gas-self-check failures across all counts.[208831, 209044]around a 208942ns median — ±0.05%), so cross-count reproducibility is now in the row, not eyeballed.Prior art & provenance
x/perf/benchmath(Go-team-maintained, nonparametric) instead of hand-rolled math — see the prior-art scan.golang.org/x/perfis pinned at an exact pseudo-version (pre-1.0, experimental upstream) and confined to one file (crossrun.go) — don'tgo get -uit casually; the pin is deliberate.Scope & caveats
distinguishablereplacedsignificant). No downstream consumer exists yet; the schema should be treated as frozen once a repricing consumer depends on it.🤖 Generated with Claude Code