diff --git a/bin/debug-trace-server/src/data_provider.rs b/bin/debug-trace-server/src/data_provider.rs index 3b751795..90c93887 100644 --- a/bin/debug-trace-server/src/data_provider.rs +++ b/bin/debug-trace-server/src/data_provider.rs @@ -46,7 +46,8 @@ use op_alloy_rpc_types::Transaction; use quick_cache::sync::Cache; use revm::state::Bytecode; use stateless_common::{ - CodeFetchError, R2Band, RpcClient, RpcDeadlineExceeded, WitnessSizeBreakdown, r2_band, + CodeFetchError, R2Band, RpcClient, RpcDeadlineExceeded, WitnessFetchError, + WitnessSizeBreakdown, r2_band, }; use stateless_core::{ ContractStore, LightWitness, StoreResult, db::StoreError, withdrawals::MptWitness, @@ -269,6 +270,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 { @@ -3154,4 +3167,24 @@ mod tests { .into(); assert!(matches!(block_err, DataProviderError::Timeout { stage: TimeoutStage::Block, .. })); } + + /// 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 = + 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/crates/stateless-common/src/lib.rs b/crates/stateless-common/src/lib.rs index c8821ad3..d39a4c49 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::{ CodeFetchError, RpcClient, RpcClientConfig, RpcDeadlineExceeded, SetValidatedBlocksResponse, - WitnessRequestKeys, + WitnessFetchError, WitnessRequestKeys, }; /// Exponential-backoff policy used by [`RpcClient`]'s round-level retry loop: `initial` is the /// first sleep duration; each round doubles it up to `max`. diff --git a/crates/stateless-common/src/rpc_client.rs b/crates/stateless-common/src/rpc_client.rs index 2493aa74..c67f4a4a 100644 --- a/crates/stateless-common/src/rpc_client.rs +++ b/crates/stateless-common/src/rpc_client.rs @@ -225,6 +225,21 @@ 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: the caller's `skip` selected +/// 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 — which is the whole reason this type exists. +#[derive(Debug, thiserror::Error)] +pub enum WitnessFetchError { + #[error("witness fetch skip={skip} leaves none of {configured} configured providers")] + 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 @@ -744,7 +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 + 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 @@ -753,24 +776,33 @@ 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> { - self.witness_round_robin( - skip..self.witness_providers.len(), - number, - hash, - deadline, - decode_witness_response_light, - "Witness light-decoded", - ) - .await + ) -> std::result::Result<(LightWitness, MptWitness), WitnessFetchError> { + // 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 — @@ -808,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`]). /// - /// # Panics - /// Panics if `providers` is empty or out of bounds — at least one provider must remain. + /// 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. @@ -823,7 +856,7 @@ impl RpcClient { decode: fn(&str) -> std::result::Result, trace_msg: &'static str, ) -> std::result::Result { - assert!( + 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() @@ -2113,6 +2146,21 @@ mod tests { hb.stop().unwrap(); } + /// A `skip` past the configured witness endpoints 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:?}" + ); + } + /// `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 @@ -2266,15 +2314,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(