feat(platform): add GetDocumentsCount and GetDocumentsSplitCount queries#3435
feat(platform): add GetDocumentsCount and GetDocumentsSplitCount queries#3435QuantumExplorer wants to merge 11 commits intov3.1-devfrom
Conversation
Add two new gRPC endpoints for querying document counts from countable indices: - GetDocumentsCount: returns a total count matching where clauses - GetDocumentsSplitCount: returns per-key counts split by an index property Both support proof responses for cryptographic verification. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…t queries Full stack implementation of two new gRPC endpoints for querying document counts from countable indices: - GetDocumentsCount: returns total count matching where clauses - GetDocumentsSplitCount: returns per-key counts split by index property Changes across all layers: - dapi-grpc: register versioned request/response types - rs-dapi: add drive_method proxy for both endpoints - rs-drive-abci: query handlers with version dispatch - rs-dapi-client: transport request mappings - rs-drive-proof-verifier: FromProof implementations - rs-platform-version: version bounds for both query types Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds platform RPCs for document counting (per-query and split-by-property), proto messages, server transport bindings, dAPI service handlers, Drive ABCI query implementations (v0) with tests, proof-verifier types, platform-version bounds, and generated client bindings across Java, Objective‑C, Python, and Web; also adds getMostRecentShieldedAnchor RPC and minor proto field. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant gRPCTransport as gRPC Transport
participant PlatformService
participant QueryHandler as Query Handler
participant Drive
participant Verifier as Proof Verifier
Client->>gRPCTransport: GetDocumentsCountRequest
gRPCTransport->>PlatformService: route request
PlatformService->>QueryHandler: query_documents_count()
QueryHandler->>QueryHandler: validate version & bounds
QueryHandler->>Drive: query_documents_count_v0()
Drive->>Drive: resolve contract/type, parse where
alt prove = true
Drive->>Drive: execute with proof
Drive->>QueryHandler: return proof + metadata
QueryHandler->>Verifier: verify proof & checkpoint
Verifier->>QueryHandler: verification result
else prove = false
Drive->>Drive: execute query, count results
Drive->>QueryHandler: return count + metadata
end
QueryHandler->>PlatformService: GetDocumentsCountResponse
PlatformService->>gRPCTransport: response
gRPCTransport->>Client: GetDocumentsCountResponse
sequenceDiagram
participant Client
participant gRPCTransport as gRPC Transport
participant PlatformService
participant QueryHandler as Query Handler
participant Drive
participant Verifier as Proof Verifier
Client->>gRPCTransport: GetDocumentsSplitCountRequest
gRPCTransport->>PlatformService: route request
PlatformService->>QueryHandler: query_documents_split_count()
QueryHandler->>QueryHandler: validate version & bounds
QueryHandler->>Drive: query_documents_split_count_v0()
Drive->>Drive: resolve contract/type, validate split property, parse where
alt prove = true
Drive->>Drive: execute with proof
Drive->>QueryHandler: return proof + metadata
QueryHandler->>Verifier: verify proof & checkpoint
Verifier->>QueryHandler: verification result
else prove = false
Drive->>Drive: execute query, fetch docs
Drive->>Drive: group counts by property value
Drive->>QueryHandler: return SplitCountEntry list + metadata
end
QueryHandler->>PlatformService: GetDocumentsSplitCountResponse
PlatformService->>gRPCTransport: response
gRPCTransport->>Client: GetDocumentsSplitCountResponse
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
✅ Review complete (commit 1c23a28) |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3435 +/- ##
============================================
- Coverage 80.66% 80.64% -0.02%
============================================
Files 2852 2859 +7
Lines 285371 286437 +1066
============================================
+ Hits 230190 230994 +804
- Misses 55181 55443 +262
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/rs-drive-proof-verifier/src/proof/document_count.rs (1)
16-18: Remove the unnecessaryClonebound and request clone.
requestis consumed bytry_intoand not reused, soQ: Cloneandrequest.clone()can be dropped.Refactor suggestion
impl<'dq, Q> FromProof<Q> for DocumentCount where - Q: TryInto<DriveDocumentQuery<'dq>> + Clone + 'dq, + Q: TryInto<DriveDocumentQuery<'dq>> + 'dq, Q::Error: std::fmt::Display, { @@ - let request: DriveDocumentQuery<'dq> = - request - .clone() - .try_into() + let request: DriveDocumentQuery<'dq> = + request + .try_into() .map_err(|e: Q::Error| Error::RequestError { error: e.to_string(), })?;Also applies to: 35-39
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-proof-verifier/src/proof/document_count.rs` around lines 16 - 18, Remove the unnecessary Clone bound on the generic Q and stop cloning the request before conversion: in the function signatures where you have Q: TryInto<DriveDocumentQuery<'dq>> + Clone + 'dq (and the similar signature around line 35-39), drop the + Clone requirement and remove any request.clone() calls; call request.try_into() (or std::convert::TryInto::try_into(request)) directly since try_into consumes request and no further clones are needed. Ensure the Q::Error: std::fmt::Display bound remains if used for error formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-drive-abci/src/query/document_count_query/v0/mod.rs`:
- Around line 73-84: The count endpoint is incorrectly capped because
DriveDocumentQuery::from_decomposed_values is called with
Some(self.config.drive.default_query_limit) which embeds a size limit into the
query; change that argument to None so the constructed DriveDocumentQuery
(created in the drive_query variable in mod.rs) does not pass a limit into
SizedQuery and GroveDB, ensuring the subsequent count computed from
results.len() reflects all matching documents; update any related comments and,
if you prefer truncation semantics instead, add explicit response metadata
indicating the result was truncated rather than leaving the limit in place.
In `@packages/rs-drive-abci/src/query/document_split_count_query/v0/mod.rs`:
- Around line 101-112: The split count query is using a hard limit
(Some(self.config.drive.default_query_limit)) which truncates results and yields
incorrect split counts; update the DriveDocumentQuery::from_decomposed_values
call (the drive_query creation) to remove the default_query_limit by passing no
limit (e.g., None) so the query returns the full result set for accurate split
counting, ensuring you only change the limit argument and keep the other
parameters the same.
- Around line 164-172: The current code silently swallows CBOR serialization
failures by using value.to_cbor_buffer().unwrap_or_default(), which can collapse
distinct property values into the same empty key; change this to propagate the
error instead of defaulting: replace the unwrap_or_default call in the let key
assignment (where you access document.properties() and
split_count_by_index_property) with proper error handling (e.g., use
value.to_cbor_buffer()? or value.to_cbor_buffer().map_err(|e| /* wrap in the
function's error type */ )?) so that serialization failures return an error from
the surrounding function rather than producing an empty Vec<u8>.
In `@packages/rs-drive-proof-verifier/src/proof/document_split_count.rs`:
- Around line 49-63: The code currently discards the verified documents from
request.verify_proof and always returns an empty DocumentSplitCounts; change
verify_proof usage to keep the deserialized documents, compute split counts
using the request's split_count_by_index_property (or the equivalent field on
the DriveDocumentQuery) and a helper like split_count_by_index_property to group
documents and produce a BTreeMap, then return DocumentSplitCounts(populated_map)
along with mtd.clone() and proof.clone(); ensure you reference and use the
returned _documents from request.verify_proof and mirror the approach used in
document_count.rs where documents.len() is extracted from the verified proof.
In
`@packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/mod.rs`:
- Around line 11-12: DriveAbciQueryVersions struct literals in the mocks need to
be updated to include the two new mandatory fields document_count_query and
document_split_count_query; locate the initializer in
packages/rs-platform-version/src/version/mocks/v2_test.rs (the mock
DriveAbciQueryVersions instance around Line ~170) and add those fields with
appropriate FeatureVersionBounds values (e.g., the same bounds used for similar
query fields or FeatureVersionBounds::default()) so the struct literal compiles;
ensure the field names exactly match
DriveAbciQueryVersions::document_count_query and ::document_split_count_query.
---
Nitpick comments:
In `@packages/rs-drive-proof-verifier/src/proof/document_count.rs`:
- Around line 16-18: Remove the unnecessary Clone bound on the generic Q and
stop cloning the request before conversion: in the function signatures where you
have Q: TryInto<DriveDocumentQuery<'dq>> + Clone + 'dq (and the similar
signature around line 35-39), drop the + Clone requirement and remove any
request.clone() calls; call request.try_into() (or
std::convert::TryInto::try_into(request)) directly since try_into consumes
request and no further clones are needed. Ensure the Q::Error: std::fmt::Display
bound remains if used for error formatting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d1a82c8-31c5-47f1-8fa3-922c9cbf1a3d
📒 Files selected for processing (15)
packages/dapi-grpc/build.rspackages/dapi-grpc/protos/platform/v0/platform.protopackages/rs-dapi-client/src/transport/grpc.rspackages/rs-dapi/src/services/platform_service/mod.rspackages/rs-drive-abci/src/query/document_count_query/mod.rspackages/rs-drive-abci/src/query/document_count_query/v0/mod.rspackages/rs-drive-abci/src/query/document_split_count_query/mod.rspackages/rs-drive-abci/src/query/document_split_count_query/v0/mod.rspackages/rs-drive-abci/src/query/mod.rspackages/rs-drive-abci/src/query/service.rspackages/rs-drive-proof-verifier/src/proof.rspackages/rs-drive-proof-verifier/src/proof/document_count.rspackages/rs-drive-proof-verifier/src/proof/document_split_count.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/v1.rs
packages/rs-drive-abci/src/query/document_count_query/v0/mod.rs
Outdated
Show resolved
Hide resolved
packages/rs-drive-abci/src/query/document_split_count_query/v0/mod.rs
Outdated
Show resolved
Hide resolved
packages/rs-drive-abci/src/query/document_split_count_query/v0/mod.rs
Outdated
Show resolved
Hide resolved
| let (root_hash, _documents) = request | ||
| .verify_proof(&proof.grovedb_proof, platform_version) | ||
| .map_drive_error(proof, mtd)?; | ||
|
|
||
| verify_tenderdash_proof(proof, mtd, &root_hash, provider)?; | ||
|
|
||
| // For split counts from proof, the client would need to deserialize the | ||
| // verified documents and split by the requested property. For now, we | ||
| // return an empty map as the proof verification confirms the data integrity. | ||
| // Full client-side splitting can be done on top of the verified documents. | ||
| Ok(( | ||
| Some(DocumentSplitCounts(BTreeMap::new())), | ||
| mtd.clone(), | ||
| proof.clone(), | ||
| )) |
There was a problem hiding this comment.
Proof verifier returns empty results despite successful verification.
The verified documents (_documents) are discarded and DocumentSplitCounts always returns an empty BTreeMap. Compare this to DocumentCount in document_count.rs (lines 51-55) which correctly extracts documents.len() from the verified proof data.
For proof-based responses, clients receive a verified empty map even when the server returned actual split count data. This defeats the purpose of the proof verification mode.
The comment acknowledges this limitation, but it means clients cannot use proof-verified split counts. Consider implementing the split logic:
🔧 Suggested implementation to populate split counts from verified documents
- let (root_hash, _documents) = request
+ let (root_hash, documents) = request
.verify_proof(&proof.grovedb_proof, platform_version)
.map_drive_error(proof, mtd)?;
verify_tenderdash_proof(proof, mtd, &root_hash, provider)?;
- // For split counts from proof, the client would need to deserialize the
- // verified documents and split by the requested property. For now, we
- // return an empty map as the proof verification confirms the data integrity.
- // Full client-side splitting can be done on top of the verified documents.
- Ok((
- Some(DocumentSplitCounts(BTreeMap::new())),
- mtd.clone(),
- proof.clone(),
- ))
+ // TODO: Extract split_count_by_index_property from request and
+ // group verified documents by that property to populate the map.
+ // For now, return document count as a single entry to provide some utility.
+ let mut counts = BTreeMap::new();
+ counts.insert(Vec::new(), documents.len() as u64);
+ Ok((
+ Some(DocumentSplitCounts(counts)),
+ mtd.clone(),
+ proof.clone(),
+ ))Note: A complete fix requires access to the split_count_by_index_property field from the request to properly group documents. The current DriveDocumentQuery conversion may not preserve this field.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let (root_hash, _documents) = request | |
| .verify_proof(&proof.grovedb_proof, platform_version) | |
| .map_drive_error(proof, mtd)?; | |
| verify_tenderdash_proof(proof, mtd, &root_hash, provider)?; | |
| // For split counts from proof, the client would need to deserialize the | |
| // verified documents and split by the requested property. For now, we | |
| // return an empty map as the proof verification confirms the data integrity. | |
| // Full client-side splitting can be done on top of the verified documents. | |
| Ok(( | |
| Some(DocumentSplitCounts(BTreeMap::new())), | |
| mtd.clone(), | |
| proof.clone(), | |
| )) | |
| let (root_hash, documents) = request | |
| .verify_proof(&proof.grovedb_proof, platform_version) | |
| .map_drive_error(proof, mtd)?; | |
| verify_tenderdash_proof(proof, mtd, &root_hash, provider)?; | |
| // TODO: Extract split_count_by_index_property from request and | |
| // group verified documents by that property to populate the map. | |
| // For now, return document count as a single entry to provide some utility. | |
| let mut counts = BTreeMap::new(); | |
| counts.insert(Vec::new(), documents.len() as u64); | |
| Ok(( | |
| Some(DocumentSplitCounts(counts)), | |
| mtd.clone(), | |
| proof.clone(), | |
| )) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rs-drive-proof-verifier/src/proof/document_split_count.rs` around
lines 49 - 63, The code currently discards the verified documents from
request.verify_proof and always returns an empty DocumentSplitCounts; change
verify_proof usage to keep the deserialized documents, compute split counts
using the request's split_count_by_index_property (or the equivalent field on
the DriveDocumentQuery) and a helper like split_count_by_index_property to group
documents and produce a BTreeMap, then return DocumentSplitCounts(populated_map)
along with mtd.clone() and proof.clone(); ensure you reference and use the
returned _documents from request.verify_proof and mirror the approach used in
document_count.rs where documents.len() is extracted from the verified proof.
packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/mod.rs
Show resolved
Hide resolved
- Remove 100-document limit on count queries (set limit = None) - Add 3 tests for GetDocumentsCount (no-prove, empty, with-prove) - Add 4 tests for GetDocumentsSplitCount (no-prove, with-prove, errors) - Fix missing version fields in mock v2_test.rs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/rs-drive-abci/src/query/document_count_query/v0/mod.rs (2)
73-90: Construct the unlimited count query directly.Line 77 sets a default limit and Line 89 immediately removes it. Passing
Nonefor limit at construction avoids mutable post-fix state and reduces regression risk.♻️ Proposed simplification
- let mut drive_query = + let drive_query = check_validation_result_with_data!(DriveDocumentQuery::from_decomposed_values( where_clause, None, - Some(self.config.drive.default_query_limit), + None, None, true, None, contract_ref, document_type, &self.config.drive, )); - - // Remove the limit so we count ALL matching documents, not just up to the - // default query limit. A count query needs to return the total number of - // documents matching the where clause. - drive_query.limit = None;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/query/document_count_query/v0/mod.rs` around lines 73 - 90, The code currently constructs a DriveDocumentQuery with Some(self.config.drive.default_query_limit) and then mutates drive_query.limit = None; instead, call DriveDocumentQuery::from_decomposed_values with None for the limit parameter so the query is constructed without any limit upfront; update the call site (DriveDocumentQuery::from_decomposed_values) to pass None instead of Some(self.config.drive.default_query_limit) and remove the subsequent drive_query.limit = None mutation.
152-160: Use one platform version source per test.These tests mix
PlatformVersion::latest()withversionfromsetup_platform. Using a single source (preferablyversion) makes tests less brittle across versioned behavior changes.🧪 Example adjustment pattern
- let platform_version = PlatformVersion::latest(); + let platform_version = version;Apply consistently in the three tests where documents/contracts are created and stored.
Also applies to: 171-175, 181-182, 213-221, 257-265, 276-280, 286-287
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/query/document_count_query/v0/mod.rs` around lines 152 - 160, The tests call PlatformVersion::latest() directly when creating contracts/documents (e.g., in json_document_to_contract_with_ids) while the test harness provides a platform version variable (commonly named version from setup_platform); change all uses of PlatformVersion::latest() in those tests to use the single source 'version' instead so the tests rely on the setup_platform-provided version consistently (apply this replacement in the calls to json_document_to_contract_with_ids and any other contract/document creation or storage sites mentioned around the blocks using PlatformVersion::latest()).packages/rs-drive-abci/src/query/document_split_count_query/v0/mod.rs (1)
101-117: Consider passingNonedirectly for the limit parameter.The code passes
Some(self.config.drive.default_query_limit)on line 105, then immediately removes the limit on line 117. This two-step approach is redundant - you could simplify by passingNonedirectly.♻️ Proposed simplification
let mut drive_query = check_validation_result_with_data!(DriveDocumentQuery::from_decomposed_values( where_clause, None, - Some(self.config.drive.default_query_limit), + None, // No limit - split count needs all matching documents None, true, None, contract_ref, document_type, &self.config.drive, )); - // Remove the limit so we count ALL matching documents, not just up to the - // default query limit. A split count query needs to return complete counts - // across all values of the split property. - drive_query.limit = None;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/query/document_split_count_query/v0/mod.rs` around lines 101 - 117, DriveDocumentQuery::from_decomposed_values is being called with Some(self.config.drive.default_query_limit) and then immediately overwritten by setting drive_query.limit = None, which is redundant; update the call to DriveDocumentQuery::from_decomposed_values to pass None for the limit parameter (the argument currently supplying default_query_limit) and remove the subsequent drive_query.limit = None line so the query is created without a limit from the start.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/rs-drive-abci/src/query/document_count_query/v0/mod.rs`:
- Around line 73-90: The code currently constructs a DriveDocumentQuery with
Some(self.config.drive.default_query_limit) and then mutates drive_query.limit =
None; instead, call DriveDocumentQuery::from_decomposed_values with None for the
limit parameter so the query is constructed without any limit upfront; update
the call site (DriveDocumentQuery::from_decomposed_values) to pass None instead
of Some(self.config.drive.default_query_limit) and remove the subsequent
drive_query.limit = None mutation.
- Around line 152-160: The tests call PlatformVersion::latest() directly when
creating contracts/documents (e.g., in json_document_to_contract_with_ids) while
the test harness provides a platform version variable (commonly named version
from setup_platform); change all uses of PlatformVersion::latest() in those
tests to use the single source 'version' instead so the tests rely on the
setup_platform-provided version consistently (apply this replacement in the
calls to json_document_to_contract_with_ids and any other contract/document
creation or storage sites mentioned around the blocks using
PlatformVersion::latest()).
In `@packages/rs-drive-abci/src/query/document_split_count_query/v0/mod.rs`:
- Around line 101-117: DriveDocumentQuery::from_decomposed_values is being
called with Some(self.config.drive.default_query_limit) and then immediately
overwritten by setting drive_query.limit = None, which is redundant; update the
call to DriveDocumentQuery::from_decomposed_values to pass None for the limit
parameter (the argument currently supplying default_query_limit) and remove the
subsequent drive_query.limit = None line so the query is created without a limit
from the start.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 960f7945-798d-456b-b54c-622573fc270f
📒 Files selected for processing (3)
packages/rs-drive-abci/src/query/document_count_query/v0/mod.rspackages/rs-drive-abci/src/query/document_split_count_query/v0/mod.rspackages/rs-platform-version/src/version/mocks/v2_test.rs
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… lint Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Well-structured new query endpoints following existing patterns. One confirmed bug: the DocumentSplitCounts FromProof implementation verifies the GroveDB proof but returns BTreeMap::new() instead of extracting the actual count data. Any client using prove=true for split-count queries receives a valid-looking but empty result. Convergent finding from two independent codex agents (rust-quality + security-auditor), confirmed by verifier.
Reviewed commit: 99686a3
🔴 1 blocking
🤖 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-proof-verifier/src/proof/document_split_count.rs`:
- [BLOCKING] line 60: DocumentSplitCounts FromProof always returns empty BTreeMap
The `FromProof` implementation for `DocumentSplitCounts` verifies the GroveDB proof and Tenderdash proof but discards the verified documents (`_documents` on line 49) and returns `BTreeMap::new()` on line 60. The `prove=false` server-side path (`document_split_count_query/v0/mod.rs:154-180`) correctly deserializes documents and groups counts by the split property, but this client-side proof verification path never performs the equivalent aggregation.
Any client using `prove=true` for split-count queries receives `Some(DocumentSplitCounts(BTreeMap::new()))` — a valid-looking but empty result. The verified documents need to be grouped by the split property and counted, matching the server-side logic.
| // return an empty map as the proof verification confirms the data integrity. | ||
| // Full client-side splitting can be done on top of the verified documents. | ||
| Ok(( | ||
| Some(DocumentSplitCounts(BTreeMap::new())), |
There was a problem hiding this comment.
🔴 Blocking: DocumentSplitCounts FromProof always returns empty BTreeMap
The FromProof implementation for DocumentSplitCounts verifies the GroveDB proof and Tenderdash proof but discards the verified documents (_documents on line 49) and returns BTreeMap::new() on line 60. The prove=false server-side path (document_split_count_query/v0/mod.rs:154-180) correctly deserializes documents and groups counts by the split property, but this client-side proof verification path never performs the equivalent aggregation.
Any client using prove=true for split-count queries receives Some(DocumentSplitCounts(BTreeMap::new())) — a valid-looking but empty result. The verified documents need to be grouped by the split property and counted, matching the server-side logic.
source: ['codex']
🤖 Fix this 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-proof-verifier/src/proof/document_split_count.rs`:
- [BLOCKING] line 60: DocumentSplitCounts FromProof always returns empty BTreeMap
The `FromProof` implementation for `DocumentSplitCounts` verifies the GroveDB proof and Tenderdash proof but discards the verified documents (`_documents` on line 49) and returns `BTreeMap::new()` on line 60. The `prove=false` server-side path (`document_split_count_query/v0/mod.rs:154-180`) correctly deserializes documents and groups counts by the split property, but this client-side proof verification path never performs the equivalent aggregation.
Any client using `prove=true` for split-count queries receives `Some(DocumentSplitCounts(BTreeMap::new()))` — a valid-looking but empty result. The verified documents need to be grouped by the split property and counted, matching the server-side logic.
…mment Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
libz-sys 1.1.26 removed bundled zlib sources, breaking compilation on macOS. Pin to 1.1.25 which matches v3.1-dev. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This push is an incremental re-review of PR #3435 at commit 8ffbf55a. The new commit only regenerates dapi-grpc client artifacts and removes a stale // Force rebuild comment from build.rs, so the previously reported proof-verifier bug is the main question. That blocker is still present in the current snapshot: the DocumentSplitCounts proof path verifies the proof but still discards the verified documents and returns an empty map.
Reviewed commit: 8ffbf55
🔴 1 blocking
🤖 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-proof-verifier/src/proof/document_split_count.rs`:
- [BLOCKING] lines 49-60: prove=true split-count verification still returns an empty result
This re-review commit does not touch the split-count proof verifier, and the same bug is still present in the current snapshot. `verify_proof()` returns the verified documents into `_documents` on lines 49-51, but the implementation immediately discards them and returns `DocumentSplitCounts(BTreeMap::new())` on line 60. The server-side non-proof path in `rs-drive-abci` groups matching documents by the split property and returns counts, so `prove=true` and `prove=false` still diverge semantically: a valid proof-backed split-count query succeeds but always produces an empty map. This means the new generated client bindings now expose an endpoint whose proof-verification path still cannot return correct split counts.
| let (root_hash, _documents) = request | ||
| .verify_proof(&proof.grovedb_proof, platform_version) | ||
| .map_drive_error(proof, mtd)?; | ||
|
|
||
| verify_tenderdash_proof(proof, mtd, &root_hash, provider)?; | ||
|
|
||
| // For split counts from proof, the client would need to deserialize the | ||
| // verified documents and split by the requested property. For now, we | ||
| // return an empty map as the proof verification confirms the data integrity. | ||
| // Full client-side splitting can be done on top of the verified documents. | ||
| Ok(( | ||
| Some(DocumentSplitCounts(BTreeMap::new())), |
There was a problem hiding this comment.
🔴 Blocking: prove=true split-count verification still returns an empty result
This re-review commit does not touch the split-count proof verifier, and the same bug is still present in the current snapshot. verify_proof() returns the verified documents into _documents on lines 49-51, but the implementation immediately discards them and returns DocumentSplitCounts(BTreeMap::new()) on line 60. The server-side non-proof path in rs-drive-abci groups matching documents by the split property and returns counts, so prove=true and prove=false still diverge semantically: a valid proof-backed split-count query succeeds but always produces an empty map. This means the new generated client bindings now expose an endpoint whose proof-verification path still cannot return correct split counts.
source: ['manual']
🤖 Fix this 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-proof-verifier/src/proof/document_split_count.rs`:
- [BLOCKING] lines 49-60: prove=true split-count verification still returns an empty result
This re-review commit does not touch the split-count proof verifier, and the same bug is still present in the current snapshot. `verify_proof()` returns the verified documents into `_documents` on lines 49-51, but the implementation immediately discards them and returns `DocumentSplitCounts(BTreeMap::new())` on line 60. The server-side non-proof path in `rs-drive-abci` groups matching documents by the split property and returns counts, so `prove=true` and `prove=false` still diverge semantically: a valid proof-backed split-count query succeeds but always produces an empty map. This means the new generated client bindings now expose an endpoint whose proof-verification path still cannot return correct split counts.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental re-review of PR #3435 at commit 8c5d2b4f. This push only updates Cargo.lock to pin libz-sys (plus lockfile-adjacent version churn such as semver and arc-swap), so the key question is whether the previously reported split-count proof bug was fixed elsewhere. It was not: the DocumentSplitCounts proof verifier still discards the verified documents and returns an empty map, so prove=true split-count queries are still incorrect in the current tip.
Reviewed commit: 8c5d2b4
🔴 1 blocking
🤖 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-proof-verifier/src/proof/document_split_count.rs`:
- [BLOCKING] lines 49-60: The split-count proof verifier still drops the verified result set
This incremental push does not touch the split-count verifier, and the same blocker remains in the current tip. `request.verify_proof()` returns the verified documents into `_documents` on lines 49–51, but the implementation then ignores them and returns `DocumentSplitCounts(BTreeMap::new())` on line 60. The non-proof server path in `rs-drive-abci/src/query/document_split_count_query/v0/mod.rs` groups matching documents by the split property and returns actual counts, so `prove=true` and `prove=false` still diverge semantically: a valid proof-backed split-count query succeeds but always yields an empty map. The lockfile pin in this commit may help CI, but it does not make the new split-count endpoint correct.
| let (root_hash, _documents) = request | ||
| .verify_proof(&proof.grovedb_proof, platform_version) | ||
| .map_drive_error(proof, mtd)?; | ||
|
|
||
| verify_tenderdash_proof(proof, mtd, &root_hash, provider)?; | ||
|
|
||
| // For split counts from proof, the client would need to deserialize the | ||
| // verified documents and split by the requested property. For now, we | ||
| // return an empty map as the proof verification confirms the data integrity. | ||
| // Full client-side splitting can be done on top of the verified documents. | ||
| Ok(( | ||
| Some(DocumentSplitCounts(BTreeMap::new())), |
There was a problem hiding this comment.
🔴 Blocking: The split-count proof verifier still drops the verified result set
This incremental push does not touch the split-count verifier, and the same blocker remains in the current tip. request.verify_proof() returns the verified documents into _documents on lines 49–51, but the implementation then ignores them and returns DocumentSplitCounts(BTreeMap::new()) on line 60. The non-proof server path in rs-drive-abci/src/query/document_split_count_query/v0/mod.rs groups matching documents by the split property and returns actual counts, so prove=true and prove=false still diverge semantically: a valid proof-backed split-count query succeeds but always yields an empty map. The lockfile pin in this commit may help CI, but it does not make the new split-count endpoint correct.
source: ['manual']
🤖 Fix this 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-proof-verifier/src/proof/document_split_count.rs`:
- [BLOCKING] lines 49-60: The split-count proof verifier still drops the verified result set
This incremental push does not touch the split-count verifier, and the same blocker remains in the current tip. `request.verify_proof()` returns the verified documents into `_documents` on lines 49–51, but the implementation then ignores them and returns `DocumentSplitCounts(BTreeMap::new())` on line 60. The non-proof server path in `rs-drive-abci/src/query/document_split_count_query/v0/mod.rs` groups matching documents by the split property and returns actual counts, so `prove=true` and `prove=false` still diverge semantically: a valid proof-backed split-count query succeeds but always yields an empty map. The lockfile pin in this commit may help CI, but it does not make the new split-count endpoint correct.
…l documents Rewrites both count query handlers to read counts directly from CountTree elements in GroveDB instead of fetching and counting all matching documents: - Count query: navigates index path using where clause values, fetches CountTree element at terminal key [0], returns count_value_or_default() - Split count: iterates split property values at the appropriate index level, sums CountTree counts for each value - Both now require a countable index — returns error if none found - Prove path unchanged (still uses DriveDocumentQuery for proofs) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Introduces DriveDocumentCountQuery in rs-drive with the CountTree counting logic. ABCI handlers are now thin wrappers that parse gRPC requests and delegate to Drive. - New: DriveDocumentCountQuery struct with execute_no_proof() - Supports total count and split-by-property count - 5 Drive-level tests + 7 ABCI integration tests - ABCI handlers simplified to parse + delegate pattern Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This push rewrites the non-proof count paths to use CountTree-based aggregation, but it introduces a correctness regression: the fast path only applies equality predicates and silently ignores all other where operators. That makes prove=false return broader counts than prove=true for range/prefix-style queries. The previously reported DocumentSplitCounts proof-verifier bug is also still present, so both the proof and non-proof split-count paths remain incorrect in the current tip.
Reviewed commit: e7c98f8
🔴 3 blocking
🤖 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/src/query/document_count_query/v0/mod.rs`:
- [BLOCKING] lines 152-165: The CountTree fast path silently drops non-equality filters
The new `prove=false` path chooses a countable index from `all_where_clauses`, but `find_countable_index_for_where_clauses()` only looks at clauses whose operator is `WhereOperator::Equal` (lines 198-202), and `count_from_count_tree()` likewise only serializes equality clauses into the path (lines 275-289). Any other predicate in the request (`>`, `>=`, `<`, `startsWith`, etc.) is neither validated nor applied. As a result, a query like `where = [["age", ">", 18]]` can still hit the CountTree path and return the count for the entire prefix / whole index instead of the filtered subset, while the `prove=true` path above still evaluates the full `DriveDocumentQuery`. `prove=false` and `prove=true` therefore diverge semantically for the same request. The no-proof path needs to reject unsupported operators or fall back to the full query engine whenever any non-equality clause is present.
In `packages/rs-drive-abci/src/query/document_split_count_query/v0/mod.rs`:
- [BLOCKING] lines 175-189: The split-count CountTree path also ignores non-equality where clauses
The same issue exists in the split-count refactor. `find_countable_index_for_split()` builds its prefix using only `WhereOperator::Equal` clauses (lines 221-225, 232-249), and `split_count_from_count_tree()` only pushes equality values into the CountTree path (lines 290-303). Any non-equality predicate in the request is silently ignored rather than rejected or evaluated. That means `prove=false` split counts can now be computed over the entire split subtree even when the original query included additional range/pattern filters, while the `prove=true` path still uses the full `DriveDocumentQuery`. This breaks correctness for non-equality split-count queries and makes proof/no-proof results disagree.
In `packages/rs-drive-proof-verifier/src/proof/document_split_count.rs`:
- [BLOCKING] lines 49-60: The proof-backed split-count verifier still returns an empty map
The previously reported proof-verifier bug is still present in this tip. `request.verify_proof()` returns the verified documents into `_documents` on lines 49-51, but the implementation immediately discards them and returns `DocumentSplitCounts(BTreeMap::new())` on line 60. So even after this refactor, a valid `prove=true` split-count query still verifies the proof and then returns an empty result instead of grouping the verified documents by the split property. This keeps the proof-backed endpoint incorrect even if the non-proof path were fixed.
| let countable_index = Self::find_countable_index_for_where_clauses( | ||
| document_type.indexes(), | ||
| &all_where_clauses, | ||
| ); | ||
|
|
||
| let count = if let Some(index) = countable_index { | ||
| // Build the path to the CountTree(s) and fetch count(s). | ||
| self.count_from_count_tree( | ||
| contract_id.to_buffer(), | ||
| document_type_name.as_str(), | ||
| document_type, | ||
| index, | ||
| &all_where_clauses, | ||
| platform_version, |
There was a problem hiding this comment.
🔴 Blocking: The CountTree fast path silently drops non-equality filters
The new prove=false path chooses a countable index from all_where_clauses, but find_countable_index_for_where_clauses() only looks at clauses whose operator is WhereOperator::Equal (lines 198-202), and count_from_count_tree() likewise only serializes equality clauses into the path (lines 275-289). Any other predicate in the request (>, >=, <, startsWith, etc.) is neither validated nor applied. As a result, a query like where = [["age", ">", 18]] can still hit the CountTree path and return the count for the entire prefix / whole index instead of the filtered subset, while the prove=true path above still evaluates the full DriveDocumentQuery. prove=false and prove=true therefore diverge semantically for the same request. The no-proof path needs to reject unsupported operators or fall back to the full query engine whenever any non-equality clause is present.
source: ['manual']
🤖 Fix this 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/src/query/document_count_query/v0/mod.rs`:
- [BLOCKING] lines 152-165: The CountTree fast path silently drops non-equality filters
The new `prove=false` path chooses a countable index from `all_where_clauses`, but `find_countable_index_for_where_clauses()` only looks at clauses whose operator is `WhereOperator::Equal` (lines 198-202), and `count_from_count_tree()` likewise only serializes equality clauses into the path (lines 275-289). Any other predicate in the request (`>`, `>=`, `<`, `startsWith`, etc.) is neither validated nor applied. As a result, a query like `where = [["age", ">", 18]]` can still hit the CountTree path and return the count for the entire prefix / whole index instead of the filtered subset, while the `prove=true` path above still evaluates the full `DriveDocumentQuery`. `prove=false` and `prove=true` therefore diverge semantically for the same request. The no-proof path needs to reject unsupported operators or fall back to the full query engine whenever any non-equality clause is present.
| let countable_index = Self::find_countable_index_for_split( | ||
| document_type.indexes(), | ||
| &all_where_clauses, | ||
| &split_count_by_index_property, | ||
| ); | ||
|
|
||
| let entries = if let Some(index) = countable_index { | ||
| self.split_count_from_count_tree( | ||
| contract_id.to_buffer(), | ||
| document_type_name.as_str(), | ||
| document_type, | ||
| index, | ||
| &all_where_clauses, | ||
| &split_count_by_index_property, | ||
| platform_version, |
There was a problem hiding this comment.
🔴 Blocking: The split-count CountTree path also ignores non-equality where clauses
The same issue exists in the split-count refactor. find_countable_index_for_split() builds its prefix using only WhereOperator::Equal clauses (lines 221-225, 232-249), and split_count_from_count_tree() only pushes equality values into the CountTree path (lines 290-303). Any non-equality predicate in the request is silently ignored rather than rejected or evaluated. That means prove=false split counts can now be computed over the entire split subtree even when the original query included additional range/pattern filters, while the prove=true path still uses the full DriveDocumentQuery. This breaks correctness for non-equality split-count queries and makes proof/no-proof results disagree.
source: ['manual']
🤖 Fix this 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/src/query/document_split_count_query/v0/mod.rs`:
- [BLOCKING] lines 175-189: The split-count CountTree path also ignores non-equality where clauses
The same issue exists in the split-count refactor. `find_countable_index_for_split()` builds its prefix using only `WhereOperator::Equal` clauses (lines 221-225, 232-249), and `split_count_from_count_tree()` only pushes equality values into the CountTree path (lines 290-303). Any non-equality predicate in the request is silently ignored rather than rejected or evaluated. That means `prove=false` split counts can now be computed over the entire split subtree even when the original query included additional range/pattern filters, while the `prove=true` path still uses the full `DriveDocumentQuery`. This breaks correctness for non-equality split-count queries and makes proof/no-proof results disagree.
| let (root_hash, _documents) = request | ||
| .verify_proof(&proof.grovedb_proof, platform_version) | ||
| .map_drive_error(proof, mtd)?; | ||
|
|
||
| verify_tenderdash_proof(proof, mtd, &root_hash, provider)?; | ||
|
|
||
| // For split counts from proof, the client would need to deserialize the | ||
| // verified documents and split by the requested property. For now, we | ||
| // return an empty map as the proof verification confirms the data integrity. | ||
| // Full client-side splitting can be done on top of the verified documents. | ||
| Ok(( | ||
| Some(DocumentSplitCounts(BTreeMap::new())), |
There was a problem hiding this comment.
🔴 Blocking: The proof-backed split-count verifier still returns an empty map
The previously reported proof-verifier bug is still present in this tip. request.verify_proof() returns the verified documents into _documents on lines 49-51, but the implementation immediately discards them and returns DocumentSplitCounts(BTreeMap::new()) on line 60. So even after this refactor, a valid prove=true split-count query still verifies the proof and then returns an empty result instead of grouping the verified documents by the split property. This keeps the proof-backed endpoint incorrect even if the non-proof path were fixed.
source: ['manual']
🤖 Fix this 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-proof-verifier/src/proof/document_split_count.rs`:
- [BLOCKING] lines 49-60: The proof-backed split-count verifier still returns an empty map
The previously reported proof-verifier bug is still present in this tip. `request.verify_proof()` returns the verified documents into `_documents` on lines 49-51, but the implementation immediately discards them and returns `DocumentSplitCounts(BTreeMap::new())` on line 60. So even after this refactor, a valid `prove=true` split-count query still verifies the proof and then returns an empty result instead of grouping the verified documents by the split property. This keeps the proof-backed endpoint incorrect even if the non-proof path were fixed.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental re-review of PR #3435 at commit 1c23a28c. This push moves the CountTree counting logic from rs-drive-abci into a shared rs-drive::query::DriveDocumentCountQuery, but it does not fix the correctness issues from the previous review. The new shared implementation still only honors equality clauses in the no-proof path, so unsupported operators are silently ignored, and the proof-backed split-count verifier still returns an empty map.
Reviewed commit: 1c23a28
🔴 3 blocking
🤖 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/src/query/drive_document_count_query.rs`:
- [BLOCKING] lines 69-111: The shared CountTree total-count path still ignores non-equality filters
`find_countable_index_for_where_clauses()` now lives in `rs-drive`, but it still builds `equality_fields` by discarding every where clause whose operator is not `WhereOperator::Equal` (lines 73-76). `execute_total_count()` then only serializes equality clauses into the index path (lines 199-218) and never validates that there were additional predicates in the original query. That means a request with mixed filters like `["age", ">", 18]` or `["name", "startsWith", "A"]` can still take the no-proof CountTree fast path and return the broader prefix count / full index count, while the `prove=true` path above still uses the full `DriveDocumentQuery`. So the refactor preserved the bug where `prove=false` silently ignores non-equality constraints instead of rejecting them or falling back to the full query engine.
- [BLOCKING] lines 118-145: The shared split-count path still drops non-equality where clauses
`find_countable_index_for_split()` has the same problem in the shared implementation: it only considers `WhereOperator::Equal` clauses when choosing an index (lines 121-124), and `execute_split_count()` only pushes equality values into the path before enumerating split buckets (lines 263-285). Any other predicate in the original query is silently ignored rather than enforced. As a result, `prove=false` split-count queries can still return counts for the whole split subtree even when the request included additional range/pattern filters, while `prove=true` uses the full query semantics. Moving the code from `rs-drive-abci` into `rs-drive` did not fix the correctness gap; it just centralized it.
In `packages/rs-drive-proof-verifier/src/proof/document_split_count.rs`:
- [BLOCKING] lines 49-60: The proof-backed split-count verifier still returns an empty map
The old proof-verifier bug is unchanged in this tip. `request.verify_proof()` still returns the verified documents into `_documents` on lines 49-51, but the implementation immediately discards them and returns `DocumentSplitCounts(BTreeMap::new())` on line 60. So even after the CountTree refactor, a valid `prove=true` split-count query still verifies the proof and then returns an empty result instead of grouping the verified documents by the split property.
| pub fn find_countable_index_for_where_clauses<'b>( | ||
| indexes: &'b BTreeMap<String, Index>, | ||
| where_clauses: &[WhereClause], | ||
| ) -> Option<&'b Index> { | ||
| let equality_fields: BTreeSet<&str> = where_clauses | ||
| .iter() | ||
| .filter(|wc| wc.operator == WhereOperator::Equal) | ||
| .map(|wc| wc.field.as_str()) | ||
| .collect(); | ||
|
|
||
| let mut best_match: Option<(&Index, usize)> = None; | ||
|
|
||
| for index in indexes.values() { | ||
| if !index.countable { | ||
| continue; | ||
| } | ||
|
|
||
| // Check that where clause equality fields form a prefix of the index properties. | ||
| let mut prefix_len = 0; | ||
| for prop in &index.properties { | ||
| if equality_fields.contains(prop.name.as_str()) { | ||
| prefix_len += 1; | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // All equality where clause fields must be consumed as a prefix | ||
| if prefix_len < equality_fields.len() { | ||
| continue; | ||
| } | ||
|
|
||
| // Prefer the index with the longest matching prefix (most specific) | ||
| match &best_match { | ||
| None => best_match = Some((index, prefix_len)), | ||
| Some((_, best_len)) if prefix_len > *best_len => { | ||
| best_match = Some((index, prefix_len)); | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
|
|
||
| best_match.map(|(index, _)| index) |
There was a problem hiding this comment.
🔴 Blocking: The shared CountTree total-count path still ignores non-equality filters
find_countable_index_for_where_clauses() now lives in rs-drive, but it still builds equality_fields by discarding every where clause whose operator is not WhereOperator::Equal (lines 73-76). execute_total_count() then only serializes equality clauses into the index path (lines 199-218) and never validates that there were additional predicates in the original query. That means a request with mixed filters like ["age", ">", 18] or ["name", "startsWith", "A"] can still take the no-proof CountTree fast path and return the broader prefix count / full index count, while the prove=true path above still uses the full DriveDocumentQuery. So the refactor preserved the bug where prove=false silently ignores non-equality constraints instead of rejecting them or falling back to the full query engine.
source: ['manual']
🤖 Fix this 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/src/query/drive_document_count_query.rs`:
- [BLOCKING] lines 69-111: The shared CountTree total-count path still ignores non-equality filters
`find_countable_index_for_where_clauses()` now lives in `rs-drive`, but it still builds `equality_fields` by discarding every where clause whose operator is not `WhereOperator::Equal` (lines 73-76). `execute_total_count()` then only serializes equality clauses into the index path (lines 199-218) and never validates that there were additional predicates in the original query. That means a request with mixed filters like `["age", ">", 18]` or `["name", "startsWith", "A"]` can still take the no-proof CountTree fast path and return the broader prefix count / full index count, while the `prove=true` path above still uses the full `DriveDocumentQuery`. So the refactor preserved the bug where `prove=false` silently ignores non-equality constraints instead of rejecting them or falling back to the full query engine.
| pub fn find_countable_index_for_split<'b>( | ||
| indexes: &'b BTreeMap<String, Index>, | ||
| where_clauses: &[WhereClause], | ||
| split_property: &str, | ||
| ) -> Option<&'b Index> { | ||
| let equality_fields: BTreeSet<&str> = where_clauses | ||
| .iter() | ||
| .filter(|wc| wc.operator == WhereOperator::Equal) | ||
| .map(|wc| wc.field.as_str()) | ||
| .collect(); | ||
|
|
||
| for index in indexes.values() { | ||
| if !index.countable { | ||
| continue; | ||
| } | ||
|
|
||
| // Check that where clause equality fields form a prefix | ||
| let mut prefix_len = 0; | ||
| for prop in &index.properties { | ||
| if equality_fields.contains(prop.name.as_str()) { | ||
| prefix_len += 1; | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if prefix_len < equality_fields.len() { | ||
| continue; |
There was a problem hiding this comment.
🔴 Blocking: The shared split-count path still drops non-equality where clauses
find_countable_index_for_split() has the same problem in the shared implementation: it only considers WhereOperator::Equal clauses when choosing an index (lines 121-124), and execute_split_count() only pushes equality values into the path before enumerating split buckets (lines 263-285). Any other predicate in the original query is silently ignored rather than enforced. As a result, prove=false split-count queries can still return counts for the whole split subtree even when the request included additional range/pattern filters, while prove=true uses the full query semantics. Moving the code from rs-drive-abci into rs-drive did not fix the correctness gap; it just centralized it.
source: ['manual']
🤖 Fix this 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/src/query/drive_document_count_query.rs`:
- [BLOCKING] lines 118-145: The shared split-count path still drops non-equality where clauses
`find_countable_index_for_split()` has the same problem in the shared implementation: it only considers `WhereOperator::Equal` clauses when choosing an index (lines 121-124), and `execute_split_count()` only pushes equality values into the path before enumerating split buckets (lines 263-285). Any other predicate in the original query is silently ignored rather than enforced. As a result, `prove=false` split-count queries can still return counts for the whole split subtree even when the request included additional range/pattern filters, while `prove=true` uses the full query semantics. Moving the code from `rs-drive-abci` into `rs-drive` did not fix the correctness gap; it just centralized it.
| let (root_hash, _documents) = request | ||
| .verify_proof(&proof.grovedb_proof, platform_version) | ||
| .map_drive_error(proof, mtd)?; | ||
|
|
||
| verify_tenderdash_proof(proof, mtd, &root_hash, provider)?; | ||
|
|
||
| // For split counts from proof, the client would need to deserialize the | ||
| // verified documents and split by the requested property. For now, we | ||
| // return an empty map as the proof verification confirms the data integrity. | ||
| // Full client-side splitting can be done on top of the verified documents. | ||
| Ok(( | ||
| Some(DocumentSplitCounts(BTreeMap::new())), |
There was a problem hiding this comment.
🔴 Blocking: The proof-backed split-count verifier still returns an empty map
The old proof-verifier bug is unchanged in this tip. request.verify_proof() still returns the verified documents into _documents on lines 49-51, but the implementation immediately discards them and returns DocumentSplitCounts(BTreeMap::new()) on line 60. So even after the CountTree refactor, a valid prove=true split-count query still verifies the proof and then returns an empty result instead of grouping the verified documents by the split property.
source: ['manual']
🤖 Fix this 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-proof-verifier/src/proof/document_split_count.rs`:
- [BLOCKING] lines 49-60: The proof-backed split-count verifier still returns an empty map
The old proof-verifier bug is unchanged in this tip. `request.verify_proof()` still returns the verified documents into `_documents` on lines 49-51, but the implementation immediately discards them and returns `DocumentSplitCounts(BTreeMap::new())` on line 60. So even after the CountTree refactor, a valid `prove=true` split-count query still verifies the proof and then returns an empty result instead of grouping the verified documents by the split property.
2 participants
Summary
Full stack implementation of two new gRPC endpoints for querying document counts from countable indices (added in #2516):
Layers touched
drive_method!passthrough for both endpointsFromProofimplementationsHow it works
GetDocumentsCountRequestwith contract ID, document type, and where clausesDriveDocumentQuerywith no limit and executes itNote on macOS CI
The macOS CI runner (
dcg) has a broken build environment (missing zlib source files forlibz-sys). This causes clippy to fail with exit code 101 regardless of code changes. Thelatte-sshrunner works fine. This is an infrastructure issue, not a code issue.Test plan
cargo checkpasses across all affected cratescargo clippy --workspace --all-features --locked -- --no-deps -D warningspasses locally🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
getDocumentsCountAPI endpoint to retrieve document counts with optional proof generation.getDocumentsSplitCountAPI endpoint to retrieve document counts split by a specified property with optional proof generation.Tests