feat: replace indexed secondary rows with canonical references - #816
Closed
QuantumExplorer wants to merge 3 commits into
Closed
feat: replace indexed secondary rows with canonical references#816QuantumExplorer wants to merge 3 commits into
QuantumExplorer wants to merge 3 commits into
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #816 +/- ##
===========================================
- Coverage 92.18% 91.94% -0.24%
===========================================
Files 267 267
Lines 82354 83478 +1124
===========================================
+ Hits 75920 76756 +836
- Misses 6434 6722 +288
🚀 New features to boost your workflow:
|
QuantumExplorer
added a commit
that referenced
this pull request
Aug 19, 2026
…ains (#814) Ports the genuinely better parts of the alternative implementation in PR #816 while keeping this PR's proof-size property, and drops the merk wire-format change that #816 avoided. **Phase 3 is no longer deferred.** Every non-aggregate indexed read now returns `IndexedAxisEntry { ordering_value, primary_key, value }`, so a top-k result carries values rather than pointers — no follow-up `db.get` per row, and no extra inclusion proof per row for a verified read. A reference-shaped primary resolves to its TERMINAL, exactly as `db.get` on that key would, while the row stays BOUND to the immediate primary node so the mirror's invariant remains local. I previously judged this migration infeasible at ~500 call sites. #816 showed the way with a `PartialEq<(T, Vec<u8>)>` shim; the real cost was ~25 compile errors. The shim is documented as ignoring `value` — it answers "is this row in the right place", not "does it carry the right value" — and `key_pair()` is there for callers that genuinely only rank. Assertions that should check resolved values now do so explicitly, including one that previously could not tell a stale row from a fresh one at a fixed avg sort key. **Target chains replace the proof node variant.** Each returned row carries a chain of `(bytes, IndexedTargetCommitment)` entries — the immediate primary, then any reference hops to the terminal. The commitment enum (`Simple` / `Layered` / `IndexedSingle` / `IndexedMulti` / `Reference`) is #816's idea and it is the right one: it covers every target shape, including the nested indexed trees this PR previously refused with `NotSupported`. Unlike #816, a chain carries NO per-row path proofs. It authenticates itself from the row's own committed hash: each entry's commitment is rebuilt from its bytes plus the next entry's, and the head's is what the row binds. That is the same trust model shipped `KVRefValueHash*` proofs already use — they bind a reference's committed target hash to the returned value without separately proving the target's path inclusion — so a chain is neither weaker nor stronger than reading the same reference through an ordinary proof. Measured on a 32-entry PCIT with tree-shaped children, marginal proof cost per returned row is 83 bytes. Re-proving each primary from the grove root instead costs ~618 bytes/row and makes a k=16 proof 5.5x larger. A regression test pins the per-row figure. Because chains carry the layered commitment, the new merk `Node` variant this PR added is no longer needed: `grovedb-query` encoding, `proofs/tree.rs`, the merk verifiers and the chunk/branch matches are all reverted to develop. The proof wire change is now confined to the unshipped indexed-axis envelope. Full workspace suite passes (43 targets, 2736 grovedb tests); clippy three warnings below the pre-change baseline.
QuantumExplorer
added a commit
that referenced
this pull request
Aug 19, 2026
…iene (#814) Codex's review of #816 vs #817 identified two real correctness defects in this branch's target chains. Both are confirmed, reproduced by new tests, and fixed. It also flagged three hygiene items worth adopting. **Defect 1 — multi-hop chains folded the wrong hash.** A GroveDB reference commits its TERMINAL's value hash, not the next hop's: `follow_reference_get_value_hash` recurses past every intermediate reference before the hash reaches `PutCombinedReference` (batch/mod.rs:2176). The chain fold composed hop-by-hop, which happens to agree at one hop and diverges at two, so the existing one-hop test could not catch it. A two-hop primary failed verification with a spurious "bound to a different primary commitment". The chain is now at most TWO entries — head, and the terminal when the head is a reference. Intermediate hops are not carried at all, because nothing binds them: the head commits the terminal directly, so carrying the middle would hand a verifier bytes it cannot check. **Defect 2 — relative references resolved against the wrong path.** `SiblingReference` appends its key to the path it is given, so that path must be the entry's PARENT. Both the chain builder and the direct-read resolver passed parent‖key, one segment too deep, sending resolution underneath the entry itself. An `UpstreamRootHeightReference` masks this (it truncates to the first N segments and lands in the same place), which is why the existing test passed. A sibling-reference primary failed at prove time with "parent exists but is not a tree". Both now have dedicated tests asserting direct and proved reads agree. **Adopted from #816:** - **Removed the `PartialEq<(T, Vec<u8>)>` shim.** Codex is right that an equality impl silently ignoring `value` lets an assertion keep passing while resolution returns the wrong element. Replaced with an explicit `IndexedAxisEntrySliceExt::key_pairs()` projection, so each call site says which half it compares — and ranking-only callers get a real API instead of a comparison trick. 107 assertions migrated. - **`primary_unreachable_node` / `secondary_unreachable_node` sentinels.** A raw-iterated node the AVL cannot reach is corruption with its own name; silently skipping its commitment check made an operator guess. - **`cargo clippy -D warnings` clean** on grovedb and grovedb-merk. Also removed `CountOffsetReturnedItem::reference_element_hash`, which the chain redesign left set but never read. Full workspace suite passes (43 targets, 2739 grovedb tests). Per-row marginal proof cost unchanged at 83 bytes.
QuantumExplorer
added a commit
that referenced
this pull request
Aug 19, 2026
Adopts #816's factoring — the per-entry row refresh moves into the propagation loop, so a typed write path opts in with one call instead of ~20 lines of deferred-seed plumbing. Net −79 lines while adding a call site. The fiddly part was never the mirror; it was the deferred per-axis root state. Single-axis variants seed one slot and PCPSIT another, and seeding the wrong one leaves state set for an iteration with no indexed element to apply it to. That belongs in the one place already managing it. **This caught a fifth write path I had missed.** `replace_subtree_root` rewrites an entry in place and then propagates, exactly like the four non-Merk appends, so it left the canonical row bound to a commitment that no longer existed. #816 covers it; I did not. That is the factoring argument demonstrated rather than asserted: with the refresh inside the walk a new caller is one line and cannot forget, whereas per-call-site plumbing makes every new site opt-in and missable — which is how I missed this one. **Kept the old-state capture rather than refreshing in place.** #816's in-loop refresh passes the same aggregates on both sides, which only rewrites the row at its existing key. That is sound for the non-Merk appends, whose aggregates provably cannot change, but not for `replace_subtree_root`: its element is CALLER-SUPPLIED, so its aggregates — and therefore the row's sort key — can differ from what was there, and an in-place refresh would strand the old row at the old key. Callers capture pre-rewrite state with `capture_indexed_entry_state` (one line) and the walk applies a full old → new transition. The new test states a count the subtree's contents do not support, which moves the sort key. It asserts the row MOVED and that no indexed-row sentinel appears — while deliberately tolerating the child's own aggregate mismatch, which is the hash-vs-state correctness this unsafe API hands to the caller. Reverting the fix makes it fail with the row stranded at the old count, so it tests what it claims to. Default suite: 43 targets, 2739 grovedb tests. With `unsafe-dump-load`: 2742. `clippy -D warnings` passes on both feature sets. Per-row proof cost unchanged at 83 bytes.
QuantumExplorer
added a commit
that referenced
this pull request
Aug 21, 2026
… wrapper boundary (#814) Three review follow-ups, each pinning a case the suite asserted only adjacently: - A batch RefreshReference on a reference-shaped primary whose aggregates do NOT move — the "old_entry == new_entry swallow" the issue's §3 names. The refresh re-binds the primary's combined hash to the terminal's new value while (count, sum) stays put, so an aggregate-only mirror comparison would strand a stale row. RefreshReference reaches the mirror through its own op arm, so the Replace-based value-only tests did not cover it. Also asserts the intermediate state: an external terminal update alone must NOT stale the row — that locality is the point of the immediate-binding rule. - Each non-Merk append (MMR, bulk, commitment, dense) produces the IDENTICAL grove through the direct API and the batch op. The two entry points are separate implementations of the same mutation — the direct APIs refresh the row inside the propagation walk, the batch path through the mirror — so root-hash equality is the cheapest guard that they stay in sync. - A NonCounted-wrapped child is REJECTED by an indexed primary, on both write doors. This pins a boundary rather than a behaviour: direct and proved reads build their returned value differently, so a wrapper that could live in a primary would need its own read-equivalence coverage (a divergence of exactly this shape exists in the competing #816). No such coverage is needed BECAUSE the merk layer refuses wrappers in Provable* count trees; if that guard is ever relaxed, this test fails and says what to add. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
retiring this one in favor of 817 |
QuantumExplorer
added a commit
that referenced
this pull request
Aug 21, 2026
…#814) (#817) * feat: canonical reference rows for indexed-tree secondaries (#814) Replaces the placeholder indexed-secondary row representation with canonical one-hop references back to the primary entry, before indexed trees ship. Every axis now stores the same element family: ReferenceWithSumItem(SiblingReference(primary_key), Some(1), axis_payload_sum) written as a COMBINED reference so the row's committed value hash is combine_hash(H(reference bytes), primary_node_value_hash). Key decisions (per the issue's converged review): - All three axes stay on the dual-aggregate ProvableCountProvableSumTree. A single-aggregate count secondary would reopen the #809 finding-C proof-relabeling forgery, and a plain Reference folds to (1, 0) in a PCPS tree, silently zeroing the #806 band Total. The count axis therefore carries count_value_as_sum(count) as its payload sum. - Rows bind the IMMEDIATE primary node's committed value hash, not a terminal. That keeps the invariant local and mirror-maintainable. This is dedicated indexed-tree behaviour selected explicitly at each call site — ordinary user references keep their terminal contract and diagnostics, and nothing infers the rule from max_reference_hop == 1. - The secondary has no SubtreePath, so reference resolution for rows is purpose-built machinery keyed on the primary's logical path, not the generic path-keyed follow_reference. Write path: - Mirrors track the primary node's value hash alongside (count, sum), so a value-only update — and equally a deep mutation that only moves a child subtree root — refreshes every configured axis. This is the intended write amplification. - Capture widened from can_mutate_child_count to a new can_mutate_indexed_secondary_row: the non-Merk append ops leave (count, sum) alone but rewrite the entry's commitment. The direct MMR append also mirrors its entry, which the propagation walk cannot see because that mutation lands at the start path. Proof path: - Axis proofs resolve rows and emit reference-aware nodes carrying the referenced primary value. The verifier authenticates the canonical reference metadata by reconstructing the expected row bytes from the secondary-key suffix and the node's aggregates and checking them against the committed reference hash — so a row's target, hop budget and carried sum are checked, not assumed. - New Node::KVRefValueHashCountSumWithTargetChildHash carries a layered target's child commitment. Tree-shaped primaries are the NORMAL case under a count-indexed tree, and the existing ref node family cannot express their combined hash. - The count-offset gap is closed rather than bypassed, on both the indexed-axis path and the generic one: the short-circuit now runs a reference post-pass before encoding, and the prover/verifier rejections are replaced by resolution plus authentication. Integrity verification compares the exact canonical row, the reference target against the key suffix, and the row's committed hash against the primary node's current one, with distinct sentinels for non-canonical shape, wrong target, wrong payload sum and stale commitment. Costs: rows are sized from the real canonical row (they scale with the primary key they reference), and the combined-reference write charges its extra hash. Full workspace test suite passes. * feat: authenticate canonical axis rows and resolve them in proofs (#814) Completes the proof and verification half of the reference-row change. **Axis proofs authenticate the canonical row, rather than assuming it.** A verifier never sees a row's reference bytes — only its committed reference hash — so previously it would have been trusting that a committed reference points at the key encoded in the row it sits in. It no longer has to: from the AUTHENTICATED primary value the verifier re-derives the (count, sum) the mirror would have seen, rebuilds both the secondary key and the canonical row those aggregates imply, and compares them against what the proof committed. One comparison covers the ordering prefix, the primary-key suffix, the reference path, the one-hop budget and the carried payload sum. A row filed under `…‖a` whose reference points at `b` cannot verify. Notably this needs no new wire field: everything it uses is already bound to the secondary root. Chain checks now run before row decoding, so a relabeled envelope is still reported as "not for the requested axis" rather than as the downstream row symptom. **Corruption coverage** for each way a row can be wrong, each with its own sentinel: legacy placeholder payload, plain Reference, non-sibling reference, wrong hop budget, wrong target, wrong payload sum, stale commitment. Plus the two write paths that must refresh a row without any aggregate moving (value-only update, deep mutation under a tree entry), a proof-level mis-targeted-row test, and a guard that ordinary user reference chains still resolve to their terminal. **Docs** updated to describe the reference-backed system: the book's secondary-row layout and proof walkthrough, the stale `resolve_values` design text (resolution is now normal read behaviour), the two count-offset comments that claimed a reference post-pass already existed, and the hop=1 well-formed-user contract, which now says explicitly that it governs ordinary references while indexed rows use the immediate-node rule through their own path. Deferred, deliberately: surfacing the resolved Element through the public read and result types (issue #814 Phase 3). The proof authenticates the value today and then drops it, because threading it out is ~500 call sites of pure API churn across 20 test files with no security content — better as its own reviewable change. The authentication itself was not deferrable and is here. Full workspace suite passes (43 targets); clippy warnings one below the pre-change baseline. * fix: refresh canonical rows on every direct non-Merk append (#814) The MMR fix generalised: all four direct non-Merk append APIs (MMR, commitment, bulk-append, dense) share a shape where the updated element is written straight into the primary Merk and propagation starts only afterwards, so the propagation walk — which mirrors entries it discovers as it climbs — never sees the entry that actually moved. That was genuinely a no-op under aggregate-only rows: a non-Merk child contributes a constant count of 1, so an append moved nothing the row was keyed or valued on. Under canonical rows it is not, because the append rewrites the entry's non-Merk root and therefore its commitment. Bulk-append and dense were each leaving one stale row behind. Extracts the per-entry mirror into `capture_indexed_entry_state` + `mirror_indexed_entry_and_seed` rather than repeating it four times, and covers all four APIs in one test — each as its own case, since each has its own copy of the write-then-propagate sequence. * feat: resolved values in indexed reads, via path-proof-free target chains (#814) Ports the genuinely better parts of the alternative implementation in PR #816 while keeping this PR's proof-size property, and drops the merk wire-format change that #816 avoided. **Phase 3 is no longer deferred.** Every non-aggregate indexed read now returns `IndexedAxisEntry { ordering_value, primary_key, value }`, so a top-k result carries values rather than pointers — no follow-up `db.get` per row, and no extra inclusion proof per row for a verified read. A reference-shaped primary resolves to its TERMINAL, exactly as `db.get` on that key would, while the row stays BOUND to the immediate primary node so the mirror's invariant remains local. I previously judged this migration infeasible at ~500 call sites. #816 showed the way with a `PartialEq<(T, Vec<u8>)>` shim; the real cost was ~25 compile errors. The shim is documented as ignoring `value` — it answers "is this row in the right place", not "does it carry the right value" — and `key_pair()` is there for callers that genuinely only rank. Assertions that should check resolved values now do so explicitly, including one that previously could not tell a stale row from a fresh one at a fixed avg sort key. **Target chains replace the proof node variant.** Each returned row carries a chain of `(bytes, IndexedTargetCommitment)` entries — the immediate primary, then any reference hops to the terminal. The commitment enum (`Simple` / `Layered` / `IndexedSingle` / `IndexedMulti` / `Reference`) is #816's idea and it is the right one: it covers every target shape, including the nested indexed trees this PR previously refused with `NotSupported`. Unlike #816, a chain carries NO per-row path proofs. It authenticates itself from the row's own committed hash: each entry's commitment is rebuilt from its bytes plus the next entry's, and the head's is what the row binds. That is the same trust model shipped `KVRefValueHash*` proofs already use — they bind a reference's committed target hash to the returned value without separately proving the target's path inclusion — so a chain is neither weaker nor stronger than reading the same reference through an ordinary proof. Measured on a 32-entry PCIT with tree-shaped children, marginal proof cost per returned row is 83 bytes. Re-proving each primary from the grove root instead costs ~618 bytes/row and makes a k=16 proof 5.5x larger. A regression test pins the per-row figure. Because chains carry the layered commitment, the new merk `Node` variant this PR added is no longer needed: `grovedb-query` encoding, `proofs/tree.rs`, the merk verifiers and the chunk/branch matches are all reverted to develop. The proof wire change is now confined to the unshipped indexed-axis envelope. Full workspace suite passes (43 targets, 2736 grovedb tests); clippy three warnings below the pre-change baseline. * fix: correct reference-chain semantics; adopt #816's API and lint hygiene (#814) Codex's review of #816 vs #817 identified two real correctness defects in this branch's target chains. Both are confirmed, reproduced by new tests, and fixed. It also flagged three hygiene items worth adopting. **Defect 1 — multi-hop chains folded the wrong hash.** A GroveDB reference commits its TERMINAL's value hash, not the next hop's: `follow_reference_get_value_hash` recurses past every intermediate reference before the hash reaches `PutCombinedReference` (batch/mod.rs:2176). The chain fold composed hop-by-hop, which happens to agree at one hop and diverges at two, so the existing one-hop test could not catch it. A two-hop primary failed verification with a spurious "bound to a different primary commitment". The chain is now at most TWO entries — head, and the terminal when the head is a reference. Intermediate hops are not carried at all, because nothing binds them: the head commits the terminal directly, so carrying the middle would hand a verifier bytes it cannot check. **Defect 2 — relative references resolved against the wrong path.** `SiblingReference` appends its key to the path it is given, so that path must be the entry's PARENT. Both the chain builder and the direct-read resolver passed parent‖key, one segment too deep, sending resolution underneath the entry itself. An `UpstreamRootHeightReference` masks this (it truncates to the first N segments and lands in the same place), which is why the existing test passed. A sibling-reference primary failed at prove time with "parent exists but is not a tree". Both now have dedicated tests asserting direct and proved reads agree. **Adopted from #816:** - **Removed the `PartialEq<(T, Vec<u8>)>` shim.** Codex is right that an equality impl silently ignoring `value` lets an assertion keep passing while resolution returns the wrong element. Replaced with an explicit `IndexedAxisEntrySliceExt::key_pairs()` projection, so each call site says which half it compares — and ranking-only callers get a real API instead of a comparison trick. 107 assertions migrated. - **`primary_unreachable_node` / `secondary_unreachable_node` sentinels.** A raw-iterated node the AVL cannot reach is corruption with its own name; silently skipping its commitment check made an operator guess. - **`cargo clippy -D warnings` clean** on grovedb and grovedb-merk. Also removed `CountOffsetReturnedItem::reference_element_hash`, which the chain redesign left set but never read. Full workspace suite passes (43 targets, 2739 grovedb tests). Per-row marginal proof cost unchanged at 83 bytes. * refactor: refresh indexed rows inside the propagation walk (#814) Adopts #816's factoring — the per-entry row refresh moves into the propagation loop, so a typed write path opts in with one call instead of ~20 lines of deferred-seed plumbing. Net −79 lines while adding a call site. The fiddly part was never the mirror; it was the deferred per-axis root state. Single-axis variants seed one slot and PCPSIT another, and seeding the wrong one leaves state set for an iteration with no indexed element to apply it to. That belongs in the one place already managing it. **This caught a fifth write path I had missed.** `replace_subtree_root` rewrites an entry in place and then propagates, exactly like the four non-Merk appends, so it left the canonical row bound to a commitment that no longer existed. #816 covers it; I did not. That is the factoring argument demonstrated rather than asserted: with the refresh inside the walk a new caller is one line and cannot forget, whereas per-call-site plumbing makes every new site opt-in and missable — which is how I missed this one. **Kept the old-state capture rather than refreshing in place.** #816's in-loop refresh passes the same aggregates on both sides, which only rewrites the row at its existing key. That is sound for the non-Merk appends, whose aggregates provably cannot change, but not for `replace_subtree_root`: its element is CALLER-SUPPLIED, so its aggregates — and therefore the row's sort key — can differ from what was there, and an in-place refresh would strand the old row at the old key. Callers capture pre-rewrite state with `capture_indexed_entry_state` (one line) and the walk applies a full old → new transition. The new test states a count the subtree's contents do not support, which moves the sort key. It asserts the row MOVED and that no indexed-row sentinel appears — while deliberately tolerating the child's own aggregate mismatch, which is the hash-vs-state correctness this unsafe API hands to the caller. Reverting the fix makes it fail with the row stranded at the old count, so it tests what it claims to. Default suite: 43 targets, 2739 grovedb tests. With `unsafe-dump-load`: 2742. `clippy -D warnings` passes on both feature sets. Per-row proof cost unchanged at 83 bytes. * fix: keep the verify-only build compiling (#814) CI's `cargo clippy --workspace --all-features -- -D warnings` was failing on four unresolved imports, and the cause was worse than a lint: this branch broke `--no-default-features --features verify` outright. That is the light-client build — no storage, no transactions — so a verify-only consumer could not compile the crate at all. Two things were reaching into `minimal`-gated code from modules that must survive without it: - `target_chain.rs` was entirely unconditional, but BUILDING a chain reads storage. Split it: the builder moves behind `minimal`, while `shape_commitment` / `authenticate_target_chain` stay unconditional. Authenticating a chain is pure arithmetic over bytes the proof already carries, which is exactly what a light client needs. - The axis verifier rebuilds the canonical row a proof claims, so it needs the row definition — which lived in the `minimal`-gated write path. Moved the pure helpers (`axis_row_reference`, `axis_payload_sum`, `make_axis_secondary_key`, `axis_sort_key_len`, `count_value_as_sum`, `INDEXED_SECONDARY_MAX_HOP`) into a new verify-available `indexed_axis::canonical_row`, re-exported from `indexed_tree` so every existing write-path caller is unchanged. The placement matters for the property, not just the build: a light client rebuilds the row from the SAME definition the mirror wrote with, which is what makes the check meaningful rather than a restatement of whatever the prover sent. `clippy --workspace --all-features -- -D warnings` passes; the verify-only build has zero errors; 2739 grovedb tests pass. * fix: address CodeRabbit review — read consistency, cost sizing, hop bound (#814) Seven valid findings from the review. Several others referenced code this branch has since deleted (`reference_resolution.rs`) or reverted (the merk `Node` variant), so they no longer apply. **Corruption was being read as absence, in three places.** All three built `Option<IndexedEntryState>` with `value_hash.map(...)`, collapsing "the entry does not exist" and "the entry exists but its node is unreachable from the committed root" into the same `None`. On the new side that hands the mirror `None` and DELETES a live row; on the old side it skips the delete of a row that moved. Both now fail loudly, matching what the propagation path already did for the identical condition. **Indexed reads used two snapshots.** `resolve_axis_entries` built its own `TxRef`, so with `transaction: None` the primary resolution ran under a different snapshot than the secondary scan that produced the rows. A commit in between could pair a stale row with a newer primary value, or report a primary the row still names as corrupted. It now takes the caller's transaction and passes it to `follow_reference` too. **The secondary layer was described as `AllItems`.** Its rows are `ReferenceWithSumItem`, and the two variants carry different element overheads (+3 vs +15), so every row was under-charged by 12 bytes. `added_bytes` is the one dimension a storage-fee reservation must never come in under. **A reference in a `ProvableCountSumTree` hard-errored.** That host is eligible for count-offset pagination but commits only the count into its node hash, so its feature type is `ProvableCountedSummedMerkNode` — which the post-pass did not match. It now takes the count-only node, the same variant `emit_returned_node` picks for that host's directly-valued rows. Mutation-checked: reverting the arm makes the new test fail with the original error. **The chain builder allowed one hop more than `follow_reference`.** `0..=MAX_REFERENCE_HOPS` let the prover build a chain `db.get` would refuse. Now `0..`. Also: the paginated verify path now runs the layer binding before row decoding, matching the range path, so both name the same defect for the same forgery; `assert_only_issue` asserts row-sentinel exclusivity (scoped to `__cidx_*`, since damaging a row legitimately moves the element's H1-A binding too); the commitment-tree append — the one non-Merk append live on mainnet — is now covered alongside the other three; and the book's verified-result type is corrected to `AxisEntries`. 43 targets, 2740 grovedb tests (2743 with `unsafe-dump-load`). `clippy --workspace --all-features -- -D warnings` passes; verify-only build clean. * fix: drop a dead binding left by the paginated verify reorder (#814) `cargo clippy --workspace --all-features -- -D warnings` — the exact CI command — caught it; my earlier per-crate --lib runs did not. * fix: gate a test-only trait import behind the feature that uses it (#814) `IndexedAxisEntrySliceExt` was imported at module scope but only used by the `unsafe-dump-load`-gated test, so a default-feature `--all-targets` build saw an unused import. Moved into the gated test body. CI's lint (`--workspace --all-features`, no `--all-targets`) did not cover this; CodeRabbit's `--tests` run did. * test: adversarial coverage for the resolved-target chain (#814) An indexed-axis proof hands the verifier the primary value a row points at WITHOUT a per-row inclusion proof. That saving rests on one narrow claim — the row's committed hash is bound into the secondary root, and the chain reconstructs that hash from its own bytes, so no substitution survives. The claim was argued in comments and demonstrated only by the honest path; now it is attacked directly. Twelve tests take an honest proof, decode the envelope, change exactly one thing about a chain, re-encode, and require refusal: - the resolved primary value, and the TERMINAL a reference resolves to (the attacks the design exists to stop); - the reference head itself; - a layered commitment downgraded to `Simple`, and a tampered layered child root (the element bytes stay honest, so only the fold can catch these); - a directly-valued head promoted to `Reference` with an attacker terminal appended; - a reference head with its terminal stripped, an over-long chain, an empty chain, a chain-count mismatch; - two rows' chains SWAPPED — both chains well-formed, both values genuinely in the tree, so only the per-row binding catches it. Mutation-checked so the suite is known to be discriminating rather than incidentally green: disabling the commitment comparison in `authenticate_axis_row` fails 7 of the 12, including every value substitution across all commitment shapes. The other 5 are shape guards that fire earlier, which is the intended ordering. This is the evidence for the design choice the two competing implementations disagree on. It does not settle whether per-hop path proofs buy something else — they do attest a target's current location — but it does show the returned value is unforgeable without them. 2752 grovedb tests; `clippy --workspace --all-features -- -D warnings` passes. * fix: carry the count for ProvableCountSumTree references in V1 proofs (#814) A `ProvableCountSumTree` hashes via `node_hash_with_count` — only PCPS binds the sum in — so its references need the COUNT exactly as a `ProvableCountTree`'s do. The V1 reference dispatch matched only `ProvableCountedMerkNode`, so a reference in such a tree downgraded to the aggregateless `KVRefValueHash` and the host's node hash could not be reconstructed. The proof verified nowhere. Reproduced on develop with identical hashes, so this is pre-existing and not introduced by the indexed-tree work. It surfaced because I fixed the same defect in the count-offset dispatch last round and the ordinary path was left inconsistent with it. Mutation-checked: reverting the arm reproduces the original "V1 mismatch in lower layer hash". **V0 has the identical defect and is deliberately untouched.** V0 is shipped, consensus-frozen wire format; changing what it emits is a different kind of decision from fixing a bug, and it wants its own review rather than riding along in this PR. Nothing is lost by waiting: no valid proof exists for this shape under V0 today either, so the case is unreachable through a verifying client on both envelopes. Recorded in the new test's doc comment so the asymmetry is visible rather than implied. 2753 grovedb tests; `clippy --workspace --all-features -- -D warnings` passes. * fix: cover PrivateDocumentStoreInsert in the indexed-row exhaustive match (#814) develop's #787 added `GroveOp::PrivateDocumentStoreInsert`, which `can_mutate_indexed_secondary_row` did not cover. The match is exhaustive on purpose, so the new variant was a compile error in the PR merge commit — exactly the signal it exists to produce. The op is rewritten into `ReplaceNonMerkTreeRoot` by `preprocess_private_document_store_ops` before the level executor runs, so the arm is unreachable in the current pipeline. It answers `true`, matching what the op becomes, which keeps it correct if that preprocessing is ever reordered or removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: cover the multi-axis nested primary and two chain-shape guards (#814) Three paths the patch left untested, all reachable and worth testing on their own merit rather than for the metric: - A nested PCPSIT primary entry. A multi-axis indexed tree folds an axes DIGEST into its commitment where a single-axis one folds a bare secondary root, so it is a distinct commitment shape; a chain that rebuilt it as single-axis would not reproduce the row's hash. - A directly-valued head carrying a terminal, and a terminal that is itself a reference. These are the two chain-shape guards the existing tamper cases did not reach — the mirrors of the head-promotion and stripped-terminal cases already covered. The two new tamper cases assert on the specific rejection message, so they prove the intended guard fired rather than any guard. Adds `assert_rejected_because` for that. Also renames `mis_targeted` to `mistargeted` in an existing case: the typos hook scans the whole file once it is touched, and flagged it. Not covered, deliberately: the `KVRefValueHashCount{,Sum}` arms of the count-offset verifier. The count-offset prover emits `KVValueHashFeatureType` for reference rows (emit.rs:598), so those arms are defensive against proofs the honest prover cannot produce. The end-to-end reference-resolution behaviour they guard is already covered by `count_offset_resolves_reference_entries_to_their_target` and its count-sum sibling. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: guard unconsumed deferred_axes, not just deferred_secondary (#814) `deferred_secondary` and `deferred_axes` are set mutually exclusively by the same code path — single-axis (PCIT/PSIT) sets the former, PCPSIT the latter — and both are consumed by the same loop. The end-of-walk guard checked only `deferred_secondary`, so a walk that reached the root with per-axis state still staged returned Ok(()) instead of failing. That is the identical corruption the existing check catches, undetected for PCPSIT alone. Kept as a separate check with its own message so a report says WHICH half was stranded. The new test mirrors the single-axis one and is mutation-checked: disabling the guard makes it fail. Reported by CodeRabbit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: charge the mirror's bracketing primary reads in average-case estimates (#814) The batch mirror brackets the primary apply with a pre- and a post-state read of each touched entry (read_entry_aggregates, pre and post), each a Merk::get plus a Merk::get_value_hash on the primary node — the node's STORED hash is what the row must bind, and for tree- or reference-shaped entries it is a combined hash that cannot be recomputed from the element bytes. Those four node fetches per touched key were not charged, so the estimate was not a categorical superset of the write path on the seek_count / storage_loaded_bytes axes (in practice it stayed over because merk-open charges dominate, but by accident, not construction). Charged at the caller's per-key loop rather than inside average_case_indexed_secondary_mirror: the reads are per-KEY while that function is per-axis additive — one capture feeds every axis's rewrite — and the standalone mirror-cost coverage tests pin that additivity. Worst-case is untouched on purpose: its indexed gap is broader and already documented as a KNOWN GAP (WorstCaseLayerInformation cannot even identify an indexed primary). Also adds the spec §8 write-amplification fixtures: a value-only update (same count, same sum, different bytes) on PCIT and on a three-axis PCPSIT, each asserting the estimate does not come in under actual on seeks, loaded bytes, added bytes, combined written bytes, and hash calls. These are the estimated-vs-actual cases most tempted to assume "aggregates unchanged ⇒ no secondary write". Write bytes are asserted as added+replaced combined because the estimator models the row rewrite as delete+insert while the real apply replaces in place — the split differs by construction, the total must not. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: RefreshReference swallow case, direct-vs-batch append equality, wrapper boundary (#814) Three review follow-ups, each pinning a case the suite asserted only adjacently: - A batch RefreshReference on a reference-shaped primary whose aggregates do NOT move — the "old_entry == new_entry swallow" the issue's §3 names. The refresh re-binds the primary's combined hash to the terminal's new value while (count, sum) stays put, so an aggregate-only mirror comparison would strand a stale row. RefreshReference reaches the mirror through its own op arm, so the Replace-based value-only tests did not cover it. Also asserts the intermediate state: an external terminal update alone must NOT stale the row — that locality is the point of the immediate-binding rule. - Each non-Merk append (MMR, bulk, commitment, dense) produces the IDENTICAL grove through the direct API and the batch op. The two entry points are separate implementations of the same mutation — the direct APIs refresh the row inside the propagation walk, the batch path through the mirror — so root-hash equality is the cheapest guard that they stay in sync. - A NonCounted-wrapped child is REJECTED by an indexed primary, on both write doors. This pins a boundary rather than a behaviour: direct and proved reads build their returned value differently, so a wrapper that could live in a primary would need its own read-equivalence coverage (a divergence of exactly this shape exists in the competing #816). No such coverage is needed BECAUSE the merk layer refuses wrappers in Provable* count trees; if that guard is ever relaxed, this test fails and says what to add. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: give the book's verify examples the real signatures (#814) The three verify_indexed_count_* examples in the count-indexed-tree chapter predate the final API and dropped arguments — top_k's example omitted `descending` and `grove_version`, and both query examples omitted `expected_limit` and `grove_version`. Copying either would not compile. Now byte-matched to the shipped signatures, with the limit bound positionally the same way the prover was called. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- 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.
Closes #814.
Summary
ReferenceWithSumItem(SiblingReference(primary_key), Some(1), axis_sum).IndexedAxisEntry { ordering_value, primary_key, value }from value-bearing indexed reads, with ordinary primary references resolved to terminal values.KVRefValueHashCountandKVRefValueHashCountSumproof nodes; there is no raw-reference escape hatch.verify_grovedbto detect non-canonical rows, wrong sibling paths/hop limits/axis sums, stale target hashes, relational drift, and unreachable physical nodes.Proof trust model
The immediate primary value is supplied in the indexed proof envelope and is cryptographically bound by the authenticated secondary row's combined value hash. Re-proving its path would be redundant.
Ordinary reference hops are different: they identify nodes elsewhere in the grove. Their layer proofs and ancestor attestations authenticate those locations against the Grove root. Reference commitments are reconstructed using GroveDB's existing terminal-target hash semantics, including multi-hop reference chains.
This keeps ordinary direct-primary proofs compact while retaining path-location authentication only for the exceptional reference-shaped-primary case.
Review follow-up
PartialEqmigration shim; assertions that care about resolved values now inspect them explicitly.k=4, 2,334 bytes atk=16, and about 113 bytes per additional ordinary row.Why a plain
RefreshReferenceis not enoughThe indexed row must bind the post-mutation committed value hash of the immediate primary node, while its explicit sum and secondary key remain canonical for the selected axis. Refreshing the primary reference first does not update the separate secondary Merk, and refreshing the secondary by following the primary's ordinary reference chain would bind the terminal target rather than the immediate primary node. The implementation therefore applies the primary mutation, reads its committed value hash, and performs an explicit canonical reference write in every affected secondary.
Compatibility
This indexed-tree representation has not shipped, so this intentionally replaces the current on-disk, proof, and result shape without a migration path.
Verification
cargo test -p grovedb— 2,726 passed, 2 ignored; all 3 doctests passedcargo test -p grovedb indexed_reference_row_tests -- --nocapture— 6 passedcargo test -p grovedb indexed_rows_resolve_primary_references_in_reads_and_proofs -- --nocapturecargo test -p grovedb indexed_target_witness_size_stays_compact_as_k_grows -- --nocapturecargo clippy -p grovedb -p grovedb-merk --lib -- -D warningscargo fmt --all