Skip to content

feat: R2-first witnesses on the validator, and a chain-anchored frontier band in both binaries - #221

Merged
flyq merged 11 commits into
mainfrom
liquan/feat/validator-r2-then-rpc
Sep 20, 2026
Merged

flyq merged 11 commits into
mainfrom
liquan/feat/validator-r2-then-rpc

Conversation

@flyq

@flyq flyq commented Sep 16, 2026

Copy link
Copy Markdown
Member

Summary

Turns on the R2 witness route for the validator, and makes configuring an R2 target the only switch that turns it on. With a target configured every witness fetch tries the bucket before the --witness-endpoint RPC chain, and any R2 failure hands that one block to the chain instead of stalling it. With no --r2-* flag set, witnesses come from RPC alone, exactly as today. Bulk history then streams from R2 at object-storage parallelism over HTTP/2 while the RPC gateway only ever sees the blocks R2 could not serve.

Root cause

The custom-domain / HTTP/2 / --r2-connections transport already reached the validator in #187, but only behind --witness-source r2, which had no fallback: a pre-upload frontier miss, a throttle or a corrupt object was re-enqueued until R2 served it. The trace server pairs the same transport with the RPC chain (#182), so R2 speeds up the common case there without becoming a single point of failure. The validator had no equivalent, so turning R2 on for speed meant turning the RPC safety net off.

Fix

  • The R2 flags are the switch; --witness-source is gone. Every rule the mode flag gated was already a rule about the flags themselves, so build_r2_transport (bin/stateless-validator/src/app.rs:431) builds a transport when validate_r2_flags selects a target and nothing when it does not. --witness-endpoint is now always required (bin/stateless-validator/src/app.rs:415), being either the only witness path or the fallback behind R2.
  • Any R2 failure falls back (bin/stateless-validator/src/chain_sync.rs:62). The R2 client records and logs the failure, so the witness fetch stays as infallible to the pipeline as the RPC-only path always was.
  • The fast path is bounded per block. A failure surfaces at once on a 3-attempt budget (bin/stateless-validator/src/r2_witness.rs:50), and the whole stage — concurrency-permit wait plus every attempt — shares one --rpc-per-attempt-timeout-ms (bin/stateless-validator/src/r2_witness.rs:71, wired at bin/stateless-validator/src/app.rs:325). The attempt count alone did not bound it: an endpoint that accepts connections and then stalls spent a full per-attempt timeout on every try, about a minute per block at the default, and blocks queued behind the cap waited through several such holders. The decode stays outside that budget, since abandoning it would only re-fetch and re-decode the same witness over RPC.
  • Removed with the R2-only mode: the R2FailurePolicy enum, the deterministic-failure and exhausted-retry pauses that paced the pipeline's blind re-enqueue, and the pre-split concurrency-cap guard whose whole premise was the missing fallback.
  • A missing is split by band, through one shared rule anchored on the chain. R2Band and r2_band live in stateless-common (crates/stateless-common/src/r2_witness.rs:49, classified by crates/stateless-common/src/r2_witness.rs:66) rather than once per binary, and the validator asks them whether a failure is a routine near-tip miss (bin/stateless-validator/src/r2_witness.rs:92). Those misses are counted on their own r2_witness_frontier_misses_total (bin/stateless-validator/src/metrics.rs:111), so r2_witness_errors_total stays an error rate and its kind="missing" means a hole in objects that must exist — which matters because a tip-following validator produces essentially nothing but frontier misses.
  • Two diagnostics the validator gains by validating its R2 flags on every startup: an orphaned tuning flag, and a blank env value. Accepted silently, either would run the RPC-only path while the operator believed R2 was on.
  • Carrying only the pre-split --witness-max-concurrent-requests warns (bin/stateless-validator/src/app.rs:459). It sizes the RPC witness path alone, so that configuration leaves R2 uncapped — and the fetcher cannot flag that itself: with no cap there is no per-connection share to compare against the edge's stream limit, so the queueing happens inside the HTTP/2 connection where it is invisible and still spends the per-attempt budget.

Shared with the trace server

  • validate_r2_flags returns the values its rules proved rather than only naming the target (crates/stateless-common/src/r2_args.rs:106), and each arm binds them in the same let that proves them, so nothing is asserted twice. The in-flight cap rides on that verdict too: the rules check it against the connection count, so the transport has to be built with the cap they checked, and they name an orphaned cap themselves rather than each binary listing it as a tuning flag.
  • R2WitnessTransport::from_config (crates/stateless-common/src/r2_witness.rs:263) is the single place either binary turns that verdict into a transport, publishing what it built through the new R2Metrics trait (crates/stateless-common/src/r2_witness.rs:178), which mirrors how RpcMetrics already works for the RPC client.
  • The frontier band's arithmetic is shared too, not just its width. The two binaries had already drifted on it: at exactly tip - 32 the trace server said "hole" and the validator said "frontier", each pinned by a test whose comment contradicted the other's. r2_band is now one rule with the trace server's shipped convention, so the block that flips is the validator's, whose label this PR introduces and has never shipped.
  • And they now anchor it on the same thing: the chain head. Both serve witnesses out of the same bucket, filled by the same uploader, so the question — has the uploader had time to reach this block — is about the chain, not about how far a given reader has ingested. The trace server bands against the higher of DataProvider::tip_hint (bin/debug-trace-server/src/data_provider.rs:970) and its local DB tip (bin/debug-trace-server/src/data_provider.rs:1314), so no new upstream call is needed. Neither is the chain tip alone — tip_hint is raised only by by-number and tag resolutions, the DB tip only by what sync has ingested — and each leads the other in a different mode; both are bounded by the real chain, so the maximum is the better estimate and still bands a miss on the safe side.
  • And the rule that reads the band is shared now too, which the band consolidation had stopped one line short of: R2WitnessError::is_frontier_miss (crates/stateless-common/src/r2_witness.rs:133) replaces the conjunction each binary used to spell for itself. Same shape that had already drifted once on the edge — the trace server's spelling is covered by a shared test for the first time, and the validator's duplicate test is gone rather than trimmed.
  • That retired the third band, and with it a real blind spot. AboveTip existed only because the DB tip could lag the chain without bound, and it was suppressing holes: through a catch-up, every genuine gap between the DB tip and the chain head went to kind="missing_above_tip" instead of the alarm. A block 500 seconds below the head is overdue whatever this process has ingested. R2Band is two variants now and missing_above_tip is gone.
  • Together that retires eight expect()s restating checks made elsewhere and two copies of a rule about which flags each target requires. The S3 secret on the verdict is a RedactedSecret, so its Debug is derived rather than hand-written. What stays per binary is what genuinely differs: the trace server's startup wording, its per-band budget share — a speculative eighth of the stage in the frontier against half past it, where R2 is the primary source — and how each keeps frontier misses off the error counter, the trace server on the per-source series it already labels by band and the validator on a counter of its own.
  • The --r2-* flag declarations stay per binary. A macro over the env prefix does work (clap accepts env = concat!(…)), but the doc comments are the --help text and several genuinely differ; flattening them to save the declarations was not worth it.

Testing

  • cargo test workspace-wide: 486 passed, 0 failed. cargo fmt --check, cargo clippy --workspace --all-targets --all-features, cargo sort --check, and cargo test -p stateless-core --no-default-features --lib --no-run are all clean.
  • Startup wiring (bin/stateless-validator/src/app.rs:600): a configured target is the only switch, on both target arms and with none; the R2 cap rather than the RPC one reaches the transport; the two new diagnostics; and that the witness chain is required with R2 configured.
  • Fetch behaviour, against a mock RPC plus a scripted R2 (bin/stateless-validator/tests/integration.rs:507): a bucket hit never touches the RPC witness path, and a miss falls back with one GET and one RPC call (bin/stateless-validator/tests/integration.rs:523). A corrupt object takes the identical branch — fetch_witness never inspects the error kind — so it is pinned where the difference lives, on the decode path in r2_witness.rs.
  • The stage budget (bin/stateless-validator/src/r2_witness.rs:307) drives a held connection with a per-attempt timeout an order of magnitude above the budget. Reverting the deadline to None makes it fail after the full three per-attempt timeouts, 15.0s against 0.2s at the test's scale.
  • The band's edge and the conjunction that reads it are pinned once each, beside the classifier (crates/stateless-common/src/r2_witness.rs:358, crates/stateless-common/src/r2_witness.rs:379): an absent object near the polled head is a frontier miss, the deep edge and below is a hole, and no other kind splits. Both binaries are covered by those two, where each used to carry its own.
  • The anchor itself is pinned through the budget share, which the band also selects (bin/debug-trace-server/src/data_provider.rs:2565): with the DB at 4000 and the head known to be 5000, block 4500 takes the historical half of the stage. Reverting the anchor to db_tip cuts it at the speculative eighth and fails the test at 256ms.
  • And the other direction, where only the DB knows the chain (bin/debug-trace-server/src/data_provider.rs:2606): with the tip hint never raised — what a server asked only for block hashes and transactions sees for its whole life — a block 1000 below the DB tip still takes the historical half. Reverting to the hint alone fails it while the test above still passes, so the two cover opposite directions.
  • The verdict carries its values, the cap included, and redacts the S3 secret in Debug (crates/stateless-common/src/r2_args.rs:487); an orphaned cap is named by the rules alone (crates/stateless-common/src/r2_args.rs:597); the pre-split cap spelling alone builds uncapped rather than being refused (bin/stateless-validator/src/app.rs:572).

Notes

  • Breaking for anyone who sets --witness-source. Nothing in mainnet_env does, and the flag is gone rather than kept as a no-op, so a stale command line fails loudly. A stale STATELESS_VALIDATOR_WITNESS_SOURCE env line is simply ignored.
  • A blank --r2-* env line now fails startup on an RPC-only validator, where it used to be inert. That is the loud direction, and it is what makes an inferred switch safe: "no R2 configured" can no longer be confused with "R2 configured wrong". The same applies to an inherited STATELESS_VALIDATOR_R2_MAX_CONCURRENT_REQUESTS on a role that configures no target: the orphan rule now names it, where the flag used to be read by nobody.
  • Two limits of that claim, both pre-existing and previously undocumented: "blank value" covers the --r2-* flags that travel as text, while the two numeric tuning flags are parsed by clap, so a blank one aborts before the rules run with clap's unnamed "invalid value for one of the arguments"; and --witness-max-concurrent-requests set alone alongside an R2 target warns rather than failing, since R2 is left uncapped but the RPC path it does size is genuinely in use.
  • The RPC fallback is a second path to the same bytes, not a second copy of them. The witness gateway reads this same bucket, so what the fallback covers is our client path failing (the CDN edge, an Access token, HTTP/2, credentials, this fetcher). A true bucket hole does not resolve by falling back; it moves the retry onto the shared gateway, which is what kind="missing" is there to catch.
  • Metric semantics change on both binaries. debug_trace_r2_witness_errors_total{kind="missing_above_tip"} is retired; those misses now either stay frontier (above the head) or join kind="missing" (below it, where they were always holes). On the validator, a miss within 32 blocks of the polled head is no longer an error at all. It lands on stateless_validator_r2_witness_frontier_misses_total, and ..._r2_witness_errors_total stays an error rate — which it would not, with a tip-following validator producing essentially nothing but frontier misses.
  • One statement this PR had made false is corrected. The trace server's --data-dir gate — its comment and its user-facing bail message (bin/debug-trace-server/src/main.rs:703) — still said the R2 route anchors block age on the local DB tip. Once the anchor moved to the chain head that reason no longer held: tip_hint is fed by tag lookups and canonical resolutions and needs no DB. The gate itself is unchanged; what it cites now is the old-block budget clamp and generator routing, which do read the DB tip. The same retired anchor is corrected in three places in README.md, one of which contradicted the line two below it. Worth a follow-up decision: with the band off the DB tip, the gate's remaining justification is weaker than it was, since both DB-tip consumers already fall back to their conservative branch.
  • The window is 32 blocks, which on MegaETH is 32 seconds, since the chain produces one block per second. That is the number to reason about when retuning it: an object still absent that long after its block existed means the witness generation pipeline is behind, and the same figure is the alarm's detection latency.
  • Which counter is meaningful depends on how far behind the run is. A tip-following validator fetches at head - tip_buffer, and every deployed buffer is far inside a 32-block window, so all of its misses are frontier misses — routine and numerous in practice — and kind="missing" stays at zero by construction; the frontier rate is the signal there. kind="missing" earns its name during catch-up and fixed --end-block backfills, where blocks sit far below the head.
  • A hole that first appears near the tip is consequently not detected here: the block is fetched once, falls back, and is never probed again. Left that way on purpose. The fallback already served the block, so this process has nothing to act on, and re-probing purely to keep a counter honest belongs with whatever watches the uploader rather than in a persisted recheck queue inside the validator.
  • R2 retries are spaced by the existing --rpc-*-backoff-ms ramp, up to about two seconds per affected block at the defaults, always inside the stage budget.
  • R2 GET retries log at debug in the shared fetcher (crates/stateless-r2/src/fetch.rs:885), in both binaries. Each failed fetch still warns once with its final error and every retry is counted, so a throttle brownout no longer turns each block into three warnings; this changes the trace server's logs as well.
  • Follow-ups, deliberately left out of this PR:
    • A shorter R2-specific retry pacing, which hands blocks to RPC sooner during a throttle brownout.
    • A breaker that skips R2 for a while after N consecutive stage timeouts. Pacing trims the retries but not an accept-then-stall, which holds every in-flight block for the full budget and floors throughput near fetcher_max_in_flight / --rpc-per-attempt-timeout-ms — fine for tip-following, an order of magnitude slower for catch-up.
    • Skipping the probe for in-band blocks, if r2_witness_frontier_misses_total sits near 100% of the validated-block rate after rollout. That gives back the one edge round trip every tip block pays before its RPC witness call.
  • R2WitnessError::is_retryable and R2ObjectFetcher::pacing() are removed; their only caller was the deleted pause. mega-reth pins v2.0.18 and references neither.

🤖 Generated with Claude Code

Add `--witness-source r2-then-rpc`, the trace server's R2-first shape for the
validator's pipeline: every witness fetch tries the configured R2 target first
(the signed S3 endpoint or the HTTP/2 custom domain, exactly as `r2` does) and
any R2 failure hands that block to the `--witness-endpoint` RPC chain instead
of stalling it. Bulk history streams from the bucket at object-storage
parallelism while the RPC gateway only sees the blocks R2 could not serve,
without giving up the fallback that `--witness-source r2` lacks.

The R2 client now carries an `R2FailurePolicy`: `Surface` (sole source, the
existing 9-attempt budget plus the surfaced-failure pauses that pace the
pipeline's blind re-enqueue) or `FallBackToRpc` (3 attempts, no pauses — the
block's next stop is RPC). `--witness-endpoint` is required under the new
source, as under `rpc`; the pre-split concurrency-cap guard stays `r2`-only,
since there the RPC cap sizes a path that is really in use.

Both R2 sources now classify a `missing` against the fetcher's last polled
remote head using the shared `R2_FRONTIER_WINDOW` (hoisted from the trace
server into `stateless-common`): inside the band it lands on the new
`kind="missing_frontier"` label, so `kind="missing"` keeps meaning a hole in
objects that must exist rather than the uploader still catching up.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@mega-maxwell

mega-maxwell Bot commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Claude review status

Living comment — rewritten in place. The review workflow keeps this single comment up to date instead of posting a new one each round, so it always describes the latest reviewed commit and the earlier text is intentionally gone. No reply is needed here; reply to a finding in its own review thread, and answer an open question in a reply on this PR. The next review round reconciles your answer.

🛠️ Review did not finish

Attempted dd0cbc4c..6e614975 · updated 2026-09-20T05:17:55+00:00

This round did not publish: MODEL_ACTION_FAILED in phase review_retry. Anything listed below is from the last round that did. Re-run the workflow or push a new commit to try again.

…uild

Delete `--witness-source`. Configuring an R2 target is the switch: with one
configured the validator tries the bucket before its `--witness-endpoint`
chain and hands any failed block to that chain, and with no `--r2-*` flag set
witnesses come from RPC alone. `--witness-endpoint` is now always required,
since it is either the only witness path or the fallback behind R2.

That removes the R2-only mode, and with it the `R2FailurePolicy` enum and the
surfaced-failure pauses that paced the pipeline's blind re-enqueue: every
failure now has somewhere to go, so it surfaces at once on a short budget.
It also removes the reason the pre-split concurrency-cap guard existed, and
lets the validator validate its R2 flags on every startup like the trace
server does, so a half-configured target, a blank env value or an orphaned
tuning flag is named rather than read as "no R2 configured" and silently
downgraded to the RPC path.

Shared what the two binaries were duplicating around it. `validate_r2_flags`
now returns the values its rules proved rather than only naming the target,
and `R2WitnessTransport::from_config` is the single place either binary turns
that verdict into a transport, publishing what it built through the new
`R2Metrics` trait the way `RpcMetrics` already works for the RPC client. That
retires eight `expect()`s restating checks made elsewhere, and two copies of
a rule about which flags each target requires.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@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: 973463125e

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread bin/stateless-validator/src/r2_witness.rs Outdated
@flyq flyq changed the title feat(validator): R2-first witness source with the RPC chain as fallback feat(validator): fetch witnesses from R2 first, falling back to the RPC chain Sep 17, 2026
@flyq flyq added the enhancement New feature or request label Sep 17, 2026
`kind="missing"` was described as the bucket-integrity alarm without
qualification. On a tip-following validator it cannot be: the fetcher works at
`head - tip_buffer`, every deployed buffer is far inside the 32-block frontier
window, so every miss is a frontier miss and that counter sits at zero by
construction. Frontier misses are routine and numerous there in practice, which
is what the split exists to keep off the alarm; the rate of them is the signal.
`kind="missing"` earns its name during catch-up and fixed `--end-block`
backfills, where blocks sit far below the head.

Also states the consequence plainly: a hole first seen near the tip is fetched
once, falls back, and is never probed again, so it is not detected here. Left
that way deliberately rather than carrying a persisted recheck queue — the
fallback already served the block, so the validator has nothing to act on, and
verifying bucket completeness belongs with whatever watches the uploader.

Documentation only; no behaviour change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@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

.get_block_object(number, hash, MAX_ATTEMPTS, None, metrics::on_r2_witness_retry)

P1 Badge Put a total deadline on the R2 fast path

When R2 accepts a connection but stalls, passing None leaves both the concurrency-permit wait and all three GET attempts without an aggregate deadline. With the default 20-second per-attempt timeout, one block waits roughly a minute before reaching RPC; with --r2-max-concurrent-requests, queued blocks can wait through multiple such holders, effectively stalling the validator during an R2 brownout despite the fallback. Give the R2 stage a short total deadline that also bounds permit waiting before handing the block to RPC.

AGENTS.md reference: AGENTS.md:L154-L154

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread bin/stateless-validator/src/app.rs
The R2 stage passed no deadline, so neither the concurrency-permit wait nor the
three GET attempts had an aggregate bound. An endpoint that accepts a connection
and then stalls therefore cost a full per-attempt timeout on every try — about a
minute per block at the 20s default — before the block reached the RPC chain,
and blocks queued behind `--r2-max-concurrent-requests` waited through several
such holders. That is the brownout the fallback exists to absorb, absorbed far
too slowly to keep the pipeline moving.

Give the client a `stage_timeout` covering the permit wait and every attempt
together, and pass one `--rpc-per-attempt-timeout-ms` from the wiring site: R2 is
an optimisation in front of a path that retries forever, so it may never cost a
block more wall clock than a single upstream hop. A healthy fetch is sub-second
and every fast failure mode still fits the full retry count, so the budget only
bites on a stall.

The decode deliberately stays outside it: those bytes are already in hand and
the work is our own CPU, so abandoning it would only re-fetch and re-decode the
same witness over RPC.

Answers the Codex P1 on this PR. `a_stalling_endpoint_is_abandoned_on_the_stage
_budget` drives a held connection with a per-attempt timeout an order of
magnitude above the stage budget; reverting the deadline to `None` makes it fail
after the full 3 x per-attempt instead.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@flyq

flyq commented Sep 17, 2026

Copy link
Copy Markdown
Member Author

Confirmed and fixed in d732b51.

The stage now carries a total deadline covering the permit wait and every attempt together, and the wiring site passes one --rpc-per-attempt-timeout-ms for it: R2 sits in front of a path that retries forever, so it may never cost a block more wall clock than a single upstream hop. A healthy fetch is sub-second and every fast failure mode still fits the full retry count, so the budget only bites on exactly the case you describe.

The decode deliberately stays outside that deadline. Those bytes are already in hand and the work is our own CPU, so abandoning it would only re-fetch and re-decode the same witness over RPC.

a_stalling_endpoint_is_abandoned_on_the_stage_budget in bin/stateless-validator/src/r2_witness.rs drives a held connection with a per-attempt timeout an order of magnitude above the stage budget. Reverting the deadline to None makes it fail after the full three per-attempt timeouts instead of passing on the budget — 15.0s versus 0.2s at the test's scale, which is the ~60s versus 20s you predicted at the 20s default.

Four parallel reviews (reuse, simplification, efficiency, altitude) over this
PR, deduped and applied. Net -41 lines, no behaviour change.

Shared API:
- The in-flight cap now rides on the `R2Config` verdict. It reached the shared
  layer three ways per binary (as the count flag the rules check against the
  connection count, as a tuning flag for the orphan rule, and as a separate
  `from_config` argument), so the cap the rules validated and the cap the
  transport got were two independent reads — the exact drift `R2Config` exists
  to remove. The rules now name an orphaned cap themselves, the way they already
  do the connection count, and `from_config` loses the argument.
- `R2Config::S3.secret_access_key` is a `RedactedSecret`, so `Debug` is derived
  instead of a 22-line hand-written impl that would silently drop any field
  added later; the unused `Clone` goes too. `RedactedSecret` gains `From<&str>`.
- Removed `R2WitnessError::is_retryable` and `R2ObjectFetcher::pacing()`, whose
  only caller was the surfaced-failure pause this PR deleted. mega-reth pins
  v2.0.18 and references neither (checked against a local checkout).

Validator:
- The remote head is passed as a plain `u64`. The `Option` wrapper changed no
  answer: `Some(0)` and `None` classify identically in the band predicate, and a
  head of 0 correctly puts every block in the frontier before the first poll.
- `override_ms` for the R2 connect timeout, as for every other override in `run`.
- Tests reuse one transport builder, and the stall test asserts its own premise
  (per-attempt timeout well above the stage budget). The R2 fetcher tests read
  the shared fixtures and encode a payload only where R2 actually serves one.

Docs: stale wording from the deleted mode and the deleted pause removed across
both binaries, `stateless-common`, `stateless-r2`, and one pipeline comment in
`stateless-core` that still pointed at "the R2 witness client's throttle". The
two adapters' module docs now state what genuinely differs between them — full
vs light decode, and a fixed stage budget that stops at the GET vs the caller's
request deadline that includes the decode. Repeated rationale trimmed to one
copy each.

Skipped on purpose: passing the pipeline's tip into `BlockFetcher::fetch` (a
core trait mega-reth implements, and this PR does not touch core); making the
three fallback tests table-driven (each carries its own rationale); a separate
R2 retry pacing (a behaviour change — the MAX_ATTEMPTS doc now states the
existing spacing honestly instead).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@RealiCZ RealiCZ left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed the full diff and verified the parts the design rests on:

  • The stage budget (Codex P1) is implemented end to end: R2ObjectFetcher::get_block_object clamps the permit wait, each request's timeout, and the backoff sleep against the deadline, and stage = per-attempt = --rpc-per-attempt-timeout-ms, so a stall costs exactly one hop.
  • "The validator only fetches at or below the head it last polled" holds: block_fetcher polls latest_block_number before spawning any fetch and spawns up to chain_latest - tip_buffer, and remote_head is that same value, so dropping the above-tip band is correct.
  • R2Config's derived Debug is safe: RedactedSecret and CfAccessCredentials both redact.
  • None of the removed public items (R2Target, R2WitnessError::is_retryable, R2ObjectFetcher::pacing, the R2WitnessClient::new signature, the ValidatorFetcher fields) are referenced by mega-reth (grep over a local checkout).
  • Merges cleanly onto main and alongside #197.
  • Ran locally: the r2_witness unit tests, the app::tests wiring tests, and the fallback / bucket-hit integration tests all pass; the two timing-bound tests (failures_surface_immediately_for_the_rpc_fallback, a_stalling_endpoint_is_abandoned_on_the_stage_budget) passed 10/10 repeated runs.

The inline notes are all non-blocking. Two more for the record:

  • On Codex's STATELESS_VALIDATOR_WITNESS_SOURCE point: dropping it silently is defensible and I won't hold the PR on it, but it is the one stale-configuration case this PR does not name — orphaned tuning flags and blank values all fail by name. A std::env::var_os guard kept for one release would make the rule uniform; your call.
  • stateless-common / stateless-r2 lose public items, so this is technically a breaking change on crates mega-reth pins; whether that warrants a minor bump over the usual patch is for the release process.

Approving.

Comment thread bin/stateless-validator/src/r2_witness.rs
Comment thread bin/stateless-validator/src/chain_sync.rs
Comment thread bin/stateless-validator/src/r2_witness.rs
Comment thread bin/stateless-validator/src/app.rs Outdated
flyq and others added 2 commits September 19, 2026 17:17
`?args.r2_bucket` printed the `Option` itself, so the startup line read
`bucket=Some("witness-mainnet")` on the S3 target and `bucket=None` on the
custom domain. Both binaries now print the bucket name, or `-` where the
target has none.

Answers the review nit on `bin/stateless-validator/src/app.rs:466`; the trace
server's line (`bin/debug-trace-server/src/main.rs:912`) had the same shape.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
During a throttle brownout each block produced up to three warnings: one per
retry from the shared fetcher ("R2 witness GET failed, backing off") and then
the caller's own line when the fetch finally gave up and fell back. At catch-up
block rates that multiplies into a flood of lines that say the same thing.

Both callers already carry the per-attempt signal elsewhere: each passes a
retry counter as `on_retry` (`r2_witness_retry_attempts_total` on the
validator, its `debug_trace_` counterpart on the trace server), and each logs
one warning per failed fetch with the final error. So the per-attempt line
drops to debug in the shared fetcher, for both binaries alike; the attempt
number, backoff and intermediate error stay visible at debug.

Answers the review note on `bin/stateless-validator/src/r2_witness.rs:138`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
flyq and others added 2 commits September 20, 2026 11:03
…or counter

Three review findings, one mechanism.

**The band arithmetic is now one rule.** The window constant was already shared
but the predicate was not, and the two copies had already drifted: at exactly
`tip - 32` the trace server said Historical (alarm) while the validator said
Frontier (no alarm), each pinned by a test whose comment contradicted the
other's. `R2Band` and `r2_band` move into `stateless-common`, with the trace
server's shipped convention — deep edge exclusive, top edge inclusive — so the
block that flips is the validator's, whose label is unreleased. Each binary
keeps what it does with a band: the trace server its per-band budget share and
its `missing_above_tip` series, the validator a collapse of `AboveTip` into
`Frontier`, which is unreachable for it anyway since the pipeline spawns at
`head - tip_buffer` against the same poll that set the head.

**Frontier misses leave `r2_witness_errors_total`.** They are routine and
numerous — a tip-following validator produces essentially nothing else — so
counting them on a series named `*_errors_total` made the obvious alert
(`rate(...) > 0`) permanently hot. They now have their own
`r2_witness_frontier_misses_total`, which is also what the trace server already
does through its per-source series. The synthetic `missing_frontier` label and
`error_kind` go with them; `R2WitnessError::KINDS` is the error counter's label
set again.

**Carrying only `--witness-max-concurrent-requests` into an R2 deployment now
warns.** The startup guard that refused it was dropped with the R2-only mode,
but the configuration still leaves R2 uncapped, and the fetcher cannot flag that
itself: with no cap there is no per-connection share to compare against the
edge's stream limit, so the queueing happens inside the HTTP/2 connection where
it is invisible and still spends the per-attempt budget.

Docs: "a blank value is rejected by name" holds for the `--r2-*` flags that
travel as text, not for the two clap-parsed numerics, where a blank env line
aborts earlier with clap's unnamed error; and an RPC-only validator inheriting
`STATELESS_VALIDATOR_R2_MAX_CONCURRENT_REQUESTS` from a shared template now
fails to boot where that variable used to be inert. Both were true before this
commit and undocumented.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both readers serve witnesses out of the same bucket, filled by the same
uploader, so "is this absent object expected or a hole?" has one answer. It was
being answered against two different quantities: the validator asked its last
polled chain head, the trace server asked how far its own DB had ingested.
Those are not the same question. A reader lagging the chain does not make an
overdue object any less overdue.

The trace server now bands against `DataProvider::tip_hint`, the monotonic
maximum on-chain height it has already observed for the canonical-hash memo, so
no new upstream call is needed. `db_tip` stays where it belongs, routing (may
the generator have pruned this?) and clamping the old-block budget.

That retires the third band. `AboveTip` only ever existed because the DB tip
could lag the chain without bound, and it was suppressing real holes: through a
catch-up, every genuine gap between the DB tip and the chain head was routed to
`kind="missing_above_tip"` instead of the alarm. A block 500 seconds below the
head is overdue whatever this process has ingested. `R2Band` is two variants,
`missing_above_tip` is gone, and the validator's collapse of the third band
goes with it.

`the_band_follows_the_chain_tip_not_the_ingested_tip` pins it through the
budget share, which the band also selects: with the DB at 4000 and the head at
5000, block 4500 takes the historical half of the stage. Reverting the anchor
to `db_tip` cuts it at the speculative eighth and fails the test at 256ms.

Also records what the window means in time: MegaETH produces one block per
second, so 32 blocks is 32 seconds of uploader grace, which is both the
threshold for calling the generation pipeline late and the alarm's detection
latency.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@flyq flyq changed the title feat(validator): fetch witnesses from R2 first, falling back to the RPC chain feat: R2-first witnesses on the validator, and a chain-anchored frontier band in both binaries Sep 20, 2026

@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: e1832b3454

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread bin/debug-trace-server/src/data_provider.rs
A concision pass over #221: the rationale prose had accreted across seven
review rounds and several clusters were written at four or five altitudes,
while a handful of tests re-asserted what a test closer to the code already
owned. Net -166 lines, no behaviour change.

Hoists the frontier-miss rule into `stateless-common` as
`R2WitnessError::is_frontier_miss`. The previous round moved `r2_band` and
`R2Band` there but stopped one line short: the conjunction that uses them
stayed spelled out in both binaries, the same shape that had already drifted
once on the band edge. The trace server's spelling is now covered by a shared
test for the first time, and the validator's duplicate test is gone.

Corrects a statement this PR made false. The trace server's `--data-dir` gate,
its comment and its user-facing bail message all still said the R2 route
anchors block age on the local DB tip; since the anchor moved to the chain
head that reason no longer holds. The gate is unchanged — what it now cites is
the old-block budget clamp and generator routing, which do read the DB tip.
The same retired anchor is corrected in three places in README.md, one of
which contradicted the line two below it.

Tests removed as redundant, each verified to die on the same mutation as the
test that survives it: the corrupt-object fallback (identical branch to the
404 — `fetch_witness` never inspects the error kind), the `--witness-source`
removal check (asserts clap rejects an undeclared flag), the parse-time
witness-endpoint check (subsumed by `witness_endpoints_are_always_required`),
and the band-vs-routing test (its `r2_band` half is character-for-character
the shared one; its `is_historical` half moved into `historical_routing_boundary`).
Trimmed in place: the conflicting-targets shape already covered by a
pre-existing loop, two clap echoes, a subsumed uncapped case, and two
deterministic retry arms that `stateless-r2` classifies and tests itself.

485 tests pass; fmt, clippy, cargo sort and the no-std build are clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@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: 0b26548fc0

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +130 to 132
// Outside the deadline on purpose: our own CPU on bytes already in hand, and
// abandoning it would only re-fetch and re-decode the same witness over RPC.
let witness = decode_on_blocking_pool(bytes, number, hash, None, |bytes| {

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 Include decoding in the R2 stage deadline

When an R2 GET consumes most of stage_timeout, or a large/pathological payload is slow to decompress and deserialize, passing None here lets decoding continue beyond the advertised per-block budget and delays the RPC fallback indefinitely. Pass the existing deadline to decode_on_blocking_pool; it already skips an expired decode and returns DecodeTimeout, allowing fetch_witness to fall back promptly.

AGENTS.md reference: AGENTS.md:L159-L159

Useful? React with 👍 / 👎.

Answers Codex P2 r4055971897. `tip_hint` is raised only by by-number and
tag resolutions, so a server whose traffic arrives as `debug_traceBlockByHash`
or transaction lookups never raises it at all: it stays `0` for the process
lifetime, which bands every block frontier, cuts every historical probe to the
speculative eighth of the witness budget, and keeps genuine bucket holes out of
`kind="missing"` indefinitely.

The finding is narrower than the defect. `tip_hint` is the maximum height
request traffic has revealed, so it is a lower bound on the chain tip in every
mode, not just the hash-only one — a server serving historical backfill by
number sits far below the real tip too. Meanwhile the DB tip, which e1832b3
moved away from, is a good estimate exactly when sync is caught up, and a poor
one exactly during the catch-up that motivated moving off it.

So the band takes the higher of the two. Each leads the other in a different
mode, both are bounded by the real chain, and the maximum is therefore a better
estimate than either alone while still banding a miss on the safe side. The
derivation sits at the banding seam in `fetch_witness`, where both values are
already parameters, so it costs no extra redb read and is covered by the tests
that already drive that seam.

`an_unraised_tip_hint_bands_on_the_db_tip_instead` pins the reported case and
is mutation-verified: reverting to `r2_band(chain_tip, ..)` fails it while
`the_band_follows_the_chain_tip_not_the_ingested_tip` still passes, so the two
cover opposite directions. The `--data-dir` gate's text, AGENTS.md and README
are corrected to match — the band reads the DB tip again, as one of two inputs.

486 tests pass; fmt, clippy, cargo sort and the no-std build are clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@flyq
flyq merged commit 841fefb into main Sep 20, 2026
20 of 21 checks passed
@flyq
flyq deleted the liquan/feat/validator-r2-then-rpc branch September 20, 2026 05:24
flyq added a commit that referenced this pull request Sep 20, 2026
…/fix/empty-witness-endpoints-and-typed-range-error

Resolves the conflict by dropping this PR's "witness-less RpcClient" half,
which #221 superseded.

That half existed because `--witness-source r2` handed the data endpoints to
the client as placeholder witness endpoints. #221 deleted `--witness-source`
and made `--witness-endpoint` always required, so R2 is now tried first with
the RPC endpoints as its fallback and no placeholder is passed. Passing an
empty witness list would delete that fallback, so:

- app.rs keeps main's `witness_apis(&args)?`
- the constructor's non-empty witness check is restored
- the two tests asserting an empty list is legal are adapted: one now pins the
  restored check, the other keeps only the reachable `skip`-past-the-end case

What survives is the typed-failure half: `WitnessFetchError::NoProviderInRange`
replaces the `assert!` in `witness_round_robin`, so a routing bug fails one
request instead of the process.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants