feat: retry-safe Base RPC failover transport with chain-id validation (#869) - #884
feat: retry-safe Base RPC failover transport with chain-id validation (#869)#884laurentketterle-hub wants to merge 15 commits into
Conversation
…13#869) - Extend BaseRpcUrlConfig to accept ordered comma-separated HTTPS endpoint lists - Add FailoverJsonRpcTransport with chain ID 8453 validation - Deterministic exponential backoff: retry only bounded transport, HTTP 429, and HTTP 5xx - Never retry confirmed JSON-RPC execution errors; credentials are never logged - Add resolve_endpoint_list() and failover_transport() convenience methods - Add offline fixtures: 429 failover, wrong-chain rejection, backoff sequence, endpoint exhaustion - Backward-compatible resolve() returns first endpoint as before
Claim the funded bounty before continuingThis PR is reviewable, but it does not reserve or earn the linked bounty.
Follow the returned wallet request. Continue only after canonical |
NSPG13
left a comment
There was a problem hiding this comment.
Thanks for taking on the RPC reliability problem. The ordered configuration and deterministic backoff helpers are useful foundations, and thank you for helping the community grow. I’m sorry the linked #869 claim returned CanonicalStateUnavailable; that routing problem is on the platform side and does not count against the contribution.
Decision: request-changes / manual-security-review.
What passed:
- The config can represent multiple endpoints.
- Chain ID is checked before a request endpoint is selected.
- Retry limits and backoff are bounded.
What blocks main:
discover_valid_endpoint()selects one chain-valid endpoint, thenpost_json_value()retries only that same endpoint. A 429/5xx/transport failure never advances to the next configured endpoint, so the main failover acceptance criterion is not implemented.- The PR does not migrate the inventory and routed-activation read checks to this transport.
crates/chain-base/tests/failover_fixtures.rsusesuse super::*in an integration-test crate without importingchain_base, and the tests only exercise helpers; they do not simulate 429 failover, wrong-chain rejection, preserved JSON-RPC errors, or exhaustion.- Configured endpoints are not explicitly validated as credential-free HTTPS URLs.
Repair path:
- Loop over endpoints outside the per-endpoint retry loop: validate HTTPS + chain 8453, retry only transport/429/5xx, then advance to the next endpoint.
- Return confirmed JSON-RPC execution errors immediately and preserve their exact error payload.
- Inject a fake transport and clock so offline tests prove request order, retry count, endpoint advance, wrong-chain skip, and exhaustion without sleeping or using live RPC.
- Import the library crate correctly in the integration test.
- Migrate the two required production read paths.
- Run
cargo test -p chain-base --test failover_fixtures,cargo test -p chain-base, and the immutable #869 benchmark.
Done when: a 429 on endpoint A produces a bounded retry and then a successful request on endpoint B, while a JSON-RPC execution error produces no retry.
Next owner: the contributor should update this same PR; maintainer should re-run line-by-line security review. Do not create a collaboration branch for chain/runtime code.
This review does not approve bounty acceptance, payout, or settlement.
|
Status update for #869: the separate canonical submission on PR #890 timed out and was permissionlessly expired on Base. Its solver bond was returned and the bounty is claimable again; no submission was accepted or paid. Your PR was not the canonical round-1 submission and its existing requested-changes review remains the repair path. Please do not claim until the implementation and exact clean-snapshot benchmark are ready. The verifier incident and new pre-submit preflight work are tracked in #905. |
NSPG13
left a comment
There was a problem hiding this comment.
Finding: Bounded intake passes, but this latest head still does not satisfy issue #869’s production scope. It adds a separate Rust transport in crates/chain-base without migrating scripts/bounty_inventory_guard.py or scripts/check_routed_v3_activation_readiness.py, the two maintainer read paths named by the acceptance criteria. The latest ull-check fails immediately at cargo fmt --all -- --check in crates/chain-base/src/lib.rs and the fixture files.\n\nImpact: The maintainer workflows remain on their old transport, and the branch cannot pass the required main gate.\n\nAction: Please coordinate with #890 to avoid two incompatible shared transports. If continuing this PR, migrate the named production read checks to one shared implementation, add offline caller-level tests for 429, wrong chain, execution-error preservation, and exhaustion, then apply cargo fmt. Next owner: @laurentketterle-hub. Recheck: cargo fmt --all -- --check && cargo test -p chain-base --test failover_fixtures.\n\nDone when: the actual inventory and routed-activation readers use the reviewed failover path, formatting is clean, the immutable benchmark passes, and all three required checks are green on the latest head.\n\nSorry this overlap created extra work, and thank you for helping the community and project grow. Review/CI is separate from bounty acceptance and payment settlement.
- Add Testing Strategy section covering offline failover fixtures and shared integration fixtures for the retry-safe Base RPC transport
chore: refresh mergeability state (empty commit)
chore: refresh mergeability
chore: refresh mergeability
chore: refresh mergeability
chore: refresh mergeability
chore: refresh mergeability
chore: refresh mergeability
…on shared transport Address the NSPG13#869 requested-changes repair path: - FailoverJsonRpcTransport now loops over every configured endpoint instead of retrying a single chain-valid endpoint. Each endpoint is chain-ID validated (8453) before use; bounded transport/429/5xx failures are retried with deterministic backoff and then advance to the next endpoint. Confirmed JSON-RPC execution errors return immediately with their exact payload. - Make the transport generic over its inner JsonRpcTransport so offline tests can inject a scripted fake (with_transport). Keep new() on the reqwest default. - Rewrite tests/failover_fixtures.rs: fix the integration-test import (use super::* -> use chain_base::*), expose is_retriable_http_status, and add offline fixtures proving 429 failover, wrong-chain skip, execution-error preservation, and endpoint exhaustion. - scripts/_shared/rpc.py gains FailoverJsonRpcTransport + RpcExecutionError / RetriableRpcError / ChainIdMismatch, mirroring the Rust transport semantics (credential-free HTTPS validation, chain-ID check, bounded retry, failover). - scripts/test_failover_rpc.py adds offline characterization tests (5 cases). - cargo fmt applied to both Rust files. Signed-off-by: laurentketterle-hub <noreply@users.noreply.github.com>
|
Ciao @NSPG13 — addressed the second review on this PR:
Recheck: Grazie. |
…e fns (E0283) The status classifier and error classifier do not depend on the transport type, so keeping them as associated functions on FailoverJsonRpcTransport<T> forced callers to supply a concrete T (E0283 on the fixture's FailoverJsonRpcTransport::is_retriable_http_status(429) calls). Hoist them to module-level free functions and update call sites. Verified: cargo check -p chain-base --tests, cargo fmt --all -- --check, and cargo test -p chain-base --test failover_fixtures (10 passed). Signed-off-by: laurentketterle-hub <noreply@users.noreply.github.com>
|
Follow-up: the first push had an Verified locally:
Recheck command is green on this head. |
NSPG13
left a comment
There was a problem hiding this comment.
Thank you for implementing real endpoint advance and injectable offline Rust tests. On exact head 2fc87e61, trusted docs-contract intake passes and all five Python failover tests pass. Sorry the workspace-level breakage was not visible in the narrow local command, and thank you for helping the community and project grow.
Finding
- Required
full-checkandpostgres-syncfail on this exact head. ChangingBaseRpcUrlConfigfields fromOption<String>toVec<String>leaves API tests constructing the old type (E0308), and clippy reports the now-unusednon_empty_env. - The acceptance-required inventory and routed-activation readers are still not migrated to the failover transport; the PR only adds the transport/shared module.
- Python
FailoverJsonRpcTransport.postcatches everyRuntimeError. A deterministic probe of HTTP 400 retried twice on endpoint A and twice on endpoint B, although only transport failures, 429, and 5xx may retry. - Rust accepts endpoint strings without enforcing credential-free HTTPS, despite that boundary being enforced in Python and required by the prior review.
Impact
The workspace does not compile under required gates, production maintainer readers still have no failover, and non-retriable client errors can be replayed across providers.
Action
- Migrate every
BaseRpcUrlConfigconsumer/test (or preserve a compatible constructor) and remove the dead helper. - Wire
scripts/bounty_inventory_guard.pyandscripts/check_routed_v3_activation_readiness.pyto the reviewed ordered transport. - Give Python explicit retriable exception classes; propagate HTTP 4xx other than 429 immediately.
- Apply the same credential-free HTTPS validation in Rust, without logging full credential-bearing URLs.
- Add caller-level offline tests for both named readers.
Done when
A 429 advances after bounded retries, HTTP 400 and JSON-RPC execution errors make one request only, the named production readers use the shared path, and all three required checks pass.
Next owner: @laurentketterle-hub. Recheck: cargo clippy --workspace -- -D warnings && cargo test -p chain-base --test failover_fixtures && cargo test -p api && python scripts/test_failover_rpc.py -v && python benchmarks/direct-inventory-v1/rpc-failover/check.py. This review is not bounty acceptance or payment evidence; only canonical BountySettled proves payment.
…op dead non_empty_env - crates/api/src/main.rs: test_state_with_base_rpc now builds base_sepolia/base_mainnet as Vec<String> (E0308 fix) - crates/chain-base/src/lib.rs: remove unused non_empty_env helper and its now-dead std::env import (clippy -D warnings) Signed-off-by: laurentketterle-hub <laurentketterle-hub@users.noreply.github.com>
|
@NSPG13 advanced the head to |
#869: Retry-safe Base RPC failover for maintainer workflows
Changes
Acceptance Criteria
Closes #869