fix: type the unsatisfiable witness-provider-range failure

bin/debug-trace-server/src/data_provider.rs

@@ -46,13 +46,14 @@ 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,
 };
 use stateless_db::ContractCache;
-use tracing::{debug, instrument, trace, warn};
+use tracing::{debug, error, instrument, trace, warn};
 
 use crate::{
     block_data_cache::BlockDataCache,
@@ -269,6 +270,18 @@ impl From<RpcDeadlineExceeded> for DataProviderError {
     }
 }
 
+impl From<WitnessFetchError> 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<CodeFetchError> for DataProviderError {
     fn from(e: CodeFetchError) -> Self {
         match e {
@@ -1332,19 +1345,35 @@ async fn fetch_witness(
         }
         Err(e) => {
             metrics.record_request(false, start.elapsed().as_secs_f64());
-            let budget = deadline.saturating_duration_since(start);
-            // Attribution-grade context for the next timeout incident: the effective stage
-            // budget, the route taken, and whether the old-block clamp applied — the client
-            // only ever sees the generic `-32001` message.
-            warn!(
-                block_number,
-                block_hash = %block_hash,
-                source,
-                old_block = is_old_block(db_tip, block_number),
-                budget_ms = budget.as_millis() as u64,
-                elapsed_ms = start.elapsed().as_millis() as u64,
-                "Witness fetch deadline exceeded",
-            );
+            match &e {
+                WitnessFetchError::Deadline(_) => {
+                    let budget = deadline.saturating_duration_since(start);
+                    // Attribution-grade context for the next timeout incident: the effective
+                    // stage budget, the route taken, and whether the old-block clamp applied —
+                    // the client only ever sees the generic `-32001` message.
+                    warn!(
+                        block_number,
+                        block_hash = %block_hash,
+                        source,
+                        old_block = is_old_block(db_tip, block_number),
+                        budget_ms = budget.as_millis() as u64,
+                        elapsed_ms = start.elapsed().as_millis() as u64,
+                        "Witness fetch deadline exceeded",
+                    );
+                }
+                // A routing bug, not a timeout: no upstream attempt ran, so the deadline
+                // warning and its budget fields would misattribute it.
+                WitnessFetchError::NoProviderInRange { skip, configured } => {
+                    error!(
+                        block_number,
+                        block_hash = %block_hash,
+                        source,
+                        skip,
+                        configured,
+                        "Witness route selected no configured provider",
+                    );
+                }
+            }
             Err(e.into())
         }
     }
@@ -3154,4 +3183,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, .. }
+        ));
+    }
 }

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`.

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<Instant>,
     ) -> 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<Instant>,
-    ) -> 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<T, crate::WitnessDecodingError>,
         trace_msg: &'static str,
     ) -> std::result::Result<T, RpcDeadlineExceeded> {
-        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(