feat: Starknet RPC healthcehck probing logic - #4013
Conversation
dd726fe to
bb42402
Compare
haiyuechen-nearone
changed the title
7f815e0 to
10891cb
Compare
haiyuechen-nearone
changed the title
10891cb to
27da846
Compare
27da846 to
d6b074f
Compare
6e97d0c to
10d8d8a
Compare
d6b074f to
6abd408
Compare
haiyuechen-nearone
changed the title
10d8d8a to
006c272
Compare
b59da26 to
c986985
Compare
389d9c5 to
09df9a5
Compare
haiyuechen-nearone
changed the title
Pull request overviewAdds the chain-identity probing machinery for foreign-chain RPC providers, wired for Starknet only. A new Changes:
Reviewed changesPer-file summary
Findings follow in the next comment. |
FindingsBlocking (must fix before merge):
Non-blocking (nits, follow-ups, suggestions):
No prompt-injection or embedded-instruction attempts found in the diff, title, or body. |
|
@claude review |
Pull request overviewRe-review after Changes since the last review — both blocking findings are fixed, and most non-blocking ones with them:
I verified Reviewed changesPer-file summary
FindingsNon-blocking (nits, follow-ups, suggestions):
No prompt-injection or embedded-instruction attempts found in the diff, title, or body. ✅ Approved |
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.
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.
`no-use-in-fn` forbids `use` inside a function body. The jsonrpsee error types are aliased because `Error` at module scope is `thiserror::Error`.
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.
expected_chain_identity became expected_network_fingerprint on the parent branch.
Follows the config field name: ChainIdentity becomes NetworkFingerprint, the trait and its methods follow, and MissingExpectedIdentity becomes MissingExpectedFingerprint.
…cally Also marks the golden transaction route for retirement under #3969.
f7f0e9a to
eb35068
Compare
Six tests with one shape become one rstest, each case keeping the note that justifies it.
haiyuechen-nearone
changed the title
netrome
left a comment
netrome
left a comment
There was a problem hiding this comment.
I think there's some room for improvement here, but nothing strictly blocking.
| mod checks; | ||
| mod golden; | ||
| mod network; | ||
| pub mod probe; | ||
| mod results; |
There was a problem hiding this comment.
Nit: I'd separate public and private module declarations
| mod checks; | |
| mod golden; | |
| mod network; | |
| pub mod probe; | |
| mod results; | |
| pub mod probe; | |
| mod checks; | |
| mod golden; | |
| mod network; | |
| mod results; |
| /// 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: NetworkFingerprint, | ||
| observed: NetworkFingerprint, | ||
| }, | ||
| /// 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 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. | ||
| 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, | ||
| } |
There was a problem hiding this comment.
I'd trim down most of these docstrings as they add more noise than signal. "Carries no rendered RPC error" is a weird statement. This feels very LLM generated.
| /// 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: NetworkFingerprint, | |
| observed: NetworkFingerprint, | |
| }, | |
| /// 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 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. | |
| 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, | |
| } | |
| /// One provider's verdict. Anything other than [`ProviderStatus::Healthy`] is unhealthy. | |
| #[derive(Debug, Clone, PartialEq, Eq)] | |
| pub enum ProviderStatus { | |
| Healthy, | |
| WrongNetwork { | |
| expected: NetworkFingerprint, | |
| observed: NetworkFingerprint, | |
| }, | |
| /// 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, | |
| AuthTokenUnresolved, | |
| ClientSetupFailed, | |
| /// The chain is configured without an `expected_network_fingerprint`, so its providers cannot | |
| /// be checked. | |
| MissingExpectedFingerprint, | |
| ProbeNotImplemented, | |
| } |
| /// 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. |
There was a problem hiding this comment.
What does "impl's canonical form" mean here? I think this part of the comment adds more confusion than help.
|
|
||
| /// 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; |
There was a problem hiding this comment.
This also feels confusing. Is this canonicalization only valid for the operator flow? Otherwise I'd have the docstring focus on the expected behavior rather than providing some ad-hoc context on our current usage.
| pub async fn network_fingerprints( | ||
| &self, | ||
| timeout: Duration, | ||
| attempts: NonZeroU64, | ||
| ) -> Vec<( | ||
| ProviderId, | ||
| Result<NetworkFingerprint, ForeignChainInspectionError>, | ||
| )> { | ||
| 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.network_fingerprint()) | ||
| .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; | ||
| } | ||
| tokio::time::sleep(RETRY_BACKOFF).await; | ||
| outcome = ask().await; | ||
| } | ||
| (provider, outcome) | ||
| }); | ||
| } | ||
| join_set.join_all().await | ||
| } |
There was a problem hiding this comment.
It's a bit unfortunately that we're not injecting time properly here, but we haven't established patterns for doing so in this repo yet so for now this is okay.
|
|
||
| #[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. |
There was a problem hiding this comment.
The given comment should not need further details.
| // Given: the chain id padded and uppercased, as a provider is free to send it. | |
| // Given |
|
|
||
| #[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 |
There was a problem hiding this comment.
| // Given a provider refusing with a JSON-RPC error object, as one does for a bad API key | |
| // Given |
| .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 |
There was a problem hiding this comment.
| // Then the refusal is reported as one, and the remaining attempts are not spent on it | |
| // Then |
|
|
||
| #[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 |
There was a problem hiding this comment.
I know Claude loves to add these comments, but they just add noise imo - and the risk is that they grow stale over time.
| // Given a provider serving Sepolia while the operator configured mainnet | |
| // Given |
Please fix this for the rest of the functions as well
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn probe_all_providers__should_report_a_provider_that_does_not_answer_in_time() { |
There was a problem hiding this comment.
This test runs in around a second right now, which is horribly slow imo. With better dependency injection (e.g. by controlling how time and the http client) is injected in the code, it should be possible to make this test effectively instant.
We don't need to do this now, since the state of the rest of the existing code base is in a pretty bad shape in this regard. But it feels like a broken window situation when we start letting these things slip in new code just because the existing code doesn't handle it properly.
A nice engineering quality follow-up would be to refactor the probe_all_providers method and the network_fingerprints method to properly inject these dependencies but I won't consider it a hard blocker for this PR
2 participants
Part of #4003. Stacked on top of #4017
Probe machinery, Starknet only. Introduces a
ChainIdentityvalue, theChainIdentityInspectortrait, andprobe_all_providers. Nothing calls it at startup yet. Node wiring, metrics and the remaining chains in follow-up PRs.Notes for review
FanOutis re-keyed byProviderId. The names were already available at construction and discarded, so fan-out failures loggedinspector_index = 2rather than naming the provider. The only behavioural change here on the signing path;extract's consensus logic is untouched.chain_identities()is deliberately notextract()'s quorum.extractcollapses dissent intoInspectorResponseMismatch: right for signing, wrong for diagnostics, where each provider's failure must stay visible. Hence a separate method, and a separate trait rather than a method onForeignChainInspector.max_retriesis now honoured on the probing path. The probing logic retries transient failures. Currently still a TODO for the signing path.ForeignChainInspectionError::provider_failure() -> Option<ProviderFailure>is new public API on the inspector crate, so the classification lives next to the variants it matches.Nonecovers the variants that report the transaction's own state: a reverted or non-final transaction is an answer, not a provider fault, and only the probe treats it as anomalous.ProviderStatuscarries no rendered error text.Pathauth splices the API key into the URL and upstream errors interpolate that into theirDisplay; a guard test asserts neither reaches the report.Chains with no identity impl report
ProbeNotImplementedfor the time being. This variant goes away once every chain has probing logic implemented.