From 0b90bee3fc3e76fd8edd67a2974688240035be6a Mon Sep 17 00:00:00 2001 From: Haiyue Chen Date: Thu, 30 Jul 2026 13:43:11 +0200 Subject: [PATCH 1/9] feat(starknet): probe RPC providers for the chain identity they serve Adds everything needed to check that a foreign-chain RPC provider is on the network the operator intended, implemented end to end for Starknet: - `expected_chain_identity` per chain in config, plus seeded templates. - A `ChainIdentity` value and `ChainIdentityInspector` trait, implemented for Starknet over `starknet_chainId`. - `FanOut` re-keyed by `ProviderId`, so fan-out failures name the provider instead of a list position, and `FanOut::chain_identities` to ask every provider concurrently under a per-provider timeout. - `probe::probe_all_providers`, reporting one typed status per configured provider, with the chains that have no identity impl yet reported as such rather than silently omitted. Nothing calls the probe yet: node wiring, metrics and the debug endpoint follow. The remaining chains get their own `ChainIdentity` impls after that. --- Cargo.lock | 2 + crates/foreign-chain-health-check/Cargo.toml | 4 +- crates/foreign-chain-health-check/src/lib.rs | 9 +- .../foreign-chain-health-check/src/probe.rs | 484 ++++++++++++++++++ crates/foreign-chain-inspector/src/lib.rs | 122 ++++- .../src/starknet/inspector.rs | 22 +- .../foreign-chain-inspector/tests/common.rs | 1 + .../foreign-chain-inspector/tests/fanout.rs | 8 +- .../tests/starknet_inspector.rs | 124 ++++- .../tests/starknet_rpc_manual.rs | 23 + .../src/starknet.rs | 92 +++- .../src/types/foreign_chain.rs | 13 +- crates/node-config/src/foreign_chains.rs | 1 + .../node/src/providers/verify_foreign_tx.rs | 6 +- docs/foreign-chain-transactions.md | 11 + 15 files changed, 898 insertions(+), 24 deletions(-) create mode 100644 crates/foreign-chain-health-check/src/probe.rs diff --git a/Cargo.lock b/Cargo.lock index aea7899224..0ac03da669 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3811,11 +3811,13 @@ dependencies = [ "foreign-chain-inspector", "foreign-chain-rpc-auth", "foreign-chain-rpc-interfaces", + "futures", "hex", "http", "httpmock", "mpc-node-config", "near-mpc-bounded-collections", + "near-mpc-contract-interface", "serde_json", "tokio", ] diff --git a/crates/foreign-chain-health-check/Cargo.toml b/crates/foreign-chain-health-check/Cargo.toml index 1098570f6e..a1827e606c 100644 --- a/crates/foreign-chain-health-check/Cargo.toml +++ b/crates/foreign-chain-health-check/Cargo.toml @@ -14,15 +14,17 @@ clap = { workspace = true, optional = true } foreign-chain-inspector = { workspace = true } foreign-chain-rpc-auth = { workspace = true } foreign-chain-rpc-interfaces = { workspace = true } +futures = { workspace = true } hex = { workspace = true } http = { workspace = true } mpc-node-config = { workspace = true } +near-mpc-bounded-collections = { workspace = true } +near-mpc-contract-interface = { workspace = true } tokio = { workspace = true } [dev-dependencies] assert_matches = { workspace = true } httpmock = { workspace = true } -near-mpc-bounded-collections = { workspace = true } serde_json = { workspace = true } [lints] diff --git a/crates/foreign-chain-health-check/src/lib.rs b/crates/foreign-chain-health-check/src/lib.rs index dcb521b9c4..6b4bb194eb 100644 --- a/crates/foreign-chain-health-check/src/lib.rs +++ b/crates/foreign-chain-health-check/src/lib.rs @@ -1,10 +1,13 @@ -//! Foreign-chain RPC provider health checks: probe every configured provider -//! with a fixed golden request and report a per-provider result. Sui is the -//! exception — see `run_sui`. +//! Foreign-chain RPC provider health checks, in two independent routes: +//! +//! * [`check_all_providers`] replays a fixed golden transaction per chain. +//! * [`probe::probe_all_providers`] asks each provider for the network it serves and compares that +//! against the operator's configured expectation. mod checks; mod golden; mod network; +pub mod probe; mod results; use std::future::Future; diff --git a/crates/foreign-chain-health-check/src/probe.rs b/crates/foreign-chain-health-check/src/probe.rs new file mode 100644 index 0000000000..1570090b12 --- /dev/null +++ b/crates/foreign-chain-health-check/src/probe.rs @@ -0,0 +1,484 @@ +//! Asks every configured RPC provider which network it serves and compares the answer against the +//! operator's `expected_chain_identity`. + +use std::collections::BTreeMap; +use std::time::Duration; + +use foreign_chain_inspector::starknet::inspector::StarknetInspector; +use foreign_chain_inspector::{ + ChainIdentity, FanOut, ForeignChainInspectionError, ProviderFailure, +}; +use mpc_node_config::{ForeignChainConfig, ForeignChainProviderConfig, ForeignChainsConfig}; +use near_mpc_bounded_collections::NonEmptyVec; +use near_mpc_contract_interface::types::{ForeignChain, ProviderId}; + +use crate::prepare_jsonrpc; + +/// One provider's verdict. Anything other than [`ProviderStatus::Healthy`] is unhealthy. +/// +/// Carries no rendered RPC error: `Path`/`Query` auth splices the operator's API key into the URL, +/// and upstream errors interpolate that URL into their text. Dropping the text here keeps the key +/// out of anything built from a report. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum ProviderStatus { + Healthy, + WrongNetwork { + expected: ChainIdentity, + observed: ChainIdentity, + }, + /// DNS, TLS, connection refused, 5xx, or rate limiting. + Unreachable, + /// The provider answered and refused: credentials invalid, or not enabled for this chain. + RequestRejected, + MalformedResponse, + TimedOut, + /// The RPC client could not be built, usually an unresolvable auth token. + ClientSetupFailed, + /// The chain is configured without an `expected_chain_identity`, so its providers cannot be + /// checked. Reported rather than skipped: silence would read as healthy. + MissingExpectedIdentity, + /// The node can inspect this chain's transactions but has no identity probe for it yet. + ProbeNotImplemented, +} + +impl ProviderStatus { + pub fn is_healthy(&self) -> bool { + matches!(self, Self::Healthy) + } +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ProviderHealth { + pub chain: ForeignChain, + pub provider: ProviderId, + pub status: ProviderStatus, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub struct ProviderCounts { + pub configured: usize, + pub healthy: usize, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ProbeReport { + rows: Vec, +} + +impl ProbeReport { + pub fn rows(&self) -> &[ProviderHealth] { + &self.rows + } + + /// Only configured chains appear, never reports on a chain the operator did not configure. + pub fn counts_per_chain(&self) -> BTreeMap { + let mut counts: BTreeMap = BTreeMap::new(); + for row in &self.rows { + let entry = counts.entry(row.chain).or_default(); + entry.configured += 1; + if row.status.is_healthy() { + entry.healthy += 1; + } + } + counts + } +} + +/// Probe every configured provider concurrently. +/// +/// Each provider is tried up to `max_retries` times with `timeout_sec` per try +/// This returns within the largest configured `timeout_sec * max_retries`. +pub async fn probe_all_providers(config: &ForeignChainsConfig) -> ProbeReport { + let probe_attempts = config + .iter_chains() + .map(|(chain, chain_config)| async move { + match chain { + ForeignChain::Starknet => { + probe_chain(chain, chain_config, |provider| { + Ok(StarknetInspector::new(prepare_jsonrpc(provider)?)) + }) + .await + } + // Other chains to be added in follow up PRs + _ => rows_of(chain, chain_config, ProviderStatus::ProbeNotImplemented), + } + }); + + let report_rows = futures::future::join_all(probe_attempts).await.concat(); + ProbeReport { rows: report_rows } +} + +async fn probe_chain( + chain: ForeignChain, + config: &ForeignChainConfig, + new_inspector: impl Fn(&ForeignChainProviderConfig) -> anyhow::Result, +) -> Vec +where + I: foreign_chain_inspector::ChainIdentityInspector + Clone + Send + Sync + 'static, +{ + let Some(expected) = config.expected_chain_identity.clone() else { + return rows_of(chain, config, ProviderStatus::MissingExpectedIdentity); + }; + let expected = ChainIdentity::from(expected); + + let mut inspectors = Vec::new(); + let mut rows = Vec::new(); + for (name, provider) in config.providers.iter() { + let provider_id = ProviderId(name.as_str().to_owned()); + match new_inspector(provider) { + Ok(inspector) => inspectors.push((provider_id, inspector)), + Err(_) => rows.push(ProviderHealth { + chain, + provider: provider_id, + status: ProviderStatus::ClientSetupFailed, + }), + } + } + + let Ok(inspectors) = NonEmptyVec::try_from(inspectors) else { + return rows; + }; + + let timeout = Duration::from_secs(config.timeout_sec.get()); + let identities = FanOut::new(inspectors) + .chain_identities(timeout, config.max_retries) + .await; + for (provider, identity) in identities { + rows.push(ProviderHealth { + chain, + provider, + status: classify(&expected, identity), + }); + } + rows +} + +fn rows_of( + chain: ForeignChain, + config: &ForeignChainConfig, + status: ProviderStatus, +) -> Vec { + config + .providers + .keys() + .map(|name| ProviderHealth { + chain, + provider: ProviderId(name.as_str().to_owned()), + status: status.clone(), + }) + .collect() +} + +fn classify( + expected: &ChainIdentity, + identity: Result, +) -> ProviderStatus { + match identity { + Ok(observed) if &observed == expected => ProviderStatus::Healthy, + Ok(observed) => ProviderStatus::WrongNetwork { + expected: expected.clone(), + observed, + }, + Err(error) => match error.provider_failure() { + Some(ProviderFailure::Unreachable) => ProviderStatus::Unreachable, + Some(ProviderFailure::Rejected) => ProviderStatus::RequestRejected, + Some(ProviderFailure::TimedOut) => ProviderStatus::TimedOut, + Some(ProviderFailure::Malformed) => ProviderStatus::MalformedResponse, + // The probe inspects no transaction, so a transaction-level error means an impl + // answered outside its contract. + None => ProviderStatus::MalformedResponse, + }, + } +} + +#[cfg(test)] +#[expect(non_snake_case)] +mod tests { + use super::*; + use mpc_node_config::{AuthConfig, TokenConfig}; + use near_mpc_bounded_collections::NonEmptyBTreeMap; + use std::num::NonZeroU64; + + /// Starknet mainnet's chain id, `SN_MAIN` in ASCII. + const MAINNET: &str = "0x534e5f4d41494e"; + const SEPOLIA: &str = "0x534e5f5345504f4c4941"; + /// Reserved as "discard", so nothing listens there. + const CLOSED_PORT_URL: &str = "http://127.0.0.1:9"; + + fn provider(rpc_url: &str) -> ForeignChainProviderConfig { + ForeignChainProviderConfig { + rpc_url: rpc_url.to_string(), + auth: AuthConfig::None, + } + } + + fn chain_config( + expected: Option<&str>, + providers: NonEmptyBTreeMap< + mpc_node_config::foreign_chains::RpcProviderName, + ForeignChainProviderConfig, + >, + ) -> ForeignChainConfig { + ForeignChainConfig { + timeout_sec: NonZeroU64::new(1).unwrap(), + max_retries: NonZeroU64::new(1).unwrap(), + expected_chain_identity: expected.map(str::to_string), + providers, + } + } + + fn one_provider( + name: &str, + rpc_url: &str, + ) -> NonEmptyBTreeMap< + mpc_node_config::foreign_chains::RpcProviderName, + ForeignChainProviderConfig, + > { + NonEmptyBTreeMap::new(name.to_string().into(), provider(rpc_url)) + } + + fn starknet_only(config: ForeignChainConfig) -> ForeignChainsConfig { + ForeignChainsConfig { + starknet: Some(config), + ..Default::default() + } + } + + async fn mock_chain_id<'a>( + server: &'a httpmock::MockServer, + chain_id: &str, + ) -> httpmock::Mock<'a> { + let body = serde_json::json!({"jsonrpc": "2.0", "result": chain_id, "id": 0}); + server + .mock_async(|when, then| { + when.method(httpmock::Method::POST); + then.status(200).json_body(body); + }) + .await + } + + fn status_of(report: &ProbeReport, provider: &str) -> ProviderStatus { + report + .rows() + .iter() + .find(|row| row.provider.0 == provider) + .unwrap_or_else(|| panic!("missing row for provider `{provider}`")) + .status + .clone() + } + + #[tokio::test] + async fn probe_all_providers__should_report_a_provider_on_the_expected_network_as_healthy() { + // Given + let server = httpmock::MockServer::start_async().await; + let mock = mock_chain_id(&server, MAINNET).await; + let config = starknet_only(chain_config( + Some(MAINNET), + one_provider("publicnode", &server.base_url()), + )); + + // When + let report = probe_all_providers(&config).await; + + // Then + mock.assert_async().await; + assert_eq!(status_of(&report, "publicnode"), ProviderStatus::Healthy); + } + + #[tokio::test] + async fn probe_all_providers__should_report_a_provider_on_another_network_as_wrong_network() { + // Given a provider serving Sepolia while the operator configured mainnet + let server = httpmock::MockServer::start_async().await; + mock_chain_id(&server, SEPOLIA).await; + let config = starknet_only(chain_config( + Some(MAINNET), + one_provider("publicnode", &server.base_url()), + )); + + // When + let report = probe_all_providers(&config).await; + + // Then + assert_eq!( + status_of(&report, "publicnode"), + ProviderStatus::WrongNetwork { + expected: ChainIdentity::from(MAINNET.to_string()), + observed: ChainIdentity::from(SEPOLIA.to_string()), + } + ); + } + + #[tokio::test] + async fn probe_all_providers__should_normalize_the_reported_identity_before_comparing() { + // Given a provider padding and uppercasing the chain id, as the spec permits + let server = httpmock::MockServer::start_async().await; + mock_chain_id(&server, "0x00534E5F4D41494E").await; + let config = starknet_only(chain_config( + Some(MAINNET), + one_provider("publicnode", &server.base_url()), + )); + + // When + let report = probe_all_providers(&config).await; + + // Then + assert_eq!(status_of(&report, "publicnode"), ProviderStatus::Healthy); + } + + #[tokio::test] + async fn probe_all_providers__should_report_a_chain_without_an_expected_identity_without_probing() + { + // Given + let server = httpmock::MockServer::start_async().await; + let mock = mock_chain_id(&server, MAINNET).await; + let config = starknet_only(chain_config( + None, + one_provider("publicnode", &server.base_url()), + )); + + // When + let report = probe_all_providers(&config).await; + + // Then the provider is reported rather than skipped, and no request was sent + assert_eq!( + status_of(&report, "publicnode"), + ProviderStatus::MissingExpectedIdentity + ); + mock.assert_calls_async(0).await; + } + + #[tokio::test] + async fn probe_all_providers__should_report_an_unreachable_provider() { + // Given + let config = starknet_only(chain_config( + Some(MAINNET), + one_provider("publicnode", CLOSED_PORT_URL), + )); + + // When + let report = probe_all_providers(&config).await; + + // Then + assert_eq!( + status_of(&report, "publicnode"), + ProviderStatus::Unreachable + ); + } + + #[tokio::test] + async fn probe_all_providers__should_report_a_provider_whose_client_cannot_be_built() { + // Given a provider whose auth token comes from an environment variable that is not set + let config = starknet_only(chain_config( + Some(MAINNET), + NonEmptyBTreeMap::new( + "keyed".to_string().into(), + ForeignChainProviderConfig { + rpc_url: CLOSED_PORT_URL.to_string(), + auth: AuthConfig::Header { + name: http::HeaderName::from_static("authorization"), + scheme: Some("Bearer".to_string()), + token: TokenConfig::Env { + env: "PROBE_TEST_TOKEN_THAT_IS_NOT_SET".to_string(), + }, + }, + }, + ), + )); + + // When + let report = probe_all_providers(&config).await; + + // Then + assert_eq!( + status_of(&report, "keyed"), + ProviderStatus::ClientSetupFailed + ); + } + + #[tokio::test] + async fn probe_all_providers__should_report_each_provider_of_a_chain_separately() { + // Given one healthy provider and one that is unreachable + let server = httpmock::MockServer::start_async().await; + mock_chain_id(&server, MAINNET).await; + let mut providers = one_provider("healthy", &server.base_url()); + providers.insert("broken".to_string().into(), provider(CLOSED_PORT_URL)); + let config = starknet_only(chain_config(Some(MAINNET), providers)); + + // When + let report = probe_all_providers(&config).await; + + // Then + assert_eq!(status_of(&report, "healthy"), ProviderStatus::Healthy); + assert_eq!(status_of(&report, "broken"), ProviderStatus::Unreachable); + assert_eq!( + report.counts_per_chain()[&ForeignChain::Starknet], + ProviderCounts { + configured: 2, + healthy: 1, + } + ); + } + + #[tokio::test] + async fn probe_all_providers__should_report_a_chain_with_no_identity_probe_as_not_implemented() + { + // Given a chain the node can inspect but has no identity probe for + let config = ForeignChainsConfig { + base: Some(chain_config( + Some("8453"), + one_provider("publicnode", CLOSED_PORT_URL), + )), + ..Default::default() + }; + + // When + let report = probe_all_providers(&config).await; + + // Then it is visible in the report rather than silently absent + assert_eq!( + status_of(&report, "publicnode"), + ProviderStatus::ProbeNotImplemented + ); + } + + #[tokio::test] + async fn probe_all_providers__should_return_an_empty_report_when_no_chains_are_configured() { + // Given + let config = ForeignChainsConfig::default(); + + // When + let report = probe_all_providers(&config).await; + + // Then + assert!(report.rows().is_empty()); + assert!(report.counts_per_chain().is_empty()); + } + + #[tokio::test] + async fn probe_all_providers__should_keep_auth_material_out_of_the_report() { + // Given a provider whose API key is spliced into the URL path + let config = starknet_only(chain_config( + Some(MAINNET), + NonEmptyBTreeMap::new( + "keyed".to_string().into(), + ForeignChainProviderConfig { + rpc_url: format!("{CLOSED_PORT_URL}/v2/API_KEY"), + auth: AuthConfig::Path { + placeholder: "API_KEY".to_string(), + token: TokenConfig::Val { + val: "super-secret".to_string(), + }, + }, + }, + ), + )); + + // When + let report = probe_all_providers(&config).await; + + // Then neither the token nor the URL it was spliced into reaches the report + let rendered = format!("{report:?}"); + assert!(!rendered.contains("super-secret"), "{rendered}"); + assert!(!rendered.contains("127.0.0.1"), "{rendered}"); + } +} diff --git a/crates/foreign-chain-inspector/src/lib.rs b/crates/foreign-chain-inspector/src/lib.rs index 26c60fef74..3ecc71b7f9 100644 --- a/crates/foreign-chain-inspector/src/lib.rs +++ b/crates/foreign-chain-inspector/src/lib.rs @@ -1,10 +1,13 @@ use std::hash::Hash; +use std::num::NonZeroU64; +use std::time::Duration; use derive_more::{Deref, Display, From}; use ethereum_types::H256; use http::{HeaderMap, HeaderName, HeaderValue}; use jsonrpsee::http_client::{HttpClient, HttpClientBuilder}; use near_mpc_bounded_collections::NonEmptyVec; +use near_mpc_contract_interface::types::ProviderId; use thiserror::Error; pub use jsonrpsee::http_client; @@ -35,6 +38,21 @@ pub trait ForeignChainInspector { ) -> impl Future, ForeignChainInspectionError>> + Send; } +/// The network a provider serves, as the chain itself reports it: a chain id or a genesis hash, in +/// one canonical text form per chain. +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Display, From)] +pub struct ChainIdentity(String); + +/// Reports the [`ChainIdentity`] of the provider an inspector talks to. +/// +/// Compared verbatim, so impls must normalize to the canonical form their doc states. Fetches a +/// chain-wide constant providers never prune. +pub trait ChainIdentityInspector { + fn chain_identity( + &self, + ) -> impl Future> + Send; +} + /// Combines multiple inspectors that target the same chain into a single inspector. /// /// All inner inspectors are queried concurrently. The fan-out treats every @@ -58,7 +76,7 @@ pub trait ForeignChainInspector { /// inner fields differ. #[derive(Clone, derive_more::Constructor)] pub struct FanOut { - inspectors: NonEmptyVec, + inspectors: NonEmptyVec<(ProviderId, Inspector)>, } impl ForeignChainInspector for FanOut @@ -81,25 +99,30 @@ where extractors: Vec, ) -> Result, ForeignChainInspectionError> { let mut join_set = tokio::task::JoinSet::new(); - for (idx, inspector) in self.inspectors.iter().enumerate() { + for (provider, inspector) in self.inspectors.iter() { let tx_id = tx_id.clone(); let finality = finality.clone(); let extractors = extractors.clone(); let inspector = inspector.clone(); - join_set - .spawn(async move { (idx, inspector.extract(tx_id, finality, extractors).await) }); + let provider = provider.clone(); + join_set.spawn(async move { + ( + provider, + inspector.extract(tx_id, finality, extractors).await, + ) + }); } - let mut successes: Vec<(usize, Vec)> = Vec::new(); - let mut non_transient_errors: Vec<(usize, ForeignChainInspectionError)> = Vec::new(); + let mut successes: Vec<(ProviderId, Vec)> = Vec::new(); + let mut non_transient_errors: Vec<(ProviderId, ForeignChainInspectionError)> = Vec::new(); let mut first_transient_error: Option = None; - for (idx, result) in join_set.join_all().await { + for (provider, result) in join_set.join_all().await { match result { - Ok(values) => successes.push((idx, values)), + Ok(values) => successes.push((provider, values)), Err(err) if err.is_transient() => { tracing::warn!( - inspector_index = idx, + %provider, error = %err, "fan-out inspector failed (transient)", ); @@ -107,11 +130,11 @@ where } Err(err) => { tracing::error!( - inspector_index = idx, + %provider, error = %err, "fan-out inspector failed (non-transient)", ); - non_transient_errors.push((idx, err)); + non_transient_errors.push((provider, err)); } } } @@ -168,6 +191,46 @@ where } } +impl FanOut +where + Inspector: ChainIdentityInspector + Clone + Send + Sync + 'static, +{ + /// Ask every provider for the network it serves, concurrently, one result each. + /// Unlike [`FanOut::extract`], disagreement is not an error: a diagnostic caller needs the + /// individual answers. Each provider gets up to `attempts` tries, `timeout` per try, and only + /// a transient failure is retried. + pub async fn chain_identities( + &self, + timeout: Duration, + attempts: NonZeroU64, + ) -> Vec<( + ProviderId, + Result, + )> { + let mut join_set = tokio::task::JoinSet::new(); + for (provider, inspector) in self.inspectors.iter() { + let inspector = inspector.clone(); + let provider = provider.clone(); + join_set.spawn(async move { + let ask = || async { + tokio::time::timeout(timeout, inspector.chain_identity()) + .await + .unwrap_or(Err(ForeignChainInspectionError::Timeout)) + }; + let mut outcome = ask().await; + for _ in 1..attempts.get() { + if !matches!(&outcome, Err(err) if err.is_transient()) { + break; + } + outcome = ask().await; + } + (provider, outcome) + }); + } + join_set.join_all().await + } +} + #[derive(Debug, Clone)] pub enum RpcAuthentication { /// The key is in the URL (e.g., Alchemy, QuickNode). @@ -218,6 +281,8 @@ pub enum ForeignChainInspectionError { /// Transient provider failure (transport error, timeout, rate limit, 5xx). #[error("RPC request failed: {0}")] RpcRequestFailed(String), + #[error("RPC request did not complete within the configured timeout")] + Timeout, /// The provider rejected the request with a deterministic client error (4xx other than /// 408/429); retrying cannot change the outcome. #[error("RPC rejected the request: {0}")] @@ -281,10 +346,45 @@ impl ForeignChainInspectionError { self, Self::ClientError(_) | Self::RpcRequestFailed(_) + | Self::Timeout | Self::NotFinalized | Self::NotEnoughBlockConfirmations { .. } ) } + + /// How the provider failed, or [`None`] when the provider did not: the remaining variants + /// report the transaction's own state, which is an answer rather than a fault. + pub fn provider_failure(&self) -> Option { + match self { + Self::ClientError(_) | Self::RpcRequestFailed(_) => Some(ProviderFailure::Unreachable), + Self::Timeout => Some(ProviderFailure::TimedOut), + Self::RpcRequestRejected(_) => Some(ProviderFailure::Rejected), + Self::MalformedRpcResponse(_) + | Self::InconsistentRpcResponse { .. } + | Self::LogNotBoundToReceipt { .. } + | Self::EventLogFailedBorshSerialization(_) + | Self::InspectorResponseMismatch => Some(ProviderFailure::Malformed), + Self::NotFinalized + | Self::NotEnoughBlockConfirmations { .. } + | Self::NonCanonicalBlock { .. } + | Self::TransactionFailed + | Self::TransactionNotFound + | Self::LogIndexOutOfBounds => None, + } + } +} + +/// Groups the ways a provider itself can fail, for callers that report an outcome rather than act +/// on it. Says nothing about retryability: see [`ForeignChainInspectionError::is_transient`]. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub enum ProviderFailure { + /// No answer arrived: transport failure, 5xx, or rate limiting. + Unreachable, + /// The provider answered and refused. Retrying cannot change it. + Rejected, + /// The provider answered with something the caller could not use. + Malformed, + TimedOut, } /// Builds an HTTP client with the specified authentication method. diff --git a/crates/foreign-chain-inspector/src/starknet/inspector.rs b/crates/foreign-chain-inspector/src/starknet/inspector.rs index 4f33844852..fb6e065e5c 100644 --- a/crates/foreign-chain-inspector/src/starknet/inspector.rs +++ b/crates/foreign-chain-inspector/src/starknet/inspector.rs @@ -1,14 +1,20 @@ use crate::starknet::{StarknetExtractedValue, StarknetTransactionHash}; -use crate::{ForeignChainInspectionError, ForeignChainInspector}; +use crate::{ + ChainIdentity, ChainIdentityInspector, ForeignChainInspectionError, ForeignChainInspector, +}; use foreign_chain_rpc_interfaces::starknet::{ - BlockId, GetBlockWithTxHashesArgs, GetBlockWithTxHashesResponse, GetTransactionReceiptArgs, - GetTransactionReceiptResponse, H256, StarknetExecutionStatus, StarknetFinalityStatus, + BlockId, ChainIdResponse, GetBlockWithTxHashesArgs, GetBlockWithTxHashesResponse, + GetTransactionReceiptArgs, GetTransactionReceiptResponse, H256, StarknetExecutionStatus, + StarknetFinalityStatus, }; use jsonrpsee::core::client::ClientT; use near_mpc_contract_interface::types::{StarknetFelt, StarknetLog}; const GET_TRANSACTION_RECEIPT_METHOD: &str = "starknet_getTransactionReceipt"; const GET_BLOCK_WITH_TX_HASHES_METHOD: &str = "starknet_getBlockWithTxHashes"; +const CHAIN_ID_METHOD: &str = "starknet_chainId"; +/// `starknet_chainId` takes no arguments. Sent as an explicit empty array. +const NO_PARAMS: [(); 0] = []; #[derive(Clone)] pub struct StarknetInspector { @@ -21,6 +27,16 @@ pub enum StarknetFinality { AcceptedOnL1, } +impl ChainIdentityInspector for StarknetInspector +where + Client: ClientT + Send + Sync, +{ + async fn chain_identity(&self) -> Result { + let chain_id: ChainIdResponse = self.client.request(CHAIN_ID_METHOD, NO_PARAMS).await?; + Ok(chain_id.canonical_text().into()) + } +} + impl ForeignChainInspector for StarknetInspector where Client: ClientT + Send + Sync, diff --git a/crates/foreign-chain-inspector/tests/common.rs b/crates/foreign-chain-inspector/tests/common.rs index b43d94f836..a66d870bdc 100644 --- a/crates/foreign-chain-inspector/tests/common.rs +++ b/crates/foreign-chain-inspector/tests/common.rs @@ -11,6 +11,7 @@ use serde::{Deserialize, Serialize}; /// Useful for tests. /// Note: We have to hold a closure and not just the response /// because `RpcClientError` does not implement `Clone`. +#[derive(Clone)] pub struct FixedResponseRpcClient { response_fn: RespFn, } diff --git a/crates/foreign-chain-inspector/tests/fanout.rs b/crates/foreign-chain-inspector/tests/fanout.rs index 0a237d721f..24526393a8 100644 --- a/crates/foreign-chain-inspector/tests/fanout.rs +++ b/crates/foreign-chain-inspector/tests/fanout.rs @@ -15,6 +15,7 @@ use foreign_chain_inspector::{ BlockConfirmations, FanOut, ForeignChainInspectionError, ForeignChainInspector, }; use near_mpc_bounded_collections::NonEmptyVec; +use near_mpc_contract_interface::types::ProviderId; mockall::mock! { Inspector {} @@ -93,7 +94,12 @@ fn err(make: impl Fn() -> ForeignChainInspectionError + Send + Sync + 'static) - } fn fan_out_of(inspectors: Vec) -> FanOut { - let inspectors: NonEmptyVec = inspectors + let named: Vec<(ProviderId, MockInspector)> = inspectors + .into_iter() + .enumerate() + .map(|(index, inspector)| (ProviderId(format!("provider-{index}")), inspector)) + .collect(); + let inspectors: NonEmptyVec<(ProviderId, MockInspector)> = named .try_into() .expect("test must provide at least one inspector"); FanOut::new(inspectors) diff --git a/crates/foreign-chain-inspector/tests/starknet_inspector.rs b/crates/foreign-chain-inspector/tests/starknet_inspector.rs index 80caef9552..4bf3a3672d 100644 --- a/crates/foreign-chain-inspector/tests/starknet_inspector.rs +++ b/crates/foreign-chain-inspector/tests/starknet_inspector.rs @@ -7,7 +7,8 @@ use crate::common::{ }; use foreign_chain_inspector::{ - ForeignChainInspectionError, ForeignChainInspector, RpcAuthentication, build_http_client, + ChainIdentityInspector, FanOut, ForeignChainInspectionError, ForeignChainInspector, + RpcAuthentication, build_http_client, starknet::{ StarknetBlockHash, StarknetExtractedValue, StarknetTransactionHash, inspector::{StarknetExtractor, StarknetFinality, StarknetInspector}, @@ -22,8 +23,14 @@ use foreign_chain_rpc_interfaces::starknet::{ use httpmock::prelude::*; use httpmock::{HttpMockRequest, HttpMockResponse}; use jsonrpsee::core::client::error::Error as RpcClientError; +use near_mpc_bounded_collections::NonEmptyVec; +use near_mpc_contract_interface::types::ProviderId; use near_mpc_contract_interface::types::{StarknetFelt, StarknetLog}; use rstest::rstest; +use std::num::NonZeroU64; +use std::sync::Arc; +use std::sync::atomic::{AtomicUsize, Ordering}; +use std::time::Duration; fn mock_receipt( finality_status: StarknetFinalityStatus, @@ -507,3 +514,118 @@ async fn extract__should_return_event_log_for_specific_index_via_http_rpc_client extracted_values, ); } + +/// Starknet mainnet's chain id, `SN_MAIN` in ASCII. +const MAINNET_CHAIN_ID: &str = "0x534e5f4d41494e"; + +#[tokio::test] +async fn chain_identity__should_return_the_canonical_chain_id() { + // Given: the chain id padded and uppercased, as a provider is free to send it. + let inspector = StarknetInspector::new(mock_client_from_fixed_response("0x00534E5F4D41494E")); + + // When + let identity = inspector + .chain_identity() + .await + .expect("chain_identity should succeed"); + + // Then + assert_eq!(identity.to_string(), MAINNET_CHAIN_ID); +} + +/// Builds a fan-out of one Starknet provider whose client runs `respond` on each call, and reports +/// the number of calls it received. +#[expect( + clippy::type_complexity, + reason = "the client holds an unnameable closure type, so the tuple has to spell it out" +)] +fn single_provider_fan_out( + respond: impl Fn(usize) -> Result + Send + Sync + 'static, +) -> ( + FanOut< + StarknetInspector< + FixedResponseRpcClient< + impl Fn() -> Result + Clone + Sync, + >, + >, + >, + Arc, +) { + let calls = Arc::new(AtomicUsize::new(0)); + let seen = Arc::clone(&calls); + let respond: Arc Result + Send + Sync> = + Arc::new(respond); + let client = FixedResponseRpcClient::new(move || respond(seen.fetch_add(1, Ordering::SeqCst))); + let inspectors: NonEmptyVec<_> = vec![( + ProviderId("only".to_string()), + StarknetInspector::new(client), + )] + .try_into() + .expect("one inspector"); + (FanOut::new(inspectors), calls) +} + +fn transport_error() -> RpcClientError { + RpcClientError::Transport(Box::new(std::io::Error::new( + std::io::ErrorKind::ConnectionRefused, + "connection refused", + ))) +} + +#[tokio::test] +async fn chain_identities__should_retry_a_transient_failure_and_report_the_later_success() { + // Given a provider that refuses the first call and answers the second + let (fan_out, calls) = single_provider_fan_out(|call| match call { + 0 => Err(transport_error()), + _ => Ok(serde_json::json!(MAINNET_CHAIN_ID)), + }); + + // When + let results = fan_out + .chain_identities(Duration::from_secs(1), NonZeroU64::new(2).unwrap()) + .await; + + // Then + assert_eq!(calls.load(Ordering::SeqCst), 2); + let identity = results[0] + .1 + .as_ref() + .expect("second attempt should succeed"); + assert_eq!(identity.to_string(), MAINNET_CHAIN_ID); +} + +#[tokio::test] +async fn chain_identities__should_stop_after_the_configured_number_of_attempts() { + // Given a provider that never answers + let (fan_out, calls) = single_provider_fan_out(|_| Err(transport_error())); + + // When + let results = fan_out + .chain_identities(Duration::from_secs(1), NonZeroU64::new(3).unwrap()) + .await; + + // Then + assert_eq!(calls.load(Ordering::SeqCst), 3); + assert_matches!( + results[0].1, + Err(ForeignChainInspectionError::ClientError(_)) + ); +} + +#[tokio::test] +async fn chain_identity__should_propagate_rpc_client_errors() { + // Given + let client = FixedResponseRpcClient::new(|| { + Err(RpcClientError::Transport(Box::new(std::io::Error::new( + std::io::ErrorKind::ConnectionRefused, + "connection refused", + )))) + }); + let inspector = StarknetInspector::new(client); + + // When + let response = inspector.chain_identity().await; + + // Then + assert_matches!(response, Err(ForeignChainInspectionError::ClientError(_))); +} diff --git a/crates/foreign-chain-inspector/tests/starknet_rpc_manual.rs b/crates/foreign-chain-inspector/tests/starknet_rpc_manual.rs index ab85b67109..2939c2b93e 100644 --- a/crates/foreign-chain-inspector/tests/starknet_rpc_manual.rs +++ b/crates/foreign-chain-inspector/tests/starknet_rpc_manual.rs @@ -108,3 +108,26 @@ fn parse_starknet_felt_hash +/// +/// The spec types this as `CHAIN_ID`, `^0x[a-fA-F0-9]+$`, not as a `FELT`. It carries no length +/// bound and permits leading zeros, so it is kept as Hex text rather than parsed into an [`H256`]. +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Deserialize)] +#[serde(transparent)] +pub struct ChainIdResponse(pub String); + +impl ChainIdResponse { + /// Lowercase, no leading zeros. + pub fn canonical_text(&self) -> String { + let Some(digits) = self.0.strip_prefix("0x") else { + return self.0.clone(); + }; + let significant = digits.trim_start_matches('0'); + if significant.is_empty() { + "0x0".to_string() + } else { + format!("0x{}", significant.to_ascii_lowercase()) + } + } +} #[cfg(test)] #[expect(non_snake_case)] mod tests { use super::{ - BlockId, GetBlockWithTxHashesArgs, GetBlockWithTxHashesResponse, + BlockId, ChainIdResponse, GetBlockWithTxHashesArgs, GetBlockWithTxHashesResponse, GetTransactionReceiptResponse, StarknetExecutionStatus, StarknetFinalityStatus, parse_felt, }; const TEST_BLOCK_NUMBER: u64 = 842_750; const TEST_RECEIPT_BLOCK_NUMBER: u64 = 6_195_041; const SHORT_HEX_BLOCK_HASH: &str = "0x5"; + /// Starknet mainnet's chain id, `SN_MAIN` in ASCII. + const MAINNET_CHAIN_ID: &str = "0x534e5f4d41494e"; + + #[test] + fn chain_id_response__should_keep_a_canonical_chain_id_unchanged() { + // Given + let json = serde_json::json!(MAINNET_CHAIN_ID); + + // When + let response: ChainIdResponse = serde_json::from_value(json).unwrap(); + + // Then + assert_eq!(response.canonical_text(), MAINNET_CHAIN_ID); + } + + #[test] + fn chain_id_response__should_normalize_a_padded_uppercase_chain_id() { + // Given: the chain id padded and upper-cased, as a provider may send it. + let json = serde_json::json!("0x00534E5F4D41494E"); + + // When + let response: ChainIdResponse = serde_json::from_value(json).unwrap(); + + // Then + assert_eq!(response.canonical_text(), MAINNET_CHAIN_ID); + } + + #[test] + fn chain_id_response__should_normalize_a_zero_chain_id() { + // Given + let json = serde_json::json!("0x0000"); + + // When + let response: ChainIdResponse = serde_json::from_value(json).unwrap(); + + // Then + assert_eq!(response.canonical_text(), "0x0"); + } + + #[test] + fn chain_id_response__should_accept_a_chain_id_longer_than_a_felt() { + // Given: 66 hex digits. `CHAIN_ID` carries no length bound, though a `FELT` caps at 63. + let json = serde_json::json!( + "0x1234567890123456789012345678901234567890123456789012345678901234ab" + ); + + // When + let response: ChainIdResponse = serde_json::from_value(json).unwrap(); + + // Then + assert_eq!( + response.canonical_text(), + "0x1234567890123456789012345678901234567890123456789012345678901234ab" + ); + } + + #[test] + fn chain_id_response__should_leave_a_non_hex_chain_id_unchanged() { + // Given: the decoded name rather than the hex. + let json = serde_json::json!("NOT_CHAIN_ID"); + + // When + let response: ChainIdResponse = serde_json::from_value(json).unwrap(); + + // Then: reported as answered by the provider + assert_eq!(response.canonical_text(), "NOT_CHAIN_ID"); + } #[test] fn deserialize_receipt__should_accept_short_hex_block_hash() { diff --git a/crates/near-mpc-contract-interface/src/types/foreign_chain.rs b/crates/near-mpc-contract-interface/src/types/foreign_chain.rs index 704d0755a9..dab50a23ff 100644 --- a/crates/near-mpc-contract-interface/src/types/foreign_chain.rs +++ b/crates/near-mpc-contract-interface/src/types/foreign_chain.rs @@ -1688,7 +1688,18 @@ impl ForeignTxSignPayload { /// Stable label for an RPC provider entry (e.g. `"alchemy"`, `"ankr"`, `"drpc"`). /// Unique within a chain in the on-chain foreign-chain RPC whitelist. -#[derive(Debug, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, BorshSerialize, BorshDeserialize)] +#[derive( + Debug, + Clone, + Eq, + PartialEq, + Ord, + PartialOrd, + Hash, + BorshSerialize, + BorshDeserialize, + derive_more::Display, +)] #[cfg_attr(not(target_arch = "wasm32"), derive(Serialize, Deserialize))] #[cfg_attr( all(feature = "abi", not(target_arch = "wasm32")), diff --git a/crates/node-config/src/foreign_chains.rs b/crates/node-config/src/foreign_chains.rs index 41d7f37c29..66132c7108 100644 --- a/crates/node-config/src/foreign_chains.rs +++ b/crates/node-config/src/foreign_chains.rs @@ -45,6 +45,7 @@ pub struct ForeignChainsConfig { #[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)] pub struct ForeignChainConfig { pub timeout_sec: NonZeroU64, + /// Total attempts per provider, not additional ones: `1` means a single try. pub max_retries: NonZeroU64, /// The network fingerprint the operator expects every provider of this chain to report, in the /// chain's canonical text form. A chain id for chains that have one, a genesis hash or digest diff --git a/crates/node/src/providers/verify_foreign_tx.rs b/crates/node/src/providers/verify_foreign_tx.rs index 55f35f8c90..e624ad9883 100644 --- a/crates/node/src/providers/verify_foreign_tx.rs +++ b/crates/node/src/providers/verify_foreign_tx.rs @@ -22,6 +22,7 @@ use foreign_chain_rpc_auth::auth_config_to_rpc_auth; use foreign_chain_rpc_interfaces::aptos::ReqwestAptosClient; use foreign_chain_rpc_interfaces::sui::GrpcSuiClient; use mpc_node_config::{ConfigFile, ForeignChainConfig, ForeignChainsConfig}; +use near_mpc_contract_interface::types::ProviderId; use std::sync::Arc; use std::time::Duration; @@ -52,12 +53,13 @@ impl ForeignChainInspectors { return Ok(None); }; let timeout = Duration::from_secs(c.timeout_sec.get()); - let inspectors = c.providers.try_map_to_vec(|_, p| { + let inspectors = c.providers.try_map_to_vec(|name, p| { // `Path`/`Query` auth is substituted into `url`; `Header` auth is returned // as `RpcAuthentication::CustomHeader` for the client to install. let mut url = p.rpc_url.clone(); let rpc_auth = auth_config_to_rpc_auth(p.auth.clone(), &mut url)?; - new_inspector(url, rpc_auth, timeout) + let inspector = new_inspector(url, rpc_auth, timeout)?; + anyhow::Ok((ProviderId(name.as_str().to_owned()), inspector)) })?; Ok(Some(FanOut::new(inspectors))) } diff --git a/docs/foreign-chain-transactions.md b/docs/foreign-chain-transactions.md index 8eede23088..534dca0799 100644 --- a/docs/foreign-chain-transactions.md +++ b/docs/foreign-chain-transactions.md @@ -529,6 +529,14 @@ At startup, each resolved provider gets its self-identifying RPC called and the Taking the expected value from operator config rather than a constant in the attested binary is a deliberate trade. It makes mixed-network and local deployments checkable at all, since a config may pair one chain's mainnet with another's testnet and no binary can ship a value for a devnet. The cost is that the check no longer binds an operator: they can set the wrong value, or omit the field and get no check at all, and either way they fool only their own node's diagnostics. The network-level defenses against a wrong URL are unchanged: threshold voter review of the whitelist, and the provider fan-out, which fails the individual request when a provider disagrees with its siblings. +Not every chain has an identity probe. The table lists the ones that do, with the RPC each probes. A chain absent from it ignores `expected_chain_identity`. + +| chain | probe | value (mainnet) | value (testnet) | +|---|---|---|---| +| starknet | `starknet_chainId` | `0x534e5f4d41494e` (`SN_MAIN`) | `0x534e5f5345504f4c4941` (`SN_SEPOLIA`) | + +Starknet's identity is the chain-id felt in lowercase `0x` hex without leading zeros. Providers are free to pad and upper-case it, so the value is normalized before comparison. + #### Why drop-and-log on local-config mismatch, not hard-crash If an operator's `foreign_chains.yaml` references a `provider_id` not on the whitelist for that chain (e.g. just removed by a vote), the node logs a warning and excludes that provider from registration; the chain is still served by surviving providers. A chain falls off the registration set only when zero providers survive. Hard-crashing would let a single hostile vote-removal participant take a node offline by removing a provider that node depends on. @@ -683,6 +691,9 @@ The fingerprint is set per chain rather than once per deployment, so a config ca each value must match the network of the `rpc_url` beside it. The value is always a quoted string, including the fingerprints that look numeric. +A chain configured without one is not skipped: the probe reports every provider of that chain as +`MissingExpectedIdentity`, because a silent skip reads as healthy on a dashboard. + ## Risks * **RPC trust and correctness**: Verification relies on centralized RPC providers. A malicious From ff07a2c8d775c2b68bbb1019f3d671df0de6f725 Mon Sep 17 00:00:00 2001 From: Haiyue Chen Date: Fri, 31 Jul 2026 01:05:07 +0200 Subject: [PATCH 2/9] fix(starknet): tell a provider's refusal apart from an unreachable one Every failure of a jsonrpsee-backed provider reached the probe as `ClientError`, so a 401, a JSON-RPC error object and an unparseable body all reported `Unreachable` and spent the provider's remaining retries on an outcome no retry can change. `classify_rpc_client_error` classifies the client error by what the provider did; it is opt-in, so `FanOut::extract` keeps the whole error and the signing path is unchanged. Along with it: - `ChainIdentityInspector::canonical_identity` puts the configured identity through the same canonical form as the reported one, so a padded or upper-cased chain id no longer reads as the wrong network. - `AuthTokenUnresolved` names the one setup failure an operator can act on, without carrying an error whose text can embed the API key. - Providers get a short backoff between tries. --- .../foreign-chain-health-check/src/probe.rs | 223 ++++++++++++++++-- crates/foreign-chain-inspector/src/lib.rs | 68 +++++- .../src/starknet/inspector.rs | 10 +- .../tests/rpc_error_classification.rs | 155 ++++++++++++ .../tests/starknet_inspector.rs | 31 ++- docs/foreign-chain-transactions.md | 2 +- 6 files changed, 462 insertions(+), 27 deletions(-) create mode 100644 crates/foreign-chain-inspector/tests/rpc_error_classification.rs diff --git a/crates/foreign-chain-health-check/src/probe.rs b/crates/foreign-chain-health-check/src/probe.rs index 1570090b12..14b25885b4 100644 --- a/crates/foreign-chain-health-check/src/probe.rs +++ b/crates/foreign-chain-health-check/src/probe.rs @@ -8,7 +8,9 @@ use foreign_chain_inspector::starknet::inspector::StarknetInspector; use foreign_chain_inspector::{ ChainIdentity, FanOut, ForeignChainInspectionError, ProviderFailure, }; -use mpc_node_config::{ForeignChainConfig, ForeignChainProviderConfig, ForeignChainsConfig}; +use mpc_node_config::{ + AuthConfig, ForeignChainConfig, ForeignChainProviderConfig, ForeignChainsConfig, TokenConfig, +}; use near_mpc_bounded_collections::NonEmptyVec; use near_mpc_contract_interface::types::{ForeignChain, ProviderId}; @@ -32,7 +34,9 @@ pub enum ProviderStatus { RequestRejected, MalformedResponse, TimedOut, - /// The RPC client could not be built, usually an unresolvable auth token. + /// The provider's auth token did not resolve, e.g. an environment variable that is not set. + AuthTokenUnresolved, + /// The RPC client could not be built from the provider's URL and auth. ClientSetupFailed, /// The chain is configured without an `expected_chain_identity`, so its providers cannot be /// checked. Reported rather than skipped: silence would read as healthy. @@ -86,8 +90,9 @@ impl ProbeReport { /// Probe every configured provider concurrently. /// -/// Each provider is tried up to `max_retries` times with `timeout_sec` per try -/// This returns within the largest configured `timeout_sec * max_retries`. +/// Each provider is tried up to `max_retries` times, `timeout_sec` per try, and only for as long as +/// the failures stay transient. This returns within the largest configured `timeout_sec * +/// max_retries`, plus the [`foreign_chain_inspector::RETRY_BACKOFF`] between tries. pub async fn probe_all_providers(config: &ForeignChainsConfig) -> ProbeReport { let probe_attempts = config .iter_chains() @@ -116,10 +121,10 @@ async fn probe_chain( where I: foreign_chain_inspector::ChainIdentityInspector + Clone + Send + Sync + 'static, { - let Some(expected) = config.expected_chain_identity.clone() else { + let Some(expected) = &config.expected_chain_identity else { return rows_of(chain, config, ProviderStatus::MissingExpectedIdentity); }; - let expected = ChainIdentity::from(expected); + let expected = I::canonical_identity(expected); let mut inspectors = Vec::new(); let mut rows = Vec::new(); @@ -130,7 +135,7 @@ where Err(_) => rows.push(ProviderHealth { chain, provider: provider_id, - status: ProviderStatus::ClientSetupFailed, + status: setup_failure(provider), }), } } @@ -153,6 +158,24 @@ where rows } +/// The setup error itself is dropped for the reason [`ProviderStatus`] documents, so the one cause +/// an operator can act on gets a status of its own instead. +fn setup_failure(provider: &ForeignChainProviderConfig) -> ProviderStatus { + match auth_token(&provider.auth) { + Some(token) if token.resolve().is_err() => ProviderStatus::AuthTokenUnresolved, + _ => ProviderStatus::ClientSetupFailed, + } +} + +fn auth_token(auth: &AuthConfig) -> Option<&TokenConfig> { + match auth { + AuthConfig::None => None, + AuthConfig::Header { token, .. } + | AuthConfig::Path { token, .. } + | AuthConfig::Query { token, .. } => Some(token), + } +} + fn rows_of( chain: ForeignChain, config: &ForeignChainConfig, @@ -195,7 +218,6 @@ fn classify( #[expect(non_snake_case)] mod tests { use super::*; - use mpc_node_config::{AuthConfig, TokenConfig}; use near_mpc_bounded_collections::NonEmptyBTreeMap; use std::num::NonZeroU64; @@ -227,6 +249,13 @@ mod tests { } } + fn with_retries(config: ForeignChainConfig, max_retries: u64) -> ForeignChainConfig { + ForeignChainConfig { + max_retries: NonZeroU64::new(max_retries).unwrap(), + ..config + } + } + fn one_provider( name: &str, rpc_url: &str, @@ -257,7 +286,7 @@ mod tests { .await } - fn status_of(report: &ProbeReport, provider: &str) -> ProviderStatus { + fn must_status_of(report: &ProbeReport, provider: &str) -> ProviderStatus { report .rows() .iter() @@ -282,7 +311,10 @@ mod tests { // Then mock.assert_async().await; - assert_eq!(status_of(&report, "publicnode"), ProviderStatus::Healthy); + assert_eq!( + must_status_of(&report, "publicnode"), + ProviderStatus::Healthy + ); } #[tokio::test] @@ -300,7 +332,7 @@ mod tests { // Then assert_eq!( - status_of(&report, "publicnode"), + must_status_of(&report, "publicnode"), ProviderStatus::WrongNetwork { expected: ChainIdentity::from(MAINNET.to_string()), observed: ChainIdentity::from(SEPOLIA.to_string()), @@ -322,7 +354,10 @@ mod tests { let report = probe_all_providers(&config).await; // Then - assert_eq!(status_of(&report, "publicnode"), ProviderStatus::Healthy); + assert_eq!( + must_status_of(&report, "publicnode"), + ProviderStatus::Healthy + ); } #[tokio::test] @@ -341,7 +376,7 @@ mod tests { // Then the provider is reported rather than skipped, and no request was sent assert_eq!( - status_of(&report, "publicnode"), + must_status_of(&report, "publicnode"), ProviderStatus::MissingExpectedIdentity ); mock.assert_calls_async(0).await; @@ -360,13 +395,142 @@ mod tests { // Then assert_eq!( - status_of(&report, "publicnode"), + must_status_of(&report, "publicnode"), ProviderStatus::Unreachable ); } #[tokio::test] - async fn probe_all_providers__should_report_a_provider_whose_client_cannot_be_built() { + async fn probe_all_providers__should_report_a_provider_refusing_the_request_without_retrying() { + // Given a provider answering the way an authenticated provider answers a bad API key + let server = httpmock::MockServer::start_async().await; + let mock = server + .mock_async(|when, then| { + when.method(httpmock::Method::POST); + then.status(401).json_body(serde_json::json!({ + "jsonrpc": "2.0", + "id": 0, + "error": {"code": -32600, "message": "Must be authenticated!"}, + })); + }) + .await; + let config = starknet_only(with_retries( + chain_config(Some(MAINNET), one_provider("keyed", &server.base_url())), + 3, + )); + + // When + let report = probe_all_providers(&config).await; + + // Then the refusal is named as such, and retrying it is pointless + assert_eq!( + must_status_of(&report, "keyed"), + ProviderStatus::RequestRejected + ); + mock.assert_calls_async(1).await; + } + + #[tokio::test] + async fn probe_all_providers__should_report_a_provider_answering_with_a_jsonrpc_error() { + // Given a provider that does not serve this chain's methods + let server = httpmock::MockServer::start_async().await; + server + .mock_async(|when, then| { + when.method(httpmock::Method::POST); + then.status(200).json_body(serde_json::json!({ + "jsonrpc": "2.0", + "id": 0, + "error": {"code": -32601, "message": "Method not found"}, + })); + }) + .await; + let config = starknet_only(chain_config( + Some(MAINNET), + one_provider("publicnode", &server.base_url()), + )); + + // When + let report = probe_all_providers(&config).await; + + // Then + assert_eq!( + must_status_of(&report, "publicnode"), + ProviderStatus::RequestRejected + ); + } + + #[tokio::test] + async fn probe_all_providers__should_report_a_provider_answering_with_an_unusable_body() { + // Given a provider whose answer is not JSON-RPC at all + let server = httpmock::MockServer::start_async().await; + server + .mock_async(|when, then| { + when.method(httpmock::Method::POST); + then.status(200).body("gateway"); + }) + .await; + let config = starknet_only(chain_config( + Some(MAINNET), + one_provider("publicnode", &server.base_url()), + )); + + // When + let report = probe_all_providers(&config).await; + + // Then + assert_eq!( + must_status_of(&report, "publicnode"), + ProviderStatus::MalformedResponse + ); + } + + #[tokio::test] + async fn probe_all_providers__should_report_a_provider_that_does_not_answer_in_time() { + // Given a provider slower than the configured timeout + let server = httpmock::MockServer::start_async().await; + let body = serde_json::json!({"jsonrpc": "2.0", "result": MAINNET, "id": 0}); + server + .mock_async(|when, then| { + when.method(httpmock::Method::POST); + then.status(200) + .json_body(body) + .delay(Duration::from_secs(30)); + }) + .await; + let config = starknet_only(chain_config( + Some(MAINNET), + one_provider("slow", &server.base_url()), + )); + + // When + let report = probe_all_providers(&config).await; + + // Then + assert_eq!(must_status_of(&report, "slow"), ProviderStatus::TimedOut); + } + + #[tokio::test] + async fn probe_all_providers__should_normalize_the_configured_identity_before_comparing() { + // Given an operator writing the chain id padded and uppercased, as the spec permits + let server = httpmock::MockServer::start_async().await; + mock_chain_id(&server, MAINNET).await; + let config = starknet_only(chain_config( + Some("0x00534E5F4D41494E"), + one_provider("publicnode", &server.base_url()), + )); + + // When + let report = probe_all_providers(&config).await; + + // Then + assert_eq!( + must_status_of(&report, "publicnode"), + ProviderStatus::Healthy + ); + } + + #[tokio::test] + async fn probe_all_providers__should_report_a_provider_whose_auth_token_does_not_resolve() { // Given a provider whose auth token comes from an environment variable that is not set let config = starknet_only(chain_config( Some(MAINNET), @@ -388,9 +552,27 @@ mod tests { // When let report = probe_all_providers(&config).await; + // Then the operator learns which of the two setup failures it was + assert_eq!( + must_status_of(&report, "keyed"), + ProviderStatus::AuthTokenUnresolved + ); + } + + #[tokio::test] + async fn probe_all_providers__should_report_a_provider_whose_client_cannot_be_built() { + // Given a provider whose URL is not one a client can be built for + let config = starknet_only(chain_config( + Some(MAINNET), + one_provider("wrong-scheme", "ws://127.0.0.1:9"), + )); + + // When + let report = probe_all_providers(&config).await; + // Then assert_eq!( - status_of(&report, "keyed"), + must_status_of(&report, "wrong-scheme"), ProviderStatus::ClientSetupFailed ); } @@ -408,8 +590,11 @@ mod tests { let report = probe_all_providers(&config).await; // Then - assert_eq!(status_of(&report, "healthy"), ProviderStatus::Healthy); - assert_eq!(status_of(&report, "broken"), ProviderStatus::Unreachable); + assert_eq!(must_status_of(&report, "healthy"), ProviderStatus::Healthy); + assert_eq!( + must_status_of(&report, "broken"), + ProviderStatus::Unreachable + ); assert_eq!( report.counts_per_chain()[&ForeignChain::Starknet], ProviderCounts { @@ -436,7 +621,7 @@ mod tests { // Then it is visible in the report rather than silently absent assert_eq!( - status_of(&report, "publicnode"), + must_status_of(&report, "publicnode"), ProviderStatus::ProbeNotImplemented ); } diff --git a/crates/foreign-chain-inspector/src/lib.rs b/crates/foreign-chain-inspector/src/lib.rs index 3ecc71b7f9..49f869ca2e 100644 --- a/crates/foreign-chain-inspector/src/lib.rs +++ b/crates/foreign-chain-inspector/src/lib.rs @@ -45,12 +45,16 @@ pub struct ChainIdentity(String); /// Reports the [`ChainIdentity`] of the provider an inspector talks to. /// -/// Compared verbatim, so impls must normalize to the canonical form their doc states. Fetches a -/// chain-wide constant providers never prune. +/// Identities are compared verbatim, so both the reported and the expected one go through the +/// impl's canonical form. Fetches a chain-wide constant providers never prune. pub trait ChainIdentityInspector { fn chain_identity( &self, ) -> impl Future> + Send; + + /// Puts an operator-supplied identity into the form [`Self::chain_identity`] returns, so that a + /// spec-legal spelling of the right network does not read as the wrong network. + fn canonical_identity(expected: &str) -> ChainIdentity; } /// Combines multiple inspectors that target the same chain into a single inspector. @@ -191,14 +195,18 @@ where } } +/// Pause between two tries at the same provider, so that a rate-limiting provider is not hit again +/// immediately. +pub const RETRY_BACKOFF: Duration = Duration::from_millis(200); + impl FanOut where Inspector: ChainIdentityInspector + Clone + Send + Sync + 'static, { /// Ask every provider for the network it serves, concurrently, one result each. /// Unlike [`FanOut::extract`], disagreement is not an error: a diagnostic caller needs the - /// individual answers. Each provider gets up to `attempts` tries, `timeout` per try, and only - /// a transient failure is retried. + /// individual answers. Each provider gets up to `attempts` tries, `timeout` per try plus + /// [`RETRY_BACKOFF`] between them, and only a transient failure is retried. pub async fn chain_identities( &self, timeout: Duration, @@ -222,6 +230,7 @@ where if !matches!(&outcome, Err(err) if err.is_transient()) { break; } + tokio::time::sleep(RETRY_BACKOFF).await; outcome = ask().await; } (provider, outcome) @@ -352,6 +361,47 @@ impl ForeignChainInspectionError { ) } + /// Classifies a client error by what the provider did, unlike the [`From`] impl, which keeps it + /// whole as the [`Self::ClientError`] that [`Self::is_transient`] tolerates wholesale. A 401, a + /// JSON-RPC error object and an unparseable body are otherwise indistinguishable, which suits + /// [`FanOut::extract`] but not a caller that reports why a provider is unusable, or decides + /// whether retrying it can help. + /// + /// The messages name the HTTP status or the JSON-RPC code, never the URL: `Path`/`Query` auth + /// splices the operator's API key into it. + pub fn classify_rpc_client_error(error: jsonrpsee::core::client::error::Error) -> Self { + use jsonrpsee::core::client::error::Error as ClientError; + use jsonrpsee::core::http_helpers::HttpError; + use jsonrpsee::http_client::transport::Error as TransportError; + + match error { + ClientError::Call(object) => { + Self::RpcRequestRejected(format!("JSON-RPC error code {}", object.code())) + } + ClientError::ParseError(error) => Self::MalformedRpcResponse(error.to_string()), + ClientError::RequestTimeout => Self::Timeout, + ClientError::Transport(error) => match error.downcast_ref::() { + Some(TransportError::Rejected { status_code }) => { + let status = format!("HTTP status {status_code}"); + if is_retryable_status(*status_code) { + Self::RpcRequestFailed(status) + } else { + Self::RpcRequestRejected(status) + } + } + // Not a response the caller can use, as opposed to no response at all. + Some(TransportError::Http(HttpError::Malformed | HttpError::TooLarge)) => { + Self::MalformedRpcResponse("response was not valid JSON-RPC".to_string()) + } + Some(TransportError::Url(_)) => { + Self::RpcRequestRejected("invalid RPC URL".to_string()) + } + _ => Self::RpcRequestFailed("transport failure".to_string()), + }, + other => Self::RpcRequestFailed(other.to_string()), + } + } + /// How the provider failed, or [`None`] when the provider did not: the remaining variants /// report the transaction's own state, which is an answer rather than a fault. pub fn provider_failure(&self) -> Option { @@ -374,6 +424,16 @@ impl ForeignChainInspectionError { } } +/// Request timeout, too many requests, and anything the server blames on itself. Every other +/// status is the provider's verdict on the request, which the same request cannot change. +fn is_retryable_status(status_code: u16) -> bool { + const REQUEST_TIMEOUT: u16 = 408; + const TOO_MANY_REQUESTS: u16 = 429; + const SERVER_ERROR: u16 = 500; + + matches!(status_code, REQUEST_TIMEOUT | TOO_MANY_REQUESTS) || status_code >= SERVER_ERROR +} + /// Groups the ways a provider itself can fail, for callers that report an outcome rather than act /// on it. Says nothing about retryability: see [`ForeignChainInspectionError::is_transient`]. #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] diff --git a/crates/foreign-chain-inspector/src/starknet/inspector.rs b/crates/foreign-chain-inspector/src/starknet/inspector.rs index fb6e065e5c..9fa5bc5af4 100644 --- a/crates/foreign-chain-inspector/src/starknet/inspector.rs +++ b/crates/foreign-chain-inspector/src/starknet/inspector.rs @@ -32,9 +32,17 @@ where Client: ClientT + Send + Sync, { async fn chain_identity(&self) -> Result { - let chain_id: ChainIdResponse = self.client.request(CHAIN_ID_METHOD, NO_PARAMS).await?; + let chain_id: ChainIdResponse = self + .client + .request(CHAIN_ID_METHOD, NO_PARAMS) + .await + .map_err(ForeignChainInspectionError::classify_rpc_client_error)?; Ok(chain_id.canonical_text().into()) } + + fn canonical_identity(expected: &str) -> ChainIdentity { + ChainIdResponse(expected.to_owned()).canonical_text().into() + } } impl ForeignChainInspector for StarknetInspector diff --git a/crates/foreign-chain-inspector/tests/rpc_error_classification.rs b/crates/foreign-chain-inspector/tests/rpc_error_classification.rs new file mode 100644 index 0000000000..bdae97129e --- /dev/null +++ b/crates/foreign-chain-inspector/tests/rpc_error_classification.rs @@ -0,0 +1,155 @@ +#![allow(non_snake_case)] + +//! Integration tests for [`ForeignChainInspectionError::classify_rpc_client_error`], which decides +//! whether a provider failed to answer, answered and refused, or answered unusably. + +use assert_matches::assert_matches; +use foreign_chain_inspector::ForeignChainInspectionError; +use jsonrpsee::core::client::error::Error as RpcClientError; +use jsonrpsee::core::http_helpers::HttpError; +use jsonrpsee::http_client::transport::Error as TransportError; +use rstest::rstest; + +fn transport(error: TransportError) -> RpcClientError { + RpcClientError::Transport(Box::new(error)) +} + +#[rstest] +#[case(400)] +#[case(401)] +#[case(403)] +#[case(404)] +fn classify_rpc_client_error__should_report_a_deterministic_status_as_a_refusal( + #[case] status_code: u16, +) { + // Given + let error = transport(TransportError::Rejected { status_code }); + + // When + let classified = ForeignChainInspectionError::classify_rpc_client_error(error); + + // Then + assert_matches!( + &classified, + ForeignChainInspectionError::RpcRequestRejected(_) + ); + assert!(!classified.is_transient()); +} + +#[rstest] +#[case(408)] +#[case(429)] +#[case(500)] +#[case(503)] +fn classify_rpc_client_error__should_report_a_retryable_status_as_a_transient_failure( + #[case] status_code: u16, +) { + // Given + let error = transport(TransportError::Rejected { status_code }); + + // When + let classified = ForeignChainInspectionError::classify_rpc_client_error(error); + + // Then + assert_matches!( + &classified, + ForeignChainInspectionError::RpcRequestFailed(_) + ); + assert!(classified.is_transient()); +} + +#[test] +fn classify_rpc_client_error__should_report_a_jsonrpc_error_object_as_a_refusal() { + // Given the answer an authenticated provider gives to a request it will not serve + let error = RpcClientError::Call(jsonrpsee::types::ErrorObject::owned( + -32600, + "Must be authenticated!", + None::<()>, + )); + + // When + let classified = ForeignChainInspectionError::classify_rpc_client_error(error); + + // Then + assert_matches!( + &classified, + ForeignChainInspectionError::RpcRequestRejected(_) + ); + assert!(!classified.is_transient()); +} + +#[test] +fn classify_rpc_client_error__should_report_an_unparseable_result_as_malformed() { + // Given + let parse_error = serde_json::from_str::("7").expect_err("a number is not a string"); + let error = RpcClientError::ParseError(parse_error); + + // When + let classified = ForeignChainInspectionError::classify_rpc_client_error(error); + + // Then + assert_matches!( + classified, + ForeignChainInspectionError::MalformedRpcResponse(_) + ); +} + +#[test] +fn classify_rpc_client_error__should_report_a_body_that_is_not_jsonrpc_as_malformed() { + // Given + let error = transport(TransportError::Http(HttpError::Malformed)); + + // When + let classified = ForeignChainInspectionError::classify_rpc_client_error(error); + + // Then + assert_matches!( + classified, + ForeignChainInspectionError::MalformedRpcResponse(_) + ); +} + +#[test] +fn classify_rpc_client_error__should_report_a_connection_failure_as_transient() { + // Given + let error = transport(TransportError::Http(HttpError::Stream(Box::new( + std::io::Error::new(std::io::ErrorKind::ConnectionRefused, "connection refused"), + )))); + + // When + let classified = ForeignChainInspectionError::classify_rpc_client_error(error); + + // Then + assert_matches!( + &classified, + ForeignChainInspectionError::RpcRequestFailed(_) + ); + assert!(classified.is_transient()); +} + +#[test] +fn classify_rpc_client_error__should_keep_the_rpc_url_out_of_the_message() { + // Given a URL a client cannot be built for, as jsonrpsee reports it: with the URL in the text + let error = transport(TransportError::Url( + "http://provider.example/v2/super-secret".to_string(), + )); + + // When + let classified = ForeignChainInspectionError::classify_rpc_client_error(error); + + // Then the API key spliced into the URL by `Path`/`Query` auth cannot travel with the error + let rendered = classified.to_string(); + assert!(!rendered.contains("super-secret"), "{rendered}"); +} + +#[test] +fn classify_rpc_client_error__should_report_a_client_side_timeout_as_a_timeout() { + // Given + let error = RpcClientError::RequestTimeout; + + // When + let classified = ForeignChainInspectionError::classify_rpc_client_error(error); + + // Then + assert_matches!(classified, ForeignChainInspectionError::Timeout); +} diff --git a/crates/foreign-chain-inspector/tests/starknet_inspector.rs b/crates/foreign-chain-inspector/tests/starknet_inspector.rs index 4bf3a3672d..e474bdaa64 100644 --- a/crates/foreign-chain-inspector/tests/starknet_inspector.rs +++ b/crates/foreign-chain-inspector/tests/starknet_inspector.rs @@ -608,7 +608,31 @@ async fn chain_identities__should_stop_after_the_configured_number_of_attempts() assert_eq!(calls.load(Ordering::SeqCst), 3); assert_matches!( results[0].1, - Err(ForeignChainInspectionError::ClientError(_)) + Err(ForeignChainInspectionError::RpcRequestFailed(_)) + ); +} + +#[tokio::test] +async fn chain_identities__should_not_retry_a_provider_that_refused_the_request() { + // Given a provider refusing with a JSON-RPC error object, as one does for a bad API key + let (fan_out, calls) = single_provider_fan_out(|_| { + Err(RpcClientError::Call(jsonrpsee::types::ErrorObject::owned( + -32600, + "Must be authenticated!", + None::<()>, + ))) + }); + + // When + let results = fan_out + .chain_identities(Duration::from_secs(1), NonZeroU64::new(3).unwrap()) + .await; + + // Then the refusal is reported as one, and the remaining attempts are not spent on it + assert_eq!(calls.load(Ordering::SeqCst), 1); + assert_matches!( + results[0].1, + Err(ForeignChainInspectionError::RpcRequestRejected(_)) ); } @@ -627,5 +651,8 @@ async fn chain_identity__should_propagate_rpc_client_errors() { let response = inspector.chain_identity().await; // Then - assert_matches!(response, Err(ForeignChainInspectionError::ClientError(_))); + assert_matches!( + response, + Err(ForeignChainInspectionError::RpcRequestFailed(_)) + ); } diff --git a/docs/foreign-chain-transactions.md b/docs/foreign-chain-transactions.md index 534dca0799..77ad3dc96a 100644 --- a/docs/foreign-chain-transactions.md +++ b/docs/foreign-chain-transactions.md @@ -535,7 +535,7 @@ Not every chain has an identity probe. The table lists the ones that do, with th |---|---|---|---| | starknet | `starknet_chainId` | `0x534e5f4d41494e` (`SN_MAIN`) | `0x534e5f5345504f4c4941` (`SN_SEPOLIA`) | -Starknet's identity is the chain-id felt in lowercase `0x` hex without leading zeros. Providers are free to pad and upper-case it, so the value is normalized before comparison. +Starknet's identity is the chain-id felt in lowercase `0x` hex without leading zeros. Both providers and operators are free to pad and upper-case it, so the reported and the configured value are normalized before they are compared. #### Why drop-and-log on local-config mismatch, not hard-crash From 1b8799a147928260b484b2e8cec831406bdd2786 Mon Sep 17 00:00:00 2001 From: Haiyue Chen Date: Fri, 31 Jul 2026 01:22:52 +0200 Subject: [PATCH 3/9] style(starknet): hoist the classifier's imports to module scope `no-use-in-fn` forbids `use` inside a function body. The jsonrpsee error types are aliased because `Error` at module scope is `thiserror::Error`. --- crates/foreign-chain-inspector/src/lib.rs | 49 ++++++++++++----------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/crates/foreign-chain-inspector/src/lib.rs b/crates/foreign-chain-inspector/src/lib.rs index 49f869ca2e..11783d56ef 100644 --- a/crates/foreign-chain-inspector/src/lib.rs +++ b/crates/foreign-chain-inspector/src/lib.rs @@ -5,6 +5,9 @@ use std::time::Duration; use derive_more::{Deref, Display, From}; use ethereum_types::H256; use http::{HeaderMap, HeaderName, HeaderValue}; +use jsonrpsee::core::client::error::Error as RpcClientError; +use jsonrpsee::core::http_helpers::HttpError; +use jsonrpsee::http_client::transport::Error as HttpTransportError; use jsonrpsee::http_client::{HttpClient, HttpClientBuilder}; use near_mpc_bounded_collections::NonEmptyVec; use near_mpc_contract_interface::types::ProviderId; @@ -369,35 +372,33 @@ impl ForeignChainInspectionError { /// /// The messages name the HTTP status or the JSON-RPC code, never the URL: `Path`/`Query` auth /// splices the operator's API key into it. - pub fn classify_rpc_client_error(error: jsonrpsee::core::client::error::Error) -> Self { - use jsonrpsee::core::client::error::Error as ClientError; - use jsonrpsee::core::http_helpers::HttpError; - use jsonrpsee::http_client::transport::Error as TransportError; - + pub fn classify_rpc_client_error(error: RpcClientError) -> Self { match error { - ClientError::Call(object) => { + RpcClientError::Call(object) => { Self::RpcRequestRejected(format!("JSON-RPC error code {}", object.code())) } - ClientError::ParseError(error) => Self::MalformedRpcResponse(error.to_string()), - ClientError::RequestTimeout => Self::Timeout, - ClientError::Transport(error) => match error.downcast_ref::() { - Some(TransportError::Rejected { status_code }) => { - let status = format!("HTTP status {status_code}"); - if is_retryable_status(*status_code) { - Self::RpcRequestFailed(status) - } else { - Self::RpcRequestRejected(status) + RpcClientError::ParseError(error) => Self::MalformedRpcResponse(error.to_string()), + RpcClientError::RequestTimeout => Self::Timeout, + RpcClientError::Transport(error) => { + match error.downcast_ref::() { + Some(HttpTransportError::Rejected { status_code }) => { + let status = format!("HTTP status {status_code}"); + if is_retryable_status(*status_code) { + Self::RpcRequestFailed(status) + } else { + Self::RpcRequestRejected(status) + } } + // Not a response the caller can use, as opposed to no response at all. + Some(HttpTransportError::Http(HttpError::Malformed | HttpError::TooLarge)) => { + Self::MalformedRpcResponse("response was not valid JSON-RPC".to_string()) + } + Some(HttpTransportError::Url(_)) => { + Self::RpcRequestRejected("invalid RPC URL".to_string()) + } + _ => Self::RpcRequestFailed("transport failure".to_string()), } - // Not a response the caller can use, as opposed to no response at all. - Some(TransportError::Http(HttpError::Malformed | HttpError::TooLarge)) => { - Self::MalformedRpcResponse("response was not valid JSON-RPC".to_string()) - } - Some(TransportError::Url(_)) => { - Self::RpcRequestRejected("invalid RPC URL".to_string()) - } - _ => Self::RpcRequestFailed("transport failure".to_string()), - }, + } other => Self::RpcRequestFailed(other.to_string()), } } From aefb0c42a7a762d96f86715399d396056ba2755f Mon Sep 17 00:00:00 2001 From: Haiyue Chen Date: Fri, 31 Jul 2026 02:02:45 +0200 Subject: [PATCH 4/9] fix(starknet): scope the identity check to the chains that have a probe The config section promised `MissingExpectedIdentity` for any chain left without an identity, and listed a value for every chain, while only the chains with a probe read the field at all. It now says which chains those are and what the rest report. `canonical_text` stripped only a lowercase `0x`, so an operator writing `0X534E5F4D41494E` got a false `WrongNetwork` from a healthy provider. Both prefixes normalize now. Also: - Throttling signalled as a JSON-RPC error object keeps its retries, rather than counting as a refusal no retry can change. - The identity a provider reports is truncated before it reaches a report that ends up in logs and metric labels. - Classifying a client-setup failure no longer re-reads the environment, so it neither guesses nor copies the token again. - `provider_failure` gets a case per variant, including the ones that report the transaction's own state. --- .../foreign-chain-health-check/src/probe.rs | 196 ++++++++++++++---- crates/foreign-chain-inspector/src/lib.rs | 18 +- .../tests/rpc_error_classification.rs | 54 ++++- .../src/starknet.rs | 21 +- crates/node-config/src/foreign_chains.rs | 3 +- .../node/src/providers/verify_foreign_tx.rs | 7 +- docs/foreign-chain-transactions.md | 8 +- 7 files changed, 256 insertions(+), 51 deletions(-) diff --git a/crates/foreign-chain-health-check/src/probe.rs b/crates/foreign-chain-health-check/src/probe.rs index 14b25885b4..d635912d1f 100644 --- a/crates/foreign-chain-health-check/src/probe.rs +++ b/crates/foreign-chain-health-check/src/probe.rs @@ -8,9 +8,7 @@ use foreign_chain_inspector::starknet::inspector::StarknetInspector; use foreign_chain_inspector::{ ChainIdentity, FanOut, ForeignChainInspectionError, ProviderFailure, }; -use mpc_node_config::{ - AuthConfig, ForeignChainConfig, ForeignChainProviderConfig, ForeignChainsConfig, TokenConfig, -}; +use mpc_node_config::{ForeignChainConfig, ForeignChainProviderConfig, ForeignChainsConfig}; use near_mpc_bounded_collections::NonEmptyVec; use near_mpc_contract_interface::types::{ForeignChain, ProviderId}; @@ -41,7 +39,8 @@ pub enum ProviderStatus { /// The chain is configured without an `expected_chain_identity`, so its providers cannot be /// checked. Reported rather than skipped: silence would read as healthy. MissingExpectedIdentity, - /// The node can inspect this chain's transactions but has no identity probe for it yet. + /// The chain has no identity probe yet, either because none is written for it or because the + /// node cannot inspect it at all. ProbeNotImplemented, } @@ -104,7 +103,7 @@ pub async fn probe_all_providers(config: &ForeignChainsConfig) -> ProbeReport { }) .await } - // Other chains to be added in follow up PRs + // TODO(#4003): probe the remaining chains. _ => rows_of(chain, chain_config, ProviderStatus::ProbeNotImplemented), } }); @@ -132,10 +131,10 @@ where let provider_id = ProviderId(name.as_str().to_owned()); match new_inspector(provider) { Ok(inspector) => inspectors.push((provider_id, inspector)), - Err(_) => rows.push(ProviderHealth { + Err(error) => rows.push(ProviderHealth { chain, provider: provider_id, - status: setup_failure(provider), + status: setup_failure(&error), }), } } @@ -158,21 +157,16 @@ where rows } -/// The setup error itself is dropped for the reason [`ProviderStatus`] documents, so the one cause -/// an operator can act on gets a status of its own instead. -fn setup_failure(provider: &ForeignChainProviderConfig) -> ProviderStatus { - match auth_token(&provider.auth) { - Some(token) if token.resolve().is_err() => ProviderStatus::AuthTokenUnresolved, - _ => ProviderStatus::ClientSetupFailed, - } -} - -fn auth_token(auth: &AuthConfig) -> Option<&TokenConfig> { - match auth { - AuthConfig::None => None, - AuthConfig::Header { token, .. } - | AuthConfig::Path { token, .. } - | AuthConfig::Query { token, .. } => Some(token), +/// The error itself is dropped for the reason [`ProviderStatus`] documents, so the one cause an +/// operator can act on gets a status of its own instead. +/// +/// A token resolves from the environment or from the config file, and only the former can fail, so +/// a [`std::env::VarError`] in the chain is what names this case. +fn setup_failure(error: &anyhow::Error) -> ProviderStatus { + if error.chain().any(|cause| cause.is::()) { + ProviderStatus::AuthTokenUnresolved + } else { + ProviderStatus::ClientSetupFailed } } @@ -192,6 +186,18 @@ fn rows_of( .collect() } +/// Every chain's identity fits comfortably: the longest is Bitcoin's 66-character genesis hash. What +/// a provider answers instead is its own choice, and a report ends up in logs and metric labels. +fn bounded(observed: ChainIdentity) -> ChainIdentity { + const MAX_CHARS: usize = 96; + + let observed = observed.to_string(); + match observed.char_indices().nth(MAX_CHARS) { + None => ChainIdentity::from(observed), + Some((cutoff, _)) => ChainIdentity::from(format!("{}…", &observed[..cutoff])), + } +} + fn classify( expected: &ChainIdentity, identity: Result, @@ -200,7 +206,7 @@ fn classify( Ok(observed) if &observed == expected => ProviderStatus::Healthy, Ok(observed) => ProviderStatus::WrongNetwork { expected: expected.clone(), - observed, + observed: bounded(observed), }, Err(error) => match error.provider_failure() { Some(ProviderFailure::Unreachable) => ProviderStatus::Unreachable, @@ -218,6 +224,7 @@ fn classify( #[expect(non_snake_case)] mod tests { use super::*; + use mpc_node_config::{AuthConfig, TokenConfig}; use near_mpc_bounded_collections::NonEmptyBTreeMap; use std::num::NonZeroU64; @@ -286,12 +293,13 @@ mod tests { .await } - fn must_status_of(report: &ProbeReport, provider: &str) -> ProviderStatus { + /// Keyed by chain too: provider names repeat across chains in real configs. + fn must_status_of(report: &ProbeReport, chain: ForeignChain, provider: &str) -> ProviderStatus { report .rows() .iter() - .find(|row| row.provider.0 == provider) - .unwrap_or_else(|| panic!("missing row for provider `{provider}`")) + .find(|row| row.chain == chain && row.provider.0 == provider) + .unwrap_or_else(|| panic!("missing row for `{chain:?}` provider `{provider}`")) .status .clone() } @@ -312,7 +320,7 @@ mod tests { // Then mock.assert_async().await; assert_eq!( - must_status_of(&report, "publicnode"), + must_status_of(&report, ForeignChain::Starknet, "publicnode"), ProviderStatus::Healthy ); } @@ -332,7 +340,7 @@ mod tests { // Then assert_eq!( - must_status_of(&report, "publicnode"), + must_status_of(&report, ForeignChain::Starknet, "publicnode"), ProviderStatus::WrongNetwork { expected: ChainIdentity::from(MAINNET.to_string()), observed: ChainIdentity::from(SEPOLIA.to_string()), @@ -355,7 +363,7 @@ mod tests { // Then assert_eq!( - must_status_of(&report, "publicnode"), + must_status_of(&report, ForeignChain::Starknet, "publicnode"), ProviderStatus::Healthy ); } @@ -376,7 +384,7 @@ mod tests { // Then the provider is reported rather than skipped, and no request was sent assert_eq!( - must_status_of(&report, "publicnode"), + must_status_of(&report, ForeignChain::Starknet, "publicnode"), ProviderStatus::MissingExpectedIdentity ); mock.assert_calls_async(0).await; @@ -395,7 +403,7 @@ mod tests { // Then assert_eq!( - must_status_of(&report, "publicnode"), + must_status_of(&report, ForeignChain::Starknet, "publicnode"), ProviderStatus::Unreachable ); } @@ -424,7 +432,7 @@ mod tests { // Then the refusal is named as such, and retrying it is pointless assert_eq!( - must_status_of(&report, "keyed"), + must_status_of(&report, ForeignChain::Starknet, "keyed"), ProviderStatus::RequestRejected ); mock.assert_calls_async(1).await; @@ -454,7 +462,7 @@ mod tests { // Then assert_eq!( - must_status_of(&report, "publicnode"), + must_status_of(&report, ForeignChain::Starknet, "publicnode"), ProviderStatus::RequestRejected ); } @@ -479,7 +487,7 @@ mod tests { // Then assert_eq!( - must_status_of(&report, "publicnode"), + must_status_of(&report, ForeignChain::Starknet, "publicnode"), ProviderStatus::MalformedResponse ); } @@ -506,7 +514,10 @@ mod tests { let report = probe_all_providers(&config).await; // Then - assert_eq!(must_status_of(&report, "slow"), ProviderStatus::TimedOut); + assert_eq!( + must_status_of(&report, ForeignChain::Starknet, "slow"), + ProviderStatus::TimedOut + ); } #[tokio::test] @@ -524,7 +535,7 @@ mod tests { // Then assert_eq!( - must_status_of(&report, "publicnode"), + must_status_of(&report, ForeignChain::Starknet, "publicnode"), ProviderStatus::Healthy ); } @@ -554,7 +565,7 @@ mod tests { // Then the operator learns which of the two setup failures it was assert_eq!( - must_status_of(&report, "keyed"), + must_status_of(&report, ForeignChain::Starknet, "keyed"), ProviderStatus::AuthTokenUnresolved ); } @@ -572,7 +583,7 @@ mod tests { // Then assert_eq!( - must_status_of(&report, "wrong-scheme"), + must_status_of(&report, ForeignChain::Starknet, "wrong-scheme"), ProviderStatus::ClientSetupFailed ); } @@ -590,9 +601,12 @@ mod tests { let report = probe_all_providers(&config).await; // Then - assert_eq!(must_status_of(&report, "healthy"), ProviderStatus::Healthy); assert_eq!( - must_status_of(&report, "broken"), + must_status_of(&report, ForeignChain::Starknet, "healthy"), + ProviderStatus::Healthy + ); + assert_eq!( + must_status_of(&report, ForeignChain::Starknet, "broken"), ProviderStatus::Unreachable ); assert_eq!( @@ -621,9 +635,109 @@ mod tests { // Then it is visible in the report rather than silently absent assert_eq!( - must_status_of(&report, "publicnode"), + must_status_of(&report, ForeignChain::Base, "publicnode"), + ProviderStatus::ProbeNotImplemented + ); + } + + #[tokio::test] + async fn probe_all_providers__should_report_every_configured_chain_under_its_own_chain() { + // Given the same provider name configured for two chains + let server = httpmock::MockServer::start_async().await; + mock_chain_id(&server, MAINNET).await; + let config = ForeignChainsConfig { + starknet: Some(chain_config( + Some(MAINNET), + one_provider("publicnode", &server.base_url()), + )), + base: Some(chain_config( + Some("8453"), + one_provider("publicnode", CLOSED_PORT_URL), + )), + ..Default::default() + }; + + // When + let report = probe_all_providers(&config).await; + + // Then each chain gets its own row rather than one shadowing the other + assert_eq!( + must_status_of(&report, ForeignChain::Starknet, "publicnode"), + ProviderStatus::Healthy + ); + assert_eq!( + must_status_of(&report, ForeignChain::Base, "publicnode"), ProviderStatus::ProbeNotImplemented ); + assert_eq!(report.counts_per_chain().len(), 2); + } + + #[tokio::test] + async fn probe_all_providers__should_retry_a_provider_that_refused_with_a_rate_limit_code() { + // Given a provider signalling throttling as a JSON-RPC error object over HTTP 200 + let server = httpmock::MockServer::start_async().await; + let mock = server + .mock_async(|when, then| { + when.method(httpmock::Method::POST); + then.status(200).json_body(serde_json::json!({ + "jsonrpc": "2.0", + "id": 0, + "error": {"code": -32005, "message": "limit exceeded"}, + })); + }) + .await; + let config = starknet_only(with_retries( + chain_config(Some(MAINNET), one_provider("keyed", &server.base_url())), + 2, + )); + + // When + let report = probe_all_providers(&config).await; + + // Then throttling is the one refusal worth retrying + assert_eq!( + must_status_of(&report, ForeignChain::Starknet, "keyed"), + ProviderStatus::Unreachable + ); + mock.assert_calls_async(2).await; + } + + #[tokio::test] + async fn probe_all_providers__should_bound_the_identity_a_provider_reports() { + // Given a provider answering with far more than an identity + let server = httpmock::MockServer::start_async().await; + let flood = "n".repeat(5_000); + mock_chain_id(&server, &flood).await; + let config = starknet_only(chain_config( + Some(MAINNET), + one_provider("publicnode", &server.base_url()), + )); + + // When + let report = probe_all_providers(&config).await; + + // Then what reaches a log line or a metric label is bounded + let ProviderStatus::WrongNetwork { observed, .. } = + must_status_of(&report, ForeignChain::Starknet, "publicnode") + else { + panic!("expected the flood to read as the wrong network"); + }; + assert!(observed.to_string().chars().count() < 100); + } + + #[test] + fn classify__should_report_a_transaction_level_error_as_malformed() { + // Given an error about a transaction, which the probe never asks about + let expected = ChainIdentity::from(MAINNET.to_string()); + + // When + let status = classify( + &expected, + Err(ForeignChainInspectionError::TransactionNotFound), + ); + + // Then the inspector answered outside the contract its probe has + assert_eq!(status, ProviderStatus::MalformedResponse); } #[tokio::test] diff --git a/crates/foreign-chain-inspector/src/lib.rs b/crates/foreign-chain-inspector/src/lib.rs index 11783d56ef..c098a4bc47 100644 --- a/crates/foreign-chain-inspector/src/lib.rs +++ b/crates/foreign-chain-inspector/src/lib.rs @@ -375,7 +375,13 @@ impl ForeignChainInspectionError { pub fn classify_rpc_client_error(error: RpcClientError) -> Self { match error { RpcClientError::Call(object) => { - Self::RpcRequestRejected(format!("JSON-RPC error code {}", object.code())) + let code = object.code(); + let message = format!("JSON-RPC error code {code}"); + if is_rate_limit_error_code(code) { + Self::RpcRequestFailed(message) + } else { + Self::RpcRequestRejected(message) + } } RpcClientError::ParseError(error) => Self::MalformedRpcResponse(error.to_string()), RpcClientError::RequestTimeout => Self::Timeout, @@ -425,6 +431,16 @@ impl ForeignChainInspectionError { } } +/// Throttling reaches some providers' callers as a JSON-RPC error object over HTTP 200 rather than +/// as a 429, and it is the one refusal worth retrying: `-32005` is Alchemy's and Infura's "limit +/// exceeded", `-32029` the code others use for the same. +fn is_rate_limit_error_code(code: i32) -> bool { + const LIMIT_EXCEEDED: i32 = -32005; + const TOO_MANY_REQUESTS: i32 = -32029; + + matches!(code, LIMIT_EXCEEDED | TOO_MANY_REQUESTS) +} + /// Request timeout, too many requests, and anything the server blames on itself. Every other /// status is the provider's verdict on the request, which the same request cannot change. fn is_retryable_status(status_code: u16) -> bool { diff --git a/crates/foreign-chain-inspector/tests/rpc_error_classification.rs b/crates/foreign-chain-inspector/tests/rpc_error_classification.rs index bdae97129e..c9b348a707 100644 --- a/crates/foreign-chain-inspector/tests/rpc_error_classification.rs +++ b/crates/foreign-chain-inspector/tests/rpc_error_classification.rs @@ -4,7 +4,7 @@ //! whether a provider failed to answer, answered and refused, or answered unusably. use assert_matches::assert_matches; -use foreign_chain_inspector::ForeignChainInspectionError; +use foreign_chain_inspector::{BlockConfirmations, ForeignChainInspectionError, ProviderFailure}; use jsonrpsee::core::client::error::Error as RpcClientError; use jsonrpsee::core::http_helpers::HttpError; use jsonrpsee::http_client::transport::Error as TransportError; @@ -153,3 +153,55 @@ fn classify_rpc_client_error__should_report_a_client_side_timeout_as_a_timeout() // Then assert_matches!(classified, ForeignChainInspectionError::Timeout); } + +#[rstest] +#[case(-32005)] +#[case(-32029)] +fn classify_rpc_client_error__should_report_a_rate_limit_code_as_a_transient_failure( + #[case] code: i32, +) { + // Given a provider signalling throttling in the error object rather than with a 429 + let error = RpcClientError::Call(jsonrpsee::types::ErrorObject::owned( + code, + "limit exceeded", + None::<()>, + )); + + // When + let classified = ForeignChainInspectionError::classify_rpc_client_error(error); + + // Then + assert_matches!( + &classified, + ForeignChainInspectionError::RpcRequestFailed(_) + ); + assert!(classified.is_transient()); +} + +#[rstest] +#[case(ForeignChainInspectionError::RpcRequestFailed("_".to_string()), Some(ProviderFailure::Unreachable))] +#[case(ForeignChainInspectionError::RpcRequestRejected("_".to_string()), Some(ProviderFailure::Rejected))] +#[case(ForeignChainInspectionError::Timeout, Some(ProviderFailure::TimedOut))] +#[case(ForeignChainInspectionError::MalformedRpcResponse("_".to_string()), Some(ProviderFailure::Malformed))] +#[case( + ForeignChainInspectionError::InspectorResponseMismatch, + Some(ProviderFailure::Malformed) +)] +// The transaction's own state is an answer, not a fault of the provider that reported it. +#[case(ForeignChainInspectionError::TransactionNotFound, None)] +#[case(ForeignChainInspectionError::TransactionFailed, None)] +#[case(ForeignChainInspectionError::NotFinalized, None)] +#[case(ForeignChainInspectionError::NotEnoughBlockConfirmations { + expected: BlockConfirmations::from(6), + got: BlockConfirmations::from(1), +}, None)] +fn provider_failure__should_name_only_the_failures_the_provider_owns( + #[case] error: ForeignChainInspectionError, + #[case] expected: Option, +) { + // When + let failure = error.provider_failure(); + + // Then + assert_eq!(failure, expected); +} diff --git a/crates/foreign-chain-rpc-interfaces/src/starknet.rs b/crates/foreign-chain-rpc-interfaces/src/starknet.rs index 870290f22b..2ecdf93d5f 100644 --- a/crates/foreign-chain-rpc-interfaces/src/starknet.rs +++ b/crates/foreign-chain-rpc-interfaces/src/starknet.rs @@ -142,8 +142,15 @@ pub struct ChainIdResponse(pub String); impl ChainIdResponse { /// Lowercase, no leading zeros. + /// + /// The prefix is matched case-insensitively although the spec's pattern is not: the same + /// normalization is applied to operator-written identities, which the pattern does not bind. pub fn canonical_text(&self) -> String { - let Some(digits) = self.0.strip_prefix("0x") else { + let digits = self + .0 + .strip_prefix("0x") + .or_else(|| self.0.strip_prefix("0X")); + let Some(digits) = digits else { return self.0.clone(); }; let significant = digits.trim_start_matches('0'); @@ -193,6 +200,18 @@ mod tests { assert_eq!(response.canonical_text(), MAINNET_CHAIN_ID); } + #[test] + fn chain_id_response__should_normalize_an_upper_cased_prefix() { + // Given: a spelling only an operator can write, since the spec's pattern binds providers + let json = serde_json::json!("0X534E5F4D41494E"); + + // When + let response: ChainIdResponse = serde_json::from_value(json).unwrap(); + + // Then + assert_eq!(response.canonical_text(), MAINNET_CHAIN_ID); + } + #[test] fn chain_id_response__should_normalize_a_zero_chain_id() { // Given diff --git a/crates/node-config/src/foreign_chains.rs b/crates/node-config/src/foreign_chains.rs index 66132c7108..fdf596dff9 100644 --- a/crates/node-config/src/foreign_chains.rs +++ b/crates/node-config/src/foreign_chains.rs @@ -45,7 +45,8 @@ pub struct ForeignChainsConfig { #[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)] pub struct ForeignChainConfig { pub timeout_sec: NonZeroU64, - /// Total attempts per provider, not additional ones: `1` means a single try. + /// Total attempts per provider, not additional ones: `1` means a single try. Read by the + /// chain-identity probe; transaction verification does not retry a provider at all. pub max_retries: NonZeroU64, /// The network fingerprint the operator expects every provider of this chain to report, in the /// chain's canonical text form. A chain id for chains that have one, a genesis hash or digest diff --git a/crates/node/src/providers/verify_foreign_tx.rs b/crates/node/src/providers/verify_foreign_tx.rs index e624ad9883..9615254efb 100644 --- a/crates/node/src/providers/verify_foreign_tx.rs +++ b/crates/node/src/providers/verify_foreign_tx.rs @@ -26,10 +26,11 @@ use near_mpc_contract_interface::types::ProviderId; use std::sync::Arc; use std::time::Duration; -/// Pre-built HTTP clients for each foreign chain, keyed in provider config order. +/// Pre-built HTTP clients for each foreign chain, one per configured provider and named by its +/// [`ProviderId`]. /// -/// Built once at startup so that request handling only needs to select an index -/// instead of re-parsing config and constructing clients on every call. +/// Built once at startup so that request handling fans out over ready clients instead of re-parsing +/// config and constructing them on every call. pub(crate) struct ForeignChainInspectors { pub bitcoin: Option>>, pub abstract_chain: Option>>, diff --git a/docs/foreign-chain-transactions.md b/docs/foreign-chain-transactions.md index 77ad3dc96a..dcff7b2de4 100644 --- a/docs/foreign-chain-transactions.md +++ b/docs/foreign-chain-transactions.md @@ -685,14 +685,16 @@ separates the test networks from each other where a network *name* would not: te separates mainnet from testnet, but two devnets can collide. `solana` and `ethereum` are configurable but absent from the table: neither has an inspector, so -setting `expected_network_fingerprint` for them has no effect. +there is nothing about them to verify in the first place. The fingerprint is set per chain rather than once per deployment, so a config can mix networks, and each value must match the network of the `rpc_url` beside it. The value is always a quoted string, including the fingerprints that look numeric. -A chain configured without one is not skipped: the probe reports every provider of that chain as -`MissingExpectedIdentity`, because a silent skip reads as healthy on a dashboard. +Only the chains with an identity probe read the field at all — starknet today, the rest as their +probes are written. For those chains, leaving it unset is not a silent skip: every provider of the +chain is reported as `MissingExpectedIdentity`, because silence reads as healthy on a dashboard. A +chain with no probe yet reports `ProbeNotImplemented` whether the field is set or not. ## Risks From 17cd3618a2373ba8473ed4f132dc8d31d4d1783d Mon Sep 17 00:00:00 2001 From: Haiyue Chen Date: Fri, 31 Jul 2026 13:21:19 +0200 Subject: [PATCH 5/9] fix: follow the config field rename in the probe expected_chain_identity became expected_network_fingerprint on the parent branch. --- crates/foreign-chain-health-check/src/probe.rs | 10 +++++----- .../tests/starknet_rpc_manual.rs | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/foreign-chain-health-check/src/probe.rs b/crates/foreign-chain-health-check/src/probe.rs index d635912d1f..3435497231 100644 --- a/crates/foreign-chain-health-check/src/probe.rs +++ b/crates/foreign-chain-health-check/src/probe.rs @@ -1,5 +1,5 @@ //! Asks every configured RPC provider which network it serves and compares the answer against the -//! operator's `expected_chain_identity`. +//! operator's `expected_network_fingerprint`. use std::collections::BTreeMap; use std::time::Duration; @@ -36,8 +36,8 @@ pub enum ProviderStatus { AuthTokenUnresolved, /// The RPC client could not be built from the provider's URL and auth. ClientSetupFailed, - /// The chain is configured without an `expected_chain_identity`, so its providers cannot be - /// checked. Reported rather than skipped: silence would read as healthy. + /// The chain is configured without an `expected_network_fingerprint`, so its providers cannot + /// be checked. Reported rather than skipped: silence would read as healthy. MissingExpectedIdentity, /// The chain has no identity probe yet, either because none is written for it or because the /// node cannot inspect it at all. @@ -120,7 +120,7 @@ async fn probe_chain( where I: foreign_chain_inspector::ChainIdentityInspector + Clone + Send + Sync + 'static, { - let Some(expected) = &config.expected_chain_identity else { + let Some(expected) = &config.expected_network_fingerprint else { return rows_of(chain, config, ProviderStatus::MissingExpectedIdentity); }; let expected = I::canonical_identity(expected); @@ -251,7 +251,7 @@ mod tests { ForeignChainConfig { timeout_sec: NonZeroU64::new(1).unwrap(), max_retries: NonZeroU64::new(1).unwrap(), - expected_chain_identity: expected.map(str::to_string), + expected_network_fingerprint: expected.map(str::to_string), providers, } } diff --git a/crates/foreign-chain-inspector/tests/starknet_rpc_manual.rs b/crates/foreign-chain-inspector/tests/starknet_rpc_manual.rs index 2939c2b93e..87762f760c 100644 --- a/crates/foreign-chain-inspector/tests/starknet_rpc_manual.rs +++ b/crates/foreign-chain-inspector/tests/starknet_rpc_manual.rs @@ -109,7 +109,7 @@ fn parse_starknet_felt_hash Date: Fri, 31 Jul 2026 13:38:47 +0200 Subject: [PATCH 6/9] refactor: adopt the network fingerprint vocabulary in the probe Follows the config field name: ChainIdentity becomes NetworkFingerprint, the trait and its methods follow, and MissingExpectedIdentity becomes MissingExpectedFingerprint. --- .../foreign-chain-health-check/src/probe.rs | 63 ++++++++++--------- crates/foreign-chain-inspector/src/lib.rs | 26 ++++---- .../src/starknet/inspector.rs | 9 +-- .../tests/starknet_inspector.rs | 32 +++++----- .../tests/starknet_rpc_manual.rs | 13 ++-- .../src/starknet.rs | 2 +- crates/node-config/src/foreign_chains.rs | 2 +- ...er-node-foreign-chain-rpc-configuration.md | 4 +- docs/foreign-chain-transactions.md | 18 +++--- 9 files changed, 86 insertions(+), 83 deletions(-) diff --git a/crates/foreign-chain-health-check/src/probe.rs b/crates/foreign-chain-health-check/src/probe.rs index 3435497231..5097d8ab62 100644 --- a/crates/foreign-chain-health-check/src/probe.rs +++ b/crates/foreign-chain-health-check/src/probe.rs @@ -6,7 +6,7 @@ use std::time::Duration; use foreign_chain_inspector::starknet::inspector::StarknetInspector; use foreign_chain_inspector::{ - ChainIdentity, FanOut, ForeignChainInspectionError, ProviderFailure, + FanOut, ForeignChainInspectionError, NetworkFingerprint, ProviderFailure, }; use mpc_node_config::{ForeignChainConfig, ForeignChainProviderConfig, ForeignChainsConfig}; use near_mpc_bounded_collections::NonEmptyVec; @@ -23,8 +23,8 @@ use crate::prepare_jsonrpc; pub enum ProviderStatus { Healthy, WrongNetwork { - expected: ChainIdentity, - observed: ChainIdentity, + expected: NetworkFingerprint, + observed: NetworkFingerprint, }, /// DNS, TLS, connection refused, 5xx, or rate limiting. Unreachable, @@ -38,8 +38,8 @@ pub enum ProviderStatus { ClientSetupFailed, /// The chain is configured without an `expected_network_fingerprint`, so its providers cannot /// be checked. Reported rather than skipped: silence would read as healthy. - MissingExpectedIdentity, - /// The chain has no identity probe yet, either because none is written for it or because the + MissingExpectedFingerprint, + /// The chain has no fingerprint probe yet, either because none is written for it or because the /// node cannot inspect it at all. ProbeNotImplemented, } @@ -118,12 +118,12 @@ async fn probe_chain( new_inspector: impl Fn(&ForeignChainProviderConfig) -> anyhow::Result, ) -> Vec where - I: foreign_chain_inspector::ChainIdentityInspector + Clone + Send + Sync + 'static, + I: foreign_chain_inspector::NetworkFingerprintInspector + Clone + Send + Sync + 'static, { let Some(expected) = &config.expected_network_fingerprint else { - return rows_of(chain, config, ProviderStatus::MissingExpectedIdentity); + return rows_of(chain, config, ProviderStatus::MissingExpectedFingerprint); }; - let expected = I::canonical_identity(expected); + let expected = I::canonical_fingerprint(expected); let mut inspectors = Vec::new(); let mut rows = Vec::new(); @@ -144,14 +144,14 @@ where }; let timeout = Duration::from_secs(config.timeout_sec.get()); - let identities = FanOut::new(inspectors) - .chain_identities(timeout, config.max_retries) + let fingerprints = FanOut::new(inspectors) + .network_fingerprints(timeout, config.max_retries) .await; - for (provider, identity) in identities { + for (provider, fingerprint) in fingerprints { rows.push(ProviderHealth { chain, provider, - status: classify(&expected, identity), + status: classify(&expected, fingerprint), }); } rows @@ -186,23 +186,24 @@ fn rows_of( .collect() } -/// Every chain's identity fits comfortably: the longest is Bitcoin's 66-character genesis hash. What +/// Every chain's fingerprint fits comfortably: the longest is Bitcoin's 66-character genesis hash. +/// What /// a provider answers instead is its own choice, and a report ends up in logs and metric labels. -fn bounded(observed: ChainIdentity) -> ChainIdentity { +fn bounded(observed: NetworkFingerprint) -> NetworkFingerprint { const MAX_CHARS: usize = 96; let observed = observed.to_string(); match observed.char_indices().nth(MAX_CHARS) { - None => ChainIdentity::from(observed), - Some((cutoff, _)) => ChainIdentity::from(format!("{}…", &observed[..cutoff])), + None => NetworkFingerprint::from(observed), + Some((cutoff, _)) => NetworkFingerprint::from(format!("{}…", &observed[..cutoff])), } } fn classify( - expected: &ChainIdentity, - identity: Result, + expected: &NetworkFingerprint, + fingerprint: Result, ) -> ProviderStatus { - match identity { + match fingerprint { Ok(observed) if &observed == expected => ProviderStatus::Healthy, Ok(observed) => ProviderStatus::WrongNetwork { expected: expected.clone(), @@ -342,14 +343,14 @@ mod tests { assert_eq!( must_status_of(&report, ForeignChain::Starknet, "publicnode"), ProviderStatus::WrongNetwork { - expected: ChainIdentity::from(MAINNET.to_string()), - observed: ChainIdentity::from(SEPOLIA.to_string()), + expected: NetworkFingerprint::from(MAINNET.to_string()), + observed: NetworkFingerprint::from(SEPOLIA.to_string()), } ); } #[tokio::test] - async fn probe_all_providers__should_normalize_the_reported_identity_before_comparing() { + async fn probe_all_providers__should_normalize_the_reported_fingerprint_before_comparing() { // Given a provider padding and uppercasing the chain id, as the spec permits let server = httpmock::MockServer::start_async().await; mock_chain_id(&server, "0x00534E5F4D41494E").await; @@ -369,7 +370,7 @@ mod tests { } #[tokio::test] - async fn probe_all_providers__should_report_a_chain_without_an_expected_identity_without_probing() + async fn probe_all_providers__should_report_a_chain_without_an_expected_fingerprint_without_probing() { // Given let server = httpmock::MockServer::start_async().await; @@ -385,7 +386,7 @@ mod tests { // Then the provider is reported rather than skipped, and no request was sent assert_eq!( must_status_of(&report, ForeignChain::Starknet, "publicnode"), - ProviderStatus::MissingExpectedIdentity + ProviderStatus::MissingExpectedFingerprint ); mock.assert_calls_async(0).await; } @@ -521,7 +522,7 @@ mod tests { } #[tokio::test] - async fn probe_all_providers__should_normalize_the_configured_identity_before_comparing() { + async fn probe_all_providers__should_normalize_the_configured_fingerprint_before_comparing() { // Given an operator writing the chain id padded and uppercased, as the spec permits let server = httpmock::MockServer::start_async().await; mock_chain_id(&server, MAINNET).await; @@ -619,9 +620,9 @@ mod tests { } #[tokio::test] - async fn probe_all_providers__should_report_a_chain_with_no_identity_probe_as_not_implemented() - { - // Given a chain the node can inspect but has no identity probe for + async fn probe_all_providers__should_report_a_chain_with_no_fingerprint_probe_as_not_implemented() + { + // Given a chain the node can inspect but has no fingerprint probe for let config = ForeignChainsConfig { base: Some(chain_config( Some("8453"), @@ -703,8 +704,8 @@ mod tests { } #[tokio::test] - async fn probe_all_providers__should_bound_the_identity_a_provider_reports() { - // Given a provider answering with far more than an identity + async fn probe_all_providers__should_bound_the_fingerprint_a_provider_reports() { + // Given a provider answering with far more than a fingerprint let server = httpmock::MockServer::start_async().await; let flood = "n".repeat(5_000); mock_chain_id(&server, &flood).await; @@ -728,7 +729,7 @@ mod tests { #[test] fn classify__should_report_a_transaction_level_error_as_malformed() { // Given an error about a transaction, which the probe never asks about - let expected = ChainIdentity::from(MAINNET.to_string()); + let expected = NetworkFingerprint::from(MAINNET.to_string()); // When let status = classify( diff --git a/crates/foreign-chain-inspector/src/lib.rs b/crates/foreign-chain-inspector/src/lib.rs index c098a4bc47..86a2c98490 100644 --- a/crates/foreign-chain-inspector/src/lib.rs +++ b/crates/foreign-chain-inspector/src/lib.rs @@ -44,20 +44,20 @@ pub trait ForeignChainInspector { /// The network a provider serves, as the chain itself reports it: a chain id or a genesis hash, in /// one canonical text form per chain. #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Display, From)] -pub struct ChainIdentity(String); +pub struct NetworkFingerprint(String); -/// Reports the [`ChainIdentity`] of the provider an inspector talks to. +/// Reports the [`NetworkFingerprint`] of the provider an inspector talks to. /// -/// Identities are compared verbatim, so both the reported and the expected one go through the +/// Fingerprints are compared verbatim, so both the reported and the expected one go through the /// impl's canonical form. Fetches a chain-wide constant providers never prune. -pub trait ChainIdentityInspector { - fn chain_identity( +pub trait NetworkFingerprintInspector { + fn network_fingerprint( &self, - ) -> impl Future> + Send; + ) -> impl Future> + Send; - /// Puts an operator-supplied identity into the form [`Self::chain_identity`] returns, so that a - /// spec-legal spelling of the right network does not read as the wrong network. - fn canonical_identity(expected: &str) -> ChainIdentity; + /// Puts an operator-supplied fingerprint into the form [`Self::network_fingerprint`] returns, + /// so that a spec-legal spelling of the right network does not read as the wrong network. + fn canonical_fingerprint(expected: &str) -> NetworkFingerprint; } /// Combines multiple inspectors that target the same chain into a single inspector. @@ -204,19 +204,19 @@ pub const RETRY_BACKOFF: Duration = Duration::from_millis(200); impl FanOut where - Inspector: ChainIdentityInspector + Clone + Send + Sync + 'static, + Inspector: NetworkFingerprintInspector + Clone + Send + Sync + 'static, { /// Ask every provider for the network it serves, concurrently, one result each. /// Unlike [`FanOut::extract`], disagreement is not an error: a diagnostic caller needs the /// individual answers. Each provider gets up to `attempts` tries, `timeout` per try plus /// [`RETRY_BACKOFF`] between them, and only a transient failure is retried. - pub async fn chain_identities( + pub async fn network_fingerprints( &self, timeout: Duration, attempts: NonZeroU64, ) -> Vec<( ProviderId, - Result, + Result, )> { let mut join_set = tokio::task::JoinSet::new(); for (provider, inspector) in self.inspectors.iter() { @@ -224,7 +224,7 @@ where let provider = provider.clone(); join_set.spawn(async move { let ask = || async { - tokio::time::timeout(timeout, inspector.chain_identity()) + tokio::time::timeout(timeout, inspector.network_fingerprint()) .await .unwrap_or(Err(ForeignChainInspectionError::Timeout)) }; diff --git a/crates/foreign-chain-inspector/src/starknet/inspector.rs b/crates/foreign-chain-inspector/src/starknet/inspector.rs index 9fa5bc5af4..58c7bf3e83 100644 --- a/crates/foreign-chain-inspector/src/starknet/inspector.rs +++ b/crates/foreign-chain-inspector/src/starknet/inspector.rs @@ -1,6 +1,7 @@ use crate::starknet::{StarknetExtractedValue, StarknetTransactionHash}; use crate::{ - ChainIdentity, ChainIdentityInspector, ForeignChainInspectionError, ForeignChainInspector, + ForeignChainInspectionError, ForeignChainInspector, NetworkFingerprint, + NetworkFingerprintInspector, }; use foreign_chain_rpc_interfaces::starknet::{ BlockId, ChainIdResponse, GetBlockWithTxHashesArgs, GetBlockWithTxHashesResponse, @@ -27,11 +28,11 @@ pub enum StarknetFinality { AcceptedOnL1, } -impl ChainIdentityInspector for StarknetInspector +impl NetworkFingerprintInspector for StarknetInspector where Client: ClientT + Send + Sync, { - async fn chain_identity(&self) -> Result { + async fn network_fingerprint(&self) -> Result { let chain_id: ChainIdResponse = self .client .request(CHAIN_ID_METHOD, NO_PARAMS) @@ -40,7 +41,7 @@ where Ok(chain_id.canonical_text().into()) } - fn canonical_identity(expected: &str) -> ChainIdentity { + fn canonical_fingerprint(expected: &str) -> NetworkFingerprint { ChainIdResponse(expected.to_owned()).canonical_text().into() } } diff --git a/crates/foreign-chain-inspector/tests/starknet_inspector.rs b/crates/foreign-chain-inspector/tests/starknet_inspector.rs index e474bdaa64..acf18579af 100644 --- a/crates/foreign-chain-inspector/tests/starknet_inspector.rs +++ b/crates/foreign-chain-inspector/tests/starknet_inspector.rs @@ -7,7 +7,7 @@ use crate::common::{ }; use foreign_chain_inspector::{ - ChainIdentityInspector, FanOut, ForeignChainInspectionError, ForeignChainInspector, + FanOut, ForeignChainInspectionError, ForeignChainInspector, NetworkFingerprintInspector, RpcAuthentication, build_http_client, starknet::{ StarknetBlockHash, StarknetExtractedValue, StarknetTransactionHash, @@ -519,18 +519,18 @@ async fn extract__should_return_event_log_for_specific_index_via_http_rpc_client const MAINNET_CHAIN_ID: &str = "0x534e5f4d41494e"; #[tokio::test] -async fn chain_identity__should_return_the_canonical_chain_id() { +async fn network_fingerprint__should_return_the_canonical_chain_id() { // Given: the chain id padded and uppercased, as a provider is free to send it. let inspector = StarknetInspector::new(mock_client_from_fixed_response("0x00534E5F4D41494E")); // When - let identity = inspector - .chain_identity() + let fingerprint = inspector + .network_fingerprint() .await - .expect("chain_identity should succeed"); + .expect("network_fingerprint should succeed"); // Then - assert_eq!(identity.to_string(), MAINNET_CHAIN_ID); + assert_eq!(fingerprint.to_string(), MAINNET_CHAIN_ID); } /// Builds a fan-out of one Starknet provider whose client runs `respond` on each call, and reports @@ -573,7 +573,7 @@ fn transport_error() -> RpcClientError { } #[tokio::test] -async fn chain_identities__should_retry_a_transient_failure_and_report_the_later_success() { +async fn network_fingerprints__should_retry_a_transient_failure_and_report_the_later_success() { // Given a provider that refuses the first call and answers the second let (fan_out, calls) = single_provider_fan_out(|call| match call { 0 => Err(transport_error()), @@ -582,26 +582,26 @@ async fn chain_identities__should_retry_a_transient_failure_and_report_the_later // When let results = fan_out - .chain_identities(Duration::from_secs(1), NonZeroU64::new(2).unwrap()) + .network_fingerprints(Duration::from_secs(1), NonZeroU64::new(2).unwrap()) .await; // Then assert_eq!(calls.load(Ordering::SeqCst), 2); - let identity = results[0] + let fingerprint = results[0] .1 .as_ref() .expect("second attempt should succeed"); - assert_eq!(identity.to_string(), MAINNET_CHAIN_ID); + assert_eq!(fingerprint.to_string(), MAINNET_CHAIN_ID); } #[tokio::test] -async fn chain_identities__should_stop_after_the_configured_number_of_attempts() { +async fn network_fingerprints__should_stop_after_the_configured_number_of_attempts() { // Given a provider that never answers let (fan_out, calls) = single_provider_fan_out(|_| Err(transport_error())); // When let results = fan_out - .chain_identities(Duration::from_secs(1), NonZeroU64::new(3).unwrap()) + .network_fingerprints(Duration::from_secs(1), NonZeroU64::new(3).unwrap()) .await; // Then @@ -613,7 +613,7 @@ async fn chain_identities__should_stop_after_the_configured_number_of_attempts() } #[tokio::test] -async fn chain_identities__should_not_retry_a_provider_that_refused_the_request() { +async fn network_fingerprints__should_not_retry_a_provider_that_refused_the_request() { // Given a provider refusing with a JSON-RPC error object, as one does for a bad API key let (fan_out, calls) = single_provider_fan_out(|_| { Err(RpcClientError::Call(jsonrpsee::types::ErrorObject::owned( @@ -625,7 +625,7 @@ async fn chain_identities__should_not_retry_a_provider_that_refused_the_request( // When let results = fan_out - .chain_identities(Duration::from_secs(1), NonZeroU64::new(3).unwrap()) + .network_fingerprints(Duration::from_secs(1), NonZeroU64::new(3).unwrap()) .await; // Then the refusal is reported as one, and the remaining attempts are not spent on it @@ -637,7 +637,7 @@ async fn chain_identities__should_not_retry_a_provider_that_refused_the_request( } #[tokio::test] -async fn chain_identity__should_propagate_rpc_client_errors() { +async fn network_fingerprint__should_propagate_rpc_client_errors() { // Given let client = FixedResponseRpcClient::new(|| { Err(RpcClientError::Transport(Box::new(std::io::Error::new( @@ -648,7 +648,7 @@ async fn chain_identity__should_propagate_rpc_client_errors() { let inspector = StarknetInspector::new(client); // When - let response = inspector.chain_identity().await; + let response = inspector.network_fingerprint().await; // Then assert_matches!( diff --git a/crates/foreign-chain-inspector/tests/starknet_rpc_manual.rs b/crates/foreign-chain-inspector/tests/starknet_rpc_manual.rs index 87762f760c..3d03cc0bb1 100644 --- a/crates/foreign-chain-inspector/tests/starknet_rpc_manual.rs +++ b/crates/foreign-chain-inspector/tests/starknet_rpc_manual.rs @@ -110,11 +110,11 @@ fn parse_starknet_felt_hash String { let digits = self .0 diff --git a/crates/node-config/src/foreign_chains.rs b/crates/node-config/src/foreign_chains.rs index fdf596dff9..fef3ce2d00 100644 --- a/crates/node-config/src/foreign_chains.rs +++ b/crates/node-config/src/foreign_chains.rs @@ -46,7 +46,7 @@ pub struct ForeignChainsConfig { pub struct ForeignChainConfig { pub timeout_sec: NonZeroU64, /// Total attempts per provider, not additional ones: `1` means a single try. Read by the - /// chain-identity probe; transaction verification does not retry a provider at all. + /// network fingerprint probe; transaction verification does not retry a provider at all. pub max_retries: NonZeroU64, /// The network fingerprint the operator expects every provider of this chain to report, in the /// chain's canonical text form. A chain id for chains that have one, a genesis hash or digest diff --git a/docs/design/allowing-per-node-foreign-chain-rpc-configuration.md b/docs/design/allowing-per-node-foreign-chain-rpc-configuration.md index 85fbc40e40..894b202149 100644 --- a/docs/design/allowing-per-node-foreign-chain-rpc-configuration.md +++ b/docs/design/allowing-per-node-foreign-chain-rpc-configuration.md @@ -132,6 +132,6 @@ Landing in stacked PRs under [#3208](https://github.com/near/mpc/issues/3208): - **PR 1** ([#3216](https://github.com/near/mpc/pull/3216)): on-chain data shape and storage field (`ForeignChainRpcWhitelist`, `ProviderEntry`, `AuthScheme`, `ChainRouting`, `ProviderId`), `MpcContract` field + storage key, and the `AllowedProviders` data-structure helpers (add/remove/get). No vote endpoints, no view function, no node-side wiring. - **PR 2** ([#3249](https://github.com/near/mpc/pull/3249)): contract-side voting on the whitelist. Adds `vote_update_foreign_chain_providers(votes: Vec)`, the `ProviderVotes` pending-vote storage, canonicalization (`providers` sorted by `provider_id`; duplicate chain or `provider_id` in a batch rejected with `InvalidParameters::MalformedPayload`), and the `clean_tee_status` extension that drops votes from non-participants. Voting is **full-snapshot**: each `ChainVote` proposes the chain's complete state (provider list + RPC response quorum), and the chain's stored `ChainEntry` is replaced once the protocol's signing threshold of participants holds the same canonical `(providers, quorum)` pair (same gate as `vote_add_os_measurement`). Drops the original Add/Remove-ops design for two reasons: (1) snapshot semantics canonicalize trivially (sort the proposed list), avoiding the order-of-apply ambiguity Add/Remove batches introduced, and (2) bundling the RPC response quorum into `ChainVote.quorum` collapses what was originally going to be two separate vote endpoints (whitelist + quorum) into one. -- **PR 3**: node-side wiring — operator-yaml schema change (`provider_id` + `token` only), indexer task streaming the whitelist into a `watch::Receiver`, coordinator startup pipeline (resolve → sample-tx probe → register), per-inspector chain-identity probe. +- **PR 3**: node-side wiring — operator-yaml schema change (`provider_id` + `token` only), indexer task streaming the whitelist into a `watch::Receiver`, coordinator startup pipeline (resolve → sample-tx probe → register), per-inspector network fingerprint probe. -The chain-identity probe is not a gate in that pipeline: it reports each provider's health, its expected values come from each chain's `expected_network_fingerprint` in operator config rather than from constants in the binary, and registration does not wait on it. The sample-tx probe still gates registration. +The network fingerprint probe is not a gate in that pipeline: it reports each provider's health, its expected values come from each chain's `expected_network_fingerprint` in operator config rather than from constants in the binary, and registration does not wait on it. The sample-tx probe still gates registration. diff --git a/docs/foreign-chain-transactions.md b/docs/foreign-chain-transactions.md index dcff7b2de4..aa756c890e 100644 --- a/docs/foreign-chain-transactions.md +++ b/docs/foreign-chain-transactions.md @@ -354,7 +354,7 @@ Relevant contract methods: ## On-chain RPC Provider Whitelist > Tracked under issue [#3208](https://github.com/near/mpc/issues/3208). Landing in stacked PRs: -> PR 1 (contract storage types) → PR 2 (vote endpoints) → PR 3 (node-side wiring + chain-identity probe). +> PR 1 (contract storage types) → PR 2 (vote endpoints) → PR 3 (node-side wiring + network fingerprint probe). > The text below describes the end-state design; sections call out per-PR scope where relevant. The per-participant registration model above leaves the network with no shared notion of *which RPC providers it trusts* — a TEE-attested node binary still pulls URLs from its own config file. To close that gap the contract carries a per-chain whitelist of providers, voted in by participants. Operators reference providers from the whitelist by `provider_id` in their local `foreign_chains.yaml`; the node assembles the final URL from `base_url` + `chain_routing` + the operator-supplied token (placed per `auth_scheme`). @@ -369,7 +369,7 @@ The per-participant registration model above leaves the network with no shared n | What the operator picks | Full URL, auth scheme, token reference | `provider_id` (label) + token reference | | Adding a new provider | Every operator updates their yaml; the network effectively supports a chain once enough do | Threshold of participants vote in `(chain, ProviderEntry)`; operators reference it by `provider_id` only | | Removing a compromised provider | Every operator manually edits their yaml; coordination problem | Threshold of participants vote remove; nodes pick up the change via the indexer and drop the provider on next reconfigure | -| Testnet vs mainnet separation | Implicit — operator decides what URL goes under which chain | Per-`ForeignChain` map slot, plus a startup *chain-identity probe* that calls the chain's self-identifying RPC and compares the response against the chain's `expected_network_fingerprint` from operator config — catches both lookup-level (wrong bucket) and content-level (wrong URL voted into the right bucket) confusion. | +| Testnet vs mainnet separation | Implicit — operator decides what URL goes under which chain | Per-`ForeignChain` map slot, plus a startup *network fingerprint probe* that calls the chain's self-identifying RPC and compares the response against the chain's `expected_network_fingerprint` from operator config — catches both lookup-level (wrong bucket) and content-level (wrong URL voted into the right bucket) confusion. | ### Whitelist storage shape @@ -521,21 +521,21 @@ Two reasons together drove the snapshot model over an Add/Remove diff-ops endpoi Voting uses the protocol's existing signing threshold (`self.threshold()?.value()`), the same gate as `verify_tee` and `vote_add_os_measurement`. An earlier design proposed a separate per-chain *voting* threshold so mainnet and testnet could be voted in under different agreement requirements; that was dropped because (a) there's no setter that could safely populate it without itself being voted in, leaving a hardcoded default that's strictly weaker than the protocol threshold, and (b) the per-chain numeric on `ChainVote.quorum` already covers the *runtime* security knob — how many of N whitelisted providers must agree for a node to accept a response — which is what operators actually need to tune per chain. -#### Why the chain-identity probe in addition to per-chain keying (PR 3) +#### Why the network fingerprint probe in addition to per-chain keying (PR 3) -The per-chain map key prevents *lookup* confusion: when the node resolves the operator's `ethereum:` section, only `entries[Ethereum]` is consulted, never `entries[Sepolia]`. What it doesn't prevent is a `ChainVote { chain: Ethereum, providers: [ProviderEntry { provider_id: "ankr", chain_routing: PathSegment { segment: "eth_sepolia" }, … }, …], threshold: _ }` getting voted in — the contract just stores what threshold consensus produces; it can't tell whether `"eth_sepolia"` actually corresponds to Ethereum mainnet. Threshold voter review is the first line of defense; the fan-out across a chain's providers is the structural one. The chain-identity probe is a per-node diagnostic on top of both. +The per-chain map key prevents *lookup* confusion: when the node resolves the operator's `ethereum:` section, only `entries[Ethereum]` is consulted, never `entries[Sepolia]`. What it doesn't prevent is a `ChainVote { chain: Ethereum, providers: [ProviderEntry { provider_id: "ankr", chain_routing: PathSegment { segment: "eth_sepolia" }, … }, …], threshold: _ }` getting voted in — the contract just stores what threshold consensus produces; it can't tell whether `"eth_sepolia"` actually corresponds to Ethereum mainnet. Threshold voter review is the first line of defense; the fan-out across a chain's providers is the structural one. The network fingerprint probe is a per-node diagnostic on top of both. At startup, each resolved provider gets its self-identifying RPC called and the response is compared against that chain's `expected_network_fingerprint` from the operator's config. The probe is report-only: a provider serving the wrong network is logged, but is not dropped, because a boot-time network blip should not take a chain out of signing. Taking the expected value from operator config rather than a constant in the attested binary is a deliberate trade. It makes mixed-network and local deployments checkable at all, since a config may pair one chain's mainnet with another's testnet and no binary can ship a value for a devnet. The cost is that the check no longer binds an operator: they can set the wrong value, or omit the field and get no check at all, and either way they fool only their own node's diagnostics. The network-level defenses against a wrong URL are unchanged: threshold voter review of the whitelist, and the provider fan-out, which fails the individual request when a provider disagrees with its siblings. -Not every chain has an identity probe. The table lists the ones that do, with the RPC each probes. A chain absent from it ignores `expected_chain_identity`. +Not every chain has a fingerprint probe. The table lists the ones that do, with the RPC each probes. A chain absent from it ignores `expected_network_fingerprint`. -| chain | probe | value (mainnet) | value (testnet) | +| chain | probe | fingerprint (mainnet) | fingerprint (testnet) | |---|---|---|---| | starknet | `starknet_chainId` | `0x534e5f4d41494e` (`SN_MAIN`) | `0x534e5f5345504f4c4941` (`SN_SEPOLIA`) | -Starknet's identity is the chain-id felt in lowercase `0x` hex without leading zeros. Both providers and operators are free to pad and upper-case it, so the reported and the configured value are normalized before they are compared. +Starknet's fingerprint is the chain id felt in lowercase `0x` hex without leading zeros. Both providers and operators are free to pad and upper-case it, so the reported and the configured value are normalized before they are compared. #### Why drop-and-log on local-config mismatch, not hard-crash @@ -691,9 +691,9 @@ The fingerprint is set per chain rather than once per deployment, so a config ca each value must match the network of the `rpc_url` beside it. The value is always a quoted string, including the fingerprints that look numeric. -Only the chains with an identity probe read the field at all — starknet today, the rest as their +Only the chains with a fingerprint probe read the field at all — starknet today, the rest as their probes are written. For those chains, leaving it unset is not a silent skip: every provider of the -chain is reported as `MissingExpectedIdentity`, because silence reads as healthy on a dashboard. A +chain is reported as `MissingExpectedFingerprint`, because silence reads as healthy on a dashboard. A chain with no probe yet reports `ProbeNotImplemented` whether the field is set or not. ## Risks From 7205b97af4eefd2079069a9762fd81a94f0d6978 Mon Sep 17 00:00:00 2001 From: Haiyue Chen Date: Fri, 31 Jul 2026 14:03:26 +0200 Subject: [PATCH 7/9] docs(probe): tighten the comments and name the classify pair symmetrically Also marks the golden transaction route for retirement under #3969. --- crates/foreign-chain-health-check/src/lib.rs | 2 ++ .../foreign-chain-health-check/src/probe.rs | 24 ++++++++----------- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/crates/foreign-chain-health-check/src/lib.rs b/crates/foreign-chain-health-check/src/lib.rs index 6b4bb194eb..526568ae3e 100644 --- a/crates/foreign-chain-health-check/src/lib.rs +++ b/crates/foreign-chain-health-check/src/lib.rs @@ -38,6 +38,8 @@ use crate::golden::{AptosVector, BlockHashVector, SuiVector}; /// Chains with no reference for `network`, or configured but unsupported, are /// [`Status::Skipped`]; a chain absent from the config still yields a single /// placeholder `Skipped` result so its absence stays visible. +/// +/// TODO(#3969): retire this route in favour of [`probe::probe_all_providers`]. pub async fn check_all_providers( fc: &ForeignChainsConfig, network: Network, diff --git a/crates/foreign-chain-health-check/src/probe.rs b/crates/foreign-chain-health-check/src/probe.rs index 5097d8ab62..348b05e5df 100644 --- a/crates/foreign-chain-health-check/src/probe.rs +++ b/crates/foreign-chain-health-check/src/probe.rs @@ -147,21 +147,18 @@ where let fingerprints = FanOut::new(inspectors) .network_fingerprints(timeout, config.max_retries) .await; - for (provider, fingerprint) in fingerprints { + for (provider, reported) in fingerprints { rows.push(ProviderHealth { chain, provider, - status: classify(&expected, fingerprint), + status: classify(&expected, reported), }); } rows } -/// The error itself is dropped for the reason [`ProviderStatus`] documents, so the one cause an -/// operator can act on gets a status of its own instead. -/// -/// A token resolves from the environment or from the config file, and only the former can fail, so -/// a [`std::env::VarError`] in the chain is what names this case. +/// [`ProviderStatus`] carries no error text, so the one actionable cause gets its own variant. Only +/// a token read from the environment can fail to resolve; a [`std::env::VarError`] identifies it. fn setup_failure(error: &anyhow::Error) -> ProviderStatus { if error.chain().any(|cause| cause.is::()) { ProviderStatus::AuthTokenUnresolved @@ -186,9 +183,8 @@ fn rows_of( .collect() } -/// Every chain's fingerprint fits comfortably: the longest is Bitcoin's 66-character genesis hash. -/// What -/// a provider answers instead is its own choice, and a report ends up in logs and metric labels. +/// A provider answers what it likes and the report reaches logs and metric labels, so the length is +/// capped well clear of the longest real fingerprint: Bitcoin's genesis hash, at 66 characters. fn bounded(observed: NetworkFingerprint) -> NetworkFingerprint { const MAX_CHARS: usize = 96; @@ -201,9 +197,9 @@ fn bounded(observed: NetworkFingerprint) -> NetworkFingerprint { fn classify( expected: &NetworkFingerprint, - fingerprint: Result, + reported: Result, ) -> ProviderStatus { - match fingerprint { + match reported { Ok(observed) if &observed == expected => ProviderStatus::Healthy, Ok(observed) => ProviderStatus::WrongNetwork { expected: expected.clone(), @@ -214,8 +210,8 @@ fn classify( Some(ProviderFailure::Rejected) => ProviderStatus::RequestRejected, Some(ProviderFailure::TimedOut) => ProviderStatus::TimedOut, Some(ProviderFailure::Malformed) => ProviderStatus::MalformedResponse, - // The probe inspects no transaction, so a transaction-level error means an impl - // answered outside its contract. + // Probing does not inspect transactions, so a transaction-level error means + // an impl answered outside its contract. None => ProviderStatus::MalformedResponse, }, } From 29221d9d2c25503f684a40babefc38ccefbb76cc Mon Sep 17 00:00:00 2001 From: Haiyue Chen Date: Fri, 31 Jul 2026 14:31:08 +0200 Subject: [PATCH 8/9] test(starknet): collapse the chain id canonicalization cases Six tests with one shape become one rstest, each case keeping the note that justifies it. --- Cargo.lock | 1 + crates/foreign-chain-inspector/src/lib.rs | 23 ++--- .../foreign-chain-rpc-interfaces/Cargo.toml | 1 + .../src/starknet.rs | 90 +++++-------------- crates/node-config/src/foreign_chains.rs | 2 +- 5 files changed, 32 insertions(+), 85 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0ac03da669..8f8e6139a2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3870,6 +3870,7 @@ dependencies = [ "jsonrpsee", "mpc-primitives", "reqwest 0.13.4", + "rstest", "serde", "serde_json", "sui-rpc", diff --git a/crates/foreign-chain-inspector/src/lib.rs b/crates/foreign-chain-inspector/src/lib.rs index 86a2c98490..690eec3e58 100644 --- a/crates/foreign-chain-inspector/src/lib.rs +++ b/crates/foreign-chain-inspector/src/lib.rs @@ -198,15 +198,14 @@ where } } -/// Pause between two tries at the same provider, so that a rate-limiting provider is not hit again -/// immediately. +/// Pause between two tries at the same provider. pub const RETRY_BACKOFF: Duration = Duration::from_millis(200); impl FanOut where Inspector: NetworkFingerprintInspector + Clone + Send + Sync + 'static, { - /// Ask every provider for the network it serves, concurrently, one result each. + /// Ask every provider for the network it serves concurrently, one result each. /// Unlike [`FanOut::extract`], disagreement is not an error: a diagnostic caller needs the /// individual answers. Each provider gets up to `attempts` tries, `timeout` per try plus /// [`RETRY_BACKOFF`] between them, and only a transient failure is retried. @@ -364,14 +363,11 @@ impl ForeignChainInspectionError { ) } - /// Classifies a client error by what the provider did, unlike the [`From`] impl, which keeps it - /// whole as the [`Self::ClientError`] that [`Self::is_transient`] tolerates wholesale. A 401, a - /// JSON-RPC error object and an unparseable body are otherwise indistinguishable, which suits - /// [`FanOut::extract`] but not a caller that reports why a provider is unusable, or decides - /// whether retrying it can help. + /// Splits by what the provider did, where the [`From`] impl collapses everything into the one + /// [`Self::ClientError`] that [`Self::is_transient`] retries wholesale. A caller reporting why a + /// provider is unusable needs a 401 told apart from a 429. /// - /// The messages name the HTTP status or the JSON-RPC code, never the URL: `Path`/`Query` auth - /// splices the operator's API key into it. + /// Messages name the HTTP status or JSON-RPC code, never the URL. pub fn classify_rpc_client_error(error: RpcClientError) -> Self { match error { RpcClientError::Call(object) => { @@ -431,9 +427,8 @@ impl ForeignChainInspectionError { } } -/// Throttling reaches some providers' callers as a JSON-RPC error object over HTTP 200 rather than -/// as a 429, and it is the one refusal worth retrying: `-32005` is Alchemy's and Infura's "limit -/// exceeded", `-32029` the code others use for the same. +/// Some providers report throttling as a JSON-RPC error object over HTTP 200 rather than a 429, and +/// it is the one refusal worth retrying. Alchemy and Infura send `-32005`, others `-32029`. fn is_rate_limit_error_code(code: i32) -> bool { const LIMIT_EXCEEDED: i32 = -32005; const TOO_MANY_REQUESTS: i32 = -32029; @@ -441,8 +436,6 @@ fn is_rate_limit_error_code(code: i32) -> bool { matches!(code, LIMIT_EXCEEDED | TOO_MANY_REQUESTS) } -/// Request timeout, too many requests, and anything the server blames on itself. Every other -/// status is the provider's verdict on the request, which the same request cannot change. fn is_retryable_status(status_code: u16) -> bool { const REQUEST_TIMEOUT: u16 = 408; const TOO_MANY_REQUESTS: u16 = 429; diff --git a/crates/foreign-chain-rpc-interfaces/Cargo.toml b/crates/foreign-chain-rpc-interfaces/Cargo.toml index 6621be4d58..888adcc21e 100644 --- a/crates/foreign-chain-rpc-interfaces/Cargo.toml +++ b/crates/foreign-chain-rpc-interfaces/Cargo.toml @@ -18,6 +18,7 @@ thiserror = { workspace = true } tonic = { workspace = true } [dev-dependencies] +rstest = { workspace = true } tokio = { workspace = true } [lints] diff --git a/crates/foreign-chain-rpc-interfaces/src/starknet.rs b/crates/foreign-chain-rpc-interfaces/src/starknet.rs index 7974f33dc8..799a08456c 100644 --- a/crates/foreign-chain-rpc-interfaces/src/starknet.rs +++ b/crates/foreign-chain-rpc-interfaces/src/starknet.rs @@ -169,6 +169,7 @@ mod tests { BlockId, ChainIdResponse, GetBlockWithTxHashesArgs, GetBlockWithTxHashesResponse, GetTransactionReceiptResponse, StarknetExecutionStatus, StarknetFinalityStatus, parse_felt, }; + use rstest::rstest; const TEST_BLOCK_NUMBER: u64 = 842_750; const TEST_RECEIPT_BLOCK_NUMBER: u64 = 6_195_041; @@ -176,81 +177,32 @@ mod tests { /// Starknet mainnet's chain id, `SN_MAIN` in ASCII. const MAINNET_CHAIN_ID: &str = "0x534e5f4d41494e"; - #[test] - fn chain_id_response__should_keep_a_canonical_chain_id_unchanged() { - // Given - let json = serde_json::json!(MAINNET_CHAIN_ID); - - // When - let response: ChainIdResponse = serde_json::from_value(json).unwrap(); - - // Then - assert_eq!(response.canonical_text(), MAINNET_CHAIN_ID); - } - - #[test] - fn chain_id_response__should_normalize_a_padded_uppercase_chain_id() { - // Given: the chain id padded and upper-cased, as a provider may send it. - let json = serde_json::json!("0x00534E5F4D41494E"); - - // When - let response: ChainIdResponse = serde_json::from_value(json).unwrap(); - - // Then - assert_eq!(response.canonical_text(), MAINNET_CHAIN_ID); - } - - #[test] - fn chain_id_response__should_normalize_an_upper_cased_prefix() { - // Given: a spelling only an operator can write, since the spec's pattern binds providers - let json = serde_json::json!("0X534E5F4D41494E"); - - // When - let response: ChainIdResponse = serde_json::from_value(json).unwrap(); - - // Then - assert_eq!(response.canonical_text(), MAINNET_CHAIN_ID); - } - - #[test] - fn chain_id_response__should_normalize_a_zero_chain_id() { + /// 66 hex digits. `CHAIN_ID` carries no length bound, though a `FELT` caps at 63. + const LONGER_THAN_A_FELT: &str = + "0x1234567890123456789012345678901234567890123456789012345678901234ab"; + + #[rstest] + #[case::canonical(MAINNET_CHAIN_ID, MAINNET_CHAIN_ID)] + // Padded and upper-cased, as a provider may send it. + #[case::padded_and_upper_cased("0x00534E5F4D41494E", MAINNET_CHAIN_ID)] + // A spelling only an operator can write, since the spec's pattern binds providers. + #[case::upper_cased_prefix("0X534E5F4D41494E", MAINNET_CHAIN_ID)] + #[case::zero("0x0000", "0x0")] + #[case::longer_than_a_felt(LONGER_THAN_A_FELT, LONGER_THAN_A_FELT)] + // The decoded name rather than the hex: reported as answered by the provider. + #[case::not_hex("NOT_CHAIN_ID", "NOT_CHAIN_ID")] + fn chain_id_response__should_canonicalize_what_a_provider_answers( + #[case] answered: &str, + #[case] expected: &str, + ) { // Given - let json = serde_json::json!("0x0000"); - - // When - let response: ChainIdResponse = serde_json::from_value(json).unwrap(); - - // Then - assert_eq!(response.canonical_text(), "0x0"); - } - - #[test] - fn chain_id_response__should_accept_a_chain_id_longer_than_a_felt() { - // Given: 66 hex digits. `CHAIN_ID` carries no length bound, though a `FELT` caps at 63. - let json = serde_json::json!( - "0x1234567890123456789012345678901234567890123456789012345678901234ab" - ); + let json = serde_json::json!(answered); // When let response: ChainIdResponse = serde_json::from_value(json).unwrap(); // Then - assert_eq!( - response.canonical_text(), - "0x1234567890123456789012345678901234567890123456789012345678901234ab" - ); - } - - #[test] - fn chain_id_response__should_leave_a_non_hex_chain_id_unchanged() { - // Given: the decoded name rather than the hex. - let json = serde_json::json!("NOT_CHAIN_ID"); - - // When - let response: ChainIdResponse = serde_json::from_value(json).unwrap(); - - // Then: reported as answered by the provider - assert_eq!(response.canonical_text(), "NOT_CHAIN_ID"); + assert_eq!(response.canonical_text(), expected); } #[test] diff --git a/crates/node-config/src/foreign_chains.rs b/crates/node-config/src/foreign_chains.rs index fef3ce2d00..ca11f72246 100644 --- a/crates/node-config/src/foreign_chains.rs +++ b/crates/node-config/src/foreign_chains.rs @@ -46,7 +46,7 @@ pub struct ForeignChainsConfig { pub struct ForeignChainConfig { pub timeout_sec: NonZeroU64, /// Total attempts per provider, not additional ones: `1` means a single try. Read by the - /// network fingerprint probe; transaction verification does not retry a provider at all. + /// network fingerprint probe; transaction verification does not retry a provider at all currently. pub max_retries: NonZeroU64, /// The network fingerprint the operator expects every provider of this chain to report, in the /// chain's canonical text form. A chain id for chains that have one, a genesis hash or digest From 693b5cfd43f49f99663a5083ee4befad733b030c Mon Sep 17 00:00:00 2001 From: Haiyue Chen Date: Tue, 4 Aug 2026 09:10:42 +0200 Subject: [PATCH 9/9] refactor(probe): tighten comments and move the classification tests into the crate Trims the probe status docs, restates the fingerprint trait docs as the implementor contract, and turns every Given/When/Then marker bare, keeping the setup information in helper and constant names. Moves the RPC error classification tests from an integration test file into a unit test module, splits the oversized response message from the malformed one, and points the auth material guard test at a mock so it asserts on a status that carries provider text. --- crates/foreign-chain-health-check/src/lib.rs | 3 +- .../foreign-chain-health-check/src/probe.rs | 177 +++++++------- crates/foreign-chain-inspector/src/lib.rs | 229 +++++++++++++++++- .../src/starknet/inspector.rs | 6 +- .../tests/rpc_error_classification.rs | 207 ---------------- .../tests/starknet_inspector.rs | 31 ++- docs/foreign-chain-transactions.md | 10 +- 7 files changed, 340 insertions(+), 323 deletions(-) delete mode 100644 crates/foreign-chain-inspector/tests/rpc_error_classification.rs diff --git a/crates/foreign-chain-health-check/src/lib.rs b/crates/foreign-chain-health-check/src/lib.rs index 526568ae3e..7bd0413319 100644 --- a/crates/foreign-chain-health-check/src/lib.rs +++ b/crates/foreign-chain-health-check/src/lib.rs @@ -4,10 +4,11 @@ //! * [`probe::probe_all_providers`] asks each provider for the network it serves and compares that //! against the operator's configured expectation. +pub mod probe; + mod checks; mod golden; mod network; -pub mod probe; mod results; use std::future::Future; diff --git a/crates/foreign-chain-health-check/src/probe.rs b/crates/foreign-chain-health-check/src/probe.rs index 348b05e5df..3a98b19761 100644 --- a/crates/foreign-chain-health-check/src/probe.rs +++ b/crates/foreign-chain-health-check/src/probe.rs @@ -15,10 +15,6 @@ use near_mpc_contract_interface::types::{ForeignChain, ProviderId}; use crate::prepare_jsonrpc; /// One provider's verdict. Anything other than [`ProviderStatus::Healthy`] is unhealthy. -/// -/// Carries no rendered RPC error: `Path`/`Query` auth splices the operator's API key into the URL, -/// and upstream errors interpolate that URL into their text. Dropping the text here keeps the key -/// out of anything built from a report. #[derive(Debug, Clone, PartialEq, Eq)] pub enum ProviderStatus { Healthy, @@ -32,15 +28,11 @@ pub enum ProviderStatus { RequestRejected, MalformedResponse, TimedOut, - /// The provider's auth token did not resolve, e.g. an environment variable that is not set. AuthTokenUnresolved, - /// The RPC client could not be built from the provider's URL and auth. ClientSetupFailed, /// The chain is configured without an `expected_network_fingerprint`, so its providers cannot - /// be checked. Reported rather than skipped: silence would read as healthy. + /// be checked. MissingExpectedFingerprint, - /// The chain has no fingerprint probe yet, either because none is written for it or because the - /// node cannot inspect it at all. ProbeNotImplemented, } @@ -221,6 +213,7 @@ fn classify( #[expect(non_snake_case)] mod tests { use super::*; + use assert_matches::assert_matches; use mpc_node_config::{AuthConfig, TokenConfig}; use near_mpc_bounded_collections::NonEmptyBTreeMap; use std::num::NonZeroU64; @@ -228,6 +221,7 @@ mod tests { /// Starknet mainnet's chain id, `SN_MAIN` in ASCII. const MAINNET: &str = "0x534e5f4d41494e"; const SEPOLIA: &str = "0x534e5f5345504f4c4941"; + const PADDED_UPPERCASE_MAINNET: &str = "0x00534E5F4D41494E"; /// Reserved as "discard", so nothing listens there. const CLOSED_PORT_URL: &str = "http://127.0.0.1:9"; @@ -290,6 +284,59 @@ mod tests { .await } + async fn mock_error_object<'a>( + server: &'a httpmock::MockServer, + status: u16, + code: i32, + message: &str, + ) -> httpmock::Mock<'a> { + let body = serde_json::json!({ + "jsonrpc": "2.0", + "id": 0, + "error": {"code": code, "message": message}, + }); + server + .mock_async(|when, then| { + when.method(httpmock::Method::POST); + then.status(status).json_body(body); + }) + .await + } + + async fn mock_bad_api_key(server: &httpmock::MockServer) -> httpmock::Mock<'_> { + mock_error_object(server, 401, -32600, "Must be authenticated!").await + } + + async fn mock_unsupported_method(server: &httpmock::MockServer) -> httpmock::Mock<'_> { + mock_error_object(server, 200, -32601, "Method not found").await + } + + /// Throttling over HTTP 200, so only the JSON-RPC code tells the caller to back off. + async fn mock_throttled_over_http_200(server: &httpmock::MockServer) -> httpmock::Mock<'_> { + mock_error_object(server, 200, -32005, "limit exceeded").await + } + + async fn mock_non_jsonrpc_body(server: &httpmock::MockServer) -> httpmock::Mock<'_> { + server + .mock_async(|when, then| { + when.method(httpmock::Method::POST); + then.status(200).body("gateway"); + }) + .await + } + + async fn mock_never_answers_in_time(server: &httpmock::MockServer) -> httpmock::Mock<'_> { + let body = serde_json::json!({"jsonrpc": "2.0", "result": MAINNET, "id": 0}); + server + .mock_async(|when, then| { + when.method(httpmock::Method::POST); + then.status(200) + .json_body(body) + .delay(Duration::from_secs(30)); + }) + .await + } + /// Keyed by chain too: provider names repeat across chains in real configs. fn must_status_of(report: &ProbeReport, chain: ForeignChain, provider: &str) -> ProviderStatus { report @@ -324,7 +371,7 @@ mod tests { #[tokio::test] async fn probe_all_providers__should_report_a_provider_on_another_network_as_wrong_network() { - // Given a provider serving Sepolia while the operator configured mainnet + // Given let server = httpmock::MockServer::start_async().await; mock_chain_id(&server, SEPOLIA).await; let config = starknet_only(chain_config( @@ -347,9 +394,9 @@ mod tests { #[tokio::test] async fn probe_all_providers__should_normalize_the_reported_fingerprint_before_comparing() { - // Given a provider padding and uppercasing the chain id, as the spec permits + // Given let server = httpmock::MockServer::start_async().await; - mock_chain_id(&server, "0x00534E5F4D41494E").await; + mock_chain_id(&server, PADDED_UPPERCASE_MAINNET).await; let config = starknet_only(chain_config( Some(MAINNET), one_provider("publicnode", &server.base_url()), @@ -379,7 +426,7 @@ mod tests { // When let report = probe_all_providers(&config).await; - // Then the provider is reported rather than skipped, and no request was sent + // Then assert_eq!( must_status_of(&report, ForeignChain::Starknet, "publicnode"), ProviderStatus::MissingExpectedFingerprint @@ -407,18 +454,9 @@ mod tests { #[tokio::test] async fn probe_all_providers__should_report_a_provider_refusing_the_request_without_retrying() { - // Given a provider answering the way an authenticated provider answers a bad API key + // Given let server = httpmock::MockServer::start_async().await; - let mock = server - .mock_async(|when, then| { - when.method(httpmock::Method::POST); - then.status(401).json_body(serde_json::json!({ - "jsonrpc": "2.0", - "id": 0, - "error": {"code": -32600, "message": "Must be authenticated!"}, - })); - }) - .await; + let mock = mock_bad_api_key(&server).await; let config = starknet_only(with_retries( chain_config(Some(MAINNET), one_provider("keyed", &server.base_url())), 3, @@ -427,7 +465,7 @@ mod tests { // When let report = probe_all_providers(&config).await; - // Then the refusal is named as such, and retrying it is pointless + // Then assert_eq!( must_status_of(&report, ForeignChain::Starknet, "keyed"), ProviderStatus::RequestRejected @@ -437,18 +475,9 @@ mod tests { #[tokio::test] async fn probe_all_providers__should_report_a_provider_answering_with_a_jsonrpc_error() { - // Given a provider that does not serve this chain's methods + // Given let server = httpmock::MockServer::start_async().await; - server - .mock_async(|when, then| { - when.method(httpmock::Method::POST); - then.status(200).json_body(serde_json::json!({ - "jsonrpc": "2.0", - "id": 0, - "error": {"code": -32601, "message": "Method not found"}, - })); - }) - .await; + mock_unsupported_method(&server).await; let config = starknet_only(chain_config( Some(MAINNET), one_provider("publicnode", &server.base_url()), @@ -466,14 +495,9 @@ mod tests { #[tokio::test] async fn probe_all_providers__should_report_a_provider_answering_with_an_unusable_body() { - // Given a provider whose answer is not JSON-RPC at all + // Given let server = httpmock::MockServer::start_async().await; - server - .mock_async(|when, then| { - when.method(httpmock::Method::POST); - then.status(200).body("gateway"); - }) - .await; + mock_non_jsonrpc_body(&server).await; let config = starknet_only(chain_config( Some(MAINNET), one_provider("publicnode", &server.base_url()), @@ -491,17 +515,9 @@ mod tests { #[tokio::test] async fn probe_all_providers__should_report_a_provider_that_does_not_answer_in_time() { - // Given a provider slower than the configured timeout + // Given let server = httpmock::MockServer::start_async().await; - let body = serde_json::json!({"jsonrpc": "2.0", "result": MAINNET, "id": 0}); - server - .mock_async(|when, then| { - when.method(httpmock::Method::POST); - then.status(200) - .json_body(body) - .delay(Duration::from_secs(30)); - }) - .await; + mock_never_answers_in_time(&server).await; let config = starknet_only(chain_config( Some(MAINNET), one_provider("slow", &server.base_url()), @@ -519,11 +535,11 @@ mod tests { #[tokio::test] async fn probe_all_providers__should_normalize_the_configured_fingerprint_before_comparing() { - // Given an operator writing the chain id padded and uppercased, as the spec permits + // Given let server = httpmock::MockServer::start_async().await; mock_chain_id(&server, MAINNET).await; let config = starknet_only(chain_config( - Some("0x00534E5F4D41494E"), + Some(PADDED_UPPERCASE_MAINNET), one_provider("publicnode", &server.base_url()), )); @@ -539,7 +555,7 @@ mod tests { #[tokio::test] async fn probe_all_providers__should_report_a_provider_whose_auth_token_does_not_resolve() { - // Given a provider whose auth token comes from an environment variable that is not set + // Given let config = starknet_only(chain_config( Some(MAINNET), NonEmptyBTreeMap::new( @@ -560,7 +576,7 @@ mod tests { // When let report = probe_all_providers(&config).await; - // Then the operator learns which of the two setup failures it was + // Then assert_eq!( must_status_of(&report, ForeignChain::Starknet, "keyed"), ProviderStatus::AuthTokenUnresolved @@ -569,7 +585,7 @@ mod tests { #[tokio::test] async fn probe_all_providers__should_report_a_provider_whose_client_cannot_be_built() { - // Given a provider whose URL is not one a client can be built for + // Given let config = starknet_only(chain_config( Some(MAINNET), one_provider("wrong-scheme", "ws://127.0.0.1:9"), @@ -587,7 +603,7 @@ mod tests { #[tokio::test] async fn probe_all_providers__should_report_each_provider_of_a_chain_separately() { - // Given one healthy provider and one that is unreachable + // Given let server = httpmock::MockServer::start_async().await; mock_chain_id(&server, MAINNET).await; let mut providers = one_provider("healthy", &server.base_url()); @@ -618,7 +634,7 @@ mod tests { #[tokio::test] async fn probe_all_providers__should_report_a_chain_with_no_fingerprint_probe_as_not_implemented() { - // Given a chain the node can inspect but has no fingerprint probe for + // Given let config = ForeignChainsConfig { base: Some(chain_config( Some("8453"), @@ -630,7 +646,7 @@ mod tests { // When let report = probe_all_providers(&config).await; - // Then it is visible in the report rather than silently absent + // Then assert_eq!( must_status_of(&report, ForeignChain::Base, "publicnode"), ProviderStatus::ProbeNotImplemented @@ -639,7 +655,7 @@ mod tests { #[tokio::test] async fn probe_all_providers__should_report_every_configured_chain_under_its_own_chain() { - // Given the same provider name configured for two chains + // Given let server = httpmock::MockServer::start_async().await; mock_chain_id(&server, MAINNET).await; let config = ForeignChainsConfig { @@ -657,7 +673,7 @@ mod tests { // When let report = probe_all_providers(&config).await; - // Then each chain gets its own row rather than one shadowing the other + // Then assert_eq!( must_status_of(&report, ForeignChain::Starknet, "publicnode"), ProviderStatus::Healthy @@ -671,18 +687,9 @@ mod tests { #[tokio::test] async fn probe_all_providers__should_retry_a_provider_that_refused_with_a_rate_limit_code() { - // Given a provider signalling throttling as a JSON-RPC error object over HTTP 200 + // Given let server = httpmock::MockServer::start_async().await; - let mock = server - .mock_async(|when, then| { - when.method(httpmock::Method::POST); - then.status(200).json_body(serde_json::json!({ - "jsonrpc": "2.0", - "id": 0, - "error": {"code": -32005, "message": "limit exceeded"}, - })); - }) - .await; + let mock = mock_throttled_over_http_200(&server).await; let config = starknet_only(with_retries( chain_config(Some(MAINNET), one_provider("keyed", &server.base_url())), 2, @@ -691,7 +698,7 @@ mod tests { // When let report = probe_all_providers(&config).await; - // Then throttling is the one refusal worth retrying + // Then assert_eq!( must_status_of(&report, ForeignChain::Starknet, "keyed"), ProviderStatus::Unreachable @@ -701,7 +708,7 @@ mod tests { #[tokio::test] async fn probe_all_providers__should_bound_the_fingerprint_a_provider_reports() { - // Given a provider answering with far more than a fingerprint + // Given let server = httpmock::MockServer::start_async().await; let flood = "n".repeat(5_000); mock_chain_id(&server, &flood).await; @@ -713,7 +720,7 @@ mod tests { // When let report = probe_all_providers(&config).await; - // Then what reaches a log line or a metric label is bounded + // Then let ProviderStatus::WrongNetwork { observed, .. } = must_status_of(&report, ForeignChain::Starknet, "publicnode") else { @@ -724,7 +731,7 @@ mod tests { #[test] fn classify__should_report_a_transaction_level_error_as_malformed() { - // Given an error about a transaction, which the probe never asks about + // Given let expected = NetworkFingerprint::from(MAINNET.to_string()); // When @@ -733,7 +740,7 @@ mod tests { Err(ForeignChainInspectionError::TransactionNotFound), ); - // Then the inspector answered outside the contract its probe has + // Then assert_eq!(status, ProviderStatus::MalformedResponse); } @@ -752,13 +759,15 @@ mod tests { #[tokio::test] async fn probe_all_providers__should_keep_auth_material_out_of_the_report() { - // Given a provider whose API key is spliced into the URL path + // Given + let server = httpmock::MockServer::start_async().await; + mock_chain_id(&server, SEPOLIA).await; let config = starknet_only(chain_config( Some(MAINNET), NonEmptyBTreeMap::new( "keyed".to_string().into(), ForeignChainProviderConfig { - rpc_url: format!("{CLOSED_PORT_URL}/v2/API_KEY"), + rpc_url: format!("{}/v2/API_KEY", server.base_url()), auth: AuthConfig::Path { placeholder: "API_KEY".to_string(), token: TokenConfig::Val { @@ -772,7 +781,11 @@ mod tests { // When let report = probe_all_providers(&config).await; - // Then neither the token nor the URL it was spliced into reaches the report + // Then + assert_matches!( + must_status_of(&report, ForeignChain::Starknet, "keyed"), + ProviderStatus::WrongNetwork { .. } + ); let rendered = format!("{report:?}"); assert!(!rendered.contains("super-secret"), "{rendered}"); assert!(!rendered.contains("127.0.0.1"), "{rendered}"); diff --git a/crates/foreign-chain-inspector/src/lib.rs b/crates/foreign-chain-inspector/src/lib.rs index 690eec3e58..03fe25d9d6 100644 --- a/crates/foreign-chain-inspector/src/lib.rs +++ b/crates/foreign-chain-inspector/src/lib.rs @@ -46,18 +46,16 @@ pub trait ForeignChainInspector { #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Display, From)] pub struct NetworkFingerprint(String); -/// Reports the [`NetworkFingerprint`] of the provider an inspector talks to. -/// -/// Fingerprints are compared verbatim, so both the reported and the expected one go through the -/// impl's canonical form. Fetches a chain-wide constant providers never prune. +/// Reports the [`NetworkFingerprint`] of the provider an inspector talks to, in the form +/// [`Self::canonical_fingerprint`] produces. pub trait NetworkFingerprintInspector { fn network_fingerprint( &self, ) -> impl Future> + Send; - /// Puts an operator-supplied fingerprint into the form [`Self::network_fingerprint`] returns, - /// so that a spec-legal spelling of the right network does not read as the wrong network. - fn canonical_fingerprint(expected: &str) -> NetworkFingerprint; + /// Normalizes any spec-legal spelling of this chain's fingerprint into the single form the trait + /// compares. Idempotent. + fn canonical_fingerprint(fingerprint: &str) -> NetworkFingerprint; } /// Combines multiple inspectors that target the same chain into a single inspector. @@ -363,11 +361,7 @@ impl ForeignChainInspectionError { ) } - /// Splits by what the provider did, where the [`From`] impl collapses everything into the one - /// [`Self::ClientError`] that [`Self::is_transient`] retries wholesale. A caller reporting why a - /// provider is unusable needs a 401 told apart from a 429. - /// - /// Messages name the HTTP status or JSON-RPC code, never the URL. + /// Maps a raw RPC client error to an error with context. pub fn classify_rpc_client_error(error: RpcClientError) -> Self { match error { RpcClientError::Call(object) => { @@ -392,9 +386,12 @@ impl ForeignChainInspectionError { } } // Not a response the caller can use, as opposed to no response at all. - Some(HttpTransportError::Http(HttpError::Malformed | HttpError::TooLarge)) => { + Some(HttpTransportError::Http(HttpError::Malformed)) => { Self::MalformedRpcResponse("response was not valid JSON-RPC".to_string()) } + Some(HttpTransportError::Http(HttpError::TooLarge)) => { + Self::MalformedRpcResponse("response exceeded the size limit".to_string()) + } Some(HttpTransportError::Url(_)) => { Self::RpcRequestRejected("invalid RPC URL".to_string()) } @@ -482,3 +479,209 @@ pub fn build_http_client( Ok(client) } + +#[cfg(test)] +#[expect(non_snake_case)] +mod tests { + use super::*; + use assert_matches::assert_matches; + use rstest::rstest; + + fn transport(error: HttpTransportError) -> RpcClientError { + RpcClientError::Transport(Box::new(error)) + } + + #[rstest] + #[case(400)] + #[case(401)] + #[case(403)] + #[case(404)] + fn classify_rpc_client_error__should_report_a_deterministic_status_as_a_refusal( + #[case] status_code: u16, + ) { + // Given + let error = transport(HttpTransportError::Rejected { status_code }); + + // When + let classified = ForeignChainInspectionError::classify_rpc_client_error(error); + + // Then + assert_matches!( + &classified, + ForeignChainInspectionError::RpcRequestRejected(_) + ); + assert!(!classified.is_transient()); + } + + #[rstest] + #[case(408)] + #[case(429)] + #[case(500)] + #[case(503)] + fn classify_rpc_client_error__should_report_a_retryable_status_as_a_transient_failure( + #[case] status_code: u16, + ) { + // Given + let error = transport(HttpTransportError::Rejected { status_code }); + + // When + let classified = ForeignChainInspectionError::classify_rpc_client_error(error); + + // Then + assert_matches!( + &classified, + ForeignChainInspectionError::RpcRequestFailed(_) + ); + assert!(classified.is_transient()); + } + + #[test] + fn classify_rpc_client_error__should_report_a_jsonrpc_error_object_as_a_refusal() { + // Given + let error = RpcClientError::Call(jsonrpsee::types::ErrorObject::owned( + -32600, + "Must be authenticated!", + None::<()>, + )); + + // When + let classified = ForeignChainInspectionError::classify_rpc_client_error(error); + + // Then + assert_matches!( + &classified, + ForeignChainInspectionError::RpcRequestRejected(_) + ); + assert!(!classified.is_transient()); + } + + #[test] + fn classify_rpc_client_error__should_report_an_unparseable_result_as_malformed() { + // Given + let parse_error = + serde_json::from_str::("7").expect_err("a number is not a string"); + let error = RpcClientError::ParseError(parse_error); + + // When + let classified = ForeignChainInspectionError::classify_rpc_client_error(error); + + // Then + assert_matches!( + classified, + ForeignChainInspectionError::MalformedRpcResponse(_) + ); + } + + #[test] + fn classify_rpc_client_error__should_report_a_body_that_is_not_jsonrpc_as_malformed() { + // Given + let error = transport(HttpTransportError::Http(HttpError::Malformed)); + + // When + let classified = ForeignChainInspectionError::classify_rpc_client_error(error); + + // Then + assert_matches!( + classified, + ForeignChainInspectionError::MalformedRpcResponse(_) + ); + } + + #[test] + fn classify_rpc_client_error__should_report_a_connection_failure_as_transient() { + // Given + let error = transport(HttpTransportError::Http(HttpError::Stream(Box::new( + std::io::Error::new(std::io::ErrorKind::ConnectionRefused, "connection refused"), + )))); + + // When + let classified = ForeignChainInspectionError::classify_rpc_client_error(error); + + // Then + assert_matches!( + &classified, + ForeignChainInspectionError::RpcRequestFailed(_) + ); + assert!(classified.is_transient()); + } + + /// `Path` and `Query` auth splice the API key into the URL, and jsonrpsee puts that URL in the + /// text of the error it reports for it. + #[test] + fn classify_rpc_client_error__should_keep_the_rpc_url_out_of_the_message() { + // Given + let url_carrying_a_key = "http://provider.example/v2/super-secret".to_string(); + let error = transport(HttpTransportError::Url(url_carrying_a_key)); + + // When + let classified = ForeignChainInspectionError::classify_rpc_client_error(error); + + // Then + let rendered = classified.to_string(); + assert!(!rendered.contains("super-secret"), "{rendered}"); + } + + #[test] + fn classify_rpc_client_error__should_report_a_client_side_timeout_as_a_timeout() { + // Given + let error = RpcClientError::RequestTimeout; + + // When + let classified = ForeignChainInspectionError::classify_rpc_client_error(error); + + // Then + assert_matches!(classified, ForeignChainInspectionError::Timeout); + } + + #[rstest] + #[case(-32005)] + #[case(-32029)] + fn classify_rpc_client_error__should_report_a_rate_limit_code_as_a_transient_failure( + #[case] code: i32, + ) { + // Given + let error = RpcClientError::Call(jsonrpsee::types::ErrorObject::owned( + code, + "limit exceeded", + None::<()>, + )); + + // When + let classified = ForeignChainInspectionError::classify_rpc_client_error(error); + + // Then + assert_matches!( + &classified, + ForeignChainInspectionError::RpcRequestFailed(_) + ); + assert!(classified.is_transient()); + } + + #[rstest] + #[case(ForeignChainInspectionError::RpcRequestFailed("_".to_string()), Some(ProviderFailure::Unreachable))] + #[case(ForeignChainInspectionError::RpcRequestRejected("_".to_string()), Some(ProviderFailure::Rejected))] + #[case(ForeignChainInspectionError::Timeout, Some(ProviderFailure::TimedOut))] + #[case(ForeignChainInspectionError::MalformedRpcResponse("_".to_string()), Some(ProviderFailure::Malformed))] + #[case( + ForeignChainInspectionError::InspectorResponseMismatch, + Some(ProviderFailure::Malformed) + )] + // The transaction's own state is an answer, not a fault of the provider that reported it. + #[case(ForeignChainInspectionError::TransactionNotFound, None)] + #[case(ForeignChainInspectionError::TransactionFailed, None)] + #[case(ForeignChainInspectionError::NotFinalized, None)] + #[case(ForeignChainInspectionError::NotEnoughBlockConfirmations { + expected: BlockConfirmations::from(6), + got: BlockConfirmations::from(1), + }, None)] + fn provider_failure__should_name_only_the_failures_the_provider_owns( + #[case] error: ForeignChainInspectionError, + #[case] expected: Option, + ) { + // When + let failure = error.provider_failure(); + + // Then + assert_eq!(failure, expected); + } +} diff --git a/crates/foreign-chain-inspector/src/starknet/inspector.rs b/crates/foreign-chain-inspector/src/starknet/inspector.rs index 58c7bf3e83..68aa496fe9 100644 --- a/crates/foreign-chain-inspector/src/starknet/inspector.rs +++ b/crates/foreign-chain-inspector/src/starknet/inspector.rs @@ -41,8 +41,10 @@ where Ok(chain_id.canonical_text().into()) } - fn canonical_fingerprint(expected: &str) -> NetworkFingerprint { - ChainIdResponse(expected.to_owned()).canonical_text().into() + fn canonical_fingerprint(fingerprint: &str) -> NetworkFingerprint { + ChainIdResponse(fingerprint.to_owned()) + .canonical_text() + .into() } } diff --git a/crates/foreign-chain-inspector/tests/rpc_error_classification.rs b/crates/foreign-chain-inspector/tests/rpc_error_classification.rs deleted file mode 100644 index c9b348a707..0000000000 --- a/crates/foreign-chain-inspector/tests/rpc_error_classification.rs +++ /dev/null @@ -1,207 +0,0 @@ -#![allow(non_snake_case)] - -//! Integration tests for [`ForeignChainInspectionError::classify_rpc_client_error`], which decides -//! whether a provider failed to answer, answered and refused, or answered unusably. - -use assert_matches::assert_matches; -use foreign_chain_inspector::{BlockConfirmations, ForeignChainInspectionError, ProviderFailure}; -use jsonrpsee::core::client::error::Error as RpcClientError; -use jsonrpsee::core::http_helpers::HttpError; -use jsonrpsee::http_client::transport::Error as TransportError; -use rstest::rstest; - -fn transport(error: TransportError) -> RpcClientError { - RpcClientError::Transport(Box::new(error)) -} - -#[rstest] -#[case(400)] -#[case(401)] -#[case(403)] -#[case(404)] -fn classify_rpc_client_error__should_report_a_deterministic_status_as_a_refusal( - #[case] status_code: u16, -) { - // Given - let error = transport(TransportError::Rejected { status_code }); - - // When - let classified = ForeignChainInspectionError::classify_rpc_client_error(error); - - // Then - assert_matches!( - &classified, - ForeignChainInspectionError::RpcRequestRejected(_) - ); - assert!(!classified.is_transient()); -} - -#[rstest] -#[case(408)] -#[case(429)] -#[case(500)] -#[case(503)] -fn classify_rpc_client_error__should_report_a_retryable_status_as_a_transient_failure( - #[case] status_code: u16, -) { - // Given - let error = transport(TransportError::Rejected { status_code }); - - // When - let classified = ForeignChainInspectionError::classify_rpc_client_error(error); - - // Then - assert_matches!( - &classified, - ForeignChainInspectionError::RpcRequestFailed(_) - ); - assert!(classified.is_transient()); -} - -#[test] -fn classify_rpc_client_error__should_report_a_jsonrpc_error_object_as_a_refusal() { - // Given the answer an authenticated provider gives to a request it will not serve - let error = RpcClientError::Call(jsonrpsee::types::ErrorObject::owned( - -32600, - "Must be authenticated!", - None::<()>, - )); - - // When - let classified = ForeignChainInspectionError::classify_rpc_client_error(error); - - // Then - assert_matches!( - &classified, - ForeignChainInspectionError::RpcRequestRejected(_) - ); - assert!(!classified.is_transient()); -} - -#[test] -fn classify_rpc_client_error__should_report_an_unparseable_result_as_malformed() { - // Given - let parse_error = serde_json::from_str::("7").expect_err("a number is not a string"); - let error = RpcClientError::ParseError(parse_error); - - // When - let classified = ForeignChainInspectionError::classify_rpc_client_error(error); - - // Then - assert_matches!( - classified, - ForeignChainInspectionError::MalformedRpcResponse(_) - ); -} - -#[test] -fn classify_rpc_client_error__should_report_a_body_that_is_not_jsonrpc_as_malformed() { - // Given - let error = transport(TransportError::Http(HttpError::Malformed)); - - // When - let classified = ForeignChainInspectionError::classify_rpc_client_error(error); - - // Then - assert_matches!( - classified, - ForeignChainInspectionError::MalformedRpcResponse(_) - ); -} - -#[test] -fn classify_rpc_client_error__should_report_a_connection_failure_as_transient() { - // Given - let error = transport(TransportError::Http(HttpError::Stream(Box::new( - std::io::Error::new(std::io::ErrorKind::ConnectionRefused, "connection refused"), - )))); - - // When - let classified = ForeignChainInspectionError::classify_rpc_client_error(error); - - // Then - assert_matches!( - &classified, - ForeignChainInspectionError::RpcRequestFailed(_) - ); - assert!(classified.is_transient()); -} - -#[test] -fn classify_rpc_client_error__should_keep_the_rpc_url_out_of_the_message() { - // Given a URL a client cannot be built for, as jsonrpsee reports it: with the URL in the text - let error = transport(TransportError::Url( - "http://provider.example/v2/super-secret".to_string(), - )); - - // When - let classified = ForeignChainInspectionError::classify_rpc_client_error(error); - - // Then the API key spliced into the URL by `Path`/`Query` auth cannot travel with the error - let rendered = classified.to_string(); - assert!(!rendered.contains("super-secret"), "{rendered}"); -} - -#[test] -fn classify_rpc_client_error__should_report_a_client_side_timeout_as_a_timeout() { - // Given - let error = RpcClientError::RequestTimeout; - - // When - let classified = ForeignChainInspectionError::classify_rpc_client_error(error); - - // Then - assert_matches!(classified, ForeignChainInspectionError::Timeout); -} - -#[rstest] -#[case(-32005)] -#[case(-32029)] -fn classify_rpc_client_error__should_report_a_rate_limit_code_as_a_transient_failure( - #[case] code: i32, -) { - // Given a provider signalling throttling in the error object rather than with a 429 - let error = RpcClientError::Call(jsonrpsee::types::ErrorObject::owned( - code, - "limit exceeded", - None::<()>, - )); - - // When - let classified = ForeignChainInspectionError::classify_rpc_client_error(error); - - // Then - assert_matches!( - &classified, - ForeignChainInspectionError::RpcRequestFailed(_) - ); - assert!(classified.is_transient()); -} - -#[rstest] -#[case(ForeignChainInspectionError::RpcRequestFailed("_".to_string()), Some(ProviderFailure::Unreachable))] -#[case(ForeignChainInspectionError::RpcRequestRejected("_".to_string()), Some(ProviderFailure::Rejected))] -#[case(ForeignChainInspectionError::Timeout, Some(ProviderFailure::TimedOut))] -#[case(ForeignChainInspectionError::MalformedRpcResponse("_".to_string()), Some(ProviderFailure::Malformed))] -#[case( - ForeignChainInspectionError::InspectorResponseMismatch, - Some(ProviderFailure::Malformed) -)] -// The transaction's own state is an answer, not a fault of the provider that reported it. -#[case(ForeignChainInspectionError::TransactionNotFound, None)] -#[case(ForeignChainInspectionError::TransactionFailed, None)] -#[case(ForeignChainInspectionError::NotFinalized, None)] -#[case(ForeignChainInspectionError::NotEnoughBlockConfirmations { - expected: BlockConfirmations::from(6), - got: BlockConfirmations::from(1), -}, None)] -fn provider_failure__should_name_only_the_failures_the_provider_owns( - #[case] error: ForeignChainInspectionError, - #[case] expected: Option, -) { - // When - let failure = error.provider_failure(); - - // Then - assert_eq!(failure, expected); -} diff --git a/crates/foreign-chain-inspector/tests/starknet_inspector.rs b/crates/foreign-chain-inspector/tests/starknet_inspector.rs index acf18579af..d98055cca5 100644 --- a/crates/foreign-chain-inspector/tests/starknet_inspector.rs +++ b/crates/foreign-chain-inspector/tests/starknet_inspector.rs @@ -517,11 +517,14 @@ async fn extract__should_return_event_log_for_specific_index_via_http_rpc_client /// Starknet mainnet's chain id, `SN_MAIN` in ASCII. const MAINNET_CHAIN_ID: &str = "0x534e5f4d41494e"; +const PADDED_UPPERCASE_MAINNET_CHAIN_ID: &str = "0x00534E5F4D41494E"; #[tokio::test] async fn network_fingerprint__should_return_the_canonical_chain_id() { - // Given: the chain id padded and uppercased, as a provider is free to send it. - let inspector = StarknetInspector::new(mock_client_from_fixed_response("0x00534E5F4D41494E")); + // Given + let inspector = StarknetInspector::new(mock_client_from_fixed_response( + PADDED_UPPERCASE_MAINNET_CHAIN_ID, + )); // When let fingerprint = inspector @@ -572,9 +575,17 @@ fn transport_error() -> RpcClientError { ))) } +fn bad_api_key_error() -> RpcClientError { + RpcClientError::Call(jsonrpsee::types::ErrorObject::owned( + -32600, + "Must be authenticated!", + None::<()>, + )) +} + #[tokio::test] async fn network_fingerprints__should_retry_a_transient_failure_and_report_the_later_success() { - // Given a provider that refuses the first call and answers the second + // Given let (fan_out, calls) = single_provider_fan_out(|call| match call { 0 => Err(transport_error()), _ => Ok(serde_json::json!(MAINNET_CHAIN_ID)), @@ -596,7 +607,7 @@ async fn network_fingerprints__should_retry_a_transient_failure_and_report_the_l #[tokio::test] async fn network_fingerprints__should_stop_after_the_configured_number_of_attempts() { - // Given a provider that never answers + // Given let (fan_out, calls) = single_provider_fan_out(|_| Err(transport_error())); // When @@ -614,21 +625,15 @@ async fn network_fingerprints__should_stop_after_the_configured_number_of_attemp #[tokio::test] async fn network_fingerprints__should_not_retry_a_provider_that_refused_the_request() { - // Given a provider refusing with a JSON-RPC error object, as one does for a bad API key - let (fan_out, calls) = single_provider_fan_out(|_| { - Err(RpcClientError::Call(jsonrpsee::types::ErrorObject::owned( - -32600, - "Must be authenticated!", - None::<()>, - ))) - }); + // Given + let (fan_out, calls) = single_provider_fan_out(|_| Err(bad_api_key_error())); // When let results = fan_out .network_fingerprints(Duration::from_secs(1), NonZeroU64::new(3).unwrap()) .await; - // Then the refusal is reported as one, and the remaining attempts are not spent on it + // Then assert_eq!(calls.load(Ordering::SeqCst), 1); assert_matches!( results[0].1, diff --git a/docs/foreign-chain-transactions.md b/docs/foreign-chain-transactions.md index aa756c890e..4975589a3c 100644 --- a/docs/foreign-chain-transactions.md +++ b/docs/foreign-chain-transactions.md @@ -525,15 +525,15 @@ Voting uses the protocol's existing signing threshold (`self.threshold()?.value( The per-chain map key prevents *lookup* confusion: when the node resolves the operator's `ethereum:` section, only `entries[Ethereum]` is consulted, never `entries[Sepolia]`. What it doesn't prevent is a `ChainVote { chain: Ethereum, providers: [ProviderEntry { provider_id: "ankr", chain_routing: PathSegment { segment: "eth_sepolia" }, … }, …], threshold: _ }` getting voted in — the contract just stores what threshold consensus produces; it can't tell whether `"eth_sepolia"` actually corresponds to Ethereum mainnet. Threshold voter review is the first line of defense; the fan-out across a chain's providers is the structural one. The network fingerprint probe is a per-node diagnostic on top of both. -At startup, each resolved provider gets its self-identifying RPC called and the response is compared against that chain's `expected_network_fingerprint` from the operator's config. The probe is report-only: a provider serving the wrong network is logged, but is not dropped, because a boot-time network blip should not take a chain out of signing. +Once wired into node startup, each resolved provider gets its self-identifying RPC called and the response is compared against that chain's `expected_network_fingerprint` from the operator's config. The probe is report-only: a provider serving the wrong network is logged, but is not dropped, because a boot-time network blip should not take a chain out of signing. Taking the expected value from operator config rather than a constant in the attested binary is a deliberate trade. It makes mixed-network and local deployments checkable at all, since a config may pair one chain's mainnet with another's testnet and no binary can ship a value for a devnet. The cost is that the check no longer binds an operator: they can set the wrong value, or omit the field and get no check at all, and either way they fool only their own node's diagnostics. The network-level defenses against a wrong URL are unchanged: threshold voter review of the whitelist, and the provider fan-out, which fails the individual request when a provider disagrees with its siblings. -Not every chain has a fingerprint probe. The table lists the ones that do, with the RPC each probes. A chain absent from it ignores `expected_network_fingerprint`. +Not every chain has a fingerprint probe. The table lists the ones that do, with the RPC each probes. A chain absent from it ignores `expected_network_fingerprint`. The fingerprint values themselves are tabulated once, under [Configuration (Node)](#configuration-node). -| chain | probe | fingerprint (mainnet) | fingerprint (testnet) | -|---|---|---|---| -| starknet | `starknet_chainId` | `0x534e5f4d41494e` (`SN_MAIN`) | `0x534e5f5345504f4c4941` (`SN_SEPOLIA`) | +| chain | probe | +|---|---| +| starknet | `starknet_chainId` | Starknet's fingerprint is the chain id felt in lowercase `0x` hex without leading zeros. Both providers and operators are free to pad and upper-case it, so the reported and the configured value are normalized before they are compared.