Add private eth_call corpus mode to the RPC benchmark workflow - #12704
Add private eth_call corpus mode to the RPC benchmark workflow#12704kamilchodola wants to merge 22 commits into
Conversation
- validate eth_blockNumber results as hex before arithmetic in wait_for_rpc - run db_fingerprint in a subshell in stop-node.sh so a fingerprint failure cannot abort teardown (it die()s internally) - docker rm -fv so anonymous volumes from stale containers are reaped too
Adds an opt-in mode (tool_config.eth_call_corpus) to jsonbench and jsonbench-sweep that makes it possible to benchmark eth_call workloads whose contents must not appear in Action logs or artifacts (e.g. privately shared call sets). Corpus files live only on the benchmark runner; jobs publish sanitized aggregate summaries and counts-only parity reports. - run-rpc-sweep.sh discovers every eth-call-corpus*.jsonl.gz on the runner as its own scenario: latency cells per rps plus a full-corpus replay per client, with the first client as the response-parity baseline. Clients (ctype@image), rps_list, and duration are free-form. Corpus cells raise the uniform RPC_GAS_CAP to 1e12 so explicit multi-billion gas fields are not clamped. - corpus_parity.py replays the corpus and compares result bytes against the baseline; calls both clients reject count as agreement; reports carry counters, client labels, and divergent record indexes (positions only). - corpus_results.py sanitizes k6 summaries to a fixed numeric schema and stages only validated aggregate files for the artifact upload. - prepare-eth-call-corpus.py converts JSONL(.gz) corpora into the JSON-array fixture json-bench consumes (its JSONL reader caps lines at ~64 KiB). - Raw tool output stays in VM scratch; node logs are scanned with the usual gates but print counts only; deep-check/HTML are disabled in corpus mode. - Behavioral unit tests for both modules and the converter; README section documenting the privacy contract and dispatch recipes; friendlier tool_config input docs; sweep summary gains avg/median columns.
|
Claude finished @kamilchodola's task in 8m 38s —— View job Review — private
|
| # | File | Issue |
|---|---|---|
| 1 | corpus_results.py:70-77 |
sanitize_data hard-fails on an absent checks metric → die → cell failure. The repo elsewhere documents that metric as optional (percat-matrix.py:88, run-jsonbench.sh:485); default it to 0 like dropped_iterations. |
| 2 | corpus_parity.py:144-148 |
Baseline state records no head/chain. Clients replay sequentially off different snapshot dirs; if one is a block off, every latest call mismatches and the job blames client divergence — indistinguishable from a real one once contents are suppressed. |
| 3 | corpus_parity.py:95-100 |
HTTPError ⊂ URLError: a JSON-RPC error delivered with a non-200 status becomes transport_failure — aborts the baseline with a counts-only message, or fails the gate on a non-divergence. |
| 4 | run-rpc-sweep.sh:95-104 |
Corpora are validated only at parity time. An 11k-record corpus (the converter has no cap; load_corpus caps at 10k) burns the whole clients × rps_list matrix first, then fails. |
| 5 | run-rpc-benchmarks.yml:619 |
The 1e12 gas cap is set in run-rpc-sweep.sh only — single-node jsonbench + eth_call_corpus:true (recipe 3 in the input docs) keeps 1e9, so the same corpus reports different failure rates per mode. |
Low
corpus_parity.py:187-189—both_rpc_errorstreats any two errors as agreement, so "reverted" vs "out of gas" passes the gate. Intentional and commented, but folding the JSON-RPC errorcodeintoERROR_MARKER(!rpc_error:-32000) would tighten it at no cost to the contract — codes are client-chosen, not corpus content.run-rpc-sweep.sh:87-92— corpus filenames become public viacorpus_label(step summary line, artifact pathscorpus/<label>/…, manifest). The README privacy section covers contents but not names; a third party naming a file after their strategy would leak that. Worth one sentence in the README, or hash the label.corpus_results.py:33—summaries.manifestis allowlisted but neither validated nor rewritten, and carries runner-absolute paths. Drop it or emit relative paths (test_stage_copies_only_validated_allowlisted_filesdoesn't cover it).run-rpc-sweep.sh:141— corpus cells register asiso|…, so the published matrix shows onlytput/p99for them while the "MIXED - overall" table is emitted as a row of dashes; the avg/median columns this PR adds never appear for corpus runs.run-rpc-sweep.sh:167+ README — docs say "the first client inclientsis the baseline"; the code uses the first client that successfully started. It's printed in the table header, but a silent fallback baseline is a weaker run than the one that was requested — consider failing instead.run-rpc-benchmarks.yml:783-788— the reference-node scan is the one branch without anETH_CALL_CORPUSguard on itshead/grepoutput. Unreachable today (corpus mode rejectscomparison=true), so purely defense-in-depth.run-jsonbench.sh:329— the plaintext fixture is removed only on the path afterdocker run; adiein between, or a cancelled job, leaves it in scratch untilcleanup.sh. Atrap … EXITmakes it unconditional. Cosmetic under the stated trust model.
Nits — PARITY_STATE/<label>.json is actually gzip; wait_for_rpc's new ^0x[0-9a-fA-F]{1,15}$ silently waits out the timeout on a ≥16-digit head (unreachable, but {1,16} costs nothing).
Verified-good elsewhere: the [[ … ]] && cmd output-suppression idiom is safe (run-rpc-sweep.sh is set -uo pipefail without -e, and in the workflow the failing test isn't the final command of the AND-list); RPC_GAS_CAP="$(… && echo …)" degrading to empty is absorbed by ${RPC_GAS_CAP:-1000000000}; forcing jsonrpc_modules=Eth doesn't break wait_for_rpc or the geth/reth arg builders (they use their own *_HTTP_API lists); corpus_results.py imports corpus_parity fine under the workflow's python3 scripts/rpc-bench/… invocation; stage's is_file() or is_symlink() ordering correctly skips symlinked files.
Testing — the three suites read as well-targeted (HTTP double per classification branch, sanitizer schema enforcement, allowlist staging, sentinel assertions). I could not execute them here: python3 -m pytest / direct invocation were blocked by this job's tool permissions, so I reviewed them statically only.
Verdict: no Critical/High. Findings 1–5 are Medium — 1 and 5 affect whether a dispatched run produces usable numbers, 2–4 affect whether a red job is attributable. None require redesign; each is a small localized change. I'd address 1, 2 and 5 before merge and take 3, 4 and the Lows as follow-ups if you prefer.
· branch feature/jsonbench-extra-eth-calls
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- corpus_parity: classify non-200 responses carrying a JSON-RPC error envelope as rpc_error instead of transport_failure (status handling is not uniform across clients/proxies); record head+chain identity in the baseline state and fail compare() with a distinct 'identity mismatch' error so a snapshot at a different block is not reported as client divergence; new 'validate' subcommand (record count only). - run-rpc-sweep.sh: validate every discovered corpus before the first node starts, turning fixture problems into seconds-long failures. - corpus_results: treat absent checks/http_req_failed like dropped_iterations (k6 omits them when nothing triggered them) instead of failing the cell. - workflow: single-node jsonbench corpus runs get the same 1e12 RPC_GAS_CAP as sweep cells, so both dispatch modes agree.
corpus_label sanitizes the filename, so two distinct corpora can collapse onto the same label and then share a parity baseline and cell directory: the second baseline overwrites the first, and every later client diffs against the wrong one. It fails safe - a count mismatch rather than false agreement - but only deep into the sweep, with a message that points at nothing real. The labels are already computed for the scenario line, so detecting a duplicate there costs nothing and turns a confusing mid-sweep failure into an up-front one.
|
Label collision addressed in Labels are now materialized once into For the record, the five earlier bot findings are already in
Also worth noting from using this in anger today: |
|
@claude re-review |
|
Claude finished @kamilchodola's task in 6m 11s —— View job Re-review — private
|
| # | Fix | Verified |
|---|---|---|
| 1 | optional_metrics = (dropped_iterations, checks, http_req_failed), defaulted to {field: 0} (corpus_results.py:70-76) |
✅ dropped_iterations keeps its exact old shape ({"count": 0}); _validate_summary in stage still accepts the defaults, so a checks-less workload degrades instead of failing the cell. http_reqs.count >= 1 still gates a genuinely empty run. |
| 2 | _node_identity() recorded in the state doc and checked in compare() before replay (corpus_parity.py:87-99, 196-203) |
✅ Numbers only, so the contract is unchanged; the check is before the replay so a misaligned snapshot costs seconds, not a full pass. Sound premise confirmed: start-node.sh:206-208/230/244 parks every client with discovery and peers off, so heads can't drift between the baseline and a later compare hours later. Error text names both heads and says what to do. |
| 3 | HTTPError split out, body read and re-classified as rpc_error on a well-formed envelope (corpus_parity.py:112-122) |
✅ Still stores no bytes. Read is bounded at MAX_RESPONSE_BYTES + 1; IncompleteRead and invalid-UTF-8 both land in the ValueError arm, so a truncated body degrades to transport_failure rather than raising. Genuine 5xx/429 still aborts the baseline. |
| 4 | validate subcommand + up-front loop before the first start-node.sh (run-rpc-sweep.sh:115-120) |
✅ load_corpus is now the single authority, and it runs before any node starts. Output is a record count only. |
| 5 | RPC_GAS_CAP on the single-node start step (run-rpc-benchmarks.yml:536) |
✅ GHA's a && b || '' yields '' when false, and start-node.sh:48 ${RPC_GAS_CAP:-1000000000} absorbs empty — so non-corpus runs keep 1e9 and both corpus modes now agree at 1e12. |
Label collision (1fc40ff) — correct and well-placed: labels are materialized once (they were already being computed for the Corpus scenarios: line), sort | uniq -d catches the duplicate, and the exit happens before rm -rf "$PARITY_STATE" and before the client loop. Your a+b/a-b example is rejected; ${b:-default} keeps the single-corpus case working.
Privacy boundary re-traced against the delta — nothing new crosses it. head/chain_id go into the scratch-only state file, not parity.json, so _validate_parity's exact-key-set check is untouched and the step-summary jq (which filters on .value | type == "number") still sees only counters. The new failure strings carry block numbers, chain ids, category names and line numbers; validate prints a count. The stage allowlist is unchanged.
Findings
No Critical, High, or Medium. Four Lows, three of them inline.
validateand the converter disagree on a legal corpus file (run-rpc-sweep.sh:117, inline) —load_corpusaccepts any filename,prepare-eth-call-corpus.py:26-31requires.jsonl/.jsonl.gz. With a free-formcorpus_glob, a.json.gzfile clears the new up-front gate and then dies inside the first latency cell — the exact late failure finding 4 was added to prevent.- The record ceiling is sweep-only (
run-jsonbench.sh:254, inline) —MAX_CORPUS_RECORDSlives inload_corpus, which the single-node path never calls; recipe (3) converts an unbounded corpus straight into the fixture. Runningvalidatein the existing-fguard keeps one authority. - Corpus filenames are public, the README says only "contents" (
README.md:313, inline, carried over) — labels reach the step summary, artifact paths and the manifest. Deliberate and useful, but undocumented for whoever hands the corpus over. Same paragraph is the natural home for the two limits you found in practice (10k records; ~142 MB fixture per 497 records). - Client labels can collide the same way corpus labels could —
run-rpc-sweep.sh:129deriveslabelfrom the image tag alone, sonethermind@a/nethermind:masterandnethermind@b/nethermind:masterboth becomenethermind_masterand share cell dirs,parity.json, and the container name. Pre-existing onmaster, not introduced here, and only reachable via thectype@imageform — but it's the same failure the guard you just added prevents on the other axis, and your own README example dispatches fourctype@imageentries.
Still open from the last round, unchanged and fine to defer: both_rpc_errors treating any two errors as agreement (commented as intentional); summaries.manifest staged without validation and carrying runner-absolute paths; corpus cells registering as iso|… so the new avg/median columns never render for them; the baseline silently falling back to the first client that started rather than the first listed; the plaintext fixture removed only on the post-docker run path.
Testing — I still could not execute anything: python3 -m pytest, python3 -m unittest discover, and even bash -n were all refused by this job's tool permissions. The three suites were reviewed statically; the new cases look correct (the HTTP double now answers eth_blockNumber/eth_chainId itself, the head-mismatch test asserts self.report.exists() is false so no partial report is written, and write_corpus(lines=…) matches the helper's signature). Adding Bash(python3 -m pytest:*) to --allowedTools would let me actually run them next time.
Verdict: the five Mediums and the collision nit are all genuinely resolved, and none of the fixes weakened the privacy contract. Nothing here blocks merge — the four Lows are follow-up material, with the validate/converter mismatch the one I'd take now since it undercuts the fail-fast guarantee that was just added.
· branch feature/jsonbench-extra-eth-calls
… path corpus_parity and prepare-eth-call-corpus disagreed on what a legal corpus is, so a free-form corpus_glob could clear validation in seconds and then die inside the first latency cell: the converter hard-requires a .jsonl/.jsonl.gz suffix and rejects NaN and friends via parse_constant, while load_corpus accepted any filename and plain json.loads. Both checks now live in load_corpus, which is the gate, and a malformed constant reports a line number rather than escaping as a traceback (JSONDecodeError is itself a ValueError, so the handler catches both). The record ceiling also only existed on the sweep path, because it lives in load_corpus and single-node jsonbench never called it - a corpus the sweep rejects in seconds was instead converted into a multi-GB fixture and handed to k6. That path now runs the same validate step next to its existing file-exists check. Finally, the privacy section covered call contents but not filenames, which become the public scenario label via corpus_label - the one thing a third party handing over a corpus would not expect. Says so now, along with the two limits that bite in practice: the 10k record cap, and a k6 fixture that scales with record count.
|
All three addressed in Reader disagreement (sweep:117). Both checks now live in Single-node ceiling (run-jsonbench.sh:254). Filename boundary (README:313). Added — the section now states that everything after the
Worth flagging from real use: I hit that fixture limit today with a 50,000-record capture (~15.2 GB fixture — unusable), which is what prompted the note. I'm running a separate feasibility study on a payload-serving layer so large corpora become usable at all; if it lands it'll be a follow-up PR here plus an upstream json-bench change, not a change to this one. |
A corpus cell could only be described as 'this rate for this long', so covering a large capture meant computing the duration by hand per rate, and the request count drifted whenever either input changed. corpus_requests states the count directly; corpus_passes states it as a multiple of the corpus's own record count, reusing the count that the up-front validate step already prints. Either derives the cell length as ceil(count / rps), which is exact because k6's constant-arrival-rate executor holds the rate - the rate asked for is still the rate applied, only the length is computed. Neither key set leaves cells on JB_DURATION exactly as before.
MAX_CORPUS_RECORDS was a hard 10k wall, so a larger capture could not be replayed at all even on a runner with the memory for it. It stays 10k by default - the replay holds every record's params in memory, so the cost should not be discovered mid-sweep - but is now raised deliberately via max_corpus_records / RPC_BENCH_MAX_CORPUS_RECORDS.
|
@claude re-review |
|
Claude finished @kamilchodola's task in 7m 13s —— View job Re-review — private
|
| Fix | Verified |
|---|---|
Readers agree on suffix + JSON constants (corpus_parity.py:71-73, 86-92) |
✅ load_corpus now rejects a non-.jsonl/.jsonl.gz name and parses with parse_constant=_reject_non_json_constant, matching prepare-eth-call-corpus.py:26-31, 35. The widened except (ValueError, RecursionError) is correct — JSONDecodeError is a ValueError, so the old handler genuinely let the rejection escape as a traceback. Body is a single json.loads, so the widening doesn't overreach. |
Single-node path gated (run-jsonbench.sh:76-80) |
✅ validate runs inside the JB_ETH_CALL_CORPUS == "true" guard, before the fixture is built, and its output is a record count. One authority, both paths. |
Filename boundary + limits documented (README.md:289-299) |
✅ States that everything after the eth-call-corpus- prefix becomes the public label reaching the step summary, parity table, artifact paths and summaries.manifest. |
Request-count sizing (4474e92) — the design is right and the plumbing is careful: the count→duration derivation keeps the rate as asked and only changes length; the record count is reused from validate's own output (awk '/^corpus OK:/ {print $3}' → $3 is the count, correct) rather than re-parsing the corpus; CORPUS_RECORDS is a genuine global (declare -A inside a top-level if, not a function); the missing-count fallback warns on stderr so it can't corrupt the captured stdout; and the constant-arrival-rate premise is corroborated in-repo by dropped_iterations being in METRIC_FIELDS. The README's coverage formula N × (1 − (1 − 1/N)^requests) is the correct expected-distinct-records figure.
Privacy boundary re-traced — nothing new crosses it. The derived duration, the no record count for <label> warning and corpus OK: N records are all counts/labels already public. stage, STAGED_FILENAMES, _validate_summary and _validate_parity are untouched. One tiny wrinkle noted inline: an uncaught UnicodeDecodeError message names an offending corpus byte.
Findings
Medium (2)
| # | File | Issue |
|---|---|---|
| 1 | corpus_parity.py:27 |
max_corpus_records re-opens the late-failure hole validate was added to close. The comment blames parity RAM, but the wall is the fixture: prepare-eth-call-corpus.py has no size bound and writes to $SCRATCH_ROOT — /mnt/sda/expb-data/rpc-bench-scratch, the same volume as the client snapshots and a running node's overlay upper dir. max_corpus_records: 50000 clears validate in seconds, then writes ~15 GB (your measurement) before failing in the first cell. Also: bare int() at import, so "50k" is a traceback reported as ::error::corpus <label> failed validation — blaming a corpus that is fine. |
| 2 | test_corpus_parity.py |
All three commits are test-free (git show --stat). Uncovered: the suffix check the commit is named for; the NaN/widened-except fix — your comment says the test was written, the diff has none, and AGENTS.md requires a regression test for a bug fix; RPC_BENCH_MAX_CORPUS_RECORDS. write_corpus(lines=…) + the existing subTest loop makes this ~4 cases. |
Low (4, three inline)
corpus_parity.py:97— the readers still disagree on two inputs: invalid UTF-8 (UnicodeDecodeErroris aValueError, and decoding happens outside the innertry) and mid-stream gzip corruption (zlib.erroris not anOSError) both escape as tracebacks, where the converter reports them cleanly.except (OSError, UnicodeError, zlib.error)closes both.run-rpc-sweep.sh:84-100—CORPUS_REQUESTS/CORPUS_PASSES/rpsare unvalidated free-form values now feeding$(( )). Underset -uo pipefail(no-e) a string, a fraction orrps=0makes the arithmetic fail →cell_duration=""→run-jsonbench.shreads empty as keep the workload's duration, so the cell silently runs at an unasked-for length. Also, "never delivers fewer than the requested count" doesn't hold once k6 drops iterations — and you already publishdropped_iterations+http_reqs.count, so achieved-vs-target could be checked.README.md:295-316— the limits paragraph added in4474e92says the 10k cap is enforced and that large captures need sampling "not a raised cap";9914525then adds the raise, undocumented here. And the headline example for the new knob (corpus_passes: 5on a 50k corpus) can't load without the undocumented knob and can't run with it.run-rpc-benchmarks.yml:699—max_corpus_recordsis sweep-only, so recipe (3) is hard-capped at 10k. The gas-cap fix last round was deliberately made symmetric; this un-symmetrises the same pair.
Nits — every corpus cell re-runs validate, re-reading and re-parsing the whole corpus clients × rates × corpora times after the sweep already validated it (seconds each now, minutes with a raised cap; a JB_ETH_CALL_CORPUS_PREVALIDATED hint would skip it). corpus_requests: 0 is accepted (// empty keeps 0 — truthy in jq) and yields 0s. CORPUS_REQUESTS silently wins over CORPUS_PASSES when both are set.
Still open from earlier rounds, all fine to defer: both_rpc_errors treating any two errors as agreement (commented as intentional); summaries.manifest staged unvalidated with runner-absolute paths; corpus cells registering as iso|… so the avg/median columns never render for them; the baseline falling back to the first client that started; client labels colliding on the image-tag axis (pre-existing on master); the plaintext fixture removed only on the post-docker run path.
Testing — blocked again: python3 -m pytest, python3 -m unittest and bash -c were all refused by this job's tool permissions, so the suites were reviewed statically only. Adding Bash(python3 -m pytest:*) to --allowedTools would let me actually run them (and confirm finding 2 empirically rather than from git show --stat).
Verdict: the three Lows from last round are genuinely fixed and none of the fixes weakened the privacy contract. The two new commits are where the findings are: max_corpus_records gives past the cheap gate without bounding the expensive step, and neither commit ships a test. Both are small, localized changes — I'd take finding 1's converter bound and finding 2's tests before merge; the Lows can follow.
· branch feature/jsonbench-extra-eth-calls
| corpus_cell_duration() { | ||
| local corpus="$1" rps="$2" target="" | ||
| if [[ -n "$CORPUS_REQUESTS" ]]; then | ||
| target="$CORPUS_REQUESTS" | ||
| elif [[ -n "$CORPUS_PASSES" ]]; then | ||
| local records="${CORPUS_RECORDS[$corpus]:-}" | ||
| if [[ -z "$records" ]]; then | ||
| echo "::warning::no record count for $(corpus_label "$corpus") — falling back to JB_DURATION" >&2 | ||
| printf '%s' "$JB_DURATION"; return | ||
| fi | ||
| target=$((records * CORPUS_PASSES)) | ||
| else | ||
| printf '%s' "$JB_DURATION"; return | ||
| fi | ||
| # Round up so the cell never delivers fewer than the requested count. | ||
| printf '%ss' "$(( (target + rps - 1) / rps ))" | ||
| } |
There was a problem hiding this comment.
Low — unvalidated free-form inputs now feed $(( )), and a bad one silently falls back to the workload's own duration.
CORPUS_REQUESTS/CORPUS_PASSES come straight from tool_config via jq -r, and rps from the free-form RPS_LIST. The script is set -uo pipefail without -e, so any of these:
corpus_requests: "250k"(string) or250000.5(fraction) → arithmetic syntax errorrps_list: "0"or a fractional rate → division by zero / syntax error
makes the printf command fail, cell_duration="", and run_cell is then called with dur="" → JB_DURATION="" in run-jsonbench.sh means keep the workload's duration. The cell runs at a length nobody asked for, with one stderr line buried in a multi-hour log, and the summary looks normal. corpus_requests: 0 is also accepted by // empty (0 is truthy in jq) and yields 0s.
Since the sweep already fails fast on labels and corpora, a matching up-front check next to the JB_ETH_CALL_CORPUS case — both knobs must match ^[1-9][0-9]*$, mutually exclusive rather than REQUESTS-wins-silently — turns all of that into an actionable one-liner.
Also, line 98's "Round up so the cell never delivers fewer than the requested count" only holds if the rate is actually sustained. With k6's constant-arrival-rate executor an unreachable rate produces dropped_iterations, and heavy corpus eth_calls at high rps is exactly when that happens — the cell then delivers fewer requests than requested with nothing saying so. You already publish both numbers (dropped_iterations.count, http_reqs.count are in METRIC_FIELDS), so comparing achieved-vs-target after the cell and emitting a ::warning:: would make the guarantee real or visibly broken.
| with self.subTest(name=name): | ||
| with self.assertRaises(corpus_parity.CorpusParityError): | ||
| corpus_parity.load_corpus(self.write_corpus(lines=lines)) | ||
|
|
There was a problem hiding this comment.
Medium — the last three commits add behavior and a bug fix with no test changes.
git show --stat 7f586ca 4474e92 9914525 touches no test file. Concretely uncovered:
- the suffix check (
load_corpuslines 71-73) — the fix the commit is named for.write_corpusonly ever producescorpus.jsonl/corpus.jsonl.gz, both legal, so nothing exercises the rejection. - the
NaN/ widened-exceptfix. Your comment says "Writing the test for theNaNcase caught a second-order bug" — the fix (parse_constant=_reject_non_json_constant+except (ValueError, RecursionError)) is in the diff, the test is not. AGENTS.md: "When fixing a bug, always add a regression test." This one is worth having permanently: the widenedexcept ValueErrornow swallows anything a future edit inside that block might raise, and the only thing keeping the line-numbered message correct is the narrow body. RPC_BENCH_MAX_CORPUS_RECORDS— module-level, so a test needsimportlib.reloadundermock.patch.dict(os.environ, …); worth pinning both the raise and the reject-bad-value path.
Three cases in the existing test_load_corpus_accepts_plain_jsonl_and_rejects_bad_records subTest loop plus one reload test covers it; write_corpus(lines=…) already gives you the NaN case for free.
| Two operational limits worth knowing before capturing: `corpus_parity.py` | ||
| enforces `MAX_CORPUS_RECORDS = 10_000`, and the k6 fixture scales with record | ||
| count (~142 MB for 497 records, since eth_call records with state overrides run | ||
| to hundreds of KB each). Large captures need sampling down to a representative | ||
| subset, not a raised cap. | ||
|
|
||
| **Corpus files** (JSON Lines, one `{"method":"eth_call","params":[...]}` per | ||
| line, extra fields ignored, optionally gzipped) go to the runner at | ||
| `/mnt/sda/expb-data/rpc-bench/eth-call-corpus[-<label>].jsonl.gz`. A | ||
| `jsonbench-sweep` with `eth_call_corpus:true` discovers **every** | ||
| `eth-call-corpus*.jsonl.gz` there and runs each as its own scenario; | ||
| single-node `jsonbench` uses the default `eth-call-corpus.jsonl.gz` only. | ||
| `corpus_dir` (sweep tool_config) overrides the directory. | ||
|
|
||
| **Sizing a cell by request count.** By default a corpus cell runs for `duration` at each | ||
| `rps_list` rate. `corpus_requests` (absolute) or `corpus_passes` (a multiple of that | ||
| corpus's record count) instead size the cell by how many requests it should issue: the | ||
| rate is unchanged and the length is derived as `ceil(count / rps)`, since k6's | ||
| constant-arrival-rate executor holds the rate. `corpus_passes: 5` on a 50k corpus at | ||
| `rps_list: "500"` is 250,000 requests over 500s. Note this is *draws with replacement*, | ||
| not a guarantee every record is visited — coverage is `N x (1 - (1 - 1/N)^requests)`. | ||
|
|
There was a problem hiding this comment.
Low — the two paragraphs added in this PR contradict the knob added in the next commit.
Line 295 (from 4474e92) states the cap as a fact — "corpus_parity.py enforces MAX_CORPUS_RECORDS = 10_000" — and closes with "Large captures need sampling down to a representative subset, not a raised cap." One commit later 9914525 adds max_corpus_records to do exactly that, and the README never mentions it. The only place it is documented is the workflow input blurb, as "raise the 10k parity guard rail", which frames it as a parity concern when the hard wall is the fixture.
Then line 313's headline example for the new sizing knob — "corpus_passes: 5 on a 50k corpus at rps_list: "500"" — is a corpus that cannot load without the undocumented knob and, per your own measurement, cannot run with it (~15 GB fixture). It's the one number a reader is most likely to copy.
Suggest: document max_corpus_records here, say plainly what raising it does and does not buy (parity RAM yes, fixture size no), and pick an example size that works today (e.g. 5k) so the doc and the enforced limits agree.
| corpus_requests="$(get '.corpus_requests')"; [[ -n "${corpus_requests}" ]] && export CORPUS_REQUESTS="${corpus_requests}" | ||
| corpus_passes="$(get '.corpus_passes')"; [[ -n "${corpus_passes}" ]] && export CORPUS_PASSES="${corpus_passes}" | ||
| # Parity holds every record in memory; raising this is a deliberate act, not a default. | ||
| max_records="$(get '.max_corpus_records')"; [[ -n "${max_records}" ]] && export RPC_BENCH_MAX_CORPUS_RECORDS="${max_records}" |
There was a problem hiding this comment.
Low — max_corpus_records is sweep-only, so the two dispatch modes disagree on the cap.
Last round's gas-cap fix was deliberately made symmetric so a corpus behaves the same under jsonbench and jsonbench-sweep. This knob re-introduces an asymmetry in the other direction: run-jsonbench.sh now runs corpus_parity.py validate on the single-node path (good), but nothing exports RPC_BENCH_MAX_CORPUS_RECORDS on the Start node / single-node steps, so recipe (3) is hard-capped at 10k with no way to raise it. Same corpus, same intent, different verdict per mode.
One line on the single-node jsonbench step, mirroring line 699, keeps the two modes agreeing. (I can't push workflow changes myself — GitHub App permissions exclude .github/workflows.)
The k6 cells cannot say which corpus record a latency belongs to: corpus mode collapses every record into one named call, so all requests carry identical tags, and the generator samples with replacement while recording only a sequential row id. Aggregate percentiles are all that path can produce. The replay path already walks the corpus in order and knows each record's index, so a timings subcommand there gets the attribution for free: it replays the corpus --passes times, paces submissions to --rps, and writes one row per record with one column per pass. Every record is hit exactly --passes times rather than a random number of times, which is what makes the rows comparable across clients and across branches. Output is record indexes and milliseconds only, so it publishes under the same boundary as the parity reports, and the achieved rate is printed so a run that could not sustain --rps is visible instead of quietly slower.
|
@claude re-review |
|
Claude finished @kamilchodola's task in 5m 19s —— View job Re-review — private
|
| # | File | Issue | State |
|---|---|---|---|
| 1 | corpus_parity.py:298 |
A failed call is written into the matrix as a latency. run_one stores elapsed and discards outcome, so a record rejected in 1.2 ms is the fastest row in the CSV. Against the commit's own goal — rows "comparable across clients and across branches" — a client that rejects record 42 in 2 ms beats one that executes it in 300 ms. The outcomes: line has the total, not the rows. The grid already encodes "no value" as "", so writing the category name (rpc_error, transport_failure — already-published vocabulary) in non-ok cells costs nothing and stays content-free. |
new |
| 2 | corpus_parity.py:312 |
Head/chain is read, printed, and thrown away. The CSV — the thing that outlives the session and gets diffed — carries no identity. This is round 1's compare() finding with a worse ending: there a snapshot skew produced a red job blamed on the wrong thing, here it produces a green comparison off two different heads that nothing can detect afterwards. A sibling <out>.meta.json (head, chain, records, passes, target/achieved rps — all numbers) closes it. |
new |
| 3 | corpus_parity.py:31 |
max_corpus_records gets past the cheap gate without bounding the expensive step. Unchanged: prepare-eth-call-corpus.py still has no size bound and still writes to $SCRATCH_ROOT on the snapshot volume, and MAX_CORPUS_RECORDS is still a bare int() at import (so "50k" is a traceback the sweep reports as corpus <label> failed validation, blaming a corpus that is fine). |
carried, unanswered |
| 4 | test_corpus_parity.py:277 |
The suffix check, the NaN regression and RPC_BENCH_MAX_CORPUS_RECORDS are still untested. b5196e9 does ship tests, so the "test-free commits" point now applies only to 7f586ca/9914525 — but the gaps are the same three, and two of them are one subTest entry each in the loop that's already there. AGENTS.md requires a regression test for the NaN fix specifically. |
carried, unanswered |
Low (4)
corpus_parity.py:331-334(inline) — three things in the executor block: no keep-alive (a fresh connection per request; 250k connections at--passes 5 --rps 500parks ~30k sockets inTIME_WAIT, past the default ephemeral range — and the resulting connect failures land in the matrix as fast latencies per finding 1);pool.mapmaterializes the whole schedule before the first request, unbounded on the same axismax_corpus_recordsjust opened; and Ctrl-C doesn't stop the run —__exit__callsshutdown(wait=True)withoutcancel_futures=True, so a 250k-request paced replay drains for ~42 minutes after the operator interrupts it.corpus_parity.pyargument handling —--concurrency 0is a bareValueErrortraceback rather than the module's exit-2CorpusParityErrorcontract; a negative--rpssilently runs unpaced;passes < 1is checked only after the whole corpus has been read.README.md:323(inline) — the example can't run:corpus_parity.pyis100644, and every other in-repo call site usespython3 scripts/rpc-bench/corpus_parity.py. The line's continuations also collapsed into runs of three spaces. More substantively, the section doesn't say that a row is the record's latency under the chosen--concurrency/--rps, not its isolated cost — which is the caveat that decides how to read the table it shows.README.md:295-299— still the pre-9914525text: statesMAX_CORPUS_RECORDS = 10_000as enforced and closes with "not a raised cap", which themax_corpus_recordsknob contradicts. Same paragraph still carries the 50kcorpus_passesexample that can't load without that knob and can't run with it (~15 GB fixture).
Still open from earlier rounds and fine to defer: load_corpus's outer except OSError letting UnicodeDecodeError/zlib.error escape as tracebacks (and naming a corpus byte); unvalidated CORPUS_REQUESTS/CORPUS_PASSES/rps feeding $(( )) with an empty-duration fallback; max_corpus_records being sweep-only; both_rpc_errors treating any two errors as agreement; summaries.manifest staged unvalidated with runner-absolute paths; corpus cells registering as iso|… so the avg/median columns never render; the baseline falling back to the first client that started; client labels colliding on the image-tag axis (pre-existing on master).
Testing — blocked again. python3 -m pytest scripts/rpc-bench, python3 scripts/rpc-bench/test_corpus_parity.py and cd … && python3 -m unittest discover were all refused by this job's tool permissions, so the suites were reviewed statically only. Adding Bash(python3:*) to --allowedTools would let me actually run them and confirm finding 4 empirically rather than from the test-method list.
Verdict: the new subcommand is a good idea implemented carefully in the parts that are hard to get right, and it doesn't weaken the privacy contract. But it publishes a latency for calls that never executed, and it drops the one piece of metadata that makes two matrices safe to compare — both small, localized changes I'd take before merge, along with the two Mediums still outstanding from last round.
· branch feature/jsonbench-extra-eth-calls
The timings replay only existed as a manual command, so it could not be used against a corpus living on the runner. It now runs per client inside a corpus sweep, writing timings.csv alongside that client's parity report, gated on timings_passes. An empty rps_list now runs no k6 cells. That combination is the only way to drive a large corpus at a high rate: the k6 path materializes one full payload per request into its CSV, so 100k requests of ~300KB records is tens of GB before the node is even touched, while the replay holds one corpus in memory and streams requests at the target rate.
| category, _ = _post(url, index, params) | ||
| except Exception: # a replay must never lose the whole matrix to one bad record | ||
| category = "transport_failure" | ||
| return (time.perf_counter() - started) * 1000.0, category or "ok" |
There was a problem hiding this comment.
Medium — a failed call is written into the matrix as a latency, indistinguishable from a successful one.
_timed_post returns (elapsed, outcome) but run_one stores only elapsed; outcome is folded into a corpus-wide outcomes tally. So a record the node rejects with a JSON-RPC error in 1.2 ms lands in the CSV as 1.200 — the fastest row in the corpus — with nothing in the file saying it never executed.
That collides directly with the stated purpose ("comparable across clients and across branches"): if client A executes record 42 in 300 ms and client B rejects it in 2 ms, the matrix reports B as ~150× faster on that row. The outcomes: ok=…, rpc_error=… line gives the total but not which rows, and at 10k records nobody reconciles that by hand. Same for a transport_failure — which, per the keep-alive note below, is exactly what a long high---rps run starts producing.
The grid already distinguishes "no value" (None → ""), so the content-free fix is small: keep numeric cells for ok only and write the category name (rpc_error, transport_failure, invalid_response — all already-published vocabulary from PARITY_COUNTER_FIELDS) in the cell instead, or add a per-record outcome column. Either keeps the CSV to indexes, milliseconds and category names.
| total_records = len(params_list) | ||
| if passes < 1: | ||
| raise CorpusParityError("passes must be >= 1") | ||
| head, chain_id = _node_identity(rpc_url) |
There was a problem hiding this comment.
Medium — the head/chain is read and then thrown away, so two matrices from misaligned snapshots compare silently wrong.
_node_identity is called here and its result reaches stdout only (at head {head} chain {chain_id}). The CSV — the artifact that survives the session and gets diffed against another client or another branch — carries no identity at all.
This is the same failure mode as review round 1's finding on compare(), which you fixed by persisting head/chain_id in the state doc and hard-failing on a mismatch. There the consequence was a red job blamed on the wrong thing; here it's worse in kind — a green run that produces a plausible-looking comparison off two different heads, with no way to detect it afterwards. Clients replay off separate snapshot dirs (snap_path()), which is exactly the condition that motivated the earlier fix.
Cheapest fix that keeps the CSV a clean CSV: write a sibling <out>.meta.json with head, chain_id, records, passes, rps, achieved_rps. All numbers, so the publish boundary is unchanged, and a later comparison script can refuse to diff two matrices whose identity disagrees.
| schedule = [(p * total_records + i, i, p) for p in range(passes) for i in range(total_records)] | ||
| with concurrent.futures.ThreadPoolExecutor(max_workers=concurrency) as pool: | ||
| for _ in pool.map(lambda a: run_one(*a), schedule): | ||
| pass |
There was a problem hiding this comment.
Low — three things about this executor block.
-
No keep-alive.
_postbuilds a freshurllib.request.urlopenper call, so every cell includes a TCP connect and every request burns an ephemeral port.baseline/compareare sequential and bounded atMAX_CORPUS_RECORDS, so they get away with it;timingsat--passes 5 --rps 500is 250k connections, and with the 60 s defaultTIME_WAITthat's ~30k sockets parked at once — past the default ephemeral range. The connect then fails, and per the comment above it is written into the matrix as a fast latency. A thread-localhttp.client.HTTPConnectionreused across calls removes both the port pressure and the per-cell connect noise the matrix exists to eliminate. -
pool.mapsubmits the whole schedule up front.scheduleand oneFutureper entry are both materialized before the first request. Fine at 10k×5; withRPC_BENCH_MAX_CORPUS_RECORDSraised (added one commit earlier) it is another unbounded allocation on the same axis. -
Ctrl-C doesn't stop the run.
ThreadPoolExecutor.__exit__callsshutdown(wait=True)withoutcancel_futures=True, so aKeyboardInterruptin the main thread blocks until every queued task drains — and each one firsttime.sleeps to its due time. A 250k-request run at--rps 100is ~42 minutes of un-interruptible drain after the operator asked it to stop.pool.shutdown(cancel_futures=True)in afinally, or a bounded submit loop, fixes it.
Also unvalidated: --concurrency 0 or negative raises ValueError: max_workers must be greater than 0 as a bare traceback rather than the module's exit-2 CorpusParityError contract, and a negative --rps falls through if rps > 0 and silently runs unpaced. (passes < 1 is checked, but after load_corpus has already read the whole file.)
| record-by-record profile, replay the corpus directly against a running node: | ||
|
|
||
| ```bash | ||
| scripts/rpc-bench/corpus_parity.py timings --corpus /mnt/sda/expb-data/rpc-bench/eth-call-corpus-<label>.jsonl.gz --rpc-url http://localhost:8545 --out timings.csv --passes 5 --rps 100 --concurrency 16 |
There was a problem hiding this comment.
Low — this invocation doesn't run, and the paragraph omits the caveat that decides how to read a row.
corpus_parity.py is mode 100644 in the index (git ls-files -s), so scripts/rpc-bench/corpus_parity.py timings … is Permission denied. Every other invocation of it in the repo — run-rpc-sweep.sh, run-jsonbench.sh, the workflow — goes through python3 scripts/rpc-bench/corpus_parity.py. Same for the line itself: the continuations collapsed into runs of three spaces, so this is one ~190-char line instead of the wrapped command it was written as.
More substantively: with --concurrency 16 a row's latency is measured with 15 other corpus calls in flight, so it is that record's cost under that load, not its isolated cost — and two matrices taken at different --concurrency/--rps aren't comparable even against the same client. Given the section's framing ("record-by-record profile", "comparable across clients and across branches"), that is the caveat a reader most needs. One sentence: use --concurrency 1 --rps 0 for isolated per-record cost; keep the concurrency fixed when comparing.
While in this section, the paragraph above at line 295-299 is still the pre-9914525 text — it states MAX_CORPUS_RECORDS = 10_000 as enforced and closes with "not a raised cap", which the max_corpus_records knob added in that commit now contradicts.
| ): | ||
| with self.subTest(name=name): | ||
| with self.assertRaises(corpus_parity.CorpusParityError): | ||
| corpus_parity.load_corpus(self.write_corpus(lines=lines)) |
There was a problem hiding this comment.
Medium (carried) — the three gaps named last round are still uncovered, and this subTest loop is where two of them belong.
b5196e9 does add tests (for timings), so the "test-free commits" observation is now only true of 7f586ca and 9914525. The specific gaps are unchanged:
- the suffix check (
corpus_parity.py:78) — the fix7f586cais named for.write_corpusonly ever emitscorpus.jsonl/corpus.jsonl.gz, so nothing reaches theraise. OnesubTestwritingcorpus.json.gzcovers it. - the
NaNregression —parse_constant=_reject_non_json_constantplus the widening toexcept (ValueError, RecursionError)at line 95. Per AGENTS.md, "When fixing a bug, always add a regression test." The widenedexcept ValueErrornow swallows anything a future edit inside thattrymight raise, and the only thing keeping the line-numbered message honest is that the body stayed onejson.loads— a test pins that.write_corpus(lines=['{"method":"eth_call","params":[NaN]}'])is one more entry in this loop. RPC_BENCH_MAX_CORPUS_RECORDS— module-level, soimportlib.reloadundermock.patch.dict(os.environ, …); worth pinning both the raise and the bad-value path (which today is a bareint()traceback at import — see the open comment oncorpus_parity.py:31).
That's three subTest entries here plus one reload test.
The sweep script skips its k6 cells when RPS_LIST is empty, but the workflow made that
unreachable: '// empty' cannot distinguish an absent key from one set to "", so an
explicit empty value was overwritten with the 100/250/500 default. A corpus run asking
for parity and timings only therefore got three k6 cells per client instead of none -
on a large corpus that is the JSON fixture plus tens of GB of generated CSV, exactly
what the request was avoiding.
Keyed on has("rps_list") now: absent means default, empty means no cells.
Same defect as the workflow layer, one level down: RPS_LIST used ${VAR:-default}, which
substitutes on empty as well as unset, so an explicitly empty value asking for no k6
cells was replaced by 100/250/500. Fixing only the workflow left the script overriding
it right back. ${VAR-default} substitutes on unset alone, which is the intent.
|
@claude re-review |
|
Claude finished @kamilchodola's task in 4m 12s —— View job Re-review — private
|
| # | File | Issue |
|---|---|---|
| 1 | run-rpc-sweep.sh:313 |
Empty rps_list → zero summaries → exit 1 with "every client failed to start", skipping the parity table, the PARITY_STATE cleanup and the failure accounting. Gate on [[ -n "${RPS_LIST// /}" ]] instead of on cell count. |
| 2 | corpus_results.py:33 |
timings.csv isn't on the allowlist and corpus mode disables the normal upload, so every matrix the sweep produces is dropped. Add it with a validator (header shape, cells empty-or-finite, rows ≤ MAX_CORPUS_RECORDS). |
Medium (4, all carried and unanswered)
| # | File | Issue |
|---|---|---|
| 3 | corpus_parity.py:298 |
A failed call is written into the matrix as a latency — a record rejected in 1.2 ms is the fastest row. Now materially worse: timings is no longer a manual command, it runs unattended per client × corpus, and the k6 cells that would have shown the failure rate are exactly what rps_list: "" turns off. |
| 4 | corpus_parity.py:312 |
Head/chain is read, printed and discarded; the CSV that outlives the session carries no identity. Same escalation — the matrices are now produced automatically and compared later. A sibling <out>.meta.json (numbers only) closes it. |
| 5 | corpus_parity.py:31 |
max_corpus_records clears the cheap gate without bounding the expensive step (prepare-eth-call-corpus.py has no size bound and writes to $SCRATCH_ROOT on the snapshot volume); bare int() at import still reports a bad value as "corpus <label> failed validation". Partly mitigated on the new path — rps_list: "" skips the fixture entirely — but unchanged for every other dispatch. |
| 6 | tests | cd2c735/bd16bdd add no tests, and the three gaps from earlier rounds (suffix check, NaN regression, RPC_BENCH_MAX_CORPUS_RECORDS) are still open. Finding 2 in particular is a one-line test_corpus_results.py case — a timings.csv under the output root is staged — that would have caught it. |
Low (3)
run-rpc-sweep.sh:226-234(inline) — the three new knobs are unvalidated free-form values checked at the latest possible moment (after every k6 cell and the parity replay, then again per client × corpus);CORPUS_TIMINGS_RPSdefaults to0, i.e.{"timings_passes":5}alone is an unpaced flood at concurrency 16 with a fresh TCP connection per request; and a failed replay incrementscell_fail, so the job dies with "N load-test cell(s) failed" when no cell failed — or, withrps_list: "", when none ran.README.md— not touched by either commit.timings_passes/timings_rps/timings_concurrencyand the empty-rps_listrecipe exist only in the workflow input blurb, while the "Per-record timings" section at line 317 still presents the replay as a manual command. The limits paragraph at 295-299 is still the pre-9914525text ("not a raised cap"), and its 50kcorpus_passesexample still can't run.run-rpc-benchmarks.yml:726-729— the timings knobs are sweep-only. Consistent withmax_corpus_records, inconsistent with the gas cap that was deliberately symmetrised; worth deciding once which way that pair goes.
Still open from earlier rounds, fine to defer: no keep-alive / unbounded pool.map / non-interruptible ThreadPoolExecutor in timings; load_corpus's except OSError letting UnicodeDecodeError/zlib.error escape (and naming a corpus byte); unvalidated CORPUS_REQUESTS/CORPUS_PASSES/rps feeding $(( )); both_rpc_errors treating any two errors as agreement; summaries.manifest staged unvalidated with runner-absolute paths; corpus cells registering as iso|…; the baseline falling back to the first client that started; client labels colliding on the image-tag axis (pre-existing on master).
Testing — blocked again: python3 -m unittest discover -s scripts/rpc-bench … was refused by this job's tool permissions, in both the cd-and-run and single-command forms. Static review only. Adding Bash(python3:*) to --allowedTools would let me actually run the suites.
Verdict: the sweep-side wiring is correct where it's easy to get wrong (validation ordering, the node still being up, has() vs // empty), but the feature these two commits add cannot currently deliver its output: the run exits 1 with a wrong message and the matrix never reaches the artifact. Both fixes are a few lines each. Findings 3 and 4 also get sharper now that timings runs unattended — an automatically-produced matrix that mixes failures into latencies and carries no head identity is harder to catch than one an operator ran by hand.
· branch feature/jsonbench-extra-eth-calls
…s' into feature/jsonbench-extra-eth-calls
| "dropped_iterations": ("count",), | ||
| } | ||
| # Filenames stage will publish; everything else in the output tree is left behind. | ||
| STAGED_FILENAMES = ("summary.json", "parity.json", "jsonbench-summary.md", "summaries.manifest") |
There was a problem hiding this comment.
High — timings.csv is not on the allowlist, so the matrix the sweep now spends its whole run producing never leaves the runner.
cd2c735 writes it to $OUT_DIR/corpus/<clabel>/<label>/timings.csv (run-rpc-sweep.sh:231), right next to parity.json. But timings only ever runs in corpus mode, corpus mode disables Upload benchmark results (run-rpc-benchmarks.yml:851), and stage copies only STAGED_FILENAMES — so the CSV is left in OUT_DIR on the runner and dropped. Nothing renders it into the step summary either (the parity table at run-rpc-sweep.sh:318 covers parity.json only), so the only visible output of a timings replay is the three print lines in the job log — which are per-corpus totals, not the per-record matrix that is the entire point.
Concretely: dispatch the recipe the commit message describes (rps_list: "", timings_passes: 5), wait for the replay across every client × corpus, and the artifact contains no timings at all.
The file is content-free by construction (record indexes and milliseconds), so it belongs on the allowlist with a validator alongside _validate_summary/_validate_parity — header must be record_index,pass_1_ms,…, every cell empty or a finite non-negative float, row count bounded by MAX_CORPUS_RECORDS. That keeps the "a leak needs both a new filename and passing validation" property that makes this allowlist worth having.
| for corpus in "${CORPORA[@]}"; do | ||
| clabel="$(corpus_label "$corpus")" |
There was a problem hiding this comment.
High — an empty rps_list (the combination the last two commits exist to enable) always fails the sweep, with a wrong message, and skips the parity table. (the defect is at line 313; anchored here because that line isn't in the diff)
bd16bdd made rps_list: "" reachable so a corpus run can ask for parity and timings only. On that path the for rps in $RPS_LIST loop body never executes, so the corpus branch's only SUMMARIES+= (line 202) never fires and ${#SUMMARIES[@]} is 0. Control then reaches line 313:
else
echo "No cell summaries produced — every client failed to start." >> "$sink"; exit 1
fiwhich:
- writes "every client failed to start" — false; every client started, captured/compared parity and ran its timings matrix,
exit 1s before the## Corpus parityblock at line 318, so the parity table — the run's actual result — never reaches the step summary,- exits before
rm -rf "$PARITY_STATE", leaving the baseline response state on the scratch disk until the next sweep'scleanup.sh, - bypasses the
failaccounting at line 353, so this and a genuineparity_failare indistinguishable in the exit code.
Staging is if: always(), so an artifact still appears from parity.json — but only with ≥2 clients. A single-client rps_list: "" run stages nothing (stage raises no publishable result files found), the upload is skipped on outcome == 'success', and the run leaves no output at all.
Gate on intent rather than on cell count — treat zero summaries as fatal only when cells were actually requested ([[ -n "${RPS_LIST// /}" ]]), otherwise print "no k6 cells requested" and fall through to the parity table and the fail accounting.
| fi | ||
|
|
||
| if [[ -n "$CORPUS_TIMINGS_PASSES" ]]; then | ||
| tdir="$OUT_DIR/corpus/${clabel}/${label}"; mkdir -p "$tdir" | ||
| echo "-- TIMINGS ${clabel}: ${label} (${CORPUS_TIMINGS_PASSES} passes @ ${CORPUS_TIMINGS_RPS} rps) --" | ||
| if ! python3 "$here/corpus_parity.py" timings \ | ||
| --corpus "$corpus" --rpc-url "http://localhost:8545" \ | ||
| --out "$tdir/timings.csv" --passes "$CORPUS_TIMINGS_PASSES" \ | ||
| --rps "$CORPUS_TIMINGS_RPS" --concurrency "$CORPUS_TIMINGS_CONCURRENCY"; then |
There was a problem hiding this comment.
Low — the three new knobs are unvalidated and fail at the latest possible moment, and the default is an unpaced flood.
CORPUS_TIMINGS_PASSES / _RPS / _CONCURRENCY come straight from tool_config with no shape check. This block is the last thing in the innermost loop, so timings_passes: "5x" fails argparse only after every k6 cell and the whole parity replay for that client have already run — and then repeats per client × corpus. The sweep already validates labels and corpora up front (lines 144-158); one ^[1-9][0-9]*$ check next to the JB_ETH_CALL_CORPUS case at line 133 fits the same pattern. Same for --concurrency 0 / a negative --rps, which today are a bare traceback and a silent unpaced run respectively.
Two smaller things:
CORPUS_TIMINGS_RPSdefaults to0= unpaced (line 47). So{"timings_passes": 5}alone runs the corpus flat out at concurrency 16 — with a fresh TCP connection per request (see the open comment oncorpus_parity.py:334), which is exactly where connect failures start appearing in the matrix as fast latencies. A default that paces, or requiringtimings_rpswhenevertimings_passesis set, would make the accidental case the safe one.- attribution: a failed replay increments
cell_fail, so the job dies with "N load-test cell(s) failed — the matrix is incomplete" when no cell failed (and, withrps_list: "", when no cell ran at all). Atimings_failcounter with its own message costs three lines and matches the existingparity_fail/stop_failsplit.
A 50k-record replay is a single sequential loop that printed only on entry and exit, so an operator watching the job saw one line and then ~17 minutes of nothing, with no way to tell a slow node from a hung one. Baseline and compare now emit a count, rate and ETA every 2000 records, and load_corpus reports its own duration when decompressing and parsing takes more than a few seconds - on a multi-GB corpus that alone is minutes.
The replay issued one request at a time, so wall clock was records x per-call latency and the node sat ~99% idle: 50k heavy eth_calls took ~17 minutes per client to do a few seconds of work, and dominated a corpus sweep. At 497 records it cost 10 seconds and never warranted attention. eth_call is read-only and deterministic against a parked head, so replaying concurrently cannot change what any record returns - only how fast the set is collected - and results are stored by index, so completion order does not matter. Baseline and compare now share one indexed thread-pool replay, 16 in flight by default (RPC_BENCH_PARITY_CONCURRENCY). Measured 10x on a 20ms-per-call node with byte-identical results, taking 50k records from ~17 min to ~2 min per client.
Replaying concurrently introduces a failure mode the serial loop could not have: a node under load can drop or truncate a response, and that is indistinguishable from a real divergence in the report. A correctness gate that invents defects under load is worse than a slow one. Two serial re-runs now stand between the concurrent replay and the report. Any record whose outcome was transport_failure or invalid_response is re-run unloaded, and in compare any record that disagrees with the baseline at all is re-verified the same way - cheap, because disagreements are rare. A record that only fails while requests overlap is a load artifact; one that reproduces unloaded is a defect. rpc_error is deliberately not retried: a captured corpus legitimately contains calls that fail at the pinned head. Both directions are covered by tests: a node failing only while overlapping still reports 30/30 matched and clean, while a client returning reproducibly wrong results for two records is still caught with exactly those indexes.
stage() copies an allowlist of aggregate files, and timings.csv was never added to it when the timing matrix was wired into the sweep - so a completed run produced the matrix on the runner and then threw it away at upload. It is staged now, behind a validator that admits only a record_index column, pass_N_ms columns, and numeric or empty cells, so the same boundary that keeps call contents out of the parity reports applies here.
The matrix held durations only, with success counts printed once to stdout and stored nowhere. Anything reading timings.csv therefore could not tell a fast answer from a fast failure - and a node shedding load posts excellent percentiles, because rejected calls return early and pull every percentile down. A 500 rps run where a quarter of the calls were rejected read as a decisive win. Each pass now writes a status beside its duration, JSON-RPC error codes are kept (an integer code is protocol metadata, not call content, and is what separates 'this call reverts' from 'the node is rejecting work'), and a run with any non-ok outcome prints an explicit warning that its percentiles are not comparable. The staging validator accepts the status column against a fixed vocabulary plus optional code, so content still cannot ride out in it.
A parity report says how many records diverged and which, never how. Diagnosing a cross-client disagreement then means re-running the calls by hand, and the divergence indexes are capped at 200 so a larger disagreement cannot even be enumerated. The cap is now configurable, and parity_diffs opts in to a word-by-word characterisation of every content mismatch: response lengths, which 32-byte words differ, their values, and the numeric delta between them - a fee- or balance-shaped difference shows up as a clean integer gap. It is derived from response bytes, so it stays off unless asked for, and it records a bounded structural summary rather than whole responses.
The characterisations were computed and appended, and then never written: the patch that added the output block failed partway and only its CLI half was repaired, so a run with parity_diffs enabled produced the analysis in memory and dropped it. Also wires the divergence-index cap as a workflow input - it was made configurable but had no input, so it silently stayed at 200 and could not enumerate a larger disagreement. Covered end to end: a client short by exactly gas*gasPrice on two records now yields a diffs file naming those records and reporting the delta.
…me cap Staging runs in a separate process from the replay, so an env-tuned RPC_BENCH_MAX_DIVERGENCE_INDEXES is invisible to the validator: it fell back to the 200 default and rejected a report that had legitimately enumerated 555. The whole run was discarded at upload after the analysis had already been produced. A report cannot have more divergences than records, which is the real invariant and one that holds regardless of how the producer was configured.
2 participants
Changes
eth_callcorpus mode (opt-in viatool_config.eth_call_corpus) forjsonbenchandjsonbench-sweepinrun-rpc-benchmarks: makes it possible to benchmark call workloads whose contents must not appear in Action logs or artifacts (e.g. call sets shared privately by a third party). Corpus files live only on the benchmark runner; jobs publish sanitized aggregate summaries and counts-only reports.run-rpc-sweep.shdiscovers everyeth-call-corpus*.jsonl.gzon the runner and runs each as its own scenario — latency cells perrps_listentry plus a full-corpus replay per client. Clients (ctype@image), rates, and duration are free-form workflow inputs.clientsis the baseline;corpus_parity.pyreplays the whole corpus against every later client and compares result bytes. Calls that both clients reject count as agreement; any one-sided defect or byte mismatch fails the job. Reports contain counters, client labels, and divergent record indexes only (positions, not contents), so the corpus owner can identify diverging calls in their own copy.summary.jsonis rewritten to a fixed numeric schema bycorpus_results.py; node logs run through the usual Exception / invalid-block / shutdown gates but print match counts only; the artifact is assembled by an allowlist staging step.prepare-eth-call-corpus.pyconverts JSONL(.gz) corpora into the JSON-array fixture json-bench consumes (its JSONL reader caps lines at ~64 KiB).RPC_GAS_CAPto 1e12 so records with explicit multi-billiongasvalues are not clamped into artificial failures.tool_configinput docs (copy-paste dispatch recipes), sweep summary gains avg/median columns, plus small robustness fixes in the node lifecycle scripts (hex validation inwait_for_rpc, teardown-safe fingerprinting,docker rm -fv).Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Behavioral unit tests for
corpus_parity.pyandcorpus_results.py(19 tests: replay/classification against a local HTTP double, sanitizer schema enforcement, allowlist staging, leak-sentinel assertions) plus the existing converter suite. The mode was also validated end-to-end on thereproducible-benchmarksrunner: multi-corpus sweeps across several client/image combinations and rates, including runs where the parity gate correctly failed the job on genuine response divergence.Documentation
Requires documentation update
Documented in-repo:
scripts/rpc-bench/README.mdgains a "Private eth_call corpus" section covering the privacy contract, corpus file conventions, and dispatch recipes.Requires explanation in Release Notes
Remarks
The privacy boundary is the Action's logs and artifacts — the runner and the benchmarked client images are trusted, as documented in the README. Non-corpus benchmark paths are unchanged.