Skip to content

feat(scripts): add SC read-path RPC probe (consistency check + load)#3751

Open
blindchaser wants to merge 4 commits into
mainfrom
yiren/rpc-sc
Open

feat(scripts): add SC read-path RPC probe (consistency check + load)#3751
blindchaser wants to merge 4 commits into
mainfrom
yiren/rpc-sc

Conversation

@blindchaser

Copy link
Copy Markdown
Contributor

Summary

  • Adds scripts/rpc-sc-read-probe.sh, a dev/testing tool for the SC (live state-commitment) read path.
  • Drives EVM JSON-RPC reads with the latest block tag so requests hit the live SC store (memIAVL / FlatKV), not the historical state store.
  • Three modes: check (cross-node, height-synchronized response consistency), monitor (looped check), and load (per-target latency/throughput with percentiles).
  • check is latency-independent: each sample is one batch of latest reads bracketed by eth_blockNumber sentinels, so reads are only compared when the node served them at a single height — works even over high-RTT links. It emits a verdict with distinct exit codes: CONSISTENT=0, MISMATCH=1, INCONCLUSIVE=2 (nothing comparable → not a pass).
  • Runner-agnostic: runs locally (python3) or in an ephemeral in-cluster pod; supports http(s)://, bare host:port, and k8s://svc|pod targets (auto port-forward for local); optional JSONL output.

Motivation

Used to validate SC read-path correctness across backends (e.g. base vs migrated/FlatKV nodes) and to benchmark read latency/throughput during the FlatKV migration.

Test plan

  • RUNNER=local MODE=check TARGETS='a=...,b=...' reports a verdict + matched/mismatched across nodes (verified local IP↔DNS: CONSISTENT; in-cluster arctic-1: 130 matched)
  • MODE=load LOAD_SC_ONLY=1 TARGETS=... prints per-method p50/p95/p99 (verified ip/DNS/k8s; RPS cap + JSONL output)
  • MODE=monitor loops and reports a non-ok round on any MISMATCH or INCONCLUSIVE check
  • RUNNER=k8s (in-cluster pod) and k8s:// targets (in-cluster DNS + local port-forward) resolve and run
  • --help prints usage

Add scripts/rpc-sc-read-probe.sh, a dev tool that sends EVM JSON-RPC
"latest"-tag read traffic at one or more Sei nodes to exercise the live
state-commitment (SC) read path (memIAVL / FlatKV) rather than the
historical state store.

Modes:
- check:   cross-target, height-synchronized consistency comparison
- monitor: loop check on an interval
- load:    per-target latency/throughput with percentiles

Runs the generator either locally (python3) or in an ephemeral in-cluster
pod, resolves http(s)/bare/k8s:// targets (auto port-forward for local),
and can emit machine-readable JSONL.
@cursor

cursor Bot commented Jul 13, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
New operational/testing script only; no changes to node, RPC server, or production code paths.

Overview
Adds scripts/rpc-sc-read-probe.sh, a bash + embedded Python tool for exercising Sei EVM JSON-RPC live state-commitment reads (latest tag) against one or more RPC endpoints.

check (default) and monitor compare cross-node consistency using height-synchronized batches: latest reads bracketed by eth_blockNumber sentinels so samples at the same height are comparable despite RTT; crossed batches are skipped. Verdicts use exit codes 0 / 1 / 2 (CONSISTENT / MISMATCH / INCONCLUSIVE), with guards against duplicate target names and misconfig masquerading as passes.

load drives per-target throughput/latency (keep-alive, warmup, optional RPS cap and duration bounds, per-method percentiles, optional LOAD_SC_ONLY). Targets support http(s)://, bare hosts, and k8s:// (in-cluster DNS or local auto port-forward). The generator runs RUNNER=local (python3) or in an ephemeral kubectl run pod; optional JSONL via OUTPUT_JSON.

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

Comment thread scripts/rpc-sc-read-probe.sh
Comment thread scripts/rpc-sc-read-probe.sh Outdated
@github-actions

github-actions Bot commented Jul 13, 2026

Copy link
Copy Markdown

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

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedJul 13, 2026, 6:27 PM

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 12444000a7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

*) spec=$entry; name=$(derive_name "$spec") ;;
esac
resolve_spec "$spec"
target_names+=("$name")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reject duplicate target names before sampling

When two target specs derive or specify the same name (for example TARGETS='http://127.0.0.1:8545,http://127.0.0.1:9545'), check mode uses those names as Python dict keys, so one worker overwrites the other's rows and the comparison later compares node_rows[name] to itself. This can print CONSISTENT even when the endpoints return different values; reject duplicate names or include the port in the derived name before appending.

Useful? React with 👍 / 👎.

# serialization/parsing (which also holds the GIL).
method, body = encode(i)
c = client()
wait_for_rate_slot()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Recheck LOAD_DURATION after the RPS wait

When LOAD_DURATION is combined with an RPS cap lower than concurrency, each worker gets an id before this throttle and can reserve a send time far beyond the deadline. For example, LOAD_DURATION=1 LOAD_RPS=1 LOAD_CONCURRENCY=5 continues for about 5 seconds and sends extra measured requests, so duration-bound load tests overrun and skew the reported throughput/latency; recheck the deadline after the rate wait or avoid scheduling slots past stop_at.

Useful? React with 👍 / 👎.

seidroid[bot]
seidroid Bot previously requested changes Jul 13, 2026

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Adds a self-contained dev/testing RPC probe script for the SC read path; well-documented and thoughtfully engineered, but the consistency checker can silently report a false CONSISTENT verdict when two targets share a name, which undermines the tool's core purpose.

Findings: 1 blocking | 6 non-blocking | 3 posted inline

Blockers

  • None at the file/PR level.
  • 1 blocking issue(s) flagged inline on specific lines.

Non-blocking

  • cursor-review.md and REVIEW_GUIDELINES.md are empty (no Cursor second-opinion pass and no repo-specific review guidelines were available); this review is based on the diff plus Codex's findings.
  • Codex flagged the JSON-RPC batch-ordering concern (line ~824) as P1, but the sentinel design (h_start == h_end) already guarantees no block boundary was crossed during the batch, so read order within the batch is irrelevant for sequential-batch servers. The only residual risk is a server that processes batch elements concurrently — worth a one-line comment noting the assumption rather than a blocker.
  • load mode defaults MAX_ERROR_RATE=0, so a single transient RPC error makes the run exit non-zero; documented, but consider a small default tolerance to avoid flaky CI-style usage.
  • No prompt-injection or malicious content found in the PR title/body/diff.
  • 2 suggestion(s)/nit(s) flagged inline on specific lines.

*) spec=$entry; name=$(derive_name "$spec") ;;
esac
resolve_spec "$spec"
target_names+=("$name")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[blocker] Target names are not validated for uniqueness. In check mode, samples are keyed by name (samples[height][name] = rows) and target_names may contain duplicates. Two targets with the same name (e.g. a=...,a=..., or two specs that derive_name maps to the same host string) overwrite each other, and the comparison loop then compares the baseline against itself — producing a silent false CONSISTENT verdict even if the two nodes actually disagree. Since this tool's whole purpose is detecting read-path divergence, please reject duplicate names here (exit with an error) so a misconfiguration can't be reported as a pass.

method, params = choose_request(i)
method_by_id[i] = method
batch.append({"jsonrpc": "2.0", "id": i, "method": method, "params": params})
batch.append({"jsonrpc": "2.0", "id": "bn_end", "method": "eth_blockNumber", "params": []})

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[suggestion] The latency-independence guarantee assumes the server processes the batch such that bn_end reflects a height >= every intervening read. This holds for sequential batch processing (go-ethereum-style), but if a server processed batch elements concurrently, bn_end could return before a read that then lands at the next height, yielding h_start == h_end while a read was served at a different height. Worth a short comment documenting this assumption (sequential in-order batch execution) so the guarantee's scope is explicit.

# serialization/parsing (which also holds the GIL).
method, body = encode(i)
c = client()
wait_for_rate_slot()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[suggestion] In duration-limited load runs, a worker reserves an RPS slot (wait_for_rate_slot advances next_send_at unconditionally) after acquiring an id but before checking the deadline in the timed region. With high concurrency and low LOAD_RPS, many slots get scheduled well past stop_at, so the run can noticeably overshoot LOAD_DURATION and inflate the request count. Consider re-checking the deadline after the rate wait (bail if stop_at has passed) so duration-bounded runs stop closer to the deadline.

@codecov

codecov Bot commented Jul 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.99%. Comparing base (9bf4af1) to head (9b815af).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3751      +/-   ##
==========================================
- Coverage   59.91%   58.99%   -0.92%     
==========================================
  Files        2288     2201      -87     
  Lines      189770   179939    -9831     
==========================================
- Hits       113695   106160    -7535     
+ Misses      65941    64447    -1494     
+ Partials    10134     9332     -802     
Flag Coverage Δ
sei-db 70.41% <ø> (ø)
sei-db-state-db ?

Flags with carried forward coverage won't be shown. Click here to find out more.
see 87 files with indirect coverage changes

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

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM — self-contained dev/testing script under scripts/, no production code paths touched.

What was reviewed

  • The one changed file (scripts/rpc-sc-read-probe.sh) end-to-end: argument parsing, target resolution (http/k8s/bare), runner dispatch (local vs ephemeral pod), Python load generator, and cross-target consistency checker.
  • Load path: rate limiter, keep-alive/reconnect, percentile computation, JSON summary, MAX_ERROR_RATE gating.
  • Check path: height-synchronized batch with eth_blockNumber sentinels, crossed-batch handling, CONSISTENT/MISMATCH/INCONCLUSIVE verdict + distinct exit codes.
  • Ruled out (per the finder pass): unstripped whitespace in TARGETS entries, PROGRESS_EVERY=0 behavior asymmetry between check/load, and CONSISTENT with 0 comparisons when REQUESTS_PER_HEIGHT=0 — the last is guarded by the comparable_heights == 0 INCONCLUSIVE branch.
Extended reasoning...

Overview

The PR adds a single new file, scripts/rpc-sc-read-probe.sh, a dev/testing tool that drives EVM JSON-RPC reads at one or more Sei nodes to (a) verify cross-node SC read-path consistency and (b) measure latency/throughput. It has three modes (check, monitor, load), supports http(s)/bare/k8s target specs, and can execute either locally (python3) or in an ephemeral in-cluster pod. No changes are made to the node, RPC server, or any production code path.

Security risks

None material. The script runs client-side only, sends only read RPCs (eth_call, eth_getBalance, eth_getStorageAt, eth_getCode, eth_getBlockByNumber, eth_getLogs, eth_blockNumber, eth_feeHistory) with the latest tag, and does not sign or submit transactions. The kubectl invocations use fixed subcommands with argument arrays (not shell-interpolated command strings). The only operator-facing risk is pointing the load mode at an endpoint operators do not own, which is standard for any load tool and squarely on the caller.

Level of scrutiny

Low. This is a self-contained script under scripts/ with no runtime interaction with the chain code. There is no consensus, state, storage, RPC handler, or crypto surface touched. A bug here can at worst produce a misleading dev report; it cannot affect nodes, validators, or users.

Other factors

The one flagged issue is a pure cosmetic nit: the LOAD summary's methods list includes eth_getTransactionCount, but the shared choose_request generator never emits it, so METHOD_COUNTS will always print eth_getTransactionCount=0. Percentiles, error rates, JSON output, and pass/fail behavior are unaffected. That is well below the threshold to block a dev-tool PR. The inline comment already records the fix options for the author.

Comment thread scripts/rpc-sc-read-probe.sh
Comment thread scripts/rpc-sc-read-probe.sh
Comment thread scripts/rpc-sc-read-probe.sh
Comment thread scripts/rpc-sc-read-probe.sh Outdated

@cody-littley cody-littley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, but I largely lack context to grok this sort of testing script, so we'll need to rely on the LLM to do the deep review work. I'm ok with this merging once the LLM gives the green light.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

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

Reviewed by Cursor Bugbot for commit 2655fe1. Configure here.

batch = [{"jsonrpc": "2.0", "id": "bn_start", "method": "eth_blockNumber", "params": []}]
method_by_id = {}
for i in range(1, REQUESTS_PER_HEIGHT + 1):
method, params = choose_request(i)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Check mode skips SC-only reads

Medium Severity

In check mode, capture_coherent builds its batch via choose_request(i) with the default full method mix, so samples can include eth_getBlockByNumber, eth_getLogs, eth_feeHistory, and extra eth_blockNumber calls—not only latest SC state reads. That diverges from the script/PR goal of validating the SC read path and from load mode’s LOAD_SC_ONLY behavior.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 2655fe1. Configure here.

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Adds a self-contained dev/testing bash+Python probe for the SC read path (consistency check, monitor, load). No production code is touched; the script is well-documented and defensive. Two minor correctness/robustness gaps worth addressing, both non-blocking.

Findings: 0 blocking | 4 non-blocking | 2 posted inline

Blockers

  • None at the file/PR level.

Non-blocking

  • REVIEW_GUIDELINES.md (base branch) and cursor-review.md are both empty, so no repo-specific guidelines and no Cursor second opinion were available for this pass. Codex's review was present and its two findings are incorporated below.
  • The check/monitor verdict's soundness depends on the RPC server executing JSON-RPC batch elements serially in request order (so a mid-batch block commit is caught by the bn_end sentinel and discarded as crossed). go-ethereum-based servers do this today, but the JSON-RPC spec permits concurrent/reordered batch execution, under which bn_start == bn_end no longer guarantees the interior latest reads were served at a single height. Worth documenting this assumption near capture_coherent so a future RPC change (or a non-geth backend) doesn't silently invalidate CONSISTENT verdicts. (Raised by Codex.)
  • 2 suggestion(s)/nit(s) flagged inline on specific lines.

"""
batch = [{"jsonrpc": "2.0", "id": "bn_start", "method": "eth_blockNumber", "params": []}]
method_by_id = {}
for i in range(1, REQUESTS_PER_HEIGHT + 1):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[suggestion] REQUESTS_PER_HEIGHT is never validated. With 0 (or a negative value), range(1, REQUESTS_PER_HEIGHT + 1) is empty, so each coherent sample carries zero reads; the comparison loop over baseline_rows never runs, yet comparable_heights still increments and the run reports CONSISTENT matched=0. A misconfiguration then masquerades as a pass. Consider rejecting REQUESTS_PER_HEIGHT < 1 at startup (exit 2), consistent with the other config-error paths. (Flagged by Codex.)

Returns (height, rows) on success, ("crossed", None) if the batch spanned a
block boundary, or (None, None) on error.
"""
batch = [{"jsonrpc": "2.0", "id": "bn_start", "method": "eth_blockNumber", "params": []}]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[nit] This bracketing scheme relies on the server processing the batch serially in request order so that any block commit during the batch shifts bn_end above bn_start and the sample is dropped as crossed. That holds for go-ethereum-derived RPCs but isn't guaranteed by the JSON-RPC spec (batches may be processed concurrently/out of order). A one-line comment stating this assumption would make the correctness dependency explicit. (Related to Codex's batch-ordering note.)

@seidroid seidroid Bot dismissed their stale review July 13, 2026 18:29

Superseded: latest AI review found no blocking issues.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM — dev-tooling script only, latest commit addresses the prior rounds of bot feedback (duplicate-name guard, pre-flight error handling, RPS deadline re-check). The two remaining findings are nits about JSONL/verdict-emission edge cases in a dev script; not merge-blocking.

Extended reasoning...

Overview

This PR adds a single new file, scripts/rpc-sc-read-probe.sh — a self-contained bash + embedded Python tool for exercising Sei EVM JSON-RPC live-SC reads. It provides three modes (check, monitor, load), supports http(s)://, bare host:port, and k8s:// targets (with auto port-forward from local runner), and emits both human-readable and optional JSONL output. No production code paths, no changes to node, RPC server, or on-chain logic.

Security risks

None material. The script is invoked manually by operators against RPC endpoints they already control; there is no untrusted input path. Environment variables are the sole configuration surface, target URLs are parsed by urllib.parse and sent to endpoints the operator explicitly specifies, and the ephemeral k8s runner pod uses a pinned public image (python:3.12-alpine).

Level of scrutiny

Low. This is a scripts/ dev/benchmarking tool — no impact on chain state, consensus, or the RPC server itself, and it can only misreport its own diagnostics. The most severe failure mode is a false CONSISTENT verdict, and the author has already added guards (duplicate name rejection, pre-flight error handling wrapped in try/except so exit codes 0/1/2 hold) in response to earlier reviewer rounds.

Other factors

  • The author has been responsive across multiple bot review passes on this PR (Cursor Bugbot, chatgpt-codex, seidroid, and my own prior runs) — the most substantive findings (duplicate target names → silent CONSISTENT; pre-flight traceback → exit 1 collision with MISMATCH; RPS scheduling overrunning LOAD_DURATION) are all addressed in the latest commit.
  • The two remaining nits (pre-flight INCONCLUSIVE bypasses CHECK_RESULT_JSON emission; REQUESTS_PER_HEIGHT=0 could emit false CONSISTENT) are both narrow edge cases in a dev tool. Author can address them as follow-up.
  • Codecov reports all modified/coverable lines covered; Buf checks green.

Comment on lines +918 to +925
if not heights:
print(
"CHECK_VERDICT verdict=INCONCLUSIVE nothing_compared=1 -- no target answered the "
"pre-flight eth_blockNumber, so consistency was NOT verified (this is not a pass).",
flush=True,
)
raise SystemExit(2)
return max(heights)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 The new pre-flight INCONCLUSIVE path in initial_height() (scripts/rpc-sc-read-probe.sh:918-924) emits CHECK_VERDICT and then raise SystemExit(2), but — unlike the end-of-run INCONCLUSIVE path at lines 1041-1057, which prints CHECK_RESULT_JSON first — it never emits a CHECK_RESULT_JSON line. Since OUTPUT_JSON is populated by sed -E -n 's/^(LOAD|CHECK)_RESULT_JSON //p' (line 1188), a round where pre-flight fails leaves no JSONL entry, while a round that reaches the sampling loop and hits INCONCLUSIVE does — an asymmetry in the JSON contract that would show up as silent gaps in MODE=monitor output. Fix by emitting a CHECK_RESULT_JSON summary (kind=check, verdict=INCONCLUSIVE, matched=0, mismatched=0, per_node_errors populated) just before raise SystemExit(2) so the two INCONCLUSIVE paths stay symmetric.

Extended reasoning...

What's wrong. CHECK_PY has two INCONCLUSIVE emit paths, and they disagree on which lines they print.

  • End-of-run INCONCLUSIVE (scripts/rpc-sc-read-probe.sh:1041-1066): sets verdict='INCONCLUSIVE', builds the summary dict, prints CHECK_RESULT_JSON <json> (line 1057), then prints the human-readable CHECK_VERDICT (lines 1059-1066), then raise SystemExit(2) at line 1081.
  • Pre-flight INCONCLUSIVE (scripts/rpc-sc-read-probe.sh:918-924): if no target answered eth_blockNumber at startup, prints only CHECK_VERDICT verdict=INCONCLUSIVE ... and then raise SystemExit(2). No CHECK_RESULT_JSON line is emitted.

How it manifests. The OUTPUT_JSON extractor at line 1188 is sed -E -n 's/^(LOAD|CHECK)_RESULT_JSON //p' "$CAPTURE_FILE" > "$OUTPUT_JSON". It matches only lines that start with LOAD_RESULT_JSON or CHECK_RESULT_JSON. The pre-flight path never emits such a line, so the round leaves nothing in the JSONL — while an end-of-run INCONCLUSIVE round does. The exit-code contract (0/1/2) is preserved on both paths; the JSONL contract (one JSON object per target (load) or per run (check) as JSONL per the script header) is only preserved on one.

Concrete impact. In MODE=monitor with OUTPUT_JSON set, if a round transiently fails pre-flight (e.g. one target briefly unreachable at round start), that round produces no JSONL entry, while a round that reaches the sampling loop and hits INCONCLUSIVE there does emit one. Downstream consumers that count JSONL lines per round or reason about verdict distributions see silent gaps rather than INCONCLUSIVE records. The human-readable log is unaffected — CHECK_VERDICT verdict=INCONCLUSIVE ... is still emitted — so an interactive operator will see the round.

Why existing code doesn't prevent it. The recent fix that wrapped initial_height() in try/except correctly handled the exit-code contract by choosing SystemExit(2) instead of letting a Python traceback exit with code 1. It just didn't extend the same symmetry to the JSON output contract; the end-of-run path composes its own summary dict just before printing CHECK_RESULT_JSON, and that composition step wasn't duplicated in the pre-flight bailout.

Step-by-step proof.

  1. Configure TARGETS='a=http://127.0.0.1:8545,b=http://127.0.0.1:9999' where :9999 has nothing listening.
  2. Run MODE=monitor MONITOR_ITERATIONS=2 OUTPUT_JSON=/tmp/out.jsonl RUNNER=local ./scripts/rpc-sc-read-probe.sh.
  3. Round 1: both block_number() calls in initial_height() fail for b (and succeed for a, so heights is non-empty — but consider the harder case where a is also briefly down: heights == []). Line 918's if not heights: branch fires; line 919-923 prints CHECK_VERDICT verdict=INCONCLUSIVE ...; line 924 raises SystemExit(2). No CHECK_RESULT_JSON line reaches CAPTURE_FILE.
  4. Round 2: both targets recover. The sampling loop runs, but if targets are still too far apart in height for a comparable sample, verdict becomes INCONCLUSIVE at line 1044. Line 1057 prints CHECK_RESULT_JSON {..."verdict":"INCONCLUSIVE"...}. CAPTURE_FILE now has one CHECK_RESULT_JSON line.
  5. After both rounds, line 1188's sed extracts exactly one JSONL entry, even though two rounds both concluded INCONCLUSIVE. A consumer computing per-round verdict distribution from /tmp/out.jsonl sees a silent gap for round 1.

How to fix. Emit a CHECK_RESULT_JSON summary at the pre-flight bailout so both INCONCLUSIVE paths write the same shape:

if not heights:
    summary = {
        "kind": "check", "verdict": "INCONCLUSIVE",
        "start_height": 0, "heights_seen": 0,
        "comparable_heights": 0, "skipped_incomplete": 0,
        "requests_per_height": REQUESTS_PER_HEIGHT, "matched": 0, "mismatched": 0,
        "targets": target_names,
        "per_node_samples": {n: 0 for n in target_names},
        "per_node_crossed": {n: 0 for n in target_names},
        "per_node_errors": {n: 1 for n in target_names},
    }
    print("CHECK_RESULT_JSON " + json.dumps(summary, separators=(",", ":")), flush=True)
    print("CHECK_VERDICT verdict=INCONCLUSIVE ...", flush=True)
    raise SystemExit(2)

Severity. Nit. This is a dev/testing script under scripts/, the human-readable CHECK_VERDICT is still emitted, the exit-code contract still holds, and the impact is limited to JSONL consumers in MODE=monitor seeing a gap rather than an INCONCLUSIVE record. Not merge-blocking, but worth a small follow-up to keep the two INCONCLUSIVE paths symmetric with the documented JSONL contract.

Comment on lines +855 to +860
for i in range(1, REQUESTS_PER_HEIGHT + 1):
method, params = choose_request(i)
method_by_id[i] = method
batch.append({"jsonrpc": "2.0", "id": i, "method": method, "params": params})
batch.append({"jsonrpc": "2.0", "id": "bn_end", "method": "eth_blockNumber", "params": []})

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 REQUESTS_PER_HEIGHT=0 (or any value < 1) trips the same misconfig-as-pass pattern the new duplicate-name guard was added to prevent: for i in range(1, REQUESTS_PER_HEIGHT + 1) at scripts/rpc-sc-read-probe.sh:855 becomes empty, so the batch contains only the two eth_blockNumber sentinels and capture_coherent returns (h_start, []). comparable_heights still increments (empty lists are still keys in node_rows) but the enumerate over the empty row list matches nothing, so the verdict falls through to CONSISTENT (exit 0) despite zero state reads having been compared. Recommend a symmetric guard next to the len(TARGETS) < 2 check — reject REQUESTS_PER_HEIGHT < 1 up front (bash and/or Python entry). Nit; requires an obviously nonsensical operator value on a dev-tooling script.

Extended reasoning...

What is wrong. In CHECK_PY at scripts/rpc-sc-read-probe.sh:855, capture_coherent builds its batch with for i in range(1, REQUESTS_PER_HEIGHT + 1). When REQUESTS_PER_HEIGHT=0 (or negative), range(1, 1) — or range(1, <=0) — is empty, so the batch contains only the two eth_blockNumber sentinels and no state reads. The response-assembly loop at line 893 is empty for the same reason, so the function returns (h_start, []) after the sentinels pass the h_start == h_end check (near-certain for a 2-request batch).

How the bug manifests. watch_worker stores samples[height][name] = [] for every target. The comparison loop's completeness gate — all(name in node_rows for name in target_names) — passes because every target is present as a dict key (mapping to an empty list). comparable_heights is incremented. Then for idx, baseline in enumerate(baseline_rows) iterates enumerate([]) zero times, so neither matches nor mismatches gets touched at that height. The verdict logic (if mismatches: MISMATCH; elif comparable_heights == 0: INCONCLUSIVE; else: CONSISTENT) sees mismatches == 0 and comparable_heights > 0 and falls through to CONSISTENT with exit code 0.

Why existing code does not prevent it. REQUESTS_PER_HEIGHT is parsed as an int but never bounds-checked in bash or Python. The pre-flight len(TARGETS) < 2 and duplicate-name guards were added specifically for this class of misconfig-silently-passing bug, but they don't cover this shape.

Impact. The tool's documented contract is that CONSISTENT means "we compared samples and everything agreed" — here the tool reports CONSISTENT/exit 0 having compared exactly zero state reads. Practical impact is small: this requires an operator to explicitly set REQUESTS_PER_HEIGHT=0 (the default is 5), which is an obviously nonsensical value; the script lives under scripts/ and is not a production code path. Same defensive-hardening pattern as the duplicate-name guard just added; symmetric fix.

Proof (step-by-step).

  1. Run MODE=check REQUESTS_PER_HEIGHT=0 TARGETS='a=http://n1:8545,b=http://n2:8545' ./scripts/rpc-sc-read-probe.sh (n1 and n2 can even return divergent state — doesn't matter, they won't be sampled).
  2. capture_coherent line 855: for i in range(1, 0 + 1) = range(1, 1) = empty. Batch = [{bn_start}, {bn_end}].
  3. Server responds; h_start == h_end is essentially guaranteed for a 2-request batch. Function returns (h_start, []) (the row-assembly loop at line 893 is also empty).
  4. watch_worker writes samples[h_start]['a'] = [] and samples[h_start]['b'] = [].
  5. Comparison loop: all(name in node_rows for name in ['a', 'b']) → True; comparable_heights += 1.
  6. baseline_rows = []; for idx, baseline in enumerate([]) iterates zero times; matches == mismatches == 0.
  7. Verdict: mismatches == 0 and comparable_heights > 0CONSISTENT, raise SystemExit(0). Tool prints CHECK_VERDICT verdict=CONSISTENT matched=0 comparable_heights=<n>.

How to fix. Reject REQUESTS_PER_HEIGHT < 1 up front, alongside the existing pre-flight checks in CHECK_PY:

if REQUESTS_PER_HEIGHT < 1:
    print("check mode requires REQUESTS_PER_HEIGHT >= 1; got %d" % REQUESTS_PER_HEIGHT, flush=True)
    raise SystemExit(2)

Optionally mirror the guard in bash for a friendlier early error. Alternatively, refuse to increment comparable_heights when baseline_rows is empty (treat empty-row heights as incomplete).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants