Repository navigation
Conversation
Reject empty proof selections and endpoint-specific zero caps at the public gRPC boundary, and preserve typed query syntax errors through Drive/ABCI wrappers as InvalidArgument. Keep existing client retries, node bans, valid empty queries, zero-as-default requests and query generations unchanged. Test would have caught this in CI: red before the fix, green after. Concrete tonic/DapiClient regressions failed 4 of 6 tests before the patch: three empty contract-proof calls banned 6, then 12, then all 13 nodes. The wrapped syntax mapper also failed before the patch. All 8 final integration tests pass; each empty call now makes one attempt, has zero retries and leaves all 13 nodes available. Genuine Internal/Unavailable faults still ban and fail over. Neighboring queries pass 772 tests with one existing ignored case; targeted all-feature/all-target clippy and scoped rustfmt pass. Keep the independent unproved SpecificKeys return-cap lowering defect out of scope; supported selection cardinality is not a caller error. The book describes this residual and the old-server limitation.
|
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. 📝 WalkthroughWalkthroughQueryService maps selected query syntax errors to ChangesPublic Query Error Boundaries
Priority: ⬆️ High Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable issue is established for this change. It is mergeable after normal checks. 🚥 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-08T13:38:47.616Z |
|
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
|
✅ Final review complete — no blockers (commit a4c51b3) · triage: normal |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔇 Additional comments (4)
packages/rs-drive-abci/src/query/service.rs (1)
1235-1253: LGTM!packages/rs-drive-abci/tests/query_request_errors.rs (1)
1-1096: LGTM!packages/rs-drive-abci/Cargo.toml (1)
93-97: LGTM!book/src/error-handling/drive-errors.md-31-31 (1)
31-31: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.Fix the typo "proof generations".
Line 31 says "proof generations". Use "proof generation".
Proposed fix
-execution, fees, or proof generations. Clients connected to older servers still +execution, fees, or proof generation. Clients connected to older servers still
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: dashpay/platform/.coderabbit.yaml
- Review profile: CHILL
- Plan: Advanced
- Run ID:
252e1c7a-cc36-47fb-be62-2c77d3419e57
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
book/src/error-handling/drive-errors.mdpackages/rs-drive-abci/Cargo.tomlpackages/rs-drive-abci/src/query/service.rspackages/rs-drive-abci/tests/query_request_errors.rs
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
|
The additional wording suggestion refers to “proof generations” in the book. The plural is intentional: it names the existing versioned The advisory docstring-coverage warning includes private test functions and existing trait endpoints. Their purpose and public request contract are covered by the test module explanation, descriptive regression names, the helper's proof-validity comment, and the book update. I found no additional API-documentation gap requiring a patch. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
At ff3f09a, the production changes remain at the public query/status boundary; no blocking production issue was confirmed. One new regression assertion verifies a different identity-key query from the request sent and should use the original selection and cap. This verification was static only; Rust workspace tests were still running in the supplied CI snapshot.
🟡 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: 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: rust-quality); reviewer 6: 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 change adds substantial request validation, error classification and transport regression tests, but does not itself change consensus, cryptography, key handling, network deserialization or persisted storage. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - 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— 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:985-986: Verify SpecificKeys proofs against the requested selection and cap
The loop sends requests including `[0, 0]` and `[0, 1]` with `Some(1)`, but verifies every successful proof using an empty selection and no limit. `verify_identity_keys_by_identity_id_v0` builds its verification query from the supplied `IdentityKeysRequest`, and `into_path_query` preserves both the specific IDs and the limit. Consequently, the root comparison authenticates the proof for a different query without checking that the caller can verify the selection and cap actually requested. The absent-identity fixture appropriately limits these controls to absence rather than populated-key truncation, but the verification query must still match the request. Pass the original IDs and limit into the verifier.
Out-of-scope follow-up suggestions (2)
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.
- Resolve SpecificKeys return-cap lowering in Drive — Outside this PR's scope: the SpecificKeys fetch implementation and its optional-query lowering are unchanged in the PR diff. The PR description and new book section already explicitly identify this residual limitation as a separate Drive defect; repeating the follow-up does not identify a change required for this transport-classification fix.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
- Unproved SpecificKeys return-cap lowering defect still bans nodes — Outside this PR's scope: the unproved query handler and Drive lowering path are unchanged, and the new test deliberately checks that supported caps are not reclassified as InvalidArgument. The residual limitation is already documented in both the PR description and book, so it does not justify expanding this PR or adding another review comment.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
Pass the original SpecificKeys IDs and checked cap to the Drive verifier and assert that the absent-identity proof returns no keys. The previous control authenticated a different query, so its partial and duplicate request verification claim was unproven. This strengthens test assertions only; production behavior is unchanged. The matching-query control and all eight transport regressions pass, as do targeted all-feature/all-target clippy and scoped rustfmt. Original server regressions retain the separately observed red-to-green evidence.
|
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
The complete PR diff keeps the production changes at the unversioned gRPC request/status boundary, with no identified production correctness or consensus-safety defect. The prior SpecificKeys verifier-parameter finding is fixed, but one nonblocking assertion gap remains in the successful-proof controls. This was a static review; no builds or tests were run, and the supplied CI snapshot does not establish completed Rust validation for this head.
🟡 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: 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: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); 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: rust-quality); reviewer 10: 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 request guards, typed error mapping, and extensive transport regressions require substantive review, but the diff does not change consensus, cryptography, key handling, network deserialization, funds movement, or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% 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.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— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (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— 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:980: Require the proof variant before verifying successful proof controls
This conditional skips the verifier, returned-key assertions, and stored-root comparison if a successful proved request returns `Result::Keys` or `None` instead of `Result::Proof`. The later `record` call checks only `Code::Ok`, so the control still passes without establishing proof-backed success. The empty root-path control has the same gap at line 381. Explicitly require `Some(Result::Proof(...))` with a failing alternative before invoking each verifier; these preservation controls should enforce both proof presence and proof validity.
Out-of-scope follow-up suggestions (3)
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.
- Repair supported unproved SpecificKeys return-cap queries — Outside this PR's scope. The unproved SpecificKeys handler and optional-key fetch path are unchanged, and the PR description and book explicitly document the residual lowering defect and exclude its repair. Do not expand this request-boundary change with a separate lowering fix.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
- Track the preserved unproved SpecificKeys node-pool exhaustion path — Outside this PR's scope and already documented as a residual limitation. The shared-client exhaustion scenario depends on an application forwarding caller-controlled unproved requests through a shared pool; this PR neither introduces that path nor changes its retry or ban policy.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
- Unproved SpecificKeys return-cap lowering defect remains — Outside this PR's scope. The unchanged unproved lowering behavior is explicitly excluded and documented in the PR and book; it does not establish a defect introduced or worsened by these guards or status mappings.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
Require proof variants before verifying the empty root path and specific-key response controls. Preserve matching requested IDs, cap, authenticated root and empty key-map checks. Mutation evidence: replacing successful results with None and Keys failed both controls at runtime; restoring exact test bytes passed all eight integration tests. Targeted all-feature/all-target Clippy and scoped rustfmt passed. Production code is unchanged.
|
Waiting for bot review — coderabbitai ✓ · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The complete PR diff at a4c51b3 keeps request validation and typed-error classification at the public query boundary, without changing consensus execution, versioned proof implementations, or client ban policy; no actionable in-scope defects remain. Both prior proof-control findings are fixed in the current source. This was static verification only: no builds or tests were run, and the supplied CI snapshot from 2026-10-08T13:40:00Z still contains pending and running validation checks.
🔴 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: 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); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: 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 diff changes public query validation and error classification across multiple endpoints with substantial regression coverage, but does not change consensus, cryptography, key handling, network deserialization, or persisted storage. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% 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.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,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 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.
- Track supported unproved SpecificKeys return-cap lowering separately — The unchanged unproved SpecificKeys path preserves an explicit return cap when passing the selection to GroveDB's optional-key query lowering. The documented failure when the cap is smaller than the distinct selection cardinality can still surface as Internal; the existing DapiClient retries that status and bans responding nodes, so repeated affected requests can exhaust a shared client address pool. This path predates the PR and is explicitly excluded from its request-classification scope.
- Follow-up: Track a separate issue or maintainer-requested PR to repair return-cap lowering, with regression coverage for zero and partial caps, duplicate IDs, missing keys, populated identities, and healthy-node availability.
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
…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>
|
Superseded by merged #5333 and the transport/proof regression follow-up #5343. #5343 at da55fa3 preserves the merged production implementation and carries the additional real-gRPC, original-request proof, payload, ordering, and compatibility coverage. Its selected CI passed; independent Opus review and both CodeRabbit and thepastaclaw reviews have no unresolved findings. Human self-review/admission remains separate, and #5343 is not merged. Remaining offset-related INTERNAL responses and historical PV12/13 composite-cap limitations are recorded separately and remain in progress. Closing this duplicate does not claim those residuals or devnet deployment are complete. The original branch and work artifacts are preserved. |
2 participants
Basic explanation
What this does: Return
InvalidArgumentfor specific invalid query shapes instead of blaming the responding node. Reject empty selections that cannot produce a usable proof before constructing one.Value: Healthy nodes remain available after bad caller input. In the regression, three empty contract-proof requests previously banned all 13 nodes; each now returns immediately and bans none.
Risks: The public error status changes for the listed requests, including an empty balance proof that previously returned an unverifiable success. Consensus, persisted state, fees, valid-query proofs and client ban policy are unchanged. Older servers retain the old behavior.
Issue being fixed or feature implemented
The v5 audit reports caller-supplied empty selections and zero limits exhausting the SDK's node pool. Query syntax errors lose their classification through Drive/ABCI wrappers, and some proof requests reach storage despite having no usable proof contract. The existing client already handles
InvalidArgumentwithout retrying or banning.What was done?
QuerySyntaxErrorthrough its two Drive/ABCI wrappers toInvalidArgument; keep corruption, arbitrary GroveDB errors and proof failures classified as before.QueryService. Preserve unproved empty batches, unproved all-key/epoch zero behavior, document/shielded zero defaults, optional caps, verifiable empty root-path and SpecificKeys queries, and missing-version statuses.DapiClientaddress pool for no-ban and genuine-node-failure controls. New dependencies are test-only.Residual: A supported unproved
SpecificKeysreturn cap below the distinct selection cardinality still hits an independent lowering defect and may returnInternal/ban a node. This PR does not misclassify that valid request as bad input. Proved forms use a different path; the preserved partial/duplicate proof controls establish verified absence on an empty identity, not truncation of a populated key set. The SDK identity-key fetch uses proved requests.In-place changes to shipped generations
None. Only the unversioned public request/status boundary changes; versioned query handlers, protocol tables and consensus execution are untouched.
How Has This Been Tested?
Actual red → green: Before the patch, four of six integration tests failed, including cumulative 6/12/13 node bans; the wrapped syntax mapper failed separately. After the patch, all eight final integration tests pass and three bad calls make 1/2/3 cumulative attempts, zero retries and zero bans.
Internal/Unavailablestill ban and fail over.Review-fix controls: Successful empty root-path and SpecificKeys responses must contain a proof; verification uses the original key IDs/cap and checks the authenticated root and empty result. Replacing those results with
NoneandKeysfailed both controls at runtime; restoring the exact test bytes returned all eight tests to green.cargo test -p drive-abci --test query_request_errors -j 2 -- --nocapture --test-threads=1: 8 passed.Neighboring
drive-abciquery tests: 772 passed, 1 existing ignored (test_documents_start_after_proof_secondary_index_in_query_2), zero failures.cargo test -p rs-dapi-client --lib -j 2: 139 passed, including transport status and node-ban controls.cargo clippy -p drive-abci --all-features --all-targets -j 2 -- -D warnings, scopedrustfmt --check, andgit diff --check: passed.Final-head CI: Tests run 37785734378 passed at
a4c51b3ebed38a396dd416b5a38c36e524dd9951: 18,753 non-shielded tests passed (1,381 configured skips), 935 shielded tests passed (3 existing ignored), zero failures. The eight new integration cases pass in this run. Workspace formatting/Clippy, dependency and transport-feature checks, book, and Kotlin checks passed; configured unrelated job skips are not counted as tests run.Fixtures run locally on macOS without sockets, devnet or retrieved identity/wallet keys. Proof controls compare actual verifier roots with the stored GroveDB root. The audit's original repro scripts were unavailable; its exact live endpoint count and ban duration are not claimed as reproduced.
Breaking Changes
No consensus or schema breaking change. The listed invalid requests now return
InvalidArgument; empty proved identity balances are rejected instead of returning an unusable proof. Mixed-version clients still encounter old behavior when querying an older server.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 ·
a4c51b3/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
Bug Fixes
Documentation