Repository navigation
test(drive-abci): verify query errors over gRPC and original proofs - #5343
Conversation
About twenty query endpoints answered a request with an empty id list or a limit of 0 with gRPC UNKNOWN or INTERNAL. Clients take those for node faults, ban the node and send the request to the next one, so a few such calls banned every node. Each handler now refuses those requests itself, with INVALID_ARGUMENT, in both proof modes: - empty id lists: data contracts, identities balances, identities contract keys (ids or purposes), identity keys (specific keys or search purposes), evonode blocks by ids, identity and identities token balances and infos, token statuses, addresses infos; - a limit or count of 0: identity keys, evonode blocks by range, contract history (and above its maximum of 10), protocol upgrade vote status, epochs info, identity votes, pre-programmed distributions, group infos, group actions; - an addresses branch depth outside the allowed range, and an unproved key search without a limit. Handlers that rejected a limit by returning a Drive query error through `?` now return a validation error, and the service answers any Drive query syntax error with INVALID_ARGUMENT. A composite page with `$id IN` over 100 values, or over values that are not identifiers, is refused like the plain query instead of reporting corrupted code execution. Status messages are cut at 1024 bytes, so an error echoing an oversized document type name no longer exceeds HTTP/2 header limits. Queries are not consensus: no block execution path changes and proof contents are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ific keys Refuse only what fails today, as #5331 does: an empty id list, an identity keys limit of 0 and an epochs count of 0 are refused only when a proof is asked, since GroveDB cannot prove an empty query. Without a proof they keep their empty answer. Empty specific key ids and an empty key search, which both proof modes already answer, are no longer refused. An unproved specific-keys request whose limit is below its number of ids failed inside GroveDB with an internal error. The handler now fetches every requested id and keeps the first `limit` keys that exist, in id order: the keys the proof with that limit proves. Ported from #5331: an end-to-end test through the real `QueryService` and `DapiClient`, which checks every changed request in both proof modes, that three empty contract proof requests leave all 13 nodes available, and that a node fault still bans the node; and a book section on how query errors reach clients. The test calls the service in-process, so `rs-dapi-client` is its only new dev-dependency. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The note about dropped bytes is appended after the 1024 kept bytes, so the constant bounds the prefix, not the whole message. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Consolidate the real tonic DuplexStream/DapiClient controls from PR5331 into PR5333 without restoring the old service guards. Check successful proof variants against original requests and stored roots, preserve empty document/shielded defaults, and compare populated SpecificKeys payloads in encoded-key order instead of sorting numeric IDs. Exercise historical composite by-ID pages, count/profile joins and same-type lookup proof decoding at PV12/PV13 with fully validated version-supported fixtures; retain latest reference joins. Correct the query API compatibility and status-prefix documentation. Test would have caught these in CI: ✖ numeric-order checker at IDs 200/300 cap2, missing Proof response, wrong key bytes under unchanged ID0, and unbounded oversized gRPC messages; ✔ after the test repairs and restoring the bound. The pinned tonic/h2 stack reports ResourceExhausted for oversized headers, not Internal. All temporary mutations restored byte-for-byte; no consensus or SDK production behavior changes.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe changes update unproved specific-key result selection and expand composite-query coverage across protocol versions. They also move query request, proof, and transport tests to in-memory gRPC connections and revise guidance on query statuses, refusals, and message truncation. ChangesQuery handling
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Other · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change adds and strengthens tests for how queries report errors and proofs over real gRPC transport, and updates the related documentation. It does not change production query handling, so it carries no merge-blocking risk for users. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-10-08T18:01:00.572Z |
|
✅ Final review complete — no blockers (commit da55fa3) · triage: normal |
Document the current client handling of older-node refusal messages and empty proved selections. Explain that the raw transport test adapter leaves names_nothing at its default so the test reaches server classification. No runtime behavior changes; red-to-green testing is omitted for these documentation and comment-only clarifications. The transport and query coverage is verified separately on the final head.
|
Waiting for bot review — coderabbitai ✓ · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the supplied findings against exact head 41dca7b and the complete PR range. Changes are limited to tests, dev-dependencies, and documentation/comments; no blocking issue was confirmed, but the new successful batch-contract control still discards its response payload. This was a static review: the supplied exact-head CI snapshot reports successful Rust workspace tests, while PR Hygiene remains pending; no local builds or tests were run.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gemini-3.8-flash-high (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The large, intricate diff adds transport and proof regression tests, test dependencies, and documentation without changing production behavior or a critical surface. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— rust-quality (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 92% left, 5h 54% left - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-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-drive-abci/tests/query_request_errors.rs`:
- [SUGGESTION] packages/rs-drive-abci/tests/query_request_errors.rs:833-846: Assert the successful batch-contract response instead of only its status
The new nonempty batch control runs in both proof modes, but `record` converts every successful response to `Code::Ok` and discards the payload. Its proved iteration therefore remains green if the delivered response has no result, contains the unproved `DataContracts` variant, or carries an invalid proof. This leaves a successful-response blind spot within the PR's stated proof-regression goal. Capture the response, assert its V0 result variant matches `prove`, and verify the proved response with `Drive::verify_contracts` using the original requested ID `[1; 32]` and the fixture's stored root. The empty fixture can remain: assert the requested contract is absent in the verified result and the unproved response is empty.
|
The docstring-coverage warning concerns private test fixtures/harness helpers and comment-only changes to existing query functions; this follow-up introduces no public runtime API. Descriptive test names and the proof/transport/preflight comments explain the behavior being checked, so this advisory does not require a code change. The production SpecificKeys ordering behavior was already merged in #5333; this PR verifies and documents it. |
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
Require the per-proof response variants for the successful batch-contract, history, AllKeys, epoch and root-path controls. Verify the original requests with Drive and compare authenticated roots and actual fixture payloads, including the single absent contract entry. Harness sensitivity: the old status-only control passed a delivered missing-result mutation; the strengthened test compiled and failed at runtime for both that mutation and an empty proof under the correct tag (✖). Exact byte restoration then passed all eight transport tests (✔). All-features/all-targets clippy with -D warnings, targeted rustfmt and diff checks passed. No production or SDK logic changes.
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Independently reviewed the complete seven-file diff at da55fa3 and found no actionable in-scope defects. The changes are limited to regression tests, development dependencies, documentation and comments; the strengthened batch-contract control closes the prior response-verification gap. This is a static approval only: no builds or tests were run, and the supplied exact-head CI snapshot still has Rust validation queued and other checks pending.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gemini-3.8-flash-high (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The substantial Rust regression suite adds intricate real-gRPC transport and original-request proof checks, but the diff changes tests, documentation and development dependencies rather than production critical surfaces. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— rust-quality (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 88% left, 5h 30% left - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-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.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
|
/self-reviewed |
3 participants
Basic explanation
The query-error fix in #5333 needs tests that cross real gRPC, verify original requests against authenticated roots, and preserve response order. Its direct-service adapter missed HTTP/2 status failures, successful response checks could omit the required proof, and a numeric key map could hide encoded-key ordering.
This follow-up consolidates the real transport/proof/default controls from #5331 into the merged handler implementation. It adds test-only dependencies and corrects comments/book guidance. No production handler, SDK, consensus, storage or proof-format logic changes. Current base includes the independently merged client fix #5334 and the beta.3 release. This PR preserves both; it does not establish deployed-node behavior.
Risks: Low: this follow-up changes tests, documentation and development dependencies. The fixtures are deterministic and use in-memory transport.
The inherited pre-squash commits
994d5c7,457299aand6611c21were already merged through #5333's squashd28689f; they add no extra diff. The diff againstv5.0-devcontains only the seven test/documentation/dependency files.Issue being fixed or feature implemented
Make the Platform v5 query-error regressions catch wire-status loss, accidental node bans, missing/wrong proof responses, wrong key payload/order and historical query incompatibility. #5331 will be closed as superseded by merged #5333 plus this follow-up.
What was done?
DuplexStreamreplaces the synthetic direct-service node. The complete request matrix checks response variants as well as statuses; successful key proofs verify the original IDs/cap and stored root.DapiClientreach one node each and leave all 13 nodes available. The raw transport adapter deliberately retains defaultnames_nothing=None, so upstream client preflight cannot hide server responses; attempts/address assertions enforce this. GenuineINTERNAL/UNAVAILABLEnode faults still ban, fail over and unban a successful in-flight node.RESOURCE_EXHAUSTED/h2ENHANCE_YOUR_CALMin the pinned stack, not the previously claimedINTERNAL.indexOnly/refersTo/terminal/preallocation/ranked declarations. A unique quote lookup takes no historical cap and exercises overlapping page/sub-query proof decoding; latest retains its bounded non-unique lookup and reference join.OK->INVALID_ARGUMENT, depth 263 no longer wrapping to 7, and changed validation text/priority.Verified residuals outside this follow-up
Real transport probes still return
INTERNALfor SearchKey{purpose_map:{0:{security_level_map:{0:0,1:0}}},limit:2,offset:1}in both proof modes, and proved AllKeys/contract history/document history/identity votes withlimit:2,offset:1. Ordinary hashtag composite pages with cap 10 and supported count/profile joins also map toINTERNALat PV12/13 because per-instance limits are unsupported (public dispatcher/production-mapper probe; no transport claim).Two-terminal search cap 1 and AllKeysOfKind
{0:{1:1}}cap 10 returnOKin the seeded probe; this is status evidence, not payload/proof correctness. Empty nested search with cap 10 stays served. No rejection of served empty maps or tests locking inINTERNALship. Actual original-request SpecificKeys proofs accept missing IDs; no client absence-proof failure was reproduced. The offset and historical-cap limitations are tracked separately. The legacy-text non-retry rules in #5334 do not match those GroveDB errors, so source inspection indicates they remain retryable; this is not a measured node-ban result.In-place changes to shipped generations
Only test code and timeless comments change within query-generation files.
page_idsproduction code is untouched here. Matching-version tests cover its shared PV12/13/latest server validation and client proof-decoding callers. No new protocol generation, version table or block-execution behavior is introduced.How Has This Been Tested?
At signed head
da55fa317f53e9196dd79a8e0f5fea2413837330, on the beta.3/SDK5334 source base:cargo test -p drive-abci --test query_request_errors -j2 -- --test-threads=1: 8 passed over real tonic HTTP/2 transport. Every successful preservation control now checks its result variant, original request, authenticated stored root and actual fixture payload.cargo clippy -p drive-abci --all-features --all-targets -j2 -- -D warnings: passed. The existing dashcore manifest warning remains.rustfmt --checkandgit diff --check: passed.The unchanged library/proof source also has neighboring coverage at predecessor
41dca7b:cargo test -p drive-abci --lib query:: -j2 -- --test-threads=1passed 793, with 1 pre-existing ignored (test_documents_start_after_proof_secondary_index_in_query_2);cargo test -p drive --lib composite -j2 -- --test-threads=1passed 43;cargo check -p drive --no-default-features --features verify -j2passed. The query group includes all 12 composite tests, five SpecificKeys bounds tests and six service status tests.Independent Opus code review approved the exact signed head with no must-fixes. Current-head selected CI passed run 37820358242: 18,794 nextest passes, 1,381 skipped, including all eight real-transport controls, followed by 935 selected shielded Cargo passes, 3 ignored. CI tested this head merged into
bb78fa2; skipped, ignored and filtered-out tests are not claimed executed. Thepastaclaw approved the exact head with no findings. CodeRabbit completed actual review coverage of the exact head with no actionable comments; zero unresolved review threads remain. Its unchanged generic docstring advisory is covered by the scope assessment. Human self-review/admission is separate and unchecked.Additional success-control sensitivity: the old status-only contract-batch test passed a delivered missing-result mutation. The strengthened test compiled then failed for both a missing proved result and an empty proof under the correct tag; byte-exact restoration passed the full transport suite.
Runtime sensitivity checks compiled and failed as intended: the numeric-order checker at IDs 200/300 cap 2; delivered-result harness mutation removing Proof; wrong public-key bytes under unchanged ID 0; removal of status truncation for all three encodings. Sources were restored byte-for-byte, then the controls ran green. Fixture setup or compile errors are not regression-red evidence. All fixtures are deterministic and use no devnet.
Breaking Changes
None in this follow-up. The book explicitly describes the query API deltas already merged in #5333; this PR adds coverage and documentation only.
Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
PR Hygiene ·
da55fa3/self-reviewedWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.Summary by CodeRabbit