feat: probe Starknet by chain id and a recent transaction - #3932
feat: probe Starknet by chain id and a recent transaction#3932haiyuechen-nearone wants to merge 1 commit into
Conversation
740fa83 to
40098ed
Compare
haiyuechen-nearone
changed the title
a79a492 to
dc7ac79
Compare
haiyuechen-nearone
changed the title
b2f1b90 to
0d17ad2
Compare
Replace Starknet's pinned golden transaction with the Sui-style hybrid probe: verify the chain identity (starknet_chainId, a genesis-derived constant that is never pruned), then run the real inspector over a transaction from the latest L1-accepted block at AcceptedOnL1 finality. This removes the archive-node dependency and the per-network golden-vector maintenance while keeping the production-path smoke test (receipt -> finality -> canonicality). AcceptedOnL1 matches what real verify-foreign-tx requests use and actually exercises the finality check (the AcceptedOnL2 branch is a no-op). Discovery uses the l1_accepted block tag, so the probe requires provider JSON-RPC v0.9+. To stay robust against load-balanced providers whose backends disagree on the L1-accepted frontier, the probe walks back from the head — doubling the step each time, capped at MAX_WALKBACK_BLOCKS — past empty blocks and past receipts a lagging backend still reports below L1 finality. Expected identities carry no built-in values: they come from configuration (foreign_chain_health_check.identities), so any network — including local or custom ones — is checkable, and a configured identity-probed chain fails until its identity is set. Sui's built-in reference moves to configuration on the same terms. Operator templates ship with the identities prefilled.
0d17ad2 to
c38ac5d
Compare
haiyuechen-nearone
changed the title
Pull request overviewReplaces Starknet's pinned golden transaction with a Sui-style identity + recent-transaction probe: verifies Changes:
Reviewed changesPer-file summary
FindingsNon-blocking (nits, follow-ups, suggestions):
✅ Approved |
netrome
left a comment
netrome
left a comment
There was a problem hiding this comment.
So we need to discuss this further. Now this PR modifies a separate health check CLI. For this CLI, I'm not as convinced that querying transactions is wrong. I can see the point of doing that as long as it's stand-alone. Additionally, I am not too fond of the idea of reusing too much of the foreign-chain-health-check crate in our node code as-is, as it seems to me like a vibe-coded CLI with questionable code-quality that only passed shallow reviews in: #3716
There's a lot of code smell (large functions, helpers mutating shared collections instead of being pure functions that could be executed in parallel and tested more cleanly) that I'd be very hesitant to bring into node code.
Finally, I don't think it should be too complicated to just extend our inspector without touching any of the foreign-chain-config-tester code. I'd picture us adding some kind of trait to get the chain identity of any inspector and checking these against configured values on startup. I think this could greatly simplify things.
I'll schedule a call so we can chat about this tomorrow.
|
|
||
| /// Per-chain expected identities from config. Absent chains stay `None` (their check then | ||
| /// fails until configured); an unknown chain key or non-string value is a hard error. | ||
| pub fn detect_expected_identities( |
There was a problem hiding this comment.
Nit: What do you think about renaming this to parse_expected_identities to maintain parity with parse_foreign_chains?
| fn mark_missing_identity( | ||
| chain: &'static str, | ||
| cfg: &ForeignChainConfig, | ||
| out: &mut Vec<ProviderResult>, | ||
| ) { | ||
| for name in cfg.providers.keys() { | ||
| out.push(ProviderResult { |
There was a problem hiding this comment.
Hmm not a fan of pushing to a mutable vector here. I see this was the established pattern, but we should consider refactoring this for a cleaner and more maintainable implementation now when this becomes part of our node code.
There was a problem hiding this comment.
Something like this maybe?
enum HealthCheckStatus {
Healthy
Unhealthy
}
struct ForeignChainProviderState {
provider_name: String
health_check_status: HealthCheckStatus
}
struct ForeignChainSnapshot {
chain: ForeignChain
configured: bool
providers_state: Vec<ForeignChainProviderState>
}
struct ForeignChainProvidersSnapshot {
snapshots: Vec<ForeignChainSnapshot>
}
My habit is to introduce minimal changes for each PR, and leave any refactors for a separate PR where there is no behavior change. I can try to put the refactors upfront if that is the preferred work flow.
| # Expected identity per chain for the provider health check — see | ||
| # crates/foreign-chain-config-tester/README.md for the well-known values. | ||
| [mpc_node_config.node.foreign_chain_health_check.identities] | ||
| starknet = "0x534e5f5345504f4c4941" # mainnet: "0x534e5f4d41494e" | ||
| sui = "69WiPg3DAQiwdxfncX6wYQ2siKwAe6L9BZthQea3JNMD" # mainnet: "4btiuiMPvEENsttpZC7CZ53DruC3MAgfznDbASZ7DR6S" |
My intention was that the CLI and the healthcheck should share the core probing logic, so that the same code flow can be tested against real providers easily through the CLI (which is what I have tested this PR with as well). And then by extension the CLI needed the changes because of the config changes. |
Fair point, I'm happy to share the probing logic between the two. However, the shared surface should uphold production standards that I'm not convinced that the CLI does. To achieve this shared surface, I believe the simplest approach would be to introduce the chain ID probes in the node first, and then reuse those probes in the CLI. I'm happy to do it the other way around, but this front-loads much more code cleanups in the health-check and it's harder to picture how the probes will be integrated in the node requiring more context in reviews. |
c38ac5d to
93c9ab5
Compare
|
PR title type suggestion: This PR changes only configuration and asset files (VS Code settings, fonts, .gitignore), not source code. The type prefix should probably be Suggested title: (If this PR actually does modify source code for Starknet probing, please verify the changed files list is complete.) |
93c9ab5 to
c38ac5d
Compare
2 participants
Part of #3764; rationale: #3969. Stacked, merge bottom-up:
eth_chainId+ a recent finalized txReplaces Starknet's pinned golden tx with the Sui-style probe: verify
starknet_chainId, then inspect a real tx from a recentl1_acceptedblock atAcceptedOnL1(walks back past empty/not-yet-final blocks; needs provider JSON-RPC v0.9+).Identities come from config (
foreign_chain_health_check.identities), no built-ins — so any network is checkable, and a configured chain fails until its identity is set. Also lands the shared scaffolding the rest of the stack reuses.