fix(drive): skip the ranked offset by counting instead of walking - #4382
fix(drive): skip the ranked offset by counting instead of walking#4382shumkov wants to merge 5 commits into
Conversation
|
🕓 Ready for review — 9 ahead in queue (commit 6f70cc5) |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughRanked pagination now reports the actual number of groups skipped. Proved and unproved reads use the same value, including past-end queries. Proved responses attest the value cryptographically. Grovedb dependencies, tests, and documentation use the updated behavior. ChangesRanked pagination
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to The change replaces linear ranked-query offset skipping with counted traversal and updates the reported skipped value past the end; the supplied validation indicates the behavior is merge-ready after normal checks, with no actionable merge-blocking risk remaining. Sequence Diagram(s)sequenceDiagram
participant RankedQuery
participant Grovedb
participant RankedPage
RankedQuery->>Grovedb: Request indexed top-K page with offset
Grovedb-->>RankedQuery: Return entries and actual skipped count
RankedQuery->>RankedPage: Map entries and preserve skipped count
RankedPage-->>RankedQuery: Return ranked pagination response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-08-13T20:38:07.478Z |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/rs-drive/src/query/drive_document_ranked_query/tests.rs (1)
1682-1694: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a separate tolerance constant for proof size.
byte_slackis derived as a storage-loaded-bytes allowance (256 bytes per tree level). Line 1690 reuses it as a proof-size tolerance. The two quantities are unrelated, so a later change to the storage allowance silently changes this tripwire. Define a distinct constant for the proof-size comparison.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-drive/src/query/drive_document_ranked_query/tests.rs` around lines 1682 - 1694, Define a dedicated proof-size tolerance constant near the proof-size assertion, rather than reusing byte_slack. Update the proof_bytes_at comparison to use this new constant, while leaving byte_slack exclusively for storage-loaded-bytes allowances.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-drive-abci/src/query/document_query/v1/tests.rs`:
- Around line 3058-3090: Update empty_ranking_proof_rejection and its tests so
only the exact supported GroveError::CorruptedData message “Cannot create proof
for empty tree” is reclassified as QueryError::InvalidArgument. Replace the
substring-based contains predicate with exact message matching, and add a test
case containing the marker within unrelated text that must remain unmapped.
In `@packages/rs-drive/src/query/drive_document_ranked_query/execute_top_k.rs`:
- Around line 171-180: Update the proof-read flow in the ranked query execution
around verify_indexed_axis_top_k_paginated to use snapshot isolation; if
unavailable, add a bounded retry at the dispatcher only when the error is the
specific chain-mismatch verification failure. Preserve immediate propagation for
all other proof or GroveDB errors.
---
Nitpick comments:
In `@packages/rs-drive/src/query/drive_document_ranked_query/tests.rs`:
- Around line 1682-1694: Define a dedicated proof-size tolerance constant near
the proof-size assertion, rather than reusing byte_slack. Update the
proof_bytes_at comparison to use this new constant, while leaving byte_slack
exclusively for storage-loaded-bytes allowances.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 09b317c6-215d-46bb-8681-50ff49f5eb3c
📒 Files selected for processing (12)
book/src/drive/ranked-index-examples.mdpackages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.hpackages/dapi-grpc/protos/platform/v0/platform.protopackages/rs-drive-abci/src/query/document_query/v1/mod.rspackages/rs-drive-abci/src/query/document_query/v1/tests.rspackages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/ranked_index_e2e_tests.rspackages/rs-drive/src/query/drive_document_ranked_query/execute_top_k.rspackages/rs-drive/src/query/drive_document_ranked_query/executors/top_k_no_proof.rspackages/rs-drive/src/query/drive_document_ranked_query/mod.rspackages/rs-drive/src/query/drive_document_ranked_query/mode_detection.rspackages/rs-drive/src/query/drive_document_ranked_query/tests.rspackages/rs-drive/src/verify/document_ranked/verify_ranked_top_k_proof/v0/mod.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4382 +/- ##
============================================
- Coverage 87.67% 87.33% -0.35%
============================================
Files 2710 2711 +1
Lines 345200 346464 +1264
============================================
- Hits 302667 302576 -91
- Misses 42533 43888 +1355
🚀 New features to boost your workflow:
|
|
Responding to the nitpick from the review body (it has no thread of its own): separate tolerance constant for proof size — agreed and done in 577fe3d. There's now a For the record, the two inline comments are answered in their own threads: the retry suggestion was adopted, and the exact-match suggestion for the empty-tree marker was rejected because grovedb wraps merk's constant in its own prefix, so exact matching would prevent the mapper from ever firing. |
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
You should not use the proved path for this, instead there are unproved ways that will make this fast, even faster.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The prover-backed ranked read removes the offset-proportional walk, but the new handler-local retry crosses the state-publication boundary without refreshing the captured PlatformState. A retry that succeeds after the GroveDB commit can therefore return new-state results or proof bytes with the previous block's metadata and signature, so this requires changes before merge.
Source: reviewer backend model gpt-5.6-sol; final verifier backend model gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 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-abci/src/query/document_query/v1/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/query/document_query/v1/mod.rs:1367-1369: Retrying only Drive execution can pair new-state results with old block metadata
The retry re-executes the Drive request while retaining the `platform_state` reference captured by `QueryService` before the query began. This is unsafe in the exact commit-visible/guard-not-yet-published window the retry is intended to cover: `update_state_cache_v0` publishes the new PlatformState before the database transaction commits, `finalize_block` commits GroveDB, and only afterward stores the new `committed_block_height_guard`. A query that captured the old state before publication can have its first envelope torn by the commit, then successfully rebuild against the newly committed GroveDB state here. Because the guard still has the old height, the service post-check sees it equal to the captured old state's height and accepts the response. Lines 1409-1417 then attach metadata—and, for proved responses, the old block signature and block ID—from that old PlatformState to new-state data or proof bytes. The retry must restart at a boundary that reloads PlatformState and reruns the service consistency checks; a successful local retry cannot safely be wrapped with the existing state object.
The `prove = false` arm of a ranked query skipped its OFFSET by stepping a storage iterator once per skipped entry, so the skip alone cost `Theta(min(offset, population))` on a surface where offset has no ceiling. Ranked queries carry no fee, cannot be cancelled once dispatched, and share their rate budget with state transitions rather than having one of their own, so that made the skip an unmetered cost lever for an unauthenticated caller. The proved path never had it: its prover attests the skipped region from the counted subtree commitments instead of traversing it. grovedb now exposes that same counted descent to plain reads (dashpay/grovedb#792): it reads each subtree's aggregate count off its link and collapses any subtree that fits inside the remaining offset rather than stepping through it. Point the unproved executor at it and the skip becomes `O(log n)` at any offset — and an offset at or past the population is answered from the root's own count with no descent at all, making the worst input the cheapest request rather than the most expensive. `offset = 0` keeps the plain iterator path and never touches the tree, so the common unpaginated request costs exactly what it did. Pinned to the grovedb branch rev so this is reviewable now; to be re-pinned to the develop merge commit before merge. BEHAVIOUR CHANGE, wire-visible on unproved responses `RankedPage::skipped`, which reaches the wire as `GetDocumentsResponseV1.ResultData.Ranked.skipped`, stops echoing the request. The old read could not report how far a short walk got, so the server echoed the requested offset back; the counted descent tracks it, so both paths now report the same quantity — the requested offset when the skip succeeded, the ranking's population when the walk ran out of groups first. A client asserting `skipped == requested_offset` will see a different value past the end; one using it as the rank base for `entries[i]`, its documented purpose, is unaffected. The value is not attested on the unproved path. It equals the attested one on an honest node, and nothing forces a node to be honest — the same trust model as the entries beside it. The proto, the Objective-C client that carries proto prose, the developer book and the Rust docs all say so rather than letting "the true population" read as a guarantee. Three comments asserted things the code did not do, including the justification for leaving OFFSET uncapped. They are corrected here rather than earlier because two of them state the policy, and an accurate description of an uncapped lever is only safe to publish alongside the thing that removes it. Tests: four assertions changed across ~3,400, every one a `skipped` value — three in drive, one on the wire in drive-abci. No entry or ordering assertion moved, which is the claim: the counted read returns what the linear walk returned. drive --lib 3386 passed; drive-abci --lib query:: 623 passed; cargo clippy --workspace --all-features and cargo fmt --check --all both clean.
577fe3d to
93806d7
Compare
577fe3d to
93806d7
Compare
shumkov
changed the title
`e41d57e0` exported `IndexedTopKPage` under `any(minimal, verify)` while
the module holding it is gated on `minimal` alone, so any build enabling
`verify` without `minimal` failed to compile:
error[E0432]: unresolved import `operations::indexed_tree`
note: found an item that was configured out — gated behind `minimal`
That is drive's verifier-only cut, which CI builds as "Check
transport-free feature cut" and which the Kotlin native-library job hits
too. `cc7b3997` narrows the export's gate to match the module's, and
adds a grovedb-side test pinning that the unproved `skipped` equals the
proved path's attested value — the property this PR's assertions rest on.
Re-pinned across all 14 workspace entries with `Cargo.lock` regenerated;
no reference to the old rev remains anywhere in the tree.
Verified with the exact invocation that reproduced the failure:
`cargo check -p drive --no-default-features --features verify`, clean.
…rration Two fixes from an independent review of the rebuilt diff. The past-the-end paragraph called `skipped` the ranking's "attested total population" while the paragraph below it correctly says an unproved response carries only the node's unverified claim, so the public contract contradicted itself within one comment. It now says "reported", and the warrant distinction is stated once, in the place that explains it. Synced across the proto, the Objective-C client that carries proto prose, and the book. The request-side proto text that said the response "echoes the attested number" is corrected the same way — it echoes nothing, and the number is only attested when proving. The comments also narrated the change rather than describing the code: "as it used to", "pinned as gone", "no longer has to", "the wire-visible half of that behaviour change". That reads as a diary to anyone who arrives after the change is history, which the repo's comment rule exists to prevent. Rewritten as current behaviour; the assertions are unchanged.
|
@coderabbitai review Please review the current head ( What changed since you last saw it:
Two of your earlier findings were adopted and remain relevant to the current code: the empty-tree mapper's narrowness, and the separate proof-size tolerance constant. Your suggestion to exact-match the empty-tree marker was declined, with reasoning in that thread — grovedb wraps merk's constant in its own prefix, so an equality predicate would never fire. |
|
🧠 Learnings used✅ Action performedReview finished.
|
`cc7b3997` predated the fixes for dashpay/grovedb#792's own Linting and codecov failures. `c4ceac67` is the rev where all 11 of that PR's checks pass, so this pins the code that was actually verified rather than an intermediate commit. All 14 workspace entries plus `Cargo.lock`; no reference to any earlier rev (`cc7b3997`, `e41d57e0`, `a2791bbd`) remains anywhere in the tree. Still a branch rev, deliberately: pinning the tested commit beats pinning an untested one, and the alternative is blocking on a merge. To be re-pinned to the develop merge commit once #792 lands, since a Platform PR pinning a branch that could later be deleted is a fair review objection. Verified: `cargo check -p drive --no-default-features --features verify` (the cut that caught the last feature-gate bug) clean; drive ranked 71 passed, drive-abci ranked 18 passed, fmt clean.
|
@coderabbitai review Superseding my previous request: review Your last substantive review was of
Two of your earlier findings were adopted and still apply to the current code: the empty-tree mapper's narrowness, and a separate proof-size tolerance constant. Your suggestion to exact-match the empty-tree marker was declined with reasoning in that thread — grovedb wraps merk's constant in its own prefix, so an equality predicate would never fire on a real failure. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== HEAD =='
git rev-parse HEAD
git status --short
echo '== target commit =='
git show --no-ext-diff --no-renames --format=fuller --stat 7091b33212
echo '== merge base and changed files =='
base="$(git merge-base 7091b33212 origin/master 2>/dev/null || git merge-base 7091b33212 origin/main)"
printf 'BASE=%s\n' "$base"
git diff --no-ext-diff --no-renames --name-status "$base" 7091b33212
echo '== changed implementation outline =='
ast-grep outline packages/rs-drive/src/query/drive_document_ranked_query/execute_top_k.rs --items all --view expanded
ast-grep outline packages/rs-drive-abci/src/query/document_query/v1/mod.rs --items all --view expanded
echo '== focused symbols and call sites =='
rg -n -C 5 --glob '*.rs' \
'indexed_.*_top_k_paginated|IndexedTopKPage|empty_ranking_proof_rejection|proof_size_slack|byte_slack|execute_top_k_with_proof|execute_top_k' \
packages/rs-drive packages/rs-drive-abci
echo '== target diff =='
git diff --no-ext-diff --no-renames --unified=80 "$base" 7091b33212 -- \
packages/rs-drive/src/query/drive_document_ranked_query \
packages/rs-drive-abci/src/query/document_query/v1 \
packages/rs-drive/Cargo.toml \
packages/rs-drive-abci/Cargo.toml \
packages/rs-dpp/Cargo.toml \
packages/rs-platform-version/Cargo.toml \
packages/rs-platform-wallet/Cargo.toml \
packages/rs-sdk/Cargo.toml \
packages/dapi-grpc/protos/platform/v0/platform.proto \
book/src/drive/ranked-index-examples.mdLength of output: 3619 🏁 Script executed: #!/bin/bash
set -u
echo '== refs =='
git for-each-ref --format='%(refname:short) %(objectname)' refs/remotes refs/heads | sort
echo '== recent ancestry =='
git log --oneline --decorate --graph -25 HEAD
echo '== candidate merge bases =='
for ref in origin/develop develop upstream/develop origin/master master origin/main main; do
if git rev-parse --verify -q "$ref" >/dev/null; then
printf '%s ' "$ref"
git merge-base HEAD "$ref" || true
fi
done
echo '== all commits after a likely base, if origin/develop exists =='
if git rev-parse --verify -q origin/develop >/dev/null; then
base="$(git merge-base HEAD origin/develop)"
printf 'BASE=%s\n' "$base"
git log --oneline --reverse "$base"..HEAD
echo '== changed files =='
git diff --no-ext-diff --no-renames --name-status "$base"..HEAD
fiLength of output: 3542 🏁 Script executed: #!/bin/bash
set -euo pipefail
base="6495991c5e21f4f211ba326334ce3f717aaaf3bd"
head="7091b332120f35e7155a7867fa98a209c22bca6a"
echo '== changed files =='
git diff --no-ext-diff --no-renames --name-status "$base" "$head"
echo '== implementation outlines =='
ast-grep outline packages/rs-drive/src/query/drive_document_ranked_query/execute_top_k.rs --items all --view expanded
ast-grep outline packages/rs-drive/src/query/drive_document_ranked_query/mod.rs --items all --view expanded
ast-grep outline packages/rs-drive-abci/src/query/document_query/v1/mod.rs --items all --view expanded
echo '== direct read calls, result handling, and empty-tree mapping =='
rg -n -C 8 --glob '*.rs' \
'indexed_(value|sum|count|average)_top_k_paginated|IndexedTopKPage|empty_ranking_proof_rejection|CorruptedData|proof_size_slack|byte_slack' \
packages/rs-drive/src/query/drive_document_ranked_query \
packages/rs-drive-abci/src/query/document_query/v1
echo '== source diff: ranked implementation and ABCI handling =='
git diff --no-ext-diff --no-renames --unified=100 "$base" "$head" -- \
packages/rs-drive/src/query/drive_document_ranked_query \
packages/rs-drive-abci/src/query/document_query/v1
echo '== source diff: API docs and dependency manifests =='
git diff --no-ext-diff --no-renames --unified=60 "$base" "$head" -- \
packages/dapi-grpc/protos/platform/v0/platform.proto \
packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.h \
book/src/drive/ranked-index-examples.md \
packages/rs-dpp/Cargo.toml \
packages/rs-drive/Cargo.toml \
packages/rs-drive-abci/Cargo.toml \
packages/rs-platform-version/Cargo.toml \
packages/rs-platform-wallet/Cargo.toml \
packages/rs-sdk/Cargo.toml \
Cargo.lock
echo '== stale GroveDB pins outside the lockfile =='
rg -n --glob 'Cargo.toml' --glob 'Cargo.lock' \
'a2791bbdca756d6a6113024aec48f09f7a33faa9|cc7b3997be5819c60eb09251f963c1ed1a89306f|e41d57e0|c4ceac67' .Length of output: 199377 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== ranked response conversion =='
rg -n -C 12 --glob '*.rs' \
'DocumentRankedResponse|RankedPage|skipped:|set_skipped|RankedEntries' \
packages/rs-drive/src/query/drive_document_ranked_query \
packages/rs-drive-abci/src/query/document_query/v1/dispatch
echo '== ranked dispatch execution path =='
sed -n '80,190p' packages/rs-drive-abci/src/query/document_query/v1/dispatch/ranked.rs
sed -n '1,135p' packages/rs-drive/src/query/drive_document_ranked_query/execute_top_k.rs
sed -n '135,255p' packages/rs-drive/src/query/drive_document_ranked_query/execute_top_k.rs
echo '== proof-size tolerance definition and uses =='
rg -n -C 12 --glob '*.rs' 'proof_size_slack|byte_slack|proof.*slack|slack.*proof' packages/rs-drive packages/rs-drive-abci
echo '== offset test cases across axes =='
rg -n -C 8 --glob '*.rs' \
'at_offset\(|offset_pages|past_end|skipped.*[=,].*[0-9]|IndexedTopKPage' \
packages/rs-drive/src/query/drive_document_ranked_query/tests.rs \
packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/ranked_index_e2e_tests.rs \
packages/rs-drive-abci/src/query/document_query/v1/tests.rs
echo '== pinned GroveDB source candidates =='
find "${HOME:-/root}/.cargo/git/checkouts" -type f \
\( -name '*.rs' -o -name 'Cargo.toml' \) \
-print 2>/dev/null | grep -E '/grovedb[^/]*/|/grovedb-' | head -n 20 || true
find /tmp -type f -name '*.rs' -print 2>/dev/null | grep -E 'grovedb|indexed.*top.*k' | head -n 20 || trueLength of output: 113333 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Avg call without overlapping output =='
nl -ba packages/rs-drive/src/query/drive_document_ranked_query/execute_top_k.rs | sed -n '125,155p'
echo '== proof-size tolerance candidates across the repository =='
rg -n -i -C 8 \
'proof.{0,40}(size|slack|limit|bound|tolerance)|\
(size|slack|limit|bound|tolerance).{0,40}proof|\
byte_slack' \
. \
-g '!Cargo.lock' \
-g '!**/node_modules/**' || true
echo '== all changed lines not previously inspected =='
git diff --no-ext-diff --no-renames --unified=35 \
6495991c5e21f4f211ba326334ce3f717aaaf3bd \
7091b332120f35e7155a7867fa98a209c22bca6a -- \
packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/ranked_index_e2e_tests.rs \
packages/rs-drive/src/query/drive_document_ranked_query/mode_detection/v0/mod.rs \
packages/rs-drive/src/query/drive_document_ranked_query/tests.rs \
packages/rs-drive-abci/src/query/document_query/v1/tests.rs
echo '== GroveDB source tree paths at the pinned revision =='
gh api "repos/dashpay/grovedb/git/trees/c4ceac676630727d01b532314a7e488edbeaf115?recursive=1" \
--jq '.tree[] | select(.path | test("indexed.*top.*k|top.*k.*indexed"; "i")) | [.path, .url] | `@tsv`' \
|| trueLength of output: 1624 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Avg call =='
sed -n '125,155p' packages/rs-drive/src/query/drive_document_ranked_query/execute_top_k.rs
echo '== proof-size tolerance candidates across the repository =='
rg -n -i -C 8 \
'proof.{0,40}(size|slack|limit|bound|tolerance)|(size|slack|limit|bound|tolerance).{0,40}proof|byte_slack' \
. \
-g '!Cargo.lock' \
-g '!**/node_modules/**' || true
echo '== remaining changed test lines =='
git diff --no-ext-diff --no-renames --unified=35 \
6495991c5e21f4f211ba326334ce3f717aaaf3bd \
7091b332120f35e7155a7867fa98a209c22bca6a -- \
packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/ranked_index_e2e_tests.rs \
packages/rs-drive/src/query/drive_document_ranked_query/tests.rs \
packages/rs-drive-abci/src/query/document_query/v1/tests.rs
echo '== GroveDB source tree paths at the pinned revision =='
gh api "repos/dashpay/grovedb/git/trees/c4ceac676630727d01b532314a7e488edbeaf115?recursive=1" \
--jq '.tree[] | select(.path | test("indexed.*top.*k|top.*k.*indexed"; "i")) | [.path, .url] | `@tsv`' \
|| trueLength of output: 201544 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== exact proof-size tolerance identifiers =='
rg -n -C 6 --glob '*.rs' --glob '*.md' --glob 'Cargo.toml' \
'\bproof_size_slack\b|\bbyte_slack\b|\bproof.*slack\b|\bslack.*proof\b' \
packages/rs-drive \
packages/rs-drive-abci \
packages/rs-drive-proof-verifier \
packages/rs-sdk \
2>/dev/null || true
echo '== pinned GroveDB commit =='
gh api "repos/dashpay/grovedb/commits/c4ceac676630727d01b532314a7e488edbeaf115" \
--jq '{sha: .sha, message: .commit.message, parents: [.parents[].sha]}' \
|| true
echo '== final direct-read call inventory =='
rg -n -C 3 --glob '*.rs' \
'indexed_(count|sum|avg)_top_k_paginated\(' \
packages/rs-drive/src/query/drive_document_ranked_queryLength of output: 4821
I checked the direct Count, Sum, and Avg paginated reads. Each path returns GroveDB’s The Drive and ABCI tests cover the past-end case. They verify that an unproved response reports the reached population. The proved result reports the same value. I also checked the narrow empty-tree mapping using the prior review context. It remains limited to All GroveDB dependencies now resolve to I did not run builds or tests because the review sandbox does not permit repository code execution. 🧠 Learnings used✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The counted traversal removes offset-linear work, but positive-offset unproved reads now traverse with unsnapshotted point reads and can cross a block commit, returning mixed data under stale block metadata; this is blocking. The API request docs also still describe the skip as count-attested even though only proof results are attested, while the previous handler-local retry finding is fixed. Source: reviewer backend model gpt-5.6-sol; final verifier backend model gpt-5.6-sol; openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 1 suggestion(s)
🤖 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_ranked_query/execute_top_k.rs`:
- [BLOCKING] packages/rs-drive/src/query/drive_document_ranked_query/execute_top_k.rs:89-95: Counted reads lose the iterator's consistent view across a commit
For a positive offset below the population, these calls reach GroveDB's `counted_skip_page`, which loads children through successive `RefWalker::walk` point reads. The dispatcher passes `transaction = None`; at pinned GroveDB revision `c4ceac676630727d01b532314a7e488edbeaf115`, `TxRef::new` therefore creates an `OptimisticTransactionDB::transaction()` with default options, whose snapshot setting is false, and each child fetch ultimately calls `Transaction::get`. The replaced implementation collected the skipped and returned rows through one RocksDB iterator, which pins one view for that scan. If a block commits during the new descent, the resident parent can be from the old state while a fetched child comes from the new state. Merk's child load preserves the parent's recorded hash without checking it against the fetched child, and the counted traversal only cross-checks aggregate counts, so a same-population update can produce a mixed page rather than an error. In the existing interval after the GroveDB commit but before `committed_block_height_guard` is stored, the query service can also accept that page and attach metadata from the previously captured `PlatformState`. Run the counted traversal against a storage snapshot, or defer this switch until GroveDB exposes a snapshot-consistent counted-read API.
In `packages/dapi-grpc/protos/platform/v0/platform.proto`:
- [SUGGESTION] packages/dapi-grpc/protos/platform/v0/platform.proto:1128-1131: Request docs still describe the unproved skip as attested
The `offset` field applies to both proved and unproved ranked requests, but this paragraph still says the skip is count-attested and only describes GroveDB proving it. The implementation and the corrected response-field documentation distinguish the two paths: both use counted descent, but only the proved result attests the count. Update this paragraph and regenerate the Objective-C header, whose corresponding prose still says the response echoes an attested number.
| @@ -71,14 +94,18 @@ impl DriveDocumentRankedQuery<'_> { | |||
| transaction, | |||
| grove_version, | |||
There was a problem hiding this comment.
🔴 Blocking: Counted reads lose the iterator's consistent view across a commit
For a positive offset below the population, these calls reach GroveDB's counted_skip_page, which loads children through successive RefWalker::walk point reads. The dispatcher passes transaction = None; at pinned GroveDB revision c4ceac676630727d01b532314a7e488edbeaf115, TxRef::new therefore creates an OptimisticTransactionDB::transaction() with default options, whose snapshot setting is false, and each child fetch ultimately calls Transaction::get. The replaced implementation collected the skipped and returned rows through one RocksDB iterator, which pins one view for that scan. If a block commits during the new descent, the resident parent can be from the old state while a fetched child comes from the new state. Merk's child load preserves the parent's recorded hash without checking it against the fetched child, and the counted traversal only cross-checks aggregate counts, so a same-population update can produce a mixed page rather than an error. In the existing interval after the GroveDB commit but before committed_block_height_guard is stored, the query service can also accept that page and attach metadata from the previously captured PlatformState. Run the counted traversal against a storage snapshot, or defer this switch until GroveDB exposes a snapshot-consistent counted-read API.
source: ['codex']
There was a problem hiding this comment.
Valid, and it is a regression this PR introduced. Leaving this thread open until the fix lands rather than closing it on intent.
I verified all three claims against the pinned rev c4ceac67 rather than taking the finding on trust:
snapshot: false—storage/src/rocksdb_storage/storage.rs:630-632isfn start_transaction(&'db self) -> Self::Transaction { self.db.transaction() }: bare, no options. The onlyset_snapshot*anywhere in the storage layer isset_snapshot_consistency(false)atstorage.rs:568, on the unrelated SST-ingest path.- The replaced implementation really did pin one view —
grovedb/src/operations/indexed_tree.rs:1385built a singleKVIterator::new(secondary_merk.storage.raw_iter(), &q)and drove the entirewhile … next_kv()scan from it. A RocksDB iterator takes an implicit snapshot at creation, so the skip and the collect ran against one view. - The new path does independent point reads —
counted_skip_pagedescends viamerk.walk(...)/RefWalker, each child fetch aTransaction::get. And merk's walk does not verify a fetched child against the parent's recorded link hash, so a torn descent yields a mixed page silently rather than an error.
So the trade was a single consistent scan for O(log n) independent reads, without replacing the guarantee the iterator was providing for free.
Worth adding the asymmetry that explains why this is specific to the unproved path: the proved path performs many independent reads too, but a torn read there fails — the verifier's ancestor-chain reconciliation catches it. The unproved read has no such check, which is exactly why the missing snapshot is load-bearing here and not there.
Remedy agreed: grovedb takes the snapshot, at the layer that owns the traversal, covering both the descent and the collect. Platform cannot fix this from its side — start_transaction takes no options, so there is no snapshotted transaction to pass in. Deferring the switch was considered and rejected: it means reverting to the linear read, which reinstates the uncapped-offset cost lever this change exists to remove.
Explicitly out of scope, as pre-existing rather than introduced here: the window between a grovedb commit becoming visible and committed_block_height_guard being stored, in which the query service can attach stale PlatformState metadata. That applies to any read on this surface and is mostly absorbed by the existing retry in query/service.rs.
The accompanying suggestion is already fixed: the offset request-side paragraph and the matching Objective-C prose no longer describe the unproved skip as count-attested — both now say the descent is counted on either path and only the proved result attests the count.
There was a problem hiding this comment.
Fixed, and consumed here — re-pinned to grovedb 0100cb83.
The fix serves the whole counted page — root, descent and collect — from one transaction raw iterator with a pinned snapshot plus the transaction overlay, and drops RefWalker from the walk. That is the same mechanism the linear scan it replaced used, so the guarantee this thread identified as missing is restored at the layer that owns the traversal, rather than worked around in Platform.
Cost: one extra seek. Deep offset 22 → 23, offset 0 unchanged at 5, past-the-end flat at 4. The offset-independence this PR is for is unaffected.
What is and is not proven, stated plainly. The transaction-overlay half is pinned by a new test. The commit-interleave half — a block committing mid-descent — is not deterministically testable: there is no fetch-pause hook to force a commit at a chosen point in the walk. That half rests on the mechanism (a single pinned-snapshot iterator cannot observe a write that lands after it is created) rather than on a test asserting it. I would rather say so than let "fixed" imply coverage that does not exist.
| // OFFSET 4` is the 5th-best group. The skip is **count-attested**, | ||
| // not walked: grovedb proves it from the counted subtree | ||
| // commitments, so the proof stays `O(log n + k)` at any offset and | ||
| // the response echoes the attested number in | ||
| // the response reports the skip it performed in |
There was a problem hiding this comment.
🟡 Suggestion: Request docs still describe the unproved skip as attested
The offset field applies to both proved and unproved ranked requests, but this paragraph still says the skip is count-attested and only describes GroveDB proving it. The implementation and the corrected response-field documentation distinguish the two paths: both use counted descent, but only the proved result attests the count. Update this paragraph and regenerate the Objective-C header, whose corresponding prose still says the response echoes an attested number.
| // OFFSET 4` is the 5th-best group. The skip is **count-attested**, | |
| // not walked: grovedb proves it from the counted subtree | |
| // commitments, so the proof stays `O(log n + k)` at any offset and | |
| // the response echoes the attested number in | |
| // the response reports the skip it performed in | |
| // OFFSET 4` is the 5th-best group. The skip is **counted, not | |
| // walked** on both paths: grovedb descends using aggregate subtree | |
| // counts, while the proved path additionally attests those counts | |
| // in its proof. Work and proof size stay `O(log n + k)` at any | |
| // offset, and the response reports the skip it performed in |
source: ['codex']
The counted descent performed several point reads where the linear scan it replaced performed one, and nothing replaced the consistency the iterator had been providing for free. A block committing mid-descent could pair a parent from the old state with a child from the new one, and merk does not verify a fetched child against the parent's recorded link hash, so the result was a silently mixed page rather than an error. The proved path is not exposed the same way: a torn read there fails the verifier's ancestor-chain reconciliation. grovedb `0100cb83` serves the whole page — root, descent and collect — from one transaction raw iterator with a pinned snapshot plus the transaction overlay, the same mechanism the linear scan used, and drops `RefWalker` from the walk. The guarantee is restored where the traversal lives rather than worked around here. Cost is one extra seek: deep offset 22 -> 23, offset 0 unchanged at 5, past-the-end flat at 4. Testing scope, stated rather than implied: the transaction-overlay behaviour is pinned by a new grovedb test. The commit-interleave case is not deterministically testable — there is no hook to pause a fetch and force a commit mid-descent — so that half rests on the mechanism, not on a test. Also folds a review suggestion: the `offset` request-side prose in the proto and the Objective-C client no longer calls the unproved skip count-attested. Both paths use the counted descent; only the proved result attests the count. Verified: narrow cut `cargo check -p drive --no-default-features --features verify` clean, drive ranked 71 passed, drive-abci ranked 18 passed, fmt clean.
3 participants
Issue being fixed or feature implemented
Ranked queries (
SELECT <agg> GROUP BY <prop> ORDER BY <agg> LIMIT k OFFSET m) accept anunbounded
OFFSET, and the comment justifying that described the proving path only:The unproved arm did not work that way. It skipped by stepping a storage iterator once per
skipped entry, so the skip alone cost
Θ(min(offset, population))— work proportional to anumber the caller chooses, on a path where nothing bounds it: ranked queries carry no fee, the
dispatcher does no cost accounting, a
spawn_blockingquery cannot be cancelled by clientdisconnect or stream reset, and the gateway's only rate limit is per source IP across the whole
Platform service, so a query competes with state transitions rather than having its own budget.
The ranked surface exists only in
v4.2.0-dev.1; no stable release contains it.What was done?
grovedb gained a counted traversal for plain reads (dashpay/grovedb#792), which is the same
descent its prover already used, minus the proof. It reads each subtree's aggregate count off its
link and collapses any subtree that fits entirely inside the remaining offset rather than stepping
through it. This PR points the unproved executor at it.
Consequences:
O(log n)at any offset, not proportional to it.all — the pathological input becomes the cheapest request on the surface rather than the most
expensive.
offset = 0is untouched: it keeps the plain iterator path and never reads the tree, so thecommon unpaginated request costs exactly what it did before. grovedb pins that with an
always-on equality test.
Measured by grovedb's own harness (seek/byte counters are the machine-independent signal; the full
grid is in that PR). At a million rows, a deep offset drops from a full linear scan to a
tree-depth descent — 22 seeks, ~3.7 KB, ~32 µs — and past the end to a flat 3 seeks / 366 B / 4 µs
at every population size. The counters scale as tree depth (11 → 15 → 18 → 22 across 1e3 → 1e6),
which is the shape the design predicts.
One corner measured and accepted rather than hidden: at a small positive offset with
k = 100,the counted path costs about 5× the linear read in wall-clock (~155 µs against ~30 µs) because
k tree point-gets are slower than k sequential iterator steps. Crossover to counted-wins sits
a few hundred rows in, worst measured cost is ~155 µs, and the alternative — a threshold hybrid
falling back to the linear skip below some offset — would make the skipped-region semantics depend
on the offset value. Uniform semantics won.
Pin: currently the grovedb branch rev, so this is reviewable now; to be re-pinned to the
develop merge commit before merge. That is a one-line change and does not invalidate review of
anything else here.
Supersedes this PR's own earlier approach. It previously served unproved reads by generating a
proof internally and verifying it to recover the entries. That worked, but it paid proof
construction, serialization and verification on every read, put a floor under the common
offset = 0case, and its retry drew a blocking review for pairing new-state results with oldblock metadata. The counted read removes the floor and, by having no proof envelope on the read
path, removes the retry and the state/metadata window with it. History was rewritten because the
old commits implemented an approach the diff no longer contains.
How Has This Been Tested?
cargo test -p drive --lib→ 3386 passed, 0 failedcargo test -p drive-abci --lib query::→ 623 passed, 0 failedcargo clippy --workspace --all-features→ cleancargo fmt --check --all→ cleanThe informative result is which assertions moved. Across ~3,400 tests, four needed changing
and every one was a
skippedvalue — three in drive, one on the wire in drive-abci. No entry orordering assertion moved, on any axis, in either direction, at any offset or
k. That is theclaim this change stakes itself on: the counted read returns exactly what the linear walk
returned, and only the reported skip differs.
Read consistency
The counted page — root, descent and collect — is served from a single transaction raw iterator
with a pinned snapshot plus the transaction overlay, which is the same consistency mechanism the
linear scan it replaces relied on. This matters because the descent performs several reads where
the old scan performed one: without a pinned view, a block committing mid-descent could pair a
parent from the old state with a child from the new one, and merk does not verify a fetched child
against the parent's recorded link hash, so the result would be a silently mixed page rather than
an error. The proved path is not exposed the same way — a torn read there fails the verifier's
ancestor-chain reconciliation — which is why this was specific to the unproved read.
Cost of the guarantee: one extra seek (deep offset 22 → 23; offset 0 unchanged at 5; past-the-end
flat at 4).
Scope of the testing, stated rather than implied: the transaction-overlay behaviour is pinned by a
test. The commit-interleave case is not deterministically testable — there is no hook to pause a
fetch and force a commit mid-descent — so that half is argued from the mechanism, not proven by a
test.
A gate lesson worth keeping
This PR broke CI in a way
cargo clippy --workspace --all-featuresstructurally cannot catch, and that is worth writing down because the opposite advice is commonly given.The pinned grovedb rev exported a type under
any(minimal, verify)while the module holding it was gated onminimalalone. Any build enablingverifywithoutminimalfailed witherror[E0432]: unresolved import.--all-featuresturns every feature on, so the broken combination never occurs and the check passes; the failure only appears in a narrow cut, and it took CI'sCheck transport-free feature cutjob — reproducible locally ascargo check -p drive --no-default-features --features verify— to surface it. The Kotlin native-library job hit the same error for the same reason.So the two gates catch different classes and neither substitutes for the other:
--workspace --all-featurescatches unbuilt sibling crates and feature-gated callers of a changed API. It is blind to feature-gating bugs.--no-default-features --features verify, and the other combinations CI builds) catch gating bugs. They are blind to most of what breadth catches.If you are changing a
#[cfg], adding a re-export, or bumping a dependency that does either, run the cut as well as the breadth build.Breaking Changes
No API or wire format change. One wire-visible behaviour change on unproved responses.
RankedPage::skipped—GetDocumentsResponseV1.ResultData.Ranked.skippedon the wire — stopsechoing the request. The old read could not tell how far a short walk got, so the server echoed
the requested offset back. The counted descent tracks it, so both paths now report the same
quantity: the requested offset when the skip succeeded, and the ranking's total population when
the walk ran out of groups first. On a five-group ranking asked for a page well past the end, an
unproved response now reports
5where it previously reported the offset.A client asserting
skipped == requested_offsetsees a different value past the end. A clientusing it as the rank base for
entries[i]— its documented purpose — is unaffected, and gains apopulation count it previously had to prove to obtain.
The value is not attested on the unproved path. It equals the attested one on an honest node,
and nothing forces a node to be honest — the same trust model as the entries beside it. The proto,
the Objective-C generated client (the only generated client carrying proto prose), the developer
book and the Rust docs all say so explicitly, so "the true population" is not read as a
cryptographic guarantee. One further nuance is documented at the field: the population comes from
the secondary's root aggregate while the per-node payload check only fires on visited nodes, so in
a corrupt secondary the unproved value can disagree with the true row count where the proved one
would not. On any valid secondary they are identical by construction.
Mixed-network note: the ranked surface first appears in
v4.2.0-dev.1, so a network mixingthat tag with newer nodes returns the echoed offset from one and the population from the other for
the same unproved request. Devnet-only exposure, and the proto's new "do not assume this field
equals the offset you requested" advice is safe against both.
Comments corrected
Three comments asserted things the code did not do, one of them the justification for leaving
OFFSETuncapped. They land here rather than earlier on purpose: two of them state the policy, andan accurate description of an uncapped cost lever is only safe to publish alongside the change that
removes it.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Bug Fixes
Documentation