fix: type the unsatisfiable witness-provider-range failure - #199
Merged
flyq merged 12 commits intoSep 24, 2026
Merged
Conversation
Deletes the second sources of truth and the dead parameters/accessors the /simplify review found, with no observable change on canonical-block paths. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CHVpMMX9N69sUNbuKgpBVY
Deletes the `writer` parameter threaded through `validate_block`, `validate_block_deriving_updates`, `replay_block` and `verify_and_replay`, and with it EIP-3155 trace output. A feature deletion, not a cleanup. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CHVpMMX9N69sUNbuKgpBVY
…s_used from the executor Both are definitional rewrites of security-critical derivations, argued in the PR body: `trie_hash()` is `keccak256(encoded_2718())`, and mega-evm's `gas_used` is the last receipt's cumulative gas. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CHVpMMX9N69sUNbuKgpBVY
Block integrity verification becomes the RPC retry loop's finalize step and runs on the blocking pool; the advancer's pre-advance hooks and store commit run there too. Panic semantics are preserved and pinned by tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CHVpMMX9N69sUNbuKgpBVY
`WitnessDatabase` reads go through salt's `find()` with in-place decoding and stack-allocated key encodings; `LightWitness::metadata` stops cloning the `SaltValue` it only reads. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CHVpMMX9N69sUNbuKgpBVY
…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
Contributor
Claude review status
✅ Review clean Last reviewed: New this round: 0 finding(s), 0 question(s) · Resolved this round: 0 · Open questions: 0 |
|
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".
flyq
marked this pull request as draft
September 4, 2026 00:28
…tate-read-allocations # Conflicts: # bin/stateless-validator/src/app.rs # bin/stateless-validator/src/chain_sync.rs # bin/stateless-validator/src/metrics.rs # bin/stateless-validator/src/runner.rs # bin/stateless-validator/src/validator_db.rs # bin/stateless-validator/tests/integration.rs # crates/stateless-common/Cargo.toml # crates/stateless-common/src/metrics.rs # crates/stateless-common/src/rpc_client.rs # crates/stateless-core/src/chain_spec.rs # crates/stateless-core/src/db.rs # crates/stateless-core/src/executor.rs # crates/stateless-core/src/pipeline/advancer.rs # crates/stateless-core/src/pipeline/fetcher.rs # crates/stateless-core/src/pipeline/tests.rs # crates/stateless-r2/src/fetch.rs
… test bite The encoder docs justified themselves as "for per-state-read hot paths", which asks the next contributor an unmeasurable question; the decidable one is whether the caller needs to own the key. encode() is now documented as the owned form derived from the borrowing primaries. stack_key_encodings_match_encode compared each encoder against encode(), which delegates to it — true by construction, so it killed no mutation. It now asserts the literal byte layout, which catches a swapped concatenation (verified: the mutation fails the new test and passes the old one). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…/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>
flyq
marked this pull request as ready for review
September 20, 2026 16:34
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".
…a deadline fetch_witness's error arm predates WitnessFetchError: it logged every failure as "Witness fetch deadline exceeded" without the error, which was true while the only error was RpcDeadlineExceeded. NoProviderInRange now reaches the same arm, so a routing bug would surface as a witness timeout with a budget it never spent. Match the variant before logging: Deadline keeps the existing WARN, NoProviderInRange gets its own error! with skip and configured as fields. The returned error and the source metrics are unchanged. Addresses the Codex review on data_provider.rs (PRRT_kwDOPRLqcM6kKZdZ).
Troublor
approved these changes
Sep 23, 2026
vincent-k2026
approved these changes
Sep 24, 2026
Base automatically changed from
liquan/perf/witness-state-read-allocations
to
main
September 24, 2026 04:19
…ess-endpoints-and-typed-range-error
flyq
deleted the
liquan/fix/empty-witness-endpoints-and-typed-range-error
branch
September 24, 2026 04:39
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR 6/6 of the #170 split. #198 has merged and
mainis merged in as of29b7618, so this now targetsmaindirectly. The caller-contractassert!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. The full-range public signatures are unchanged, so mega-reth'sget_witness_with_deadlinecaller is untouched; onlyget_witness_light_with_deadline_from's error type changes, and its sole caller is the trace server'sfetch_witness.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:3191) pins both halves.fetch_witnesssplits its failure log the same way (data_provider.rs:1346):Deadlinekeeps theWitness fetch deadline exceededWARN with its budget fields, whileNoProviderInRangelogs its ownerror!carryingskip/configured(:1367), since no upstream attempt ran and a deadline line would misattribute it.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:3191).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. Thefetch_witnesslog split has no dedicated test:witness_routeskips only with at least two providers, so the variant is unreachable from that call site by construction, and the classification beside it is already pinned.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.