feat: dual-aggregate count secondary — count+Total served (closes #806) - #809
Merged
Conversation
The count-axis secondary becomes a ProvableCountProvableSumTree, mirroring each entry's count_value into its sum half. "Total of the counts in a band" is now one committed scalar, O(log n), provable through both the embedded V1 descent and the standalone envelope — the last cell of the (axis x fold) matrix turns on, and every axis's secondary now has the same dual-aggregate shape. The payload changes in four lockstep sites (batch mirror row, the mirror closure, reconcile's desired map, verify_grovedb's expected payload), all through one fail-closed conversion: count > i64::MAX is a typed error, never a clamp — a clamped total would lie through authenticated state. The mirror closure's signature widens to |count, sum| because the count-axis payload is now a function of the count, which the old |sum|-only fast-path equality check could not see (it would have silently skipped real payload updates). The three count+Total refusal arms from part 1 become live arms; the part-1 refusal test becomes the issue's own example verbatim: counts [3, 1, 5], band [2, 10] -> Population 2, Total 8, read three ways and re-checked after a delete. verify_grovedb holds across count-changing mutations; a tampered sum in the carried secondary proof breaks the combine_hash_three binding; the cost estimator sizes the count row as SumItem(i64::MAX) (the count axis now sizes exactly like the sum axis, which the mirror-scaling cost test now pins). TopK / RankOfKey / Bounded proofs survive the flip with zero merk changes: the count-offset verifier was already dual-flavor (HashWithCount / HashWithCountAndSum). Wire-affecting but free: indexed trees are GROVE_V4-gated and V4 has not activated — no migration, no new version slots. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (17)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #809 +/- ##
========================================
Coverage 92.21% 92.21%
========================================
Files 267 267
Lines 81620 81658 +38
========================================
+ Hits 75263 75300 +37
- Misses 6357 6358 +1
🚀 New features to boost your workflow:
|
Member
Author
|
This is Claude. The blockchain-security-auditor pass over this diff completed (after merge — findings below are follow-ups, none block correctness of what landed). All five probed surfaces hold: payload-site byte-identity (it found a fifth site — the cost estimator row — beyond my four), fold confusion (both wire paths authenticated or query-sourced; range-reconstructor sharing is total with no residual duplicated shape), overflow (prover/verifier symmetric i128-accumulate + final i64 gate; band-sum overflow already covered by Three findings, none critical:
Also verified clean: Population can't be skewed by the payload change; nothing consumes the count secondary's sum aggregate outside the fold; the overflow guard is consistent and unreachable; envelope trailing-byte rejection still applies. |
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 was referenced Aug 14, 2026
QuantumExplorer
added a commit
that referenced
this pull request
Aug 17, 2026
Brings in the unified PathQuery stack (#795-#809) and the V4 batch-gate cost work (#789/#790). Conflict resolutions, all additive or relocations: - grovedb-version v1..v4: union of the IndexedAxis (develop) and PrivateDocumentStore (this branch) version-family imports; both initializers already auto-merged. - merk apply: develop refactored apply_unchecked into a delegator over the new apply_unchecked_with_old_value_observer — the PDS "no ops on a PDS Merk" chokepoint moved into that new single funnel. - proof generate (V1): kept both newly-inserted arms — this branch's PDS subquery rejection and develop's indexed-axis descent. - batch apply: kept the PDS preprocess pass ahead of develop's scan_delete_tree_ops refactor (both call sites). - PDS tests: Query literals gained the new read_mode: None field. No discriminant collisions: element 24/152, TreeType 16, and GroveOp tag 19 remain unclaimed on develop. Full workspace suite green after merge (45 binaries, zero failures). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
added a commit
that referenced
this pull request
Aug 19, 2026
…it follow-up) (#811) * refactor: one axis_row_payload for every writer and checker (#809 audit) The #809 audit's finding A: the per-axis secondary payload was hand-copied at five sites — the batch mirror row, the direct-path mirror closure, reconcile's desired map, verify_grovedb's expected payload, and the cost estimator's worst-case row — and three of the surrounding comments still described the pre-#809 empty-Item shape. The enforcement of "these five must stay byte-identical" was human discipline; a contributor trusting the stale comment and "restoring" the old payload at one site would either false-flag healthy databases or make two entry points commit different root hashes for identical writes. The lockstep is now structural: one pub(crate) axis_row_payload in operations::indexed_tree, called from all five sites (the estimator feeds it worst-case aggregates, which stay within count_value_as_sum's domain by construction). A grid test pins the payload per (axis, count, sum) — including that each axis IGNORES the aggregate it doesn't store, and the fail-closed bound — plus golden serialized bytes, so any change to the shape is a deliberate, reviewed event. The stale comments are gone, replaced by pointers to the one definition. Also from the audit (finding C): axis_secondary_tree_type now documents that dual-aggregate secondaries are a SECURITY requirement — node_hash_with_count and node_hash_with_sum share an untagged preimage layout, so a single-aggregate secondary would make a band Total forgeable from a Population proof by node-type relabeling. The comment forbids "optimizing" any axis back to a single-aggregate tree without first adding domain separation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: in-place replace for fixed-key avg payload updates; fixed golden bytes Two review findings on the audit follow-up, both real: The direct write path DELETED AND REINSERTED an avg-secondary row whose payload changed at a fixed sort key ((1, 29) -> (2, 58) keeps avg = 29), while the batch mirror emits a single replacement write. Delete+reinsert rebalances the AVL twice and can settle a different shape: reproduced red on an interior node — the two write paths committed DIFFERENT grove roots for identical data (a leaf-position first attempt stayed green, which is why the test pins the modified key mid-range). The direct path now skips the delete when the key does not move, so the insert replaces in place exactly like the batch put. Pre-existing since the avg axis existed; surfaced by this PR's diff region. The golden-bytes test serialized both sides at assertion time, so a serialization-format change would move both and pass. The vectors are now fixed literals per axis; a failure here means payload bytes in authenticated state changed — a consensus event, not a refactor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 19, 2026
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.
Part 2 of #806 (part 1 was #808). Closes #806.
The flip
axis_secondary_tree_type(Count)→ProvableCountProvableSumTree, with each entry'scount_valuemirrored into the sum half. "Total of the counts in a band" is now one committed scalar, O(log n), provable through both the embedded V1 descent and the standalone envelope. Every axis's secondary now has the same dual-aggregate shape, and the (axis × fold) matrix is complete:PopulationTotalThe issue's own example, verbatim, is the headline test: counts
[3, 1, 5], band[2, 10]→Population2,Total8 — read three ways (trusted, embedded proof, standalone envelope) and re-checked after a delete.Integrity mechanics
verify_grovedb's expected payload) all go through one fail-closed conversion:count > i64::MAXis a typed error, never a clamp — a clamped total would lie through authenticated state. Unreachable for real trees; unit-tested at the boundary.|count, sum|: the count-axis payload is now a function of the count, which the old|sum|-only fast-path equality check could not see — left alone it would have silently skipped real payload updates.verify_grovedbholds across count-changing mutations and deletes (its expected-payload check doubles as the drift detector); a tampered sum inside the carried secondary proof breaks thecombine_hash_threebinding.HashWithCount/HashWithCountAndSum).SumItem(i64::MAX); the count axis now sizes exactly like the sum axis, which the mirror-scaling cost test pins (the old count-vs-avg deltas collapse to the 1-byte payload-shape difference).Wire-affecting but free: indexed trees are GROVE_V4-gated and V4 has not activated — no migration, no new version slots. The "currently unsupported" doc paragraphs from #808 are deleted, per their own text.
Verification
cargo test --workspace --all-featuresgreen, clippy-D warningsclean, verify-only build clean. Local llvm-cov patch ~94% (remaining lines are?-region artifacts in closures plus the overflow arm, which the unit test covers). New tests: the 2×2 matrix differential across all three read paths, count+Total round trip incl. post-delete,verify_grovedbacross mutations, secondary-sum tamper rejection, overflow fail-closed, gate + inverted-bounds gate for the new reader.A blockchain-security-auditor pass over this diff is running and its findings will be posted before merge.
🤖 Generated with Claude Code