From ae09a4f9a24cae2325b6b766e6d4c6b6691c0c38 Mon Sep 17 00:00:00 2001 From: "liquan.eth" Date: Thu, 3 Sep 2026 19:27:46 +0800 Subject: [PATCH 1/2] fix: allow a witness-less RpcClient and type the unsatisfiable-range 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) Claude-Session: https://claude.ai/code/session_01CHVpMMX9N69sUNbuKgpBVY --- bin/debug-trace-server/src/data_provider.rs | 38 ++++++- bin/stateless-validator/src/app.rs | 7 +- crates/stateless-common/src/lib.rs | 2 +- crates/stateless-common/src/rpc_client.rs | 117 ++++++++++++++------ 4 files changed, 127 insertions(+), 37 deletions(-) diff --git a/bin/debug-trace-server/src/data_provider.rs b/bin/debug-trace-server/src/data_provider.rs index 23ed6da7..1ca34c71 100644 --- a/bin/debug-trace-server/src/data_provider.rs +++ b/bin/debug-trace-server/src/data_provider.rs @@ -45,7 +45,9 @@ use futures::{FutureExt, future::Shared}; use op_alloy_rpc_types::Transaction; use quick_cache::sync::Cache; use revm::state::Bytecode; -use stateless_common::{CodeFetchError, RpcClient, RpcDeadlineExceeded, WitnessSizeBreakdown}; +use stateless_common::{ + CodeFetchError, RpcClient, RpcDeadlineExceeded, WitnessFetchError, WitnessSizeBreakdown, +}; use stateless_core::{ ContractStore, LightWitness, StoreResult, db::StoreError, withdrawals::MptWitness, }; @@ -277,6 +279,18 @@ impl From for DataProviderError { } } +impl From for DataProviderError { + fn from(e: WitnessFetchError) -> Self { + match e { + // Only a blown deadline is a timeout. A range failure is a wiring bug in this + // process — routing it to `Timeout { Witness }` would fire the `deadline_witness` + // alarm, which must mean "an upstream witness fetch ran out of budget". + WitnessFetchError::Deadline(d) => d.into(), + WitnessFetchError::NoProviderInRange { .. } => eyre::eyre!("{e}").into(), + } + } +} + impl From for DataProviderError { fn from(e: CodeFetchError) -> Self { match e { @@ -3068,4 +3082,26 @@ mod tests { .into(); assert!(matches!(block_err, DataProviderError::Timeout { stage: TimeoutStage::Block, .. })); } + + /// A witness fetch whose provider range is unsatisfiable is a wiring bug, not a blown + /// budget: it must land on `Internal`, never on `Timeout { Witness }`. That bucket feeds + /// the `deadline_witness` error reason, whose whole value is meaning "an upstream witness + /// fetch ran out of time" — a wiring bug landing there would page for the wrong incident. + /// The deadline variant still classifies by method, exactly as before. + #[test] + fn witness_range_failure_is_internal_not_a_witness_timeout() { + let range_err: DataProviderError = + WitnessFetchError::NoProviderInRange { skip: 2, configured: 1 }.into(); + assert!(matches!(range_err, DataProviderError::Internal(_)), "got {range_err:?}"); + + let deadline_err: DataProviderError = WitnessFetchError::Deadline(RpcDeadlineExceeded { + method: stateless_common::RpcMethod::MegaGetBlockWitness, + elapsed: Duration::from_secs(3), + }) + .into(); + assert!(matches!( + deadline_err, + DataProviderError::Timeout { stage: TimeoutStage::Witness, .. } + )); + } } diff --git a/bin/stateless-validator/src/app.rs b/bin/stateless-validator/src/app.rs index d5347f58..712bda0d 100644 --- a/bin/stateless-validator/src/app.rs +++ b/bin/stateless-validator/src/app.rs @@ -331,8 +331,6 @@ pub async fn run() -> Result<()> { ..rpc_defaults } .with_metrics(Arc::new(metrics::ValidatorMetrics)); - // In R2 mode the RpcClient's witness providers are never used, but its constructor requires - // a non-empty list — hand it the data endpoints as a placeholder. let data_apis: Vec<&str> = args.rpc_endpoint.iter().map(String::as_str).collect(); let r2_witness = match args.witness_source { WitnessSource::Rpc => { @@ -361,8 +359,11 @@ pub async fn run() -> Result<()> { } }; + // In R2 mode the client carries no witness providers: witnesses come straight from R2, and + // a witness RPC call that slipped through fails structurally instead of quietly asking the + // data endpoints for `mega_getBlockWitness`. let witness_apis: Vec<&str> = if r2_witness.is_some() { - data_apis.clone() + Vec::new() } else { args.witness_endpoint.iter().map(String::as_str).collect() }; diff --git a/crates/stateless-common/src/lib.rs b/crates/stateless-common/src/lib.rs index aeed85f8..0ee6fbdc 100644 --- a/crates/stateless-common/src/lib.rs +++ b/crates/stateless-common/src/lib.rs @@ -4,7 +4,7 @@ pub use metrics::{RpcMethod, RpcMetrics}; pub mod rpc_client; pub use rpc_client::{ BackoffPolicy, CodeFetchError, RpcClient, RpcClientConfig, RpcDeadlineExceeded, - SetValidatedBlocksResponse, WitnessRequestKeys, + SetValidatedBlocksResponse, WitnessFetchError, WitnessRequestKeys, }; pub mod witness_encoding; pub use witness_encoding::{ diff --git a/crates/stateless-common/src/rpc_client.rs b/crates/stateless-common/src/rpc_client.rs index a54cc905..78ce374c 100644 --- a/crates/stateless-common/src/rpc_client.rs +++ b/crates/stateless-common/src/rpc_client.rs @@ -254,6 +254,24 @@ pub struct SetValidatedBlocksResponse { pub last_validated_block: (U64, B256), } +/// Error returned by the witness fetches that take a caller-computed provider range. +/// +/// `NoProviderInRange` is a wiring failure, not a transport one: either the caller's `skip` +/// selected past the configured witness endpoints, or the client carries none at all (a +/// deployment that sources witnesses elsewhere — the validator's R2 mode). Both binaries +/// reject an empty witness configuration at startup, so it stays unreachable in production; +/// it is a typed error rather than an `assert!` so a routing bug fails one request instead of +/// the process. +#[derive(Debug, thiserror::Error)] +pub enum WitnessFetchError { + #[error( + "witness fetch selected providers {skip}.. of {configured} configured — no witness provider in range" + )] + NoProviderInRange { skip: usize, configured: usize }, + #[error(transparent)] + Deadline(#[from] RpcDeadlineExceeded), +} + /// Errors returned by [`RpcClient::get_codes`] / [`RpcClient::get_codes_with_deadline`]. /// /// - `VerificationFailure` is deterministic (upstream returned bytecode whose keccak does not match @@ -320,7 +338,10 @@ impl RpcClient { /// # Arguments /// * `data_apis` - HTTP URLs of the standard JSON-RPC endpoints for blocks and contract data /// (tried in order, non-empty) - /// * `witness_apis` - HTTP URLs of the witness RPC endpoints (tried in order, non-empty) + /// * `witness_apis` - HTTP URLs of the witness RPC endpoints (tried in order). May be empty + /// when the deployment sources witnesses elsewhere (the validator's R2 witness mode); a + /// witness call on such a client returns [`WitnessFetchError::NoProviderInRange`] instead of + /// silently retrying against the wrong endpoints /// * `config` - Configuration controlling verification, retry, and concurrency behavior /// * `report_api` - Optional HTTP URL of the endpoint for reporting validated blocks pub fn new_with_config( @@ -332,9 +353,6 @@ impl RpcClient { if data_apis.is_empty() { return Err(eyre!("At least one data API URL must be provided")); } - if witness_apis.is_empty() { - return Err(eyre!("At least one witness API URL must be provided")); - } // One shared HTTP client for every provider (connection pools are keyed per host), so // the connect-phase bound applies uniformly to data, witness, and report endpoints. @@ -726,7 +744,8 @@ impl RpcClient { decode_witness_response, "Witness decoded", ) - .await?; + .await + .map_err(deadline_only)?; if let Some(ref metrics) = self.config.metrics { metrics.on_witness_fetch(WitnessSizeBreakdown::new(&witness.0, &witness.1)); @@ -757,7 +776,9 @@ impl RpcClient { hash: B256, deadline: Option, ) -> std::result::Result<(LightWitness, MptWitness), RpcDeadlineExceeded> { - self.get_witness_light_with_deadline_from(0, number, hash, deadline).await + self.get_witness_light_with_deadline_from(0, number, hash, deadline) + .await + .map_err(deadline_only) } /// Like [`Self::get_witness_light_with_deadline`], but skips the first `skip` witness @@ -766,15 +787,16 @@ impl RpcClient { /// position in the full configured witness endpoint list, and the shared witness /// concurrency cap still applies. /// - /// # Panics - /// Panics if `skip >= witness_provider_count()` — at least one provider must remain. + /// Returns [`WitnessFetchError::NoProviderInRange`] when `skip` selects past the + /// configured witness endpoints — a routing bug fails this one request rather than the + /// process. pub async fn get_witness_light_with_deadline_from( &self, skip: usize, number: u64, hash: B256, deadline: Option, - ) -> std::result::Result<(LightWitness, MptWitness), RpcDeadlineExceeded> { + ) -> std::result::Result<(LightWitness, MptWitness), WitnessFetchError> { self.witness_round_robin( skip..self.witness_providers.len(), number, @@ -809,7 +831,7 @@ impl RpcClient { "Witness light-decoded", ) .await - .expect("None deadline cannot time out") + .expect("pinned 0..1 range and a None deadline cannot fail") } /// Shared `mega_getBlockWitness` retry loop: primary-failover rounds (always start from @@ -821,8 +843,8 @@ impl RpcClient { /// the logged endpoint labels stay aligned with the full configured list because each /// label bakes in its original index (see [`endpoint_label`]). /// - /// # Panics - /// Panics if `providers` is empty or out of bounds — at least one provider must remain. + /// An empty or out-of-bounds `providers` range is a wiring failure, surfaced as + /// [`WitnessFetchError::NoProviderInRange`] rather than a panic. // A `warn`-level span (not the usual `info`) so it stays enabled at the default `warn` log // filter: the generic retry loop's per-attempt failure logs then inherit `block_number`, // which they cannot see otherwise, so an endpoint stall/error is traceable to its block. @@ -835,12 +857,11 @@ impl RpcClient { deadline: Option, decode: fn(&str) -> std::result::Result, trace_msg: &'static str, - ) -> std::result::Result { - assert!( - !providers.is_empty() && providers.end <= self.witness_providers.len(), - "witness provider range ({providers:?}) must select at least one of {} providers", - self.witness_providers.len() - ); + ) -> std::result::Result { + let configured = self.witness_providers.len(); + if providers.is_empty() || providers.end > configured { + return Err(WitnessFetchError::NoProviderInRange { skip: providers.start, configured }); + } // Deadline-bound witness attempts run under the reserve-half policy: the tightest of // the configured ceiling, the general per-attempt timeout, and — recomputed at each // attempt, after any permit wait — half of what the call still has, so neither a @@ -876,6 +897,7 @@ impl RpcClient { }, ) .await + .map_err(WitnessFetchError::Deadline) } /// Reports a range of validated blocks via the dedicated report endpoint. @@ -1613,6 +1635,19 @@ async fn verify_block_on_blocking_pool(block: Block) -> Result RpcDeadlineExceeded { + match e { + WitnessFetchError::Deadline(d) => d, + WitnessFetchError::NoProviderInRange { skip, configured } => unreachable!( + "full-range witness fetch on a client with no witness providers \ + (skip={skip}, configured={configured})" + ), + } +} + /// Verifies structural integrity of a block fetched from RPC. /// /// Checks: @@ -1833,11 +1868,12 @@ mod tests { .to_string() .contains("At least one data API") ); - assert!( - RpcClient::new(&[LOCALHOST_A], &[]) - .unwrap_err() - .to_string() - .contains("At least one witness API") + // An empty witness list is a legal configuration (the validator's R2 witness mode); + // a witness call on such a client returns `NoProviderInRange` instead. + assert_eq!( + RpcClient::new(&[LOCALHOST_A], &[]).unwrap().witness_provider_count(), + 0, + "an empty witness list must construct" ); for endpoints in [&[LOCALHOST_B][..], &[LOCALHOST_B, "http://localhost:8547"]] { @@ -2116,6 +2152,32 @@ mod tests { hb.stop().unwrap(); } + /// A `skip` past the configured witness endpoints — or a client built with none at all — + /// is a wiring failure, and must fail this one request rather than take the process down. + #[tokio::test] + async fn witness_fetch_out_of_range_returns_a_typed_error() { + let client = RpcClient::new(&[LOCALHOST_A], &[LOCALHOST_B]).unwrap(); + let err = client + .get_witness_light_with_deadline_from(1, 7, B256::ZERO, None) + .await + .expect_err("skip == provider count leaves no provider"); + assert!( + matches!(err, WitnessFetchError::NoProviderInRange { skip: 1, configured: 1 }), + "unexpected error: {err:?}" + ); + + // Same variant covers the no-witness-providers deployment (R2 mode). + let witnessless = RpcClient::new(&[LOCALHOST_A], &[]).unwrap(); + let err = witnessless + .get_witness_light_with_deadline_from(0, 7, B256::ZERO, None) + .await + .expect_err("a client with no witness providers cannot fetch a witness"); + assert!( + matches!(err, WitnessFetchError::NoProviderInRange { skip: 0, configured: 0 }), + "unexpected error: {err:?}" + ); + } + /// `get_witness` pins `rr_start = 0`, so every round visits the primary first and only /// falls through to the backup on failure. We can't easily make the primary succeed in /// a unit test (a valid witness payload needs real cryptographic proof material), but @@ -2269,15 +2331,6 @@ mod tests { hc.stop().unwrap(); } - /// Skipping every configured witness provider is a caller bug and must panic loudly - /// instead of silently retrying over an empty provider set. - #[tokio::test] - #[should_panic(expected = "must select at least one")] - async fn test_witness_fetch_skip_of_all_providers_panics() { - let client = RpcClient::new(&[LOCALHOST_A], &[LOCALHOST_B]).unwrap(); - let _ = client.get_witness_light_with_deadline_from(1, 1, BlockHash::ZERO, None).await; - } - /// Serves `mega_getBlockWitness` returning a stub that decodes-fails, while recording /// the provider's label to a shared `order` log on each hit. Used to verify call routing. async fn start_ordered_witness_rpc( From a418e8e037fd069d94589b07d69ab1e118e3f3ab Mon Sep 17 00:00:00 2001 From: "liquan.eth" Date: Sun, 20 Sep 2026 16:29:17 +0800 Subject: [PATCH 2/2] refactor(common): check the witness skip where it enters, not in the rotation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- bin/debug-trace-server/src/data_provider.rs | 8 +- crates/stateless-common/src/rpc_client.rs | 82 ++++++++++----------- 2 files changed, 43 insertions(+), 47 deletions(-) diff --git a/bin/debug-trace-server/src/data_provider.rs b/bin/debug-trace-server/src/data_provider.rs index ca218595..90c93887 100644 --- a/bin/debug-trace-server/src/data_provider.rs +++ b/bin/debug-trace-server/src/data_provider.rs @@ -3168,11 +3168,9 @@ mod tests { assert!(matches!(block_err, DataProviderError::Timeout { stage: TimeoutStage::Block, .. })); } - /// A witness fetch whose provider range is unsatisfiable is a wiring bug, not a blown - /// budget: it must land on `Internal`, never on `Timeout { Witness }`. That bucket feeds - /// the `deadline_witness` error reason, whose whole value is meaning "an upstream witness - /// fetch ran out of time" — a wiring bug landing there would page for the wrong incident. - /// The deadline variant still classifies by method, exactly as before. + /// A range failure is a wiring bug: it must land on `Internal`, never on the + /// `deadline_witness` alarm's `Timeout { Witness }` (rationale at the `From` impl). The + /// deadline arm is asserted too — it is the delegation that keeps that alarm working. #[test] fn witness_range_failure_is_internal_not_a_witness_timeout() { let range_err: DataProviderError = diff --git a/crates/stateless-common/src/rpc_client.rs b/crates/stateless-common/src/rpc_client.rs index 6e1a7d47..c67f4a4a 100644 --- a/crates/stateless-common/src/rpc_client.rs +++ b/crates/stateless-common/src/rpc_client.rs @@ -231,12 +231,10 @@ pub struct SetValidatedBlocksResponse { /// past the configured witness endpoints. The constructor rejects an empty endpoint list, so /// reaching this means a routing bug in the skip computation rather than a misconfiguration. /// It is a typed error rather than an `assert!` so such a bug fails one request instead of the -/// process. +/// process — which is the whole reason this type exists. #[derive(Debug, thiserror::Error)] pub enum WitnessFetchError { - #[error( - "witness fetch selected providers {skip}.. of {configured} configured — no witness provider in range" - )] + #[error("witness fetch skip={skip} leaves none of {configured} configured providers")] NoProviderInRange { skip: usize, configured: usize }, #[error(transparent)] Deadline(#[from] RpcDeadlineExceeded), @@ -308,8 +306,7 @@ impl RpcClient { /// # Arguments /// * `data_apis` - HTTP URLs of the standard JSON-RPC endpoints for blocks and contract data /// (tried in order, non-empty) - /// * `witness_apis` - HTTP URLs of the witness RPC endpoints (tried in order, non-empty). R2 - /// does not replace them: it is tried first and these remain the fallback + /// * `witness_apis` - HTTP URLs of the witness RPC endpoints (tried in order, non-empty) /// * `config` - Configuration controlling verification, retry, and concurrency behavior /// * `report_api` - Optional HTTP URL of the endpoint for reporting validated blocks pub fn new_with_config( @@ -731,8 +728,7 @@ impl RpcClient { decode_witness_response, "Witness decoded", ) - .await - .map_err(deadline_only)?; + .await?; if let Some(ref metrics) = self.config.metrics { metrics.on_witness_fetch(WitnessSizeBreakdown::new(&witness.0, &witness.1)); @@ -763,9 +759,15 @@ impl RpcClient { hash: B256, deadline: Option, ) -> std::result::Result<(LightWitness, MptWitness), RpcDeadlineExceeded> { - self.get_witness_light_with_deadline_from(0, number, hash, deadline) - .await - .map_err(deadline_only) + self.witness_round_robin( + 0..self.witness_providers.len(), + number, + hash, + deadline, + decode_witness_response_light, + "Witness light-decoded", + ) + .await } /// Like [`Self::get_witness_light_with_deadline`], but skips the first `skip` witness @@ -784,15 +786,23 @@ impl RpcClient { hash: B256, deadline: Option, ) -> std::result::Result<(LightWitness, MptWitness), WitnessFetchError> { - self.witness_round_robin( - skip..self.witness_providers.len(), - number, - hash, - deadline, - decode_witness_response_light, - "Witness light-decoded", - ) - .await + // Checked here, where the caller-computed value enters, rather than deeper in the + // rotation: every other witness fetch builds its range from the provider count and + // cannot be out of range, so this is the only place the check has anything to do. + let configured = self.witness_providers.len(); + if skip >= configured { + return Err(WitnessFetchError::NoProviderInRange { skip, configured }); + } + Ok(self + .witness_round_robin( + skip..configured, + number, + hash, + deadline, + decode_witness_response_light, + "Witness light-decoded", + ) + .await?) } /// Like [`Self::get_witness_light`], but consults only the FIRST witness provider — @@ -818,7 +828,7 @@ impl RpcClient { "Witness light-decoded", ) .await - .expect("pinned 0..1 range and a None deadline cannot fail") + .expect("None deadline cannot time out") } /// Shared `mega_getBlockWitness` retry loop: primary-failover rounds (always start from @@ -830,8 +840,9 @@ impl RpcClient { /// the logged endpoint labels stay aligned with the full configured list because each /// label bakes in its original index (see [`endpoint_label`]). /// - /// An empty or out-of-bounds `providers` range is a wiring failure, surfaced as - /// [`WitnessFetchError::NoProviderInRange`] rather than a panic. + /// Every caller builds `providers` from the configured provider count, so the range is + /// non-empty and in bounds by construction; the one caller-supplied value (`skip`) is + /// checked in [`Self::get_witness_light_with_deadline_from`] before it gets here. // A `warn`-level span (not the usual `info`) so it stays enabled at the default `warn` log // filter: the generic retry loop's per-attempt failure logs then inherit `block_number`, // which they cannot see otherwise, so an endpoint stall/error is traceable to its block. @@ -844,11 +855,12 @@ impl RpcClient { deadline: Option, decode: fn(&str) -> std::result::Result, trace_msg: &'static str, - ) -> std::result::Result { - let configured = self.witness_providers.len(); - if providers.is_empty() || providers.end > configured { - return Err(WitnessFetchError::NoProviderInRange { skip: providers.start, configured }); - } + ) -> std::result::Result { + debug_assert!( + !providers.is_empty() && providers.end <= self.witness_providers.len(), + "witness provider range ({providers:?}) must select at least one of {} providers", + self.witness_providers.len() + ); // Deadline-bound witness attempts run under the reserve-half policy: the tightest of // the configured ceiling, the general per-attempt timeout, and — recomputed at each // attempt, after any permit wait — half of what the call still has, so neither a @@ -884,7 +896,6 @@ impl RpcClient { }, ) .await - .map_err(WitnessFetchError::Deadline) } /// Reports a range of validated blocks via the dedicated report endpoint. @@ -1628,19 +1639,6 @@ async fn verify_block_on_blocking_pool(block: Block) -> Result RpcDeadlineExceeded { - match e { - WitnessFetchError::Deadline(d) => d, - WitnessFetchError::NoProviderInRange { skip, configured } => unreachable!( - "full-range witness fetch on a client with no witness providers \ - (skip={skip}, configured={configured})" - ), - } -} - /// Verifies structural integrity of a block fetched from RPC. /// /// Checks: