Conversation
…failure R2 mode stops handing the data endpoints to the client as placeholder witness endpoints, and an unsatisfiable witness provider range fails one request as `WitnessFetchError::NoProviderInRange` instead of panicking the process. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CHVpMMX9N69sUNbuKgpBVY
Claude review status
🛠️ Review did not finish Attempted 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. |
|
This PR has no labels. Given the diff (replacing an |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae09a4f9a2
ℹ️ 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".
| WitnessFetchError::NoProviderInRange { skip, configured } => unreachable!( | ||
| "full-range witness fetch on a client with no witness providers \ | ||
| (skip={skip}, configured={configured})" | ||
| ), |
There was a problem hiding this comment.
When RpcClient is constructed with the now-supported empty witness list, both get_witness_with_deadline and get_witness_light_with_deadline pass NoProviderInRange to this unreachable!, while their unbounded wrappers and get_witness_light_first_provider_only panic through expect. This is reachable for the validator's fallback-less R2 configuration and contradicts the constructor documentation that a witness call returns the typed error; an accidental call therefore still takes down its task/process instead of failing structurally. Propagate WitnessFetchError through all witness-fetch APIs, or otherwise make the witness-less state impossible for APIs that cannot return it.
AGENTS.md reference: AGENTS.md:L154-L154
Useful? React with 👍 / 👎.
…/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>
flyq
changed the title
…rotation The range guard sat in the private `witness_round_robin`, which widened the error type of all three of its callers even though two build their range from the provider count and cannot be out of range. That widening is what forced the `deadline_only` adapter, its `unreachable!`, two `map_err` sites and a reworded `expect`. The check now lives in `get_witness_light_with_deadline_from`, the only method taking a caller-computed value. `witness_round_robin` keeps returning `RpcDeadlineExceeded`, and its precondition is a `debug_assert!` beside the one `round_robin_with_backoff` already uses. Public signatures are unchanged. Half the old guard was dead — `end > configured` is unreachable from every call site — and it misreported that case as a bad `skip`. With the range now built from the provider count at each site, the one remaining failure is `skip >= configured`, which the error's fields describe exactly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a418e8e037
ℹ️ 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".
| WitnessFetchError::Deadline(d) => d.into(), | ||
| WitnessFetchError::NoProviderInRange { .. } => eyre::eyre!("{e}").into(), |
There was a problem hiding this comment.
When skip >= configured, this new branch returns to fetch_witness, but its catch-all error arm at lines 1346–1360 still emits "Witness fetch deadline exceeded" without including e. Thus the routing defect this change makes recoverable produces a paging-style deadline warning even though no upstream attempt occurred, while the actual skip/configured cause is hidden behind the generic internal RPC error; distinguish this variant before emitting the deadline warning or log it separately.
AGENTS.md reference: AGENTS.md:L181-L181
Useful? React with 👍 / 👎.
1 participant
Summary
PR 6/6 of the #170 split, stacked on #198. The caller-contract
assert!that guarded the witness provider range becomes a typed error, so a routing bug fails one request instead of the process.vincent's review of #170: "It is also strictly better than the old 'hand it the data endpoints as a placeholder' behavior, which silently pointed witness calls at the wrong endpoints. Still worth being deliberate about
assert!vs a typed error in a validator that is expected to stay up."Scope change: the witness-less half is gone, superseded by #221
This PR originally did two things. The second — letting
RpcClientcarry no witness providers, so--witness-source r2stopped handing it--rpc-endpointas placeholder witness endpoints — has been dropped, because #221 removed the problem at its source while this PR sat in the queue.#221 deleted
--witness-sourceentirely and derives the mode from the--r2-*flags, with--witness-endpointnow always required (app.rs:415). R2 is tried first and the RPC witness endpoints are its fallback, so no placeholder is ever passed. Under that design an empty witness list is not a configuration to support — passing one would delete the fallback #221 deliberately added. The constructor's non-empty check therefore stays, and the merge commit records the resolution.What remains is the half vincent asked to be deliberate about, which is independent of how witnesses are configured.
The change
witness_round_robin'sassert!becomesWitnessFetchError::NoProviderInRange { skip, configured }(rpc_client.rs:236), following the shapeCodeFetchErroralready uses in this file (#[error(transparent)] Deadline(#[from] RpcDeadlineExceeded)plus the domain variant).The check is raised where the caller-computed value enters —
get_witness_light_with_deadline_from(rpc_client.rs:782, raised at:793) — rather than in the rotation it used to guard. The other two witness fetches build their range from the provider count and cannot be out of range, so leaving the guard deeper would have widened their error type for a case they cannot produce.witness_round_robinkeeps returningRpcDeadlineExceededand states its precondition as adebug_assert!(rpc_client.rs:859), beside the oneround_robin_with_backoffalready carries. Public signatures are unchanged, so mega-reth'sget_witness_with_deadlinecaller is untouched.Why not keep the
assert!. A routing bug in the trace server's witness-skip computation would take the process down on a request path. As a typed error it fails one request, and the reason label says which.What the variant now means. With the constructor rejecting an empty list, the fetch is unsatisfiable for exactly one reason: a caller's
skipran past the configured endpoints. That is a routing bug, not a transport condition and not a misconfiguration — it cannot be reached by deploying badly, only by computing a skip wrongly. The error's two fields describe exactly that case; the earlier form also fired on an out-of-bounds range end and reported it as a badskip, which no call site could produce.What deliberately did not change. Only
get_witness_light_with_deadline_from— the one method taking a caller-computed range — returnsWitnessFetchError. The full-range methods (get_witness_with_deadline,get_witness_light_with_deadline) keepRpcDeadlineExceeded; they pass0..len, where the range variant is unreachable, and threading the enum through them would have rippled into the trace server's error classification for no behavioural gain.get_witness_light_first_provider_onlystays infallible with a pinned0..1, its.expectnaming the startup contract — the same idiom as the.expect("None deadline cannot time out")beside it.Trace-server classification
From<WitnessFetchError> for DataProviderError(data_provider.rs:273) sendsDeadlinedown the existing method-based path andNoProviderInRangetoInternal. That distinction matters operationally:Timeout { Witness }feeds thedeadline_witnesserror reason, whose entire value is meaning "an upstream witness fetch ran out of budget" — a wiring bug landing there would page for the wrong incident.witness_range_failure_is_internal_not_a_witness_timeout(data_provider.rs:3175) pins both halves.Testing
cargo fmt --all --check,cargo clippy --workspace --all-targets --all-features(0 warnings),cargo sort --check, full workspace suite 491 passed / 0 failed,cargo test -p stateless-core --no-default-features --lib --no-runclean.New:
witness_fetch_out_of_range_returns_a_typed_error(rpc_client.rs:2152) andwitness_range_failure_is_internal_not_a_witness_timeout(data_provider.rs:3175).test_witness_fetch_skip_of_all_providers_panicsis removed — superseded by the typed-error test over the same call. Both new tests were mutation-checked (disabling the range check, and misrouting the variant toTimeout { Witness }, each kill their test). The constructor test keeps pinning that an empty witness list is rejected.Notes
Closes the last of the #170 split.
TODO-A-2in the internal ledger is satisfied by the typed error; the placeholder-endpoint half of that entry was satisfied by #221 instead.A quality pass landed in
a418e8e: the guard moved from the private rotation to the one public method taking a caller-computedskip, which deleted thedeadline_onlyadapter and itsunreachable!, bothmap_errsites and a bespokeexpectstring (net −16 lines inrpc_client.rs). Mutation-checked again at the new site.