feat(sdk): reject response metadata from an unexpected chain id

[PastaPastaPasta]

Issue being fixed or feature implemented

rs-sdk never looks at metadata.chain_id. The chain id is covered by the quorum signature, but the quorums that sign it can be the same on two Platform chains, for example before and after a testnet reset. So a node still serving the old chain can return a validly signed proof that the SDK accepts. That response then advances the SDK's height high-water mark and can ratchet the protocol version, and every later response from the real chain looks stale.

The context provider never sees the chain id, so an embedder cannot do this check before the SDK's own state changes. It has to live in rs-sdk.

This came out of the Dash Core work in dashpay/dash#7512, where dash-qt embeds dash-sdk through the dash-platform-cxx crate and currently compares the chain id itself after the fact. The check is just as useful to every other SDK consumer (mobile, wasm, CLI tools) that knows which chain it means to talk to.

What was done?

• SdkBuilder::with_expected_chain_id(chain_id). If it is not called, nothing changes. An empty string is rejected by build() with Error::Config.
• verify_response_metadata compares metadata.chain_id first, before the time and height checks and before the protocol-version ratchet. A mismatch returns the new StaleNodeError::ChainIdMismatch { expected, received }, and no SDK state moves. Both callers (the proof path and the broadcast wait path) reach it only after the signature has been verified.
• A mismatch does not mean the node is unhealthy, and the configured chain id may be the outdated one. So can_retry() is false for this variant, and sync::retry fails over to another node using the short flat exclusion (2 s) it already uses for DPNS rejections, instead of the exponential health-ban ladder. Without that, a misconfigured SDK would ban every node and then return a bare NoAvailableAddresses; with it, every call keeps returning ChainIdMismatch. The debug log on that path now says "failover stopped" instead of "DPNS failover stopped", since it is no longer DPNS-only.
• The mock SDK's expect_fetch responses report the expected chain id, so mock SDKs built with with_expected_chain_id keep working.

How Has This Been Tested?

New tests:

• sdk.rs chain_id_mismatch_is_rejected_before_any_state_advances: the error carries both ids, can_retry() is false, and the protocol version and height high-water mark are unchanged.
• sdk.rs matching_chain_id_is_accepted, chain_id_is_not_checked_when_not_configured.
• sdk.rs chain_id_is_checked_before_time: time tolerance set, both chain id and time wrong, result is ChainIdMismatch.
• sdk.rs empty_expected_chain_id_is_rejected.
• sync.rs test_retry_chain_id_mismatch_does_not_ban: two addresses both answer with a mismatch and retry is called twice. Both calls return ChainIdMismatch directly (the first reaches 2 nodes, the second 1 while the first is still excluded), and no address is banned past now + 2s.
• tests/fetch/mock_fetch.rs test_mock_fetch_identity_with_expected_chain_id.

Runs:

• cargo test -p dash-sdk: lib 218 passed; fetch suite 111 passed / 39 ignored; all other suites pass, 0 failures.
• cargo clippy -p dash-sdk --all-targets --features mocks -- -D warnings, cargo fmt --all -- --check
• cargo check -p dash-sdk --no-default-features, cargo check -p rs-sdk-ffi, cargo check -p wasm-sdk --target wasm32-unknown-unknown, cargo doc -p dash-sdk --no-deps

Breaking Changes

No behaviour changes unless with_expected_chain_id is called.

StaleNodeError is not #[non_exhaustive], so the new ChainIdMismatch variant breaks downstream code that matches on StaleNodeError exhaustively without a wildcard arm. Nothing in this repository does (wasm-sdk matches Error::StaleNode(e) as a whole). StaleNodeError::Epoch was added the same way in #4231 without a !, so I have not marked the title; happy to add ! if reviewers prefer.

Unlike the other StaleNodeError variants, ChainIdMismatch is not retryable, so wasm-sdk reports it with retriable = false.

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
• I have made corresponding changes to the documentation if needed
• If I added or changed GroveDB structure, I described it in the area's structure.rs, regenerated grovedb-structure.json, and checked the structure viewer link posted on this pull request

For repository code-owners and collaborators only

• I have assigned this pull request to a milestone

🤖 Generated with Claude Code

PR Hygiene · 1f0cb59

• Bots — coderabbitai ✓ · thepastaclaw ✓
• Self-review — post /self-reviewed
• Within your 5 open PRs — this one is beyond the limit; it waits until one merges
• Build failed
• Approvals
  • rust-sdk (packages/rs-sdk/src/error.rs, packages/rs-sdk/src/mock/sdk.rs, packages/rs-sdk/src/platform/transition/broadcast.rs and 3 more) — lklimek or shumkov

When every box is checked the PR Hygiene check passes and this can merge.

Summary by CodeRabbit

• New Features
  • SDKs can be configured with an expected chain ID. Responses from a different chain are rejected before they affect freshness or protocol-version state.
• Bug Fixes
  • Chain ID mismatches can trigger failover to another available node. The responding node is temporarily excluded from failover without receiving a health-based ban. If no alternative node is available, the mismatch is returned.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: dashpay/platform/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 49466eca-5018-4493-83d6-ca14563f1504

📥 Commits

Reviewing files that changed from the base of the PR and between a0e44d6 and 1f0cb59.

📒 Files selected for processing (4)

• packages/rs-sdk/src/error.rs
• packages/rs-sdk/src/platform/transition/broadcast.rs
• packages/rs-sdk/src/sdk.rs
• packages/rs-sdk/src/sync.rs

🚧 Files skipped from review as they are similar to previous changes (3)

• packages/rs-sdk/src/error.rs
• packages/rs-sdk/src/sdk.rs
• packages/rs-sdk/src/sync.rs

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The SDK can be configured with an expected chain ID and rejects response metadata with a different ID. Chain-ID mismatches enter the failover path with a short flat exclusion instead of the exponential health-ban ladder. Mock proof metadata and tests cover the configured ID.

Changes

Chain ID validation and failover

Layer / File(s)	Summary
Expected chain ID configuration and validation packages/rs-sdk/src/sdk.rs, packages/rs-sdk/src/mock/sdk.rs, packages/rs-sdk/tests/fetch/mock_fetch.rs	Adds optional expected chain ID configuration, rejects empty configured IDs, and checks response metadata before freshness checks or protocol-version updates. Mock proof metadata uses the configured ID when available. Tests cover matching, mismatching, and unset IDs, validation ordering, and mock fetching.
Mismatch failover with short exclusion packages/rs-sdk/src/sync.rs, packages/rs-sdk/src/error.rs, packages/rs-sdk/src/platform/transition/broadcast.rs	Routes chain-ID mismatches through failover and tests the short exclusion window. Updates comments to describe the exclusion behavior; the mismatch error declaration and Error::can_retry behavior remain unchanged.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant Sdk
  participant SyncRetry
  participant NodeA
  participant NodeB
  Sdk->>NodeA: request with expected chain ID
  NodeA-->>Sdk: response metadata with mismatched chain ID
  Sdk-->>SyncRetry: ChainIdMismatch
  SyncRetry->>NodeA: exclude for up to two seconds
  SyncRetry->>NodeB: retry request

Loading

Suggested reviewers: lklimek

Merge Risk: ⚪ Minimal · up to 1f0cb

Configured clients reject responses from a different chain, and mismatches can fail over briefly. No actionable merge risk is evident in the reviewed change.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: the SDK rejects response metadata with an unexpected chain ID.
Docstring Coverage	✅ Passed	Docstring coverage is 80.77% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 26 functions across 6 files.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[thepastaclaw]

✅ Final review complete — no blockers (commit 1f0cb59) · triage: normal

[thepastaclaw]

Final validation — Phase 1 + Phase 2

Verified the supplied findings against head a0e44d6. The chain-ID check precedes SDK freshness and protocol-version updates, and mismatch failover uses the intended short exclusion; no blocking defect was confirmed. One consolidated documentation suggestion remains; independent validation was source inspection and a passing diff whitespace check, not a rerun of the reported tests.

🟡 1 suggestion(s)

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: ffi-engineer); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)

• Triage: normal by gpt-6-astra (effort low) — The diff adds opt-in metadata validation with state-update ordering, retry/failover semantics, and a public error variant requiring ordinary logic review, but does not make large or intricate changes to consensus, cryptography, signature verification, or another qualifying critical surface.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — architecture-layering (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — ffi-engineer (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — security-auditor (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 1% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• Fresh verifier: gpt-6-astra — final-verifier; agent astra-verifier
• Phase 2 reviewers: gpt-6-astra — general (completed, effort high); agent phase2-reviewer, gpt-6-astra — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6-astra — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6-astra — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6-astra — security-auditor (completed, effort high); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-sdk/src/sdk.rs`:
- [SUGGESTION] packages/rs-sdk/src/sdk.rs:1198-1200: Describe mismatch failover as a short exclusion, not absence of banning
  The builder documentation promises failover without banning the responding server, but `sync::retry_with_additional_error` calls `address_list.ban_for(Duration::from_secs(2), ...)` when an alternative is available and banning is enabled. That updates `banned_until` and floors `ban_count` at 1, so the exclusion is observable through ban information; it avoids the exponential health-ban path rather than bypassing ban state entirely. Describe that distinction here and in `StaleNodeError::ChainIdMismatch` documentation at error.rs:408–410. Also update broadcast.rs:536–538 to describe failover rather than claiming every `StaleNode` error is retryable: this new variant has `can_retry() == false` and reaches failover through the additional-error predicate. The implementation matches the PR's stated short-exclusion design; this is a documentation correction.

Out-of-scope follow-up suggestions (1)

These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.

• Expose expected chain id to wasm and FFI builders — The absent binding option is real, but public WASM and native configuration extensions are outside the stated Rust SDK implementation scope. No existing binding behavior regresses, so this routine API follow-up is omitted.
  • Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.

[github-actions]

Your move: thepastaclaw left review threads unresolved; resolve them.
Full checklist in the description.

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

Verified the complete PR diff at 50156bc. The chain-ID guard precedes SDK state updates, and mismatch failover uses the existing bounded short-exclusion path; the prior public-documentation finding is fixed, with one non-blocking wording nit remaining in the tests. Source inspection and git diff --check completed successfully; tests were not independently rerun, and the worktree remains unchanged.

💬 1 nitpick(s)

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: ffi-engineer); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 11: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 12: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 13: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 14: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 15: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)

• Triage: normal by gpt-6-astra (effort low) — The change coordinates opt-in chain-ID validation, SDK state-update ordering, retry exclusions, and mock behavior across several files, but does not introduce large or intricate changes to signature verification or another critical surface.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — architecture-layering (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — ffi-engineer (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — security-auditor (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 1% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6-astra — final-verifier; agent astra-verifier
• Phase 2 reviewers: gpt-6-astra — general (completed, effort high); agent phase2-reviewer, gpt-6-astra — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6-astra — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6-astra — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6-astra — security-auditor (completed, effort high); agent phase2-reviewer, gpt-6-astra — general (completed, effort high); agent phase2-reviewer, gpt-6-astra — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6-astra — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6-astra — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6-astra — security-auditor (completed, effort high); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-sdk/src/sync.rs`:
- [NITPICK] packages/rs-sdk/src/sync.rs:350-353: Describe the tests as avoiding health bans, not all banning
  The test comment and name say that neither node is banned, but mismatch failover calls ban_for with a two-second duration when an alternative is available and banning is enabled. That updates banned_until and floors ban_count at 1; the test correctly permits this short exclusion. Rename the test and describe the absence of exponential health banning rather than all ban state. The same distinction applies to the assertion message at sdk.rs:1731: !error.can_retry() prevents the health-ban path, not the separate short exclusion. These are test-wording corrections only; the assertions and production behavior match the intended design.

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

Independently reviewed the complete diff at 1f0cb59 and found no actionable in-scope defects: the configured chain-ID check precedes freshness and protocol-version mutations, and mismatch failover uses the bounded short-exclusion path. Both prior documentation findings are fixed. Source inspection and git diff --check passed; tests were not rerun during this verification.

🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: ffi-engineer); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 11: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 12: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 13: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 14: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 15: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)

• Triage: normal by gpt-6-astra (effort low) — The diff adds opt-in chain-ID validation, SDK state-update guards, and retry/failover handling across several paths, requiring ordinary logic review but not changing cryptographic verification or another critical surface with sufficient size or intricacy to qualify as critical.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — architecture-layering (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — ffi-engineer (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — security-auditor (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 1% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6-astra — final-verifier; agent astra-verifier
• Phase 2 reviewers: gpt-6-astra — general (completed, effort high); agent phase2-reviewer, gpt-6-astra — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6-astra — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6-astra — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6-astra — security-auditor (completed, effort high); agent phase2-reviewer, gpt-6-astra — general (completed, effort high); agent phase2-reviewer, gpt-6-astra — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6-astra — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6-astra — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6-astra — security-auditor (completed, effort high); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.

No unresolved findings remain from the prior review on this head.

Out-of-scope follow-up suggestions (1)

These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.

• Expose expected-chain configuration through language bindings — Duplicate adjacent-feature observation: adding configuration to WASM, JavaScript, and native bindings is explicitly outside this PR's scope. The absence of these additions does not regress existing callers or prevent Rust consumers from using the implemented metadata gate.
  • Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.

[github-actions]

Bots are done — your move: post /self-reviewed.
Full checklist in the description.

[PastaPastaPasta]

/self-reviewed

---

🤖 Posted autonomously by Claude on behalf of pasta.

[PastaPastaPasta]

Closing this. On review, it does not defend against any attack, and the one operational case it covers is not worth a breaking change to StaleNodeError plus a change to the failover path in sync::retry.

Why it adds no security:

• Forging a response with a different chain id: not possible. chain_id is already part of the message the quorum signs (verify_tenderdash_signature puts it into both StateId::calculate_msg_hash and CanonicalVote), so changing it breaks the signature.
• Having a real quorum sign a different chain: this needs a threshold of that quorum's key shares. Anyone who has those can sign blocks with the correct chain id too, so this check doesn't stop them.
• Replaying responses from another network (mainnet to testnet, or devnet/regtest): the quorum isn't in that network's Core chain, so the context provider already rejects it.
• Mainnet: mainnet quorums have never signed any Platform chain other than evo1, so there is nothing to replay.

What's left is a case that only happens on testnet or devnet. After a Platform reset where Core keeps running, a node still serving the old chain can return a validly signed proof. If the context provider still has the old quorum and the time check is off, that proof advances the in-memory height high-water mark, and then responses from the new chain look stale until the process restarts. Setting with_time_tolerance already catches a stopped old chain in practice. Embedders that need a strict check can compare metadata.chain_id themselves after the call returns, and the dash-qt Platform GUI (dashpay/dash#7512) already does that in its shell, before its own height watermark.

The branch feat/sdk-expected-chain-id stays on the repo in case this is ever wanted.

---

🤖 Posted autonomously by Claude on behalf of pasta.