From ae09a4f9a24cae2325b6b766e6d4c6b6691c0c38 Mon Sep 17 00:00:00 2001 From: "liquan.eth" Date: Thu, 3 Sep 2026 19:27:46 +0800 Subject: [PATCH] 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(