feat: periodically report foreign chain RPC provider health - #4071
feat: periodically report foreign chain RPC provider health#4071haiyuechen-nearone wants to merge 12 commits into
Conversation
haiyuechen-nearone
changed the title
haiyuechen-nearone
changed the title
36311f1 to
4ff2996
Compare
4ff2996 to
c42d937
Compare
haiyuechen-nearone
changed the title
haiyuechen-nearone
changed the title
haiyuechen-nearone
changed the title
b060ade to
8b6edca
Compare
Pull request overviewWires the existing Changes:
Reviewed changesPer-file summary
FindingsBlocking (must fix before merge):
Non-blocking (nits, follow-ups, suggestions):
|
8b6edca to
6023892
Compare
|
@claude review |
Pull request overviewWires the existing Since the previous review round both blocking items were addressed: Changes:
Reviewed changesPer-file summary
FindingsBlocking (must fix before merge):
Non-blocking (nits, follow-ups, suggestions):
Still open from the previous round, no need to re-litigate: the |
Deliberately not adding tests grepping for log lines. Having test grepping for text is very prone to drift. Log output was verified by running the cluster locally and inspecting it. All green case: Case with failures: |
Since we expect the operator to correctly configure all fields in the config file, not adding expected network fingerprint is considered "incorrect" and causes us to not know if a provider is healthy or not/ |
35b22c4 to
9ec4b44
Compare
9ec4b44 to
b25b5e2
Compare
03d88b2 to
26a28f0
Compare
80d813d to
d68ec60
Compare
haiyuechen-nearone
changed the title
e293ba5 to
76ac674
Compare
| pub trait Tick { | ||
| async fn tick(&mut self); | ||
| } |
There was a problem hiding this comment.
Seeing this trait made me happy 😊
anodar
left a comment
anodar
left a comment
There was a problem hiding this comment.
Overall looks good, but one blocker:
Node will report healthy foreign chain config if all configured RPC providers are healthy. I think we should rather check what's the quorum for reach chain (in the contract, in whitelist) and consider it healthy if at least quorum number of configured RPC's are healthy. Which means also not just log warning when node has no config at all (if foreign_chains.is_empty() { warn! ....)
There was a problem hiding this comment.
Probing is split a bit awkwardly between foreign_chain_health_check and here. Why not move this run_periodic_probe there?
There was a problem hiding this comment.
I think run_periodic_probe and probe_periodically fits in node. My thought is foreign_chain_health_check holds only the logic to send probing requests (probe_all_providers), which will be shared between the node and the CLI tool. Node needing to run it periodically, so the node should import and wrap it with extra periodic logic.
probe_periodically needs to publish metrics after every run, which is also node specific.
Spawns the probe detached, so it reports which network each provider serves without delaying startup or gating anything. A status carries no provider text and no auth material, so it is logged whole.
Publishes `mpc_foreign_chain_rpc_providers_configured` and `mpc_foreign_chain_rpc_providers_healthy`, labelled by chain rather than by provider, since a provider name is operator chosen and would put an unbounded label on a time series. `ForeignChain::label()` is the one place the chain label lives, so a metric label and a config key cannot drift apart. An e2e test points one node at a mock serving the expected chain id and another at the same mock while expecting a different network, so the gauges separate a healthy provider from one on the wrong network end to end. The EVM mock answers `eth_chainId` to make that possible. Leaves a provider no probe covers out of the `x/y providers healthy` summary. It counted toward the denominator but could never reach the numerator, so a node configuring a chain without a probe read as permanently degraded.
A chain no probe covers no longer publishes gauges, and an empty foreign chains config no longer warns on every boot. The panic guard is gone: the deployed binary aborts on panic, so it never ran. The summary is a value now, asserted directly instead of through log lines, and a node config test pins every chain's config key to its label.
A node is expected to configure the chains the code supports, so an empty section is a misconfiguration rather than a quiet path. The startup probe is the only place that observes it.
The warning named our own mechanism and left the operator to work out which of their chains went unchecked; it now lists them. The docs claimed a `ton` section exists, and that chains without an inspector report a status name an operator never sees.
`ProbeReport` gains `From<Vec<ProviderHealth>>`, so the gauge tests no longer open sockets and drive another crate's probe just to reach this crate's publication. They now cover several chains in one report, and pin the label of the one chain whose key is not its variant name. Log lines carry the chain's config key rather than its variant name, and a healthy or uncheckable provider logs at info, so the documented line per provider appears at the level operators run. Also drops metric labels from the truncation rationale: no metric carries a fingerprint.
Keep only the claim the fixture does not make, and leave the Given and When markers bare.
Covers the avalanche and adi sections main added to ForeignChainsConfig, and applies review feedback from the earlier PRs in the stack: named structs in place of positional tuples in the probe tests, bare Given/When/Then markers, and comments that no longer restate the code.
check_all_providers is slated for removal, so routing its chain names through ForeignChain::label was churn in a file on its way out. Also drops two comments that restated the code beside them.
A verdict taken once at boot goes stale: a provider can start serving another network, or go down, while the node is up. The probe now runs on a ticker and the gauges carry the latest round. Tick and its test double move out of remote_attestation into a shared tick module, so both periodic loops use one abstraction.
Both nodes configure two BNB providers and expect the same network, so the healthy gauge reads 2 against 1. Neither is the gauge default, which a poll waiting for zero cannot tell from a probe that never ran. A new mock serves eth_chainId alone, all the probe asks, so the fuller EVM mock keeps its shape. Also covers the Fogo variant main added, converts the counts with try_from, and drops the label rstest the exhaustive node config test subsumes.
76ac674 to
309cf51
Compare
3 participants
Closes #4094.
The node runs the foreign chain probe on a detached hourly loop, starting at
startup. Each round logs a line per provider plus an
x/y healthysummary andexports latest status to metrics.
Notes for review
Safe to log: no
ProviderStatusvariant carries RPC error text, and theobserved fingerprint is length capped.
Metric is per chain, not per provider: provider names are operator chosen, so
the label would be unbounded. The logs contains the full name of failing providers.
A chain configured without
expected_network_fingerprintcounts as unhealthy.An empty
foreign_chainssection warns once and the loop exitsTickmoved out ofremote_attestationintotick.rsand is now shared by both attestation_submission and probing loops.