fix(rpc): verify block window on range-scoped VerifyingRpcClient responses - #2503
Open
erhnysr wants to merge 3 commits into
Open
fix(rpc): verify block window on range-scoped VerifyingRpcClient responses#2503erhnysr wants to merge 3 commits into
erhnysr wants to merge 3 commits into
Conversation
erhnysr
added a commit
to erhnysr/rust-sdk
that referenced
this pull request
Sep 5, 2026
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 053c756462
ℹ️ 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".
…onses VerifyingRpcClient checked returned note IDs, tags, nullifier prefixes, account IDs and script roots against the request, but none of the five methods scoped to a block_from/block_to window ever checked that the response fell inside it. The window was forwarded to the inner client and never mentioned again, so a node could answer a query about one block range with data stamped in another - a mis-stamped transaction record, for instance, commits a locally tracked transaction at whatever height the response carried. Add a verify_block_range helper next to the existing verify_* checks and call it from sync_notes, sync_nullifiers, sync_storage_maps, sync_account_vault and sync_transactions, rejecting out-of-window data with RpcError::InvalidResponse. Closes 0xMiden#2454
… on build_patch_update The VerifyingRpcClient wrapper range-checks only the pagination cursor of the sync_storage_maps and sync_account_vault responses, not the height of each individual update. Document that the store closes this gap by verifying the applied patch against the authenticated header, and record the caller obligation to authenticate details.header and pass its block as block_to. Claude-Session: https://claude.ai/code/session_013wTu7UVnJ4fzkixZecXLkV
erhnysr
force-pushed
the
fix/2454-verify-block-window-range
branch
from
September 14, 2026 10:09
022e089 to
11ddd65
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
VerifyingRpcClientexists to reject responses that don't answer the request that was made. It does this thoroughly for one half of the request: returned note IDs, note tags, nullifier prefixes, account IDs and script roots are all checked against what was asked for.The other half was missing. Five of its methods take a
block_from/block_towindow and none of them ever checked whether the response fell inside it — both parameters were forwarded to the inner client and then never mentioned again:sync_notes— checked tags, not heightssync_nullifiers— checked prefixes, not heightssync_storage_maps— delegated, no checksync_account_vault— delegated, no checksync_transactions— checked account IDs, not heightsSo a node could answer a query about blocks 100–200 with data stamped block 5000, and the wrapper whose whole purpose is catching that passed it straight through. The impact is concrete for
sync_transactions:record.block_numflows unchecked intocommit_transaction(record.block_num, …)(sync/state_sync_update.rs), committing a locally tracked transaction at whatever height the response carried, including one past the chain tip the client just read.This is the same class as the recent
VerifyingRpcClientfixes (#2419, #2381): a value the node controls, trusted without being checked.The fix
One helper next to the existing
verify_*checks:called from all five methods, rejecting out-of-window data with
RpcError::InvalidResponse— consistent with every other check in the file. Notes and nullifiers carry a height per item; the storage-map and vault responses carry a single pagination cursor (block_number), so those pass one value.Note on the storage-map / vault cursor
For
sync_storage_maps/sync_account_vaultthe checked value is the pagination cursor, not per-item heights. The tonic client'sBlockPaginationalready guarantees the cursor is>= block_fromand respectsblock_to, so the inclusive[block_from, block_to]check does not falsely reject honest responses, while still catching an out-of-range cursor from a misbehaving node.Verification
make lint— clean (cargo fix,cargo +nightly fmt,taplo fmt,cargo clippy --workspace --features "testing std" --all-targets -- -D warnings,cargo shear), no warningsmake test— 462 tests run: 462 passed, 2 skippedFive new tests (
sync_{notes,nullifiers,storage_maps,account_vault,transactions}_verifies_block_range) cover inside-window accept and out-of-window reject.Closes #2454