Skip to content

fix(sync): return error instead of panicking on get_account proof with no details - #2502

Open
erhnysr wants to merge 3 commits into
0xMiden:nextfrom
erhnysr:fix/2386-get-account-missing-details-panic
Open

fix(sync): return error instead of panicking on get_account proof with no details#2502
erhnysr wants to merge 3 commits into
0xMiden:nextfrom
erhnysr:fix/2386-get-account-missing-details-panic

Conversation

@erhnysr

@erhnysr erhnysr commented Sep 5, 2026

Copy link
Copy Markdown

Summary

StateSync::validate_account_proof unwrapped the optional account details from a get_account proof with .expect("node returned no details for a public account"), treating their presence as an invariant. But AccountProof allows details: None end-to-end — the RPC conversion layer in rpc/domain/account.rs produces None when the response omits them — so a malformed, buggy, or malicious node response crashes the client with a panic mid-sync instead of surfacing a validation error.

This is the same class of untrusted-input hardening as the recent VerifyingRpcClient fixes (#2419, #2381): a value the node controls was trusted without being checked.

The bug

crates/rust-client/src/sync/state_sync.rs

Ok(details.expect("node returned no details for a public account"))

Reachable during public account sync: the proof reaches validate_account_proof, passes the block / account-id / witness checks, then panics on the final .expect() if details is None.

The fix

Return ClientError::ChainValidationError when details are absent, consistent with the function's other validation failures:

details.ok_or_else(|| {
    ClientError::ChainValidationError(format!(
        "get_account returned no details for public account {account_id}"
    ))
})

The # Panics doc section is replaced with a fourth # Errors bullet, and a regression test (validate_account_proof_rejects_missing_details) builds an otherwise-honest proof with the details stripped and asserts ChainValidationError.

Verification

  • make lint — clean (cargo fix, cargo +nightly fmt, taplo fmt, cargo clippy --workspace --features "testing std" --all-targets -- -D warnings, cargo shear all pass, no warnings)
  • make test458 tests run: 458 passed, 2 skipped; the new regression test passes

Closes #2386

erhnysr added a commit to erhnysr/rust-sdk that referenced this pull request Sep 5, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0678b19849

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread CHANGELOG.md Outdated
* [FIX][rust] The RPC retry policy is now endpoint-aware: `SubmitProvenTransaction` and `SubmitProvenBatch` retry only `ResourceExhausted` and let `Unavailable` propagate, while read endpoints keep retrying both. `Unavailable` does not say whether the node processed the request, so resubmitting could hit the nullifier consumed by an accepted copy and report a conflict indistinguishable from a genuine double spend, hiding the original success ([#2441](https://github.com/0xMiden/rust-sdk/issues/2441)).
* [FIX][cli] `-V`/`--version` now work when the binary is invoked under a different name, such as through the `miden client` shim installed by midenup ([#2486] https://github.com/0xMiden/rust-sdk/pull/2486)).
<!-- TODO: update the PR link below to the actual PR number once the PR is opened (currently a placeholder). -->
* [FIX][rust] `StateSync::validate_account_proof` now returns a `ChainValidationError` when a `get_account` proof carries no account details, instead of panicking. The RPC layer allows the details to be absent, so a malformed or malicious node response could crash the client mid-sync ([#XXXX](https://github.com/0xMiden/rust-sdk/pull/XXXX)).

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Replace the placeholder changelog PR link

This checked-in Unreleased entry still links to #XXXX and is preceded by an explicit TODO, so any release notes generated before a manual follow-up will publish an invalid reference rather than the change's PR. Replace the placeholder with the actual PR number before merging.

Useful? React with 👍 / 👎.

@igamigo igamigo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Leaving some comments. Also, can you resolve conflicts?

Comment thread crates/rust-client/src/sync/state_sync.rs
Comment thread CHANGELOG.md Outdated
…oof with no details

StateSync::validate_account_proof unwrapped the optional account details
with expect(), treating their presence as an invariant. The RPC conversion
layer allows details to be None end-to-end, so a malformed or malicious
node response could crash the client mid-sync instead of surfacing a
validation error.

Return ClientError::ChainValidationError when details are absent, matching
the function's other validation failures.

Closes 0xMiden#2386
@erhnysr
erhnysr force-pushed the fix/2386-get-account-missing-details-panic branch from 7666cd9 to 888100d Compare September 8, 2026 18:26
@erhnysr

erhnysr commented Sep 9, 2026

Copy link
Copy Markdown
Author

Hi @igamigo — Integration tests failed on network_fpi (bin/integration-tests/src/tests/network_fpi.rs:143, the counter assertion), but this looks like a pre-existing flake unrelated to this PR: the network transaction did not increment the counter within wait_for_blocks(2) (left: Word([0,0,0,0]) vs right: Word([1,0,0,0])). The same test failed identically on next in run #34228789999 (the push for #2479, which does not touch state_sync.rs). Unit tests and doc tests are green.

Could you re-run the Integration tests job when you get a chance? (Fork PR workflows need maintainer approval to re-run.)

Also addressed both your review comments in the latest commits (888100d) — replying on those threads separately.

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.

bug(rust-client): panic when get_account proof omits public account details

2 participants