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 8 commits into
Conversation
|
✅ Final review complete — no blockers (commit c1b8601) |
|
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-14T06:16:26.853Z |
![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 84.83% 87.32% +2.48%
============================================
Files 2711 2711
Lines 356784 346506 -10278
============================================
- Hits 302667 302574 -93
+ Misses 54117 43932 -10185
🚀 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.
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.
|
@thepastaclaw please re-review the current head, Your blocking finding on Stated plainly because it bears on how much the fix is worth: the transaction-overlay half is pinned by a new grovedb test; the commit-interleave half 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 rather than proven by a test. Your suggestion on the |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The dependency re-pin fixes mixed node reads during descent, and the request-side documentation now correctly distinguishes counted unproved reads from cryptographically attested proved reads. However, GroveDB still discovers the secondary root key before creating the pinned iterator, so a root rotation between those steps can make a demoted node appear to be the complete ranking and silently truncate an unproved page; this remains blocking.
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 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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
(existing thread: https://github.com/dashpay/platform/pull/4382#discussion_r3778638772)
The new GroveDB revision pins root-node, descent, and collection reads to one raw iterator, but root-key discovery remains outside that pinned view. At GroveDB `0100cb83`, `indexed_axis_top_k_paginated_generic` calls `open_validated_axis_secondary` first; that function reads `secondary_root_key` from the parent through the snapshotless transaction. Only afterward does `counted_skip_page` create the raw iterator and re-fetch the node identified by that earlier key. If a commit rotates the secondary AVL root between these operations, the old root key can remain present in the new snapshot as a demoted child. The re-fetch therefore succeeds instead of triggering the absent-root guard, and the traversal treats that child's valid aggregate and links as the complete ranking. Internal count checks still pass because the node is a valid subtree, but the returned entries and `skipped` value are silently truncated. Root-key discovery must share the same pinned view as the secondary traversal, or the traversal must independently establish and validate the actual secondary root in its snapshot.
…shot The previous pin put the root node, the descent and the collect behind one pinned iterator, but root-key *discovery* stayed outside it: `open_validated_axis_secondary` read `secondary_root_key` from the parent through the snapshotless transaction, and only afterwards did the traversal create its iterator and re-fetch the node that key names. A commit rotating the secondary's AVL root between those two steps left the old root key still present in the new snapshot — as a demoted child. The re-fetch then succeeded rather than tripping the absent-root guard, and the walk treated that child's aggregate and links as the whole ranking. The internal count checks pass, because a demoted child is a valid subtree; the page and its `skipped` are simply truncated, without an error. grovedb `63df14c2` creates the pinned view first, fetches the indexed-tree element through it, and derives `secondary_root_key` from that same snapshot, so discovery and traversal cannot straddle a rotation. An element absent from the read snapshot is now an explicit error rather than a silent fallback. Testing scope, unchanged in character from the previous fix: the snapshot-visibility behaviour is pinned by grovedb's tests; the commit-interleave case remains not deterministically testable, for want of a hook to pause a fetch and force a commit mid-descent, so that half is argued from the mechanism rather than proven. Verified: narrow cut `cargo check -p drive --no-default-features --features verify` clean, drive ranked 71 passed, drive-abci ranked 18 passed, fmt clean.
|
@thepastaclaw please re-review the current head, Your carried-forward blocker on grovedb Testability, stated as before: the snapshot-visibility behaviour is pinned by grovedb's tests; the commit-interleave case is not deterministically testable for want of a fetch-pause hook, so that half is argued from the mechanism — there is no longer an interval between discovery and traversal for a rotation to land in. |
|
@QuantumExplorer — your review is on What you reviewed vs what is here now
Why it changed. The prove-then-verify version drew a blocking finding: the retry could pair new-state results with old block metadata. Rather than make the retry state-safe, the mechanism that required it was removed. Measured: Two defects found and fixed during review, both in our own work — these are the parts worth your attention:
Stated limitation, not glossed: the tx-overlay half of the consistency fix is covered by a test; the commit-interleave half is argued from the mechanism, because no fetch-pause hook exists to force a commit mid-descent. Dependency: pins grovedb CI is green (22 checks, zero failures) and all review threads are resolved. The only thing outstanding is this review. |
|
@thepastaclaw re-review request (repeat): current head is Your last verdict was on That is fixed. The pin moved to grovedb Stated limitation, unchanged: the tx-overlay half is covered by a test; the commit-interleave half is argued from the mechanism, as no fetch-pause hook exists to force a commit mid-descent. CI is green (22 checks, zero failures) and all threads are resolved. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The GroveDB re-pin fixes the prior snapshot-consistency blocker: root-key discovery, root loading, descent, and collection now use one pinned raw-iterator view. One in-scope documentation suggestion remains because two internal comments still describe the shared proved/unproved ranked route using prover-only attestation language.
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 zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 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/routing.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/query/document_query/v1/routing.rs:281-284: Internal OFFSET docs still describe the skip as attested
This routing gate applies to both proved and unproved ranked requests, but its rationale still describes only the prover and says GroveDB attests the skipped region. The unproved executor performs the same counted descent without a proof and returns the node's unverified report; only the proved path cryptographically attests the count. The same obsolete prover-only description remains in the module-level documentation at `packages/rs-drive/src/query/drive_document_ranked_query/mod.rs:58-61`. Update both comments to match the corrected public API documentation: both paths count rather than walk the skipped region, while only the proved path attests the result.
…proved Five internal comments still explained the uncapped `OFFSET` in prover-only terms — "grovedb attests the skipped region from counted subtree commitments" — on routes that serve both `prove` settings. Since the unproved read performs the same counted descent and returns the node's own report, that wording implies a guarantee an unproved response does not carry, which is the exact confusion the public API docs were corrected for earlier in this PR. They now say the skipped region is *counted* from the subtree aggregates rather than walked, that both executors take that descent, and that only the proved result additionally attests the count. The review named two sites; a sweep of the surface found three more, in `rs-drive-proof-verifier` and the SDK's ranked query builder and result type — the places a client author reads before deciding whether to prove. The two comments that already distinguished the paths correctly (`mode_detection`, the book chapter) are unchanged. Comment-only; no behaviour change, so no test.
|
Fixed in The two you named are corrected: Sweeping the rest of the surface for the same pattern turned up three more, all in places a client author reads before deciding whether to prove:
All three said grovedb "attests" the skipped region while describing behaviour that now applies to both Left alone deliberately: |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The counted GroveDB traversal is wired into the unproved ranked-query paths, and the previous attestation-language finding is fixed. One in-scope documentation suggestion remains: several comments promise equal cost across offsets even though the implementation guarantees an offset-independent asymptotic bound and deliberately uses paths with different concrete costs.
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 zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 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-sdk/src/platform/documents/document_query.rs`:
- [SUGGESTION] packages/rs-sdk/src/platform/documents/document_query.rs:366-369: Public offset docs still promise equal cost at every offset
The implementation does not give shallow and deep offsets exactly the same concrete cost. `offset == 0` retains the sequential iterator fast path, positive offsets use counted tree traversal, and an offset at or past the population can stop after inspecting the root aggregate. GroveDB's own code and cost tests distinguish these paths, and the PR measurements likewise report materially different timings. The actual guarantee needed to justify leaving the offset uncapped is that skip work is bounded by tree depth instead of growing with the requested offset. State that `O(log n + k)` bound rather than equal cost here, in `packages/dapi-grpc/protos/platform/v0/platform.proto:1136`, its generated Objective-C prose, and `packages/rs-sdk/src/platform/documents/document_ranked_entries.rs:348-352`.
… same The public offset docs promised that a deep offset "costs exactly what a shallow one does". That was a fair shorthand when the alternative was a linear walk, but it is not what the counted read does, and the SDK docs are where a caller forms their cost model. Three shapes have three costs: `offset = 0` keeps the sequential fast path, a positive offset descends the tree in `O(log n)`, and an offset at or past the population is answered from the root without descending at all. What is true — and what matters for the absence of a ceiling — is that none of them grows with how far you page. Reworded across the SDK query builder, the ranked result type, the proof-verifier module docs, and the proto plus its Objective-C copy, which now says the two offsets cost the same *order* of work rather than the same work. Comment-only; no behaviour change, so no test.
|
@QuantumExplorer — requesting your re-review when you have a moment. Your There is a full orientation comment above with a then/now table and the three defects found and fixed during review — but the short version is that everything else is now clear: CI green with zero failures, all review threads resolved, and thepastaclaw's final validation on the current head reports zero blockers after having found and confirmed the fix of all three. Yours is the last outstanding review. Happy to walk through any part of it. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The counted ranked-query pagination change correctly propagates GroveDB's actual skipped count, and the reviewed evidence identifies no remaining in-scope defects at the exact head. The prior documentation finding is fixed: the public docs now distinguish the offset-zero fast path, positive-offset counted descent, and root-only past-end case while stating the bounded asymptotic cost instead of identical concrete cost.
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 zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
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