Skip to content

feat(foreign-tx): svm inspector - #4137

Open
frolvanya wants to merge 3 commits into
feat/svm-contract-interfacefrom
feat/svm-inspector
Open

feat(foreign-tx): svm inspector#4137
frolvanya wants to merge 3 commits into
feat/svm-contract-interfacefrom
feat/svm-inspector

Conversation

@frolvanya

Copy link
Copy Markdown
Collaborator

Adds the Solana JSON-RPC types and an SvmInspector, shared by Solana and Fogo behind chain-marker types
(SolanaInspector / FogoInspector, mirroring EvmInspector<Client, Chain>)
so an inspector built for one chain cannot be wired into the other's slot.

What it does per request:

  • fetches the transaction, and checks the returned signature is the one asked
    for;
  • checks finality, then that the transaction succeeded;
  • runs the requested extractors.

Details worth reviewing:

  • For Finalized, the rooted slot is read BEFORE the transaction. Reading it
    afterwards would let a block that got orphaned in between pass the check.
  • meta.err is a required field, not an Option: serde maps an absent field
    to None, which would make a provider that omits it indistinguishable from
    one reporting success. Absence is rejected; explicit null means success.
  • innerInstructions: null means the node was not started with extended
    transaction metadata, i.e. it cannot answer — not that the instruction is
    absent. Reported as a transient provider failure so the fan-out falls
    through to a provider that does record it, instead of signing an absence.
  • Hostile-provider bounds: instruction indices are u8 (the wire format),
    inner-instruction data is capped at the runtime's 10 KiB CPI limit, the
    account list at the runtime's 255-meta limit (checked before indices are
    resolved into 32-byte pubkeys), and error messages never echo unbounded
    provider strings.

Extracted values are normalised so independent providers produce byte-identical
output — account indices are resolved against the static account keys plus the
addresses loaded from lookup tables (so v0 transactions work), and instruction
data is decoded from base58.

Tested with mocked providers, including a params-recording mock that pins the
commitment levels and encodings actually sent, plus manual (#[ignore]d) tests
that were run against live Solana mainnet.

@claude

claude Bot commented Aug 13, 2026

Copy link
Copy Markdown

Pull request overview

Adds the Solana JSON-RPC wire types (foreign-chain-rpc-interfaces::svm) and an SvmInspector<Client, Chain> shared by Solana and Fogo behind marker types, mirroring the existing EvmInspector<Client, Chain> shape. Per request it fetches the transaction at confirmed, checks the returned first signature matches the one asked for, gates finality against a rooted slot read before the transaction, rejects a failed transaction, and then runs the requested extractors (InnerInstruction, AccountState). The inspector is not yet reachable from the node — verify_foreign_tx/sign.rs:138 still bails on ForeignChainRpcRequest::Solana and the health check marks solana "not yet supported" — so this lands as a self-contained building block.

Changes:

  • New foreign-chain-rpc-interfaces::svm: getTransaction / getAccountInfo / getSlot request args and partial response types, with meta.err deliberately non-Option and base64 account-data decoding that rejects a non-requested encoding.
  • New foreign-chain-inspector::svm + svm::inspector: SvmInspector, SolanaInspector / FogoInspector aliases, SvmFinality, SvmExtractor, index→pubkey resolution across static keys ++ loaded writable ++ loaded readonly, and a getGenesisHash network-fingerprint probe.
  • Hostile-provider bounds: base58 length caps before the superlinear decode, a 255 account-meta cap checked before index resolution, a 14 000-char cap on inner-instruction data, and error messages that don't echo unbounded provider strings.
  • New AccountNotFound error variant (non-transient, provider_failure() == None), plus DTO conversions and roundtrip tests.
  • Mocked integration tests including a params-recording client that pins commitments/encodings, plus #[ignore]d live-mainnet tests.

Reviewed changes

Per-file summary
File Description
crates/foreign-chain-rpc-interfaces/src/svm.rs New SVM JSON-RPC DTOs, request-arg serializers, AccountData::decode, unit tests
crates/foreign-chain-rpc-interfaces/src/lib.rs Registers the svm module
crates/foreign-chain-rpc-interfaces/Cargo.toml Adds base64 (used by AccountData::decode)
crates/foreign-chain-inspector/src/svm/inspector.rs The inspector: finality gate, signature check, extractors, fingerprint probe, pure helpers + unit tests
crates/foreign-chain-inspector/src/svm.rs SvmTransactionSignature (64 bytes) and SvmExtractedValue
crates/foreign-chain-inspector/src/lib.rs Registers svm; adds AccountNotFound to the error enum, is_transient/provider_failure classification
crates/foreign-chain-inspector/src/contract_interface_conversions.rs SvmFinality / SvmExtractor / SvmExtractedValue ↔ DTO conversions + roundtrip tests
crates/foreign-chain-inspector/tests/svm_inspector.rs Mocked end-to-end tests, incl. a RecordingClient pinning sent params
crates/foreign-chain-inspector/tests/svm_rpc_manual.rs #[ignore]d live-mainnet tests against api.mainnet-beta.solana.com
crates/foreign-chain-inspector/Cargo.toml Adds base64 dev-dependency
crates/contract/tests/snapshots/abi__abi_has_not_changed.snap Adds an assertion_line: 47 metadata line; ABI content unchanged
Cargo.lock base64 recorded for both crates

The core protocol decisions read correct to me: reading the rooted slot before the transaction (a root read afterwards would admit a slot orphaned in between), querying getTransaction at confirmed because finalized collapses "unknown" and "not yet rooted" into the same null, and treating innerInstructions: null as a transient provider gap while an empty list is the chain's own answer. Index resolution is also safe in the degenerate direction: an omitted loadedAddresses can only make a lookup-table index go out of bounds, never resolve to the wrong pubkey.

Findings

Blocking (must fix before merge):

  • docs/foreign-chain-transactions.md:727 — "solana and ethereum are configurable but absent from the table: neither has an inspector, so there is nothing about them to verify in the first place" is false for solana as of this PR. SvmInspector implements NetworkFingerprintInspector via getGenesisHash, and crates/foreign-chain-inspector/tests/svm_rpc_manual.rs:20 describes 5eykt4UsFv8P8NJdTREpY1vzqKqZKvdpKuc147dw2N9d as "the value operators put into expected_network_fingerprint" — yet the fingerprint table at :705-:717 has no solana (or fogo) row, and the probe table at :570-:573 doesn't list getGenesisHash. Per CLAUDE.md this is a same-PR fix, not a follow-up. Note the table at :716 already carries sui, which has no probe, so the value table is populated ahead of wiring; the probe row is the one where you could reasonably say "once the health check calls it" — if you defer that, say so explicitly at :734 rather than leaving :727 asserting there is no inspector.

Non-blocking (nits, follow-ups, suggestions):

  • crates/foreign-chain-inspector/src/svm/inspector.rs:136-152 — this is the first inspector where an extractor costs a network round trip, and they are issued strictly sequentially with no dedup, while SvmRpcRequest.extractors is an unbounded Vec (crates/near-mpc-contract-interface/src/types/foreign_chain.rs:256). A request repeating the same pubkey N times costs N sequential getAccountInfo calls per provider per node, all inside the single FOREIGN_CHAIN_INSPECTION_TIMEOUT the node wraps around extract. Worth resolving the account reads with try_join_all (order is preserved) and/or deduping by pubkey before wiring the node up — or bounding the extractor count contract-side.
  • crates/foreign-chain-inspector/src/svm/inspector.rs:182-195getAccountInfo is sent without minContextSlot. Against a single node the earlier getSlot/getTransaction ordering already implies the account read is at or after tx.slot, but a load-balanced endpoint can serve this call from a backend that has not seen the transaction, i.e. return pre-transaction state that is then accepted as a substantive verdict. minContextSlot: tx.slot would close that. Caveat if you take it: the RPC answers error -32016, which classify_rpc_client_error currently maps to the non-transient RpcRequestRejected, so it would need adding to the rate-limit-style transient set.
  • crates/foreign-chain-inspector/src/lib.rs:374 / svm/inspector.rs:196-198AccountNotFound is non-transient, so a provider whose view lags (or that pruned) and answers value: null produces a substantive verdict that splits the fan-out into success + non-transient error, failing the whole request with InspectorResponseMismatch. That is the conservative direction and matches the timing caveat already documented at docs/foreign-chain-transactions.md:250-260, but it is worth a sentence in that section saying absence is classified as an answer, since the alternative (transient) is what NotFinalized does for the analogous "the provider's view hasn't caught up" case.
  • crates/foreign-chain-inspector/src/svm/inspector.rs:358 — "Report the length, not the provider-controlled string: the message is logged in full" is contradicted two and four lines below, where both error messages interpolate {s:?}. That is safe because of the cap above it, so the comment states the wrong rule; something like "the cap above is what makes echoing s below bounded" says the thing the code cannot.
  • crates/contract/tests/snapshots/abi__abi_has_not_changed.snap:3 — the only change to this file is an assertion_line: 47 metadata key, with the ABI body untouched. No other .snap in the repo carries that key; it pins a line number in abi.rs and will churn on the next cargo insta accept. Please drop it (and, if the first commit has no other content, the commit).
  • crates/foreign-chain-inspector/src/svm/inspector.rs:41-47 — nothing constructs a FogoInspector: every test uses SolanaInspector, ForeignChainsConfig has no fogo section, and the node has no dispatch arm. The stated point of the marker types — that one chain's inspector cannot be wired into the other's slot — has no coverage; a one-line test (or a compile-fail note) would pin the property the PhantomData exists for. Also worth noting NetworkFingerprintInspector is implemented for any Chain: Send + Sync, without the SvmChain bound the other impls carry.

⚠️ Issues found

Copilot AI lite review requested due to automatic review settings August 13, 2026 13:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds first-class SVM (Solana VM) support to the foreign-chain inspector stack by introducing Solana JSON-RPC request/response types and a generic SvmInspector<Client, Chain> with chain-marker types to prevent accidentally wiring a Solana inspector into a Fogo slot (and vice versa).

Changes:

  • Introduces foreign-chain-rpc-interfaces::svm with typed RPC params/partial responses for getTransaction, getAccountInfo, and getSlot.
  • Adds foreign-chain-inspector::svm with SvmInspector (plus SolanaInspector/FogoInspector aliases), finality gating, and extractors for inner instructions and account state.
  • Extends contract-interface conversion plumbing and error surface (AccountNotFound), plus adds comprehensive mocked tests and ignored live-RPC manual tests.

Reviewed changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
crates/foreign-chain-rpc-interfaces/src/svm.rs Adds SVM JSON-RPC types and param serialization helpers used by the inspector.
crates/foreign-chain-rpc-interfaces/src/lib.rs Exposes the new svm module.
crates/foreign-chain-rpc-interfaces/Cargo.toml Adds dependencies needed for SVM RPC parsing/serialization tests (base64, serde_json).
crates/foreign-chain-inspector/src/svm/inspector.rs Implements the generic SvmInspector with finality checks and extraction logic.
crates/foreign-chain-inspector/src/svm.rs Defines SVM extracted value types and exposes the inspector module.
crates/foreign-chain-inspector/src/lib.rs Exposes svm and adds the AccountNotFound inspection error variant.
crates/foreign-chain-inspector/src/contract_interface_conversions.rs Adds DTO conversions for SvmFinality, SvmExtractor, and SvmExtractedValue.
crates/foreign-chain-inspector/tests/svm_inspector.rs Adds mocked test coverage for SVM extraction, finality semantics, and hostile-provider bounds.
crates/foreign-chain-inspector/tests/svm_rpc_manual.rs Adds ignored live-RPC sanity checks against Solana mainnet.
crates/foreign-chain-inspector/Cargo.toml Adds base64 dev-dependency for SVM inspector tests.
Cargo.lock Locks new dependency usage introduced by the SVM modules/tests.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread crates/foreign-chain-inspector/src/svm/inspector.rs
@frolvanya

Copy link
Copy Markdown
Collaborator Author

@claude review

@claude

claude Bot commented Aug 13, 2026

Copy link
Copy Markdown

Pull request overview

Re-review after a9e1126. Adds the Solana JSON-RPC wire types (foreign-chain-rpc-interfaces::svm) and an SvmInspector<Client, Chain> shared by Solana and Fogo behind marker types, mirroring EvmInspector<Client, Chain>. Per request it reads the rooted slot before the transaction, fetches the transaction at confirmed, checks the returned first signature is the one asked for, gates finality, rejects a failed transaction, and then runs the requested extractors (InnerInstruction, AccountState). Stacked on #4136 for the DTOs; still unreachable from the node (the health check at foreign-chain-health-check/src/lib.rs:109 still marks solana "not yet supported"), so this lands as a self-contained building block ahead of #4138.

Changes:

  • New foreign-chain-rpc-interfaces::svm: getTransaction / getAccountInfo / getSlot request args and partial responses, with meta.err deliberately non-Option, a required context.slot on the account read, and base64 account-data decoding that rejects a non-requested encoding.
  • New foreign-chain-inspector::svm: SvmInspector, SolanaInspector / FogoInspector, SvmFinality, SvmExtractor, index→pubkey resolution over static keys ++ loaded writable ++ loaded readonly, and a getGenesisHash fingerprint probe.
  • Hostile-provider bounds: base58 length caps ahead of the superlinear decode, a 255 account-meta cap checked before index resolution, a 14 000-char cap on inner-instruction data.
  • New AccountNotFound error variant (non-transient, provider_failure() == None), DTO conversions and roundtrip tests.
  • Since the last round: account reads are deduplicated per distinct pubkey, an account answer served at a slot below the transaction's is rejected as transient, the stale decode_base58_32 comment is gone, the spurious assertion_line snapshot churn is reverted, and the docs / probe.rs TODO no longer claim solana has no inspector.

Reviewed changes

Per-file summary
File Description
crates/foreign-chain-rpc-interfaces/src/svm.rs New SVM JSON-RPC DTOs, request-arg serializers, ResponseContext, AccountData::decode, unit tests
crates/foreign-chain-rpc-interfaces/src/lib.rs Registers the svm module
crates/foreign-chain-rpc-interfaces/Cargo.toml Adds base64; also re-adds serde_json as a dev-dependency
crates/foreign-chain-inspector/src/svm/inspector.rs The inspector: finality gate, signature check, per-pubkey account cache, account-slot floor, extractors, fingerprint probe, pure helpers + unit tests
crates/foreign-chain-inspector/src/svm.rs SvmTransactionSignature (64 bytes) and SvmExtractedValue
crates/foreign-chain-inspector/src/lib.rs Registers svm; adds AccountNotFound plus its is_transient / provider_failure classification
crates/foreign-chain-inspector/src/contract_interface_conversions.rs SvmFinality / SvmExtractor / SvmExtractedValue ↔ DTO conversions + roundtrip tests
crates/foreign-chain-inspector/tests/svm_inspector.rs Mocked end-to-end tests, incl. a RecordingClient pinning sent params, dedup and slot-floor cases
crates/foreign-chain-inspector/tests/svm_rpc_manual.rs #[ignore]d live-mainnet tests against api.mainnet-beta.solana.com
crates/foreign-chain-inspector/Cargo.toml Adds base64 dev-dependency
crates/foreign-chain-health-check/src/probe.rs TODO(#4003) now lists Solana and Fogo as probe-able chains
docs/foreign-chain-transactions.md Documents the account-read slot floor; drops the claim that solana has no inspector
Cargo.lock base64 recorded for both crates

The finality argument holds: a transaction served at a slot at or below a previously observed root can only come from the rooted block, since roots never revert — reading the root afterwards would also admit a block orphaned in between. The new account-read floor is the right shape too: a floor rather than minContextSlot sidesteps the -32016 classification problem, and reporting the lag as transient drops the lagging backend from the quorum instead of letting pre-transaction state stand as a verdict.

Findings

Blocking (must fix before merge):

  • crates/foreign-chain-rpc-interfaces/src/svm.rs:123value: Option<AccountInfo> has no presence requirement, and serde maps an absent field to None, so a provider that omits value entirely is indistinguishable from one answering "value": null — which svm/inspector.rs:220 turns into AccountNotFound, a non-transient, signable verdict that the account does not exist. This is the same defect the PR deliberately guards against twice elsewhere: err is non-Option so that a provider omitting it is not "indistinguishable from one reporting success" (svm.rs:44), and ResponseContext is "Required, so that a provider omitting it cannot pass a freshness check" (svm.rs:125). A single hostile provider gains nothing (it can send an explicit null), but a truncated or buggy response shared across a node's providers — same vendor, same RPC build — gets attested as an absence instead of rejected as malformed. One line closes it, and no honest provider is affected since the RPC envelope always carries value:

    /// `None` when no account exists at the queried address. `deserialize_with` makes the field
    /// required: an absent field must not read as an absent account.
    #[serde(deserialize_with = "Option::deserialize")]
    pub value: Option<AccountInfo>,

    With deserialize_with and no default, serde emits a hard missing_field error rather than routing through the missing_field::<Option<_>> helper that silently yields None today. The alternative — serde_json::Value plus is_null, exactly as err does — works as well.

Non-blocking (nits, follow-ups, suggestions):

  • docs/foreign-chain-transactions.md:732 — the false sentence is gone, but the fingerprint value table above it (~:712-:723) still has no solana row, so a configurable chain that now has both an inspector and a network_fingerprint implementation is absent from the table with nothing explaining why, directly under a paragraph that explains ethereum's absence. The mainnet value is already pinned at crates/foreign-chain-inspector/tests/svm_rpc_manual.rs:20 and described there as "the value operators put into expected_network_fingerprint"; the table's stated convention is values read back from a live provider, which that manual test does. A getGenesisHash row in the probe table at :553-:556 is fairly deferred until the health check calls it (probe.rs:110's TODO now covers it), and a fogo value row likewise, since ForeignChainsConfig has no fogo section yet.
  • crates/foreign-chain-inspector/src/svm/inspector.rs:329LogIndexOutOfBounds is reused for an out-of-range inner-instruction index, so an SVM request naming a nonexistent CPI index reports "provided log index is out of bounds" to operators, for a chain that has no logs. lib.rs:334 already carries a TODO about per-inspector error types; a neutral spelling (or an SVM variant) would keep the diagnostic honest in the meantime.
  • crates/foreign-chain-inspector/src/svm/inspector.rs:151 — dedup collapses repeated pubkeys, which was the cheap half; N distinct pubkeys still cost N sequential getAccountInfo round trips inside the single FOREIGN_CHAIN_INSPECTION_TIMEOUT the node wraps around extract, and SvmRpcRequest.extractors is an unbounded Vec. try_join_all over the distinct set (order is preserved, and the cache already gives you that set) would make wall-clock independent of the extractor count before feat(foreign-tx): wire svm chains into node #4138 makes this reachable.
  • crates/foreign-chain-rpc-interfaces/Cargo.toml:23serde_json is already a regular dependency (line 16), so the [dev-dependencies] entry is redundant; svm.rs uses serde_json::Value in non-test code either way.
  • crates/foreign-chain-inspector/tests/svm_inspector.rs:30RecordingClient is SequentialResponseMockClientBuilder plus params recording, ~60 lines living in one test file. Moving it to tests/common.rs would let the other inspectors pin the commitments and encodings they send too, which is the property this PR shows is worth pinning.
  • crates/foreign-chain-rpc-interfaces/src/svm.rs:24, :49, :122 — plain `None` where CLAUDE.md asks for intra-doc links; the same file already writes [Option] and [Null](serde_json::Value::Null) two lines away from one of them.

Previously raised, still open, not re-argued here: nothing constructs a FogoInspector, so the property the PhantomData exists for stays uncovered; and docs/foreign-chain-transactions.md:250-:264 documents the account-state timing caveat without saying that an absent account is classified as an answer (AccountNotFound, non-transient) rather than as a lagging view. Both are reasonable to defer to #4138 if that is the plan — worth saying so explicitly if it is. My earlier nit about the missing SvmChain bound on the NetworkFingerprintInspector impl is withdrawn: EvmInspector at evm/inspector.rs:44-:47 does the same, so the PR follows the house pattern.

⚠️ Issues found

@frolvanya

Copy link
Copy Markdown
Collaborator Author

@claude review

@claude

claude Bot commented Aug 13, 2026

Copy link
Copy Markdown

Pull request overview

Re-review after b5dc987. Adds the Solana JSON-RPC wire types (foreign-chain-rpc-interfaces::svm) and an SvmInspector<Client, Chain> shared by Solana and Fogo behind marker types, mirroring EvmInspector<Client, Chain>. Per request it reads the rooted slot before the transaction, fetches the transaction at confirmed, checks the returned first signature is the one asked for, gates finality, rejects a failed transaction, and then runs the requested extractors (InnerInstruction, AccountState). Stacked on #4136 for the DTOs; still unreachable from the node (verify_foreign_tx/sign.rs:138 bails on Solana, health check lib.rs:109 marks it "not yet supported"), so this lands as a self-contained building block ahead of #4138.

Changes:

  • New foreign-chain-rpc-interfaces::svm: getTransaction / getAccountInfo / getSlot request args and partial responses, with meta.err non-Option, a required context.slot, and base64 account-data decoding that rejects a non-requested encoding.
  • New foreign-chain-inspector::svm: SvmInspector, SolanaInspector / FogoInspector, SvmFinality, SvmExtractor, index→pubkey resolution over static keys ++ loaded writable ++ loaded readonly, and a getGenesisHash fingerprint probe.
  • Hostile-provider bounds: base58 length caps ahead of the superlinear decode, a 255 account-meta cap checked before index resolution, a 14 000-char cap on inner-instruction data.
  • New AccountNotFound error variant (non-transient, provider_failure() == None), DTO conversions and roundtrip tests.
  • Since the last round: value on the account response is now required via deserialize_with = "Option::deserialize" (with a test that an omitted field reads as malformed, not as an absent account), LogIndexOutOfBounds's message is chain-neutral, and the redundant serde_json dev-dependency is gone.

Reviewed changes

Per-file summary
File Description
crates/foreign-chain-rpc-interfaces/src/svm.rs New SVM JSON-RPC DTOs, request-arg serializers, ResponseContext, AccountData::decode, unit tests
crates/foreign-chain-rpc-interfaces/src/lib.rs Registers the svm module
crates/foreign-chain-rpc-interfaces/Cargo.toml Adds base64
crates/foreign-chain-inspector/src/svm/inspector.rs The inspector: finality gate, signature check, per-pubkey account cache, account-slot floor, extractors, fingerprint probe, pure helpers + unit tests
crates/foreign-chain-inspector/src/svm.rs SvmTransactionSignature (64 bytes) and SvmExtractedValue
crates/foreign-chain-inspector/src/lib.rs Registers svm; adds AccountNotFound plus its classification; reworded LogIndexOutOfBounds
crates/foreign-chain-inspector/src/contract_interface_conversions.rs SvmFinality / SvmExtractor / SvmExtractedValue ↔ DTO conversions + roundtrip tests
crates/foreign-chain-inspector/tests/svm_inspector.rs Mocked end-to-end tests, incl. a RecordingClient pinning sent params, dedup, slot-floor and omitted-value cases
crates/foreign-chain-inspector/tests/svm_rpc_manual.rs #[ignore]d live-mainnet tests against api.mainnet-beta.solana.com
crates/foreign-chain-inspector/Cargo.toml Adds base64 dev-dependency
crates/foreign-chain-health-check/src/probe.rs TODO(#4003) now lists Solana and Fogo as probe-able chains
docs/foreign-chain-transactions.md Documents the account-read slot floor; drops the claim that solana has no inspector
Cargo.lock base64 recorded for both crates

The value fix is the right shape and matches how err and context are already handled — with deserialize_with and no default, serde emits a hard missing_field error, and the new test pins it including provider_failure() == Some(Malformed). I also checked InnerInstructionsEntry.index: u8 and CompiledInstruction.{program_id_index, accounts}: those are genuinely the RPC's own widths (UiInnerInstructions / UiCompiledInstruction), so the "mirrors the wire format" comment holds and no legitimate response is rejected by them.

Findings

Blocking (must fix before merge):

  • crates/foreign-chain-inspector/src/svm/inspector.rs:140 — the extractor loop caches AccountState reads per pubkey but recomputes InnerInstruction from scratch every iteration, and that path contains a superlinear base58 decode with no .await between iterations. The single decode is capped (:358, 14 000 chars ≈ 10 KiB out ≈ 7×10⁷ byte multiply-adds — the PR's own test comment at :284 states the premise: "Decoding it with bs58 is superlinear"), but the number of decodes is not: SvmRpcRequest.extractors has no length cap in the contract DTO and none in sign.rs, so a request carrying N copies of InnerInstruction { instruction_index: k, inner_instruction_index: j } against a transaction with a max-size CPI payload runs N full decodes back-to-back on one worker thread.

    Three things make this worse than a wall-clock nit:

    • FanOut::extract spawns one task per provider (lib.rs:142), so P providers each occupy a worker for the same duration, concurrently.
    • .timeout(FOREIGN_CHAIN_INSPECTION_TIMEOUT) at sign.rs:157 wraps the fan-out future. Dropping it aborts the JoinSet cooperatively, which cannot interrupt a synchronous loop — the CPU work keeps running past the 5 s budget, so the timeout is not a bound here.
    • Nothing else in the node is insulated: the starved workers are the same ones running the indexer, P2P and the MPC protocols.

    The fix is the one you already wrote for accounts — cache by index pair, so cost is bounded by the transaction's distinct inner instructions rather than by the request's extractor count:

    let mut inner_instructions: BTreeMap<(usize, usize), SvmExtractedValue> = BTreeMap::new();
    // ...
    SvmExtractor::InnerInstruction { instruction_index, inner_instruction_index } => {
        match inner_instructions.entry((*instruction_index, *inner_instruction_index)) {
            Entry::Occupied(entry) => entry.get().clone(),
            Entry::Vacant(entry) => entry
                .insert(extract_inner_instruction(
                    meta, &account_keys, *instruction_index, *inner_instruction_index,
                )?)
                .clone(),
        }
    }

    That leaves a residual bounded by the chain (many distinct max-size CPIs in one transaction are still seconds of blocking work), so a cap on extractors.len() — contract-side, or node-side before extract — is worth having too, and would also retire the "unbounded Vec" half of the earlier round's note. Not exploitable while sign.rs:138 still bails, so deferring behind a TODO(#NNNN) is defensible — but please don't let feat(foreign-tx): wire svm chains into node #4138 wire this up without one of the two.

Non-blocking (nits, follow-ups, suggestions):

  • crates/foreign-chain-inspector/src/svm/inspector.rs:214 and :322 — both content-level conditions (an account answered below the transaction's slot; innerInstructions: null) are reported as RpcRequestFailed, which provider_failure() maps to ProviderFailure::Unreachable and probe.rs:212 renders as ProviderStatus::Unreachable. Every other inspector reserves that variant for transport/status failures (sui/inspector.rs:116, aptos/inspector.rs:63), so an operator will read "unreachable" for a provider that answered promptly and is merely lagging or not recording CPI metadata. The classification is right (transient, provider's fault); it is the label that misleads. A transient variant mapping to a Stale/Incomplete provider failure would keep the report honest. Cheap to leave until the solana probe is actually written, since ProbeNotImplemented shadows it today.
  • crates/foreign-chain-inspector/src/svm/inspector.rs:157 / docs/foreign-chain-transactions.mdgetTransaction answering null becomes TransactionNotFound, non-transient. Solana RPC nodes keep only a couple of days of transaction history unless run with --enable-rpc-transaction-history (plus bigtable), so a fleet mixing a history-serving provider with a default one splits the fan-out into success + non-transient error and fails every request older than the window with InspectorResponseMismatch — systematically, not occasionally. Worth a line in the docs stating the provider requirement; the docs say nothing about history/archive requirements for any chain today, so this is arguably a wider gap that SVM just makes acute.
  • crates/foreign-chain-inspector/src/svm/inspector.rs:152 — dedup collapses repeated pubkeys, but N distinct pubkeys are still N sequential getAccountInfo round trips inside the single 5 s timeout. try_join_all over the distinct set (which the cache already gives you, and which preserves order) makes wall-clock independent of the extractor count. Raised last round, still open.

Previously raised, still open, not re-argued here: the fingerprint value table in docs/foreign-chain-transactions.md (the | chain | fingerprint | mainnet | testnet | one) still has no solana row, and the rewritten sentence below it now explains only ethereum's absence — so solana is missing with nothing saying why, even though the mainnet value is already pinned at tests/svm_rpc_manual.rs:20; the §AccountState paragraph still does not say that an absent account is classified as an answer (AccountNotFound, non-transient) rather than as a lagging view; nothing constructs a FogoInspector, so the property the PhantomData exists for stays uncovered; and RecordingClient would serve the other inspectors too from tests/common.rs. All four are fine to defer to #4138 if that is the plan — worth saying so explicitly.

⚠️ Issues found

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants