Validate outpost signer references and Ethereum chain IDs - #534
Conversation
heifner
left a comment
There was a problem hiding this comment.
Reviewed the outpost startup-validation changes. Core logic is solid: get_client_by_chain_id fails closed on ambiguity, parse_chain_id is overflow-guarded, and the deadline_scope is genuinely honored by the json-rpc transport via current_deadline(). A few notes below, mostly minor.
heifner
left a comment
There was a problem hiding this comment.
Re-reviewed b9a58cfdac. The helper extraction cleanly resolves the catch-block feedback (acknowledged on the individual threads), and the removal is consistent across the plugin, README, and tests. One doc note and one design note:
The ## Why and ## Validation sections of the PR description still describe Solana getGenesisHash startup validation, which this commit removed. Worth updating so the description matches the shipped behavior. Inline note below on the cluster-identity tradeoff itself.
heifner
left a comment
There was a problem hiding this comment.
Approving the outpost startup-validation changes.
Reviewed through the origin/master merge (099e239bc1): the Ethereum and signature-provider-manager changes are unchanged from prior review, the Solana signer/chain/key/empty-field validation survived the merge intact, and the merge's solana_outpost_role threading correctly matches master's create_outpost_client signature.
Signer references now require an explicitly named --signature-provider with the correct target chain and key type; explicit Ethereum chain IDs are parsed as checked uint32_t and verified against eth_chainId within a five-second bound; get_client_by_chain_id fails closed on ambiguity. The uint32_t chain-id ceiling and the deferral of mandatory Solana cluster pinning to SEC-139/WSA-225 are both intentional and documented.
All review threads resolved. Build/test CI is still running on the merge head, so this is the code sign-off; recommend merging only once CI is green.
Summary
--signature-providereth_chainIdduring startup using checkeduint32_tparsingBehavior
Signature-provider references
Outpost client configuration previously resolved signer references through the general provider registry. Anonymous
key-Naliases and programmatically created providers could therefore satisfy a reference even though the operator had not supplied a stable shared name.Both Ethereum and Solana clients now require the referenced name to come from the explicit five-field
--signature-providerform. The provider must also use the target chain's native key type.Ethereum chain IDs
The optional fourth Ethereum client field is parsed as a positive 32-bit decimal integer or Ethereum hex quantity. When present, startup calls
eth_chainId, rejects malformed/error responses, and fails on mismatch before registering the client.The legacy three-field Ethereum format intentionally remains non-probing for compatibility: when no expected chain ID is configured, client construction still validates the URL and resolves DNS, but startup does not perform a reachability/identity RPC.
Solana scope
--outpost-solana-clientremains exactly:PR #534 validates the explicit signer reference, Solana target chain/key type, and non-empty fields. It deliberately does not add an optional genesis hash or a startup
getGenesisHashprobe. Mandatory startup pinning, runtime freshness, bypass-resistant gating, and telemetry belong to SEC-139 / WSA-225 and will be implemented as a separate complete control.Review follow-up
uint32_t; wider EVM chain IDs require a separate protocol/contracts migrationorigin/master(d224ba5606) and retained its role-aware Solana IDL/envelope-layout decoding while resolving the overlapping test include blockValidation
test_signature_provider_manager_plugintest_outpost_ethereum_client_plugintest_outpost_solana_client_pluginorigin/mastermergegit diff --check origin/masterCLEANEarlier validation of the unchanged JSON-RPC transport and compatibility path also passed
json_rpc_client_tests(10/10) and 100 repeated Ethereum three-field no-RPC runs.Notes
The feature branch was created directly from
origin/masteratc3b84b93bfand now includes merge commit099e239bc1from currentorigin/master(d224ba5606) to resolve the GitHub conflict. It does not include or depend on PR #516.