Skip to content

fix(debug-trace-server): stop rotating to fallbacks for not-yet-generated frontier witnesses - #164

Merged
flyq merged 7 commits into
mainfrom
liquan/fix/frontier-witness-generator-retry
Jul 31, 2026
Merged

fix(debug-trace-server): stop rotating to fallbacks for not-yet-generated frontier witnesses#164
flyq merged 7 commits into
mainfrom
liquan/fix/frontier-witness-generator-retry

Conversation

@flyq

@flyq flyq commented Jul 30, 2026

Copy link
Copy Markdown
Member

Summary

At the sync frontier the chain-sync prefetch treated the generator's "witness not found" as a provider failure and rotated to the public fallback. But a frontier witness the generator doesn't have yet cannot exist on the fallbacks either — they receive witnesses from the same generation pipeline. The fetcher now gives the generator a short exclusive grace for frontier-fresh blocks (retrying it alone with round backoff) and only then falls back to the full endpoint chain; deep catch-up blocks keep today's full failover from the first attempt.

Motivation

A 7.7 h production log (v2.0.14, 2026-07-21) shows the race is the norm, not the exception: 18,766 sync fetches hit "witness not yet generated" at the generator (~68% of blocks) and 18,499 of them rotated to the public fallback, which also missed (~600 ms per wasted round trip) before round 1 retried the generator successfully. Twice in that window the fallback did not answer its miss at all and hung until the 20 s per-attempt cap (blocks 21832224 / 21841054), stalling tip advance ~21 s each — with the witness almost certainly ready seconds in. The fallback won the frontier race in only 267 of 18,766 cases (1.4%). Routing fresh misses back to the generator removes both the daily waste and the routine exposure to fallback stalls; the ~21 s worst case becomes ~1–2 s (generation lag) with the fallback untouched.

Fix

  • RpcClient::get_witness_light_first_provider_only (crates/stateless-common/src/rpc_client.rs) — the complement of the existing _from(skip) historical routing; the witness retry loop's provider selection generalizes from a skip prefix to an index range.
  • TraceFetcher memoizes the last remote head observed by latest_block_number (which the pipeline already polls) as the freshness anchor, and fetch_witness routes (bin/debug-trace-server/src/chain_sync.rs): blocks within FRONTIER_FRESHNESS_BLOCKS (256) of that head give the generator a GENERATOR_WITNESS_GRACE (6 s) exclusive window, then fall through to the full chain; an unknown head or deeper blocks keep the full chain from the first attempt.
  • Bounded regressions by construction: a downed generator costs at most the grace before failover (the per-attempt cap still bounds hangs), and a genuinely lost fresh witness reverts to exactly the pre-routing behavior after the grace. The grace requires the CLI-declared generator (--witness-generator-endpoint), mirroring the request path's can_skip_generator gate — single-endpoint and no-declared-generator deployments keep plain same-round failover untouched.

Testing

Six new chain_sync tests over real fixture witnesses served by scripted jsonrpsee mocks: a fresh miss retries the generator without touching the fallback; a stale (deep catch-up) miss fails over immediately with no grace; an unknown head classifies as non-fresh; a deployment without a declared generator keeps plain same-round failover; a downed generator falls back right after the grace; latest_block_number feeds the anchor. Plus an rpc_client test pinning that first_provider_only never rotates past provider 0 (mirror of the existing skip test). Full workspace: 301 tests, cargo fmt / clippy --workspace --all-targets --all-features / codespell all clean.

Notes

The public endpoint's 20 s hang-on-miss (vs. its usual ~600 ms -32603 error) is upstream behavior worth checking on the megaeth.com side for the two windows above; this PR removes the sync path's routine exposure to it, but a fallback that stalls when genuinely needed is still bounded only by the 20 s per-attempt cap. The two constants (256 blocks / 6 s) are policy values documented in code with qualitative rationale; their derivation from the log is above.

🤖 Generated with Claude Code

…ontier-fresh witnesses

At the sync frontier, "witness not found" from the generator means "not
generated yet", not "ask someone else": the fallback endpoints receive
witnesses from the same generation pipeline and cannot be ahead of it. The
fetcher now routes by freshness against the last remote head observed by the
pipeline's latest_block_number polls: frontier-fresh blocks retry the
generator alone under a bounded grace before falling back to the full
endpoint chain, while deep catch-up blocks (where the generator may have
pruned the witness) keep full failover from the first attempt. Adds
RpcClient::get_witness_light_first_provider_only, the complement of the
skip-based historical routing, by generalizing the witness retry loop's
provider selection from a skip prefix to an index range.

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

mega-maxwell Bot commented Jul 30, 2026

Copy link
Copy Markdown

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 head f3912403 · updated 2026-07-31T10:53:43+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.

@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: 1a8d50f558

ℹ️ 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/chain_sync.rs Outdated
Comment thread bin/debug-trace-server/src/chain_sync.rs Outdated
… generator, cleanup pass

The grace now requires the CLI-declared generator (generator_first threaded
into TraceFetcher::new), matching the request path's can_skip_generator gate:
without --witness-generator-endpoint no endpoint is special, and a
no-generator multi-endpoint deployment keeps plain same-round failover
instead of pinning frontier sync to endpoint 0 for 6s — pinned by a new
no_declared_generator_keeps_plain_failover test, and the two routing-policy
docs now cross-reference each other. fetch_witness returns just the
LightWitness every caller kept anyway (MptWitness import moves into the test
module), test_fetcher builds on TraceFetcher::new via functional update
instead of restating its defaults, the twin provider-range tests in
rpc_client share one two_endpoint_witness_client scaffold, and the
generator-cannot-be-behind rationale now lives once in chain_sync with the
rpc_client method doc trimmed to mechanism.

Co-Authored-By: Claude Fable 5 <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: 989b1137ac

ℹ️ 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/chain_sync.rs Outdated
flyq and others added 2 commits July 30, 2026 14:14
…ne-exceeded metric

The grace probe passed its budget as the RPC client's logical-call deadline,
so a generator lagging past the grace incremented
debug_trace_upstream_deadline_exceeded_total even when the fall-through then
succeeded — polluting a counter that otherwise fires only when a request
really fails. The grace is now a caller-side tokio timeout around a
deadline-less single-provider probe: expiry is an internal routing budget the
metric never sees, and rpc_client is untouched. The downed-generator test now
runs with a counting RpcMetrics sink and pins deadline_exceeded == 0 across a
grace expiry plus fallback success.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@vincent-k2026 vincent-k2026 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.

Approving 349c0d3. cargo test --workspace green here. The motivation is well-evidenced and the fix is fail-safe in every degraded direction I traced.

The failure mode I went looking for isn't reachable. I wanted to know whether an unverified eth_blockNumber anchor could be pushed in the dangerous direction — a head reported far in the future making everything look fresh and stalling sync on grace. It can't: the predicate is head − number <= FRONTIER_FRESHNESS_BLOCKS, so a too-high head makes the difference larger and the block classifies as not fresh, falling back to today's full chain. A lie in the harmful direction disables the grace rather than forcing it. Unknown head lands in the same place via the u64::MAX init and saturating_sub (chain_sync.rs:75, :94), and a reorg lowering the head just shrinks the fresh band — remote_head and TipTracker.tip are set from the same latest_block_number call, so they stay in lockstep and the fetcher never asks above the tip. Relaxed ordering is correct here since the value gates a heuristic only.

Your marginal-cost claim checks out arithmetically. With per_attempt_timeout = 20 s, a stalled generator costs grace (6 s, cancelled by the caller-side timeout) plus a full 20 s re-probe of provider 0 on the fall-through — ~26 s absolute. Pre-PR the full chain also probed provider 0 first and paid that same 20 s, so the increment really is exactly the grace. Cancellation at the timeout boundary is clean: the witness_concurrency permit drops as an RAII guard and the in-flight reqwest future is dropped rather than left poisoning a pool.

The shared-crate generalization is bit-for-bit compatible. _from(skip) now calls witness_round_robin(skip..len, ..); the old skip < len assert and the new !providers.is_empty() && providers.end <= len (rpc_client.rs:714-718) are equivalent for that range, and the slice is identical — so the validator binary and an external embedder see no change. first_provider_only uses 0..1 behind the witness_provider_count() > 1 gate, so it can't be empty, and an empty range would panic rather than silently no-op.

On the deferred "skip the stalled generator after the grace" — I agree with you, and I'd put the argument slightly stronger than you did. It isn't only a permanent skip that's unsafe: even a per-fetch ..._from(1, ..) on the fall-through is wrong, because the fallbacks are fed by the same pipeline. For a generator that is merely slow rather than down — witness landing at 7 s — skipping provider 0 means querying only endpoints that definitionally don't have it yet, trading generator lag for R2 upload lag. Keeping provider 0 in the fall-through is the correct call, and your livelock reasoning isn't a strawman.

There is one cheap middle ground worth a follow-up issue rather than a change here: cap the fall-through's provider-0 attempt with a shorter per-attempt timeout than the full 20 s. It just had its exclusive 6 s window, so a second full stall is pure waste — this would pull the stalled worst case from ~26 s down to ~6–8 s without the skip's livelock risk.

Tests: all seven discriminate, and two of them answer exactly the questions I'd have asked. fresh_witness_miss_retries_generator_without_touching_fallback proves non-contact with a request counter on the fallback mock (fb_hits == 0, gen_hits == 3), not just a happy-path return; fresh_fetch_falls_back_after_grace_when_generator_is_down uses a real counting RpcMetrics sink to assert deadline_exceeded == 0, which is the right way to pin the ca68d49 metric-semantics fix. no_declared_generator_keeps_plain_failover would fail if the generator_first gate were dropped, and test_witness_fetch_first_only_never_hits_later_providers would fail if 0..1 regressed to 0..len.

Two non-blocking notes:

A fully down generator now costs ~6 s per fresh block that it didn't before. Pre-PR a connection-refused generator was near-free — instant rotation. Post-PR the probe backs off and retries the dead endpoint for the whole grace before failing over, so throughput in that degraded state is roughly in_flight_window / 6 s; with window = cores·2 that needs ≥5 cores to hold ~1.6 blk/s. Comfortable on our boxes, tight on small ones. If you want to close it cheaply: break the grace early on a fast error from provider 0 (as opposed to a stall), since backing off against a refused connection buys nothing.

GENERATOR_WITNESS_GRACE's doc (chain_sync.rs:31-42) states the down/never-written failover delay but not the stalled worst case. An operator reading the constant will take 6 s as the ceiling when a stalled generator on a fresh block actually costs ~26 s. One sentence. Relatedly the 6 s isn't tied to a measured p99 generation lag in the comment — worth noting the assumption alongside the 1–2 s typical figure.

Merge-order note, no action needed here: git merge-tree against #161 and #162 shows conflicts in chain_sync.rs and rpc_client.rs, but they're all textual — the chain_sync test module's imports and helpers plus the TraceFetcher struct region. Neither of those PRs changes TraceFetcher::fetch's witness call shape, the witness deadline model, or the pipeline reorg/stale-reset hooks, so whichever lands second needs a mechanical rebase of the test module and nothing more.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

# Conflicts:
#	bin/debug-trace-server/src/chain_sync.rs

@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: 7660f5bedb

ℹ️ 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/chain_sync.rs Outdated
flyq and others added 2 commits July 31, 2026 18:39
…itness test mocks

- fold the generator+fallback predicate into main's startup-computed routing_configured and hand it to TraceFetcher as one frontier_routing switch, dropping the per-fetch witness_provider_count() re-derivation (third spelling of the same predicate)
- drop the speculative deadline parameter from get_witness_light_first_provider_only: a caller-side timeout is the designed usage (an expired routing budget must not count as an upstream deadline-exceeded), so the method retries deadline-less and returns the tuple directly; its unit test now pins the production shape
- host scripted_witness_rpc and start_mock_rpc/consistent_header in data_provider::test_support; delete the duplicate start_counting_witness_rpc and chain_sync's inline eth_blockNumber mock
- widen test_fetcher (routing switch, grace, metrics) so the down-generator test stops rebuilding the client config by hand, and drop the construct-then-patch struct-update idioms
- fix the stale --witness-local-window help sentence that still claimed the chain-sync prefetch always uses the full endpoint chain

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…observed head

During a long catch-up stretch the pipeline never re-polls the tip (the fetcher refreshes only when its spawn window runs past the ceiling), so the tail of the stretch still classified as frontier-fresh against the stale anchor and burned the full 6s grace per block on witnesses the generator may have already pruned (Codex review finding).

The freshness anchor now carries its observation instant (Mutex<Option<(u64, Instant)>> replaces the AtomicU64 with its u64::MAX sentinel) and is trusted only while no older than the grace: a block is at least as old as the anchor that classified it, so past that horizon "not generated yet" stops being a plausible reason for a missing witness and full-chain failover from the first attempt is the right shape — either the generator still has the block (first attempt succeeds identically) or it pruned it (rotation reaches the fallback without the 6s tax). Steady-state tip-following re-polls every poll_interval (100ms default) and stays far inside the horizon.

Pinned by stale_head_observation_disables_the_grace; docs aligned (AGENTS.md, README, type/const docs).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@flyq
flyq merged commit 682ec1a into main Jul 31, 2026
20 of 21 checks passed
@flyq
flyq deleted the liquan/fix/frontier-witness-generator-retry branch July 31, 2026 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants