feat: count-provable sum-axis secondaries + count-bound offset proofs for all indexed axes - #791
Merged
Merged
Conversation
… for all indexed axes Part 1 — the sum axis's secondary Merk (PSIT, and PCPSIT's sum axis) is now a ProvableCountProvableSumTree instead of a ProvableSumTree. Each SumItem row contributes (count = 1, sum), so every node commitment binds a subtree count alongside the sum (~8 extra feature bytes per node). The primary stays lean (ProvableSummedMerkNode, unchanged). The change is a single arm in axis_secondary_tree_type; open/mirror/batch/ verify_grovedb/estimators all derive from it, and the proof node family for sum-axis secondary proofs moves to the KVCountSum/KVHashCountSum/ KVDigestCountSum/KVRefValueHashCountSum/HashWithCountAndSum set automatically via feature-type-driven dispatch. Storage format amended in place — the indexed-tree family is unreleased (PV14 unshipped). Part 2 — with every axis's secondary now count-bearing, the sum axis's offset-paginated proof rides Merk::prove_count_offset_on_range like count and avg already did: the skipped prefix is attested by counted subtree commitments (O(log n + k) regardless of offset) instead of the old O(offset + k) enumeration fallback, whose u16 offset+k ceiling is gone. The verifier's skipped count is now cryptographically attested for all axes; offset-past-end verifies as an empty page with skipped < offset, which is itself a proof the total population equals skipped. The gate is structural (count-bearing secondary tree type), not per-axis. Also: - prove/verify_indexed_axis_rank_of_key: "key X is at rank R" as a paginated window (offset = R, k = 1) whose yielded entry binds X; rank computed O(log n) from the secondary's count aggregates. - Envelope decoders now reject trailing bytes (proof byte-malleability). - proof_node_type family sets gain the PSIT/PCPSIT primary arms, mirroring the existing PCIT arm. - New test suite indexed_axis_offset_proof_tests: offset 0 == top-k, mid-walk offsets, windows spanning/past the end, single-entry rank windows, ties straddling the offset boundary, both directions, bit-flip/truncation/parameter-mismatch rejection, rank-of-key. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
Next review available in: 52 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIndexed-tree sum secondaries now carry counts. Pagination for all indexed axes uses counted offset proofs. The change adds rank-of-key proof generation and verification, rejects trailing proof bytes, and adds offset and rank tests. ChangesIndexed-axis proof flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant GroveDb
participant PrimaryIndexedTree
participant SecondaryMerk
participant Verifier
Caller->>GroveDb: request paginated or rank proof
GroveDb->>PrimaryIndexedTree: read item aggregates
PrimaryIndexedTree-->>GroveDb: return sort values
GroveDb->>SecondaryMerk: prove_count_offset_on_range
SecondaryMerk-->>GroveDb: return counted subtree proof
GroveDb-->>Caller: return proof bytes and rank
Caller->>Verifier: verify proof and parameters
Verifier->>Verifier: validate skipped count and yielded entries
Verifier-->>Caller: return verified result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
grovedb/src/operations/proof/indexed_axis/generate.rs (1)
521-528: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a not-found error kind for a missing
item_key.A caller can pass a key that does not exist in the primary. That is a caller input error, not data corruption.
Error::CorruptedDatamisclassifies it, so callers cannot distinguish "the key is absent" from "the database is broken".prove_indexed_axis_rank_of_keyis a public API, so the error kind is part of its contract.♻️ Proposed error-kind change
let item_element = cost_return_on_error!( &mut cost, Element::get(&primary_merk, item_key, true, grove_version).map_err(|e| { - Error::CorruptedData(format!( - "indexed-axis rank proof: item key not found in primary: {e}" - )) + Error::PathKeyNotFound(format!( + "indexed-axis rank proof: item key {} not found in the indexed primary: {e}", + hex::encode(item_key) + )) }) );🤖 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 `@grovedb/src/operations/proof/indexed_axis/generate.rs` around lines 521 - 528, In prove_indexed_axis_rank_of_key, change the missing item_key mapping around Element::get to return the repository’s not-found error variant instead of Error::CorruptedData, while preserving the existing lookup and cost propagation behavior.
🤖 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 `@grovedb-element/src/element/mod.rs`:
- Around line 252-256: The pagination documentation must state the full
proof-size bound as O(log n + k), distinguishing skipped-prefix pagination
proofs from rank-of-key queries. Update grovedb-element/src/element/mod.rs lines
252-256, merk/src/tree_type/mod.rs lines 67-74, and merk/src/tree_type/mod.rs
lines 75-81: correct the PSIT pagination and rank wording, and correct the PCIT
pagination wording; each page’s k returned entries must be included alongside
the O(log n) prefix proof.
In `@grovedb/src/tests/verify_grovedb_indexed_tests.rs`:
- Line 1290: Update every sum and average secondary-delete corruption helper to
pass TreeType::ProvableCountProvableSumTree, including the helpers around the
remaining ProvableSumTree and ProvableCountSumTree usages (such as the deletes
near the referenced lines). Keep the type consistent with the secondary opened
by open_indexed_secondary_at_path.
---
Nitpick comments:
In `@grovedb/src/operations/proof/indexed_axis/generate.rs`:
- Around line 521-528: In prove_indexed_axis_rank_of_key, change the missing
item_key mapping around Element::get to return the repository’s not-found error
variant instead of Error::CorruptedData, while preserving the existing lookup
and cost propagation behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c1d2617b-be2f-43ec-9254-9e697dc55901
📒 Files selected for processing (14)
grovedb-element/src/element/mod.rsgrovedb-element/src/element_type.rsgrovedb/src/lib.rsgrovedb/src/operations/indexed_tree.rsgrovedb/src/operations/proof/indexed_axis/axis_api.rsgrovedb/src/operations/proof/indexed_axis/envelope.rsgrovedb/src/operations/proof/indexed_axis/generate.rsgrovedb/src/operations/proof/indexed_axis/verify.rsgrovedb/src/tests/indexed_axis_nested_and_bounds_tests.rsgrovedb/src/tests/indexed_axis_offset_proof_tests.rsgrovedb/src/tests/indexed_axis_proof_tests.rsgrovedb/src/tests/mod.rsgrovedb/src/tests/verify_grovedb_indexed_tests.rsmerk/src/tree_type/mod.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #791 +/- ##
===========================================
+ Coverage 92.17% 92.21% +0.04%
===========================================
Files 257 257
Lines 78079 78176 +97
===========================================
+ Hits 71967 72091 +124
+ Misses 6112 6085 -27
🚀 New features to boost your workflow:
|
…test helpers, full O(log n + k) pagination bound in docs The verify_grovedb corruption helpers now derive the secondary's TreeType from axis_secondary_tree_type instead of hardcoding it (two sites still carried the pre-change ProvableSumTree, and the avg arm had always carried the wrong ProvableCountSumTree), and the pagination proof-size claims in the TreeType / Element docs state the full O(log n + k) bound instead of O(log n). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
This is Claude. Both CodeRabbit findings are addressed in 13218d9:
|
Codecov flagged the new error paths: rank proving at the root path or against a non-indexed target, the rank verifier's skipped-vs-rank and empty-window rejections (rank past / at the population), and the trailing-byte rejection on the range and aggregate envelope decoders. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An absent item_key is a caller input error, not data corruption; prove_indexed_axis_rank_of_key now reports Error::PathKeyNotFound with the hex key, and the absent-key test pins the error kind. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
This is Claude. The remaining review nitpick is addressed in 8752090: |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@grovedb/src/tests/verify_grovedb_indexed_tests.rs`:
- Line 1476: Add a direct IndexAxis::Avg corruption scenario in the tests using
corrupt_pcpsit_axis_secondary_delete, mirroring the existing Count and Sum
cases, then assert that verify_grovedb reports the expected Avg-axis integrity
failure. Keep the test setup and assertions consistent with the existing
axis-corruption coverage.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0b885ba1-949a-4541-a30b-cbe2ec12c6b7
📒 Files selected for processing (5)
grovedb-element/src/element/mod.rsgrovedb/src/operations/proof/indexed_axis/generate.rsgrovedb/src/tests/indexed_axis_offset_proof_tests.rsgrovedb/src/tests/verify_grovedb_indexed_tests.rsmerk/src/tree_type/mod.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- grovedb-element/src/element/mod.rs
- merk/src/tree_type/mod.rs
- grovedb/src/operations/proof/indexed_axis/generate.rs
Mirrors the count/sum axis-drift tests: delete one avg-secondary row (key derived via the canonical make_axis_secondary_key builder) and assert verify_grovedb reports it under the __pcpsit_avg_*__ sentinel. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
Reviewed |
This was referenced Aug 13, 2026
QuantumExplorer
added a commit
that referenced
this pull request
Aug 14, 2026
…uery stack) Brings the branch across #795-#809: the aggregate_over_value_range rename, GroveVersion params on the indexed-axis family, the AggregateFold grammar, and the dual-aggregate (PCPS) count secondary. Adaptations beyond conflict resolution: - run_path_query's RankedPage dispatch consumes the new IndexedTopKPage return shape (.entries); surfacing the true 'skipped' through PathQueryRun is left as a unified-API follow-up. - lib.rs re-export conflict: both re-exports kept. Zero changes to the counted-descent core: its generic already handled ProvableCountAndSum aggregates (sum/avg secondaries were PCPS since its base, #791), so #809's count-axis flip lands on an already-supported flavor — the full workspace suite passes unmodified. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part 1 — make the sum axis's secondary count-provable
The sum-axis secondary Merk — PSIT's lone secondary, and PCPSIT's sum axis (both flow through the same
axis_secondary_tree_typemapping) — changes fromProvableSumTreetoProvableCountProvableSumTree. EachSumItemrow contributes(count = 1, sum), so every secondary node commitment now binds a subtree count alongside the sum. The primary stays lean (ProvableSummedMerkNode, unchanged).Previously the sum family was the odd one out: PCIT secondaries (
ProvableCountTree) and PCPSIT avg secondaries (ProvableCountProvableSumTree) were count-provable, so positional queries against the sum ranking ("prove group X is ranked #37", "skip M entries") could never be proven in O(log n).The change is one arm in
axis_secondary_tree_type(grovedb/src/operations/indexed_tree.rs) — secondary open, mirroring (dedicated + batch incl.InsertAggregateIndexedTreeRootKeys), reconcile,verify_grovedb, and the average-case estimators all derive from that mapping, and the sum-axis secondary's proof node family moves toKVCountSum/KVHashCountSum/KVDigestCountSum/KVRefValueHashCountSum/HashWithCountAndSumautomatically via the feature-type-driven dispatch. Per-node cost moves from the sum-only to the count+sum node shape (feature_len9 → 17); the estimators track it through the sameNodeTypesource, so the ≥-actual cost contracts hold without constant surgery. Storage format amended in place — the indexed-tree family is unreleased in Dash Platform (PV14 unshipped), same policy as #790; no version gating.Replication needs no change: state sync rejects indexed primaries wholesale (pre-existing, issue #785 adjacent).
Part 2 — count-bound offset in indexed-axis paginated proofs
With every axis's secondary now count-bearing, the sum axis's
prove_indexed_axis_top_k_paginatedroutes throughMerk::prove_count_offset_on_rangelike count and avg already did:HashWithCountAndSum), giving O(log n + k) proof size regardless of offset — the old O(offset + k) enumeration fallback and itsoffset + k ≤ u16::MAXceiling are gone.(root_hash, skipped, entries)triple now carries a cryptographically attestedskippedfor all three axes (it was best-effort for sum).skipped < offset, which — because the count commitments cover the whole walk — is itself a proof that the secondary's total population is exactlyskipped(documented onIndexedAxisPaginatedResult::skippedand pinned by tests).original_keyin walk direction, in both the skipped prefix and the yielded window (the walk order over(sort_key ‖ original_key)is total).Merk::prove_count_offset_on_rangerejects any host whose tree type doesn't hash-bind a count, and the GroveDB layer additionally checksis_count_bearing()on the opened secondary rather than assuming it per axis.Rank-of-key (the nice-to-have)
prove_indexed_axis_rank_of_key/verify_indexed_axis_rank_of_key: "key X is at rank R" (R = count of entries strictly before X in the walk, 0-based, tie-aware). It falls out of the offset machinery: the proof is a paginated envelope at(offset = R, k = 1)whose attested skip pins the position and whose single yielded entry binds X; the prover computes R in O(log n) from the secondary's count aggregates. No new wire format.Hardening
ElementType::proof_node_typefamily sets gain theProvableSumIndexedTree/ProvableCountProvableSumIndexedTreeprimary arms, mirroring the existingProvableCountIndexedTreearm (currently unreachable via chunk/state-sync since indexed trees are rejected there, but correct for when that lands).Tests
indexed_axis_offset_proof_tests(15 tests): offset 0 == top-k windows (both directions), mid-walk offsets, offset+k spanning the end, offset past end / empty tree (total-population attestation), single-entry rank windows ("4th biggest" = offset 3, k 1, plus every rank of a 10-entry fixture), ties straddling the offset boundary in both directions, exhaustive bit-flip mutation (reject or rebind — never different content under the authentic root), truncation/garbage/parameter-mismatch rejection, rank-of-key round-trips incl. mid-tie ranks, wrong-rank/wrong-key/wrong-direction rejection, absent-key prove failure.a_sum_axis_page_beyond_the_u16_proof_limit_is_refused_rather_than_truncatedrepurposed to pin the new behavior (huge offsets prove an attested empty page);psit_indexed_axis_paginated_round_trip_uses_fallbackrenamed to..._uses_count_offset.indexed_axis_nested_and_bounds_tests,provable_count_provable_sum_indexed_tree_tests,batch_indexed_overwrite_tests,verify_grovedb_indexed_tests, the estimated-cost average/worst-case suites, and the fix: make the V4 batch gates zero-cost by reusing already-loaded old elements #790 zero-marginal-cost batch-gate tests.cargo clippyclean for touched files;cargo fmtapplied;--no-default-features --features verifybuild checked.Follow-up (platform side)
After merge, Dash Platform PR #4266 re-pins;
rankedSummable→ PSIT mapping is unchanged — only platform fee estimators/constants need the secondary-size delta (~8 bytes per secondary node commitment).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation