From 356af1e88485a020929b948549ad29a37e642028 Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Wed, 12 Aug 2026 22:58:49 +0700 Subject: [PATCH 01/11] docs: record the counted-skip design for unproved ranked reads Unproved ranked reads walk the OFFSET linearly (indexed_axis_top_k_paginated_generic), one iterator step and one decode per skipped entry, while the proved path skips whole subtrees via count-bound node commitments. Measured 457ms at OFFSET 4e9 unproved versus 38us proved. The fix chosen for this repo was to give the read path the same counted descent. Investigating it established that no non-proof counted-skip primitive exists in merk at all: the counted descent lives only inside the proof emitter (proofs/query/count_offset/emit.rs), so the change is an extraction plus a new read-only entry point, not a wiring job. That finding is what reshaped the decision - the work was deferred in favour of a Platform-side mitigation (serve unproved reads through the prover internally and verify the proof to recover entries), which needs no grovedb change and measured 78-129us round trip, with the deep-offset lever flat at 48us. This note is the record of the proper long-term fix so it can be picked up cold: which decisions in emit.rs are shareable and which must not move, the shape of Merk::read_count_offset_on_range, the argument that the extraction leaves proof bytes bit-identical plus the golden-digest test strategy that would prove it, an OperationCost assertion that distinguishes a counted skip from a linear one, the risk list, and three open questions that must be answered before any code is written. Documentation only - no behaviour change, so no test. Co-Authored-By: Claude Opus 5 (1M context) --- docs/COUNTED_SKIP_DESIGN.md | 394 ++++++++++++++++++++++++++++++++++++ 1 file changed, 394 insertions(+) create mode 100644 docs/COUNTED_SKIP_DESIGN.md diff --git a/docs/COUNTED_SKIP_DESIGN.md b/docs/COUNTED_SKIP_DESIGN.md new file mode 100644 index 000000000..e692c431e --- /dev/null +++ b/docs/COUNTED_SKIP_DESIGN.md @@ -0,0 +1,394 @@ +# Counted skip for unproved ranked reads — design note + +**Status:** design only — **deferred, not implemented**. No code was written. +**Branch:** `fix/counted-skip-unproved-read`, based on `a2791bb` (`develop`). +**Problem statement:** `platform.test-indexes/docs/ranked-index-testing/FIX_SPEC_OFFSET.md`. +**Line numbers below are as of `a2791bb`** — if they have drifted, search by function name. + +### Why this is on the shelf + +**This design was deliberately not implemented. It was superseded by a cheaper mitigation, not +abandoned as unworkable.** Nothing below was found to be wrong, infeasible, or blocked — the work +simply stopped being necessary before it started, and the note was kept as the record. + +The spec's option A (this document) was the owner's first choice. It was then superseded as the +*immediate* remedy by option C: Platform serves unproved ranked reads through the prover internally +and verifies its own proof to recover the entries. That removes the linear offset walk with no +grovedb change, no query-grammar change and no cross-repo pin bump, so it shipped first. Measured: +78–129 µs round trip, with the deep-offset lever — the whole point of the exercise — dropping from +440 ms to a flat 48 µs. That is a complete fix for the denial-of-service problem, which is why this +one stopped being ship-blocking. + +This design stays the correct long-term shape. What it would buy once picked up: the unproved read +stops paying prove-then-verify overhead (proof construction, serialization, and full verification on +every read) and instead does the counted descent directly — the same `O(log n + k)` work with none of +the proof machinery. Against the 78–129 µs baseline above, that is an optimization, not a fix; treat +it as such when deciding whether it earns the risk of touching the proof emitter. It also makes the +code comment at `mode_detection.rs:302-313` true for both paths rather than only the proved one. + +The finding that reshaped the decision, recorded here so nobody re-derives it: **there is no +non-proof counted-skip primitive in merk.** The counted descent exists only inside the proof +emitter. Verified independently by an adversarial review. That is why option A is an extraction plus +a new entry point, not a wiring job — and why it was worth deferring rather than rushing. + +## 0. What the problem is, restated against this repo + +`indexed_axis_top_k_paginated_generic` (`grovedb/src/operations/indexed_tree.rs:1297`) skips the +offset by stepping a **storage** iterator once per skipped entry: + +```rust +let mut iter = KVIterator::new(secondary_merk.storage.raw_iter(), &all_query)…; +while skipped < offset { + match iter.next_kv() { Some((secondary_key, _)) => { decode(&secondary_key)?; skipped += 1 } … } +} +``` + +That is `Θ(min(offset, N))`. All three axes funnel through this one function +(`indexed_count_top_k_paginated:1483`, `indexed_sum_top_k_paginated:1677`, +`indexed_avg_top_k_paginated:1862`), so one fix covers the whole family. + +The proved path for the same query shape is +`build_indexed_axis_paginated_proof` → `Merk::prove_count_offset_on_range(RangeFull, offset, +Some(k), !descending)` (`grovedb/src/operations/proof/indexed_axis/generate.rs:904-918`). It walks +the **merk tree** and collapses any wholly-in-range subtree whose aggregate count fits inside the +remaining offset into one step — `O(log n + k)`. + +Confirmed against the tree: there is no non-proof counted-skip / rank-select primitive anywhere in +`merk`. `merk/src/proofs/query/count_offset/` is `emit / mod / prove / tests / verify`; every entry +point produces `Op`s. The independent reviewer reached the same conclusion. + +## 1. Exactly what can be shared, and what cannot + +I went through `emit_count_offset_proof` (`count_offset/emit.rs:95-503`) line by line. It does seven +things; only two of them are the counted-skip logic: + +| # | Step | Shareable with a plain read? | +|---|---|---| +| 1 | `classify_subtree(lo, hi, range)` | **Already shared** — lives in `aggregate_common.rs`, used by aggregate-count/sum too. Reuse as-is. | +| 2 | Collapse decision (Disjoint / count ≤ offset / past-limit / descend) — `emit.rs:136-148` | **Yes.** Pure function of `(class, subtree_count, offset_remaining, limit_remaining)`. | +| 3 | `own_struct = count − left_link_count − right_link_count` — `emit.rs:223-244` | **Yes.** Three lines of arithmetic on link aggregates. | +| 4 | Direction ordering (`first_dir`/`second_dir`) — `emit.rs:324-328` | **Yes**, but it is two lines; sharing it is not worth an abstraction. | +| 5 | Per-node disposition (path / offset-skipped / returned / past-limit) — `emit.rs:412-430` | **Yes.** Pure function of `(is_in_range, own_struct, offset_remaining, limit_remaining)`. | +| 6 | Op emission: `Node::HashWithCount…`, `Push`/`PushInverted`, `Parent`/`Child`, `emit_returned_node` | **No** — proof-only. Must not be touched. | +| 7 | Rejection of unsupported in-range shapes (NonCounted / Reference / non-empty tree) — `emit.rs:276-319` | **No** — those are constraints of the *wire format*, not of the traversal. The reader has its own (different) rule; see §4.3. | + +There is also one thing the reader must **not** share: the prover cannot stop early. Once the limit +is exhausted it still has to walk the rest of the tree and emit a `HashWithCount` per remaining +subtree, because the verifier reconstructs the root hash from the whole op stream. A plain read has +no such obligation and returns the moment `limit_remaining == Some(0)`. So the *traversal* is not +literally shared; the *decisions* are. + +### Proposed extraction + +New file `merk/src/proofs/query/count_offset/decide.rs`, holding only pure code moved verbatim out +of `emit.rs`: + +```rust +pub(super) enum CollapseAction { Disjoint, SkippedByOffset, PastLimit } // moved from emit.rs:511 +pub(super) enum NodeDisposition { Path, SkippedByOffset, PastLimit, Returned } + +/// Whole-subtree decision. `None` = must descend per-element. +pub(super) fn collapse_action( + class: SubtreeClassification, subtree_count: u64, + offset_remaining: u64, limit_remaining: Option, +) -> Option; + +/// Per-node decision at a descended node. +pub(super) fn node_disposition( + is_in_range: bool, own_struct: u64, + offset_remaining: u64, limit_remaining: Option, +) -> NodeDisposition; + +/// own_count = aggregate − left_link_count − right_link_count (saturating, as today). +pub(super) fn own_structural_count(node_count: u64, left: u64, right: u64) -> u64; +``` + +Both decision functions are **total, pure, allocation-free, and take no `&mut` state**. State +mutation (`offset_remaining -= 1`, `limit -= 1`, `returned += 1`) stays at each call site, exactly +where it is today. `emit.rs` shrinks by ~25 lines and gains three call sites; nothing else in it +moves. + +**Why not a visitor/callback trait over the whole recursion?** It would force the prover's +walk shape onto the reader (no early exit), thread a lifetime-heavy sink through the hot path, and — +decisively — it would rewrite the function that produces consensus-frozen bytes. The +decision-function extraction touches the emitter in three mechanical spots and can be argued +bit-identical (§3); a visitor rewrite cannot. + +## 2. The new read-only entry point + +Two new files, mirroring the prover's own split exactly: + +**`merk/src/proofs/query/count_offset/read.rs`** — the recursion: + +```rust +pub(super) struct ReadState { offset_remaining: u64, limit_remaining: Option, + left_to_right: bool, done: bool } + +pub(super) fn read_count_offset( + walker: &mut RefWalker<'_, S>, range: &QueryItem, + subtree_lo_excl: Option<&[u8]>, subtree_hi_excl: Option<&[u8]>, + state: &mut ReadState, out: &mut Vec<(Vec, Vec)>, + grove_version: &GroveVersion, +) -> CostResult<(), Error>; +``` + +Same bound-tightening (`walk left → (lo, node_key)`, `walk right → (node_key, hi)`), same direction +ordering, same `classify_subtree`, same `collapse_action` / `node_disposition`. Differences from +`emit`: no `ops`, no `Node` construction, no `tree_type` parameter (nothing is hashed, so the +single-axis vs dual-axis PCPS split is irrelevant here), returns `()` instead of a structural count +(only the verifier needs that), and returns early once `state.done`. + +**`merk/src/merk/read_count_offset.rs`** — the entry point, sibling of +`merk/src/merk/prove_count_offset.rs`: + +```rust +impl<'db, S: StorageContext<'db>> Merk { + pub fn read_count_offset_on_range( + &self, inner_range: &QueryItem, offset: u64, limit: Option, + left_to_right: bool, grove_version: &GroveVersion, + ) -> CostResult; +} + +pub struct CountOffsetReadResult { + pub entries: Vec<(Vec, Vec)>, // (key, value) in directional order + pub offset_remaining: u64, // > 0 ⇒ offset ran past the end of the population +} +``` + +It has to live under `merk/src/merk/` because `Merk::use_tree` is `pub(crate)` and `Merk::source()` +is `pub(in crate::merk)` — the same reason `prove_count_offset.rs` lives there. Same tree-type guard +as the prover (`ProvableCountTree` / `ProvableCountSumTree` / `ProvableCountProvableSumTree`), same +empty-merk behaviour (`use_tree_mut` → `None` ⇒ empty result, full offset remaining). + +Values come for free — the node is loaded either way — so the primitive returns them, even though +the current grovedb caller discards them (`Some((secondary_key, _))`). + +**GroveDB caller.** `indexed_axis_top_k_paginated_generic` replaces both loops with: + +```rust +let read = cost_return_on_error!(&mut cost, + secondary_merk.read_count_offset_on_range( + &QueryItem::RangeFull(..), offset, Some(k as u64), !descending, grove_version)); +let mut results = Vec::with_capacity(read.entries.len()); +for (secondary_key, _) in read.entries { + match decode(&secondary_key) { + Some(d) => results.push(d), + None => return Err(corrupted_secondary_key_error(axis, &secondary_key)).wrap_with_cost(cost), + } +} +``` + +`Query`/`KVIterator` disappear from this function. `indexed_axis_top_k_generic` and +`indexed_axis_range_generic` are **untouched** — neither takes an offset, so neither has the bug. + +### Ordering and tie-equivalence (the "identical entries, identical ordering" constraint) + +- The secondary's merk key **is** the sort key: `axis_sort_bytes ‖ item_key` + (`make_axis_secondary_key`, `indexed_tree.rs:78`). In-order traversal of the merk = lexicographic + order = exactly the order `raw_iter` produces. Reverse traversal = `raw_iter` backwards. +- Keys are unique (the item key is a suffix of every secondary key), so the order is **total**. + "Ties on the axis value" are broken by `item_key` inside the key bytes themselves — there is no + comparator, no stability question, and nothing for a different traversal to reorder. +- `left_to_right = !descending` in both the old read and the prover, so the offset counts in the + same directional order in all three paths. + +### Why counted-skip == positional-skip here + +The counted skip advances by *aggregate count*; the old loop advances by *iteration position*. They +agree iff every entry contributes exactly 1 to the count. In these secondaries it does, by +construction: + +- Every secondary is count-bearing: `axis_secondary_tree_type` → `ProvableCountTree` (count axis) or + `ProvableCountProvableSumTree` (sum, avg) — `indexed_tree.rs:57-70`. +- The only writer is `mirror_indexed_axis_to_secondary` (`indexed_tree.rs:2213`), which writes + exactly one of `Item(∅)` / `SumItem(sum)` / `ItemWithSumItem(∅, sum)` — never `NonCounted`, never + a tree, never a reference. Each contributes count = 1. +- `verify_grovedb`'s `verify_indexed_axis_content` (`grovedb/src/lib.rs:1614`) enforces exactly one + secondary row per primary entry, with that payload, and nothing else in the secondary. + +So on any state the DB considers valid, rank == position. §4 covers what happens when it isn't. + +## 3. Proof bytes must not move — the argument, and how to test it + +This is the load-bearing claim, so I want it argued *and* pinned by a test, not asserted. + +### 3.1 The argument + +1. Proof bytes are produced by exactly two things: the `ops.push_back(…)` calls in `emit.rs` and the + `Node` values they carry. The extraction moves **no** `push_back` and constructs **no** `Node`. + `emit_returned_node`, the `HashWithCount` / `HashWithCountAndSum` / `KVDigestCount` / + `KVDigestCountSum` construction, and the `Push`/`PushInverted`/`Parent`/`Child` sequencing are + untouched, character for character. +2. The three extracted functions are pure: no I/O, no interior mutability, no allocation, total over + their input domain. Their bodies are the existing expressions moved verbatim. +3. Therefore, for every call site, the emitted op is a pure function of (the decision returned, the + node data read from the tree). If the decision agrees with the branch the old code would have + taken, the op is identical. +4. So bit-identity reduces to a single finite claim: **the extracted functions agree with the + pre-extraction branch logic on every input.** That claim is testable exhaustively (§3.2). +5. Verifier, envelope (`proof/indexed_axis/envelope.rs`), and + `merk_versions.proof.prove_count_offset_on_range` are not touched at all. No version bump. + +### 3.2 How we test it + +Four layers, cheapest first: + +1. **Decision-table equivalence.** In `decide.rs`'s test module, keep a `reference_*` copy of the + pre-extraction branch logic (literally the `match` blocks as they exist at `a2791bb`) and assert + equality over the full behavioural domain: `class ∈ {Disjoint, Contained, Boundary}` × + `subtree_count ∈ {0,1,2,3}` × `offset_remaining ∈ {0,1,2,3}` × `limit_remaining ∈ {None, Some(0), + Some(1)}`, and the same for `node_disposition`. A few hundred cases, exhaustive over every + comparison boundary that matters. Optionally a `proptest` sweep over full `u64`s on top. +2. **Golden proof-byte digests — the real guarantee.** Before touching anything, run a generator on + the base commit that builds a deterministic fixture corpus and prints `blake3(encode_into(ops))` + per case; commit those digests as constants and assert them after. Corpus: the existing + `make_15_key_provable_count_tree` fixture plus a PCPS fixture and a ~1000-key fixture, crossed + with `offset ∈ {0, 1, 7, 14, 15, 1000}`, `limit ∈ {None, Some(0), Some(1), Some(5)}`, + `left_to_right ∈ {true, false}`, and each of the three host tree types. `count_offset/tests.rs` + already has the `encode_proof` helper, so this is a small addition in the right place. + A digest mismatch fails loudly and names the exact case. +3. **Existing suites unchanged.** `count_offset/tests.rs` (1490 lines) plus + `grovedb/src/tests/indexed_axis_offset_proof_tests.rs` and `indexed_axis_proof_tests.rs` must pass + with **zero edits**. Any test needing an edit is a red flag, not a rebase. +4. **Cross-check against the reader.** A differential test asserting the proved path and the new + unproved path return the same keys in the same order for a randomized corpus — this is what + catches a divergence in the *decisions* even if the bytes are stable. + +If review still judges any edit to `emit.rs` unacceptable, there is a strictly-safer fallback: +**do not touch `emit.rs` at all**, duplicate the two decision blocks in `read.rs`, and add the §3.2.1 +table test asserting the two copies agree. Bit-identity then holds by construction (the compiler +sees an unchanged emitter) at the cost of ~25 duplicated lines. I lean to the extraction because +duplicated decision logic silently drifts, but the fallback is a one-line change of plan and I will +take it if the owner prefers it. + +## 4. Testable assertion that the skip is counted, not linear + +Noted that the executors discard `CostContext` (`cost: _` at `execute_top_k.rs:66/84/102/182`), so +this cannot be asserted Platform-side. It has to be asserted here, and it can be — every grovedb +indexed-tree API returns `CostResult`, so a test can read `OperationCost` directly. + +The intent to encode is "**the offset costs at most one descent**", not "it is fast". Proposed +permanent regression test in `grovedb/src/tests/` (new +`indexed_axis_paginated_cost_tests.rs`, matching the existing per-topic test-file convention): + +```rust +// N entries, k results. AVL depth is bounded by 1.44·log2(N+2). +let CostContext { value: at_0, cost: cost_0 } = + db.indexed_count_top_k_paginated(path, k, 0, false, tx, v); +let CostContext { value: at_far, cost: cost_far } = + db.indexed_count_top_k_paginated(path, k, N - k as u64, false, tx, v); +at_0.expect("offset 0"); at_far.expect("deep offset"); + +let depth_bound = (1.44 * ((N + 2) as f64).log2()).ceil() as u64; + +// The counted skip pays one root-to-leaf descent for the offset — nothing per skipped entry. +assert!(cost_far.seek_count <= cost_0.seek_count + 2 * depth_bound, + "offset skip is walking entries: seek_count {} at offset {} vs {} at offset 0 (depth bound {})", + cost_far.seek_count, N - k as u64, cost_0.seek_count, depth_bound); +assert!(cost_far.storage_loaded_bytes <= cost_0.storage_loaded_bytes.saturating_mul(3) + 4096, …); +``` + +Properties that make this a real regression test rather than a snapshot: + +- **It fails today.** At `N = 10_000, k = 10`, the current loop does ≈ 10_000 iterator steps + (`seek_count` ≈ N) against a bound of ≈ `cost_0.seek_count + 2·20`. Red before, green after — the + red run is worth recording in the commit message. +- **It is machine-independent.** `OperationCost` counters only, never wall-clock. E1's own plan says + the same, for the same reason. +- **It encodes why.** The bound is stated as "offset ≤ one descent", so it keeps failing if someone + reintroduces any per-entry work in the skip, even a cheap one. + +Plus a merk-level twin in `count_offset/tests.rs`: for a 1000-key fixture, +`read_count_offset_on_range(RangeFull, 999, Some(1), …)` must return the last key and touch +`seek_count ≤ some small multiple of the depth` — this pins the primitive independently of grovedb's +wrapper. + +And, per the spec's verification plan, an **equality** test across the offset grid × all three axes × +both directions × tie-heavy populations (many entries sharing an axis value, so the item-key +tiebreak is exercised), asserting the new results equal the pre-fix results element-for-element. + +## 5. What could go wrong + +Ordered by how much I want the owner to look at it. + +1. **Storage rows that are not in the tree ("ghost rows"), and vice versa.** The old read iterates + *storage*; the new read walks the *tree*. In a drifted secondary these differ — a ghost row is + returned today and would not be after. This state is corruption by definition + (`verify_grovedb` flags it; `indexed_tree_secondary_drift_tests.rs` builds it deliberately by + injecting rows directly into storage), and the **proved** path already walks the tree, so the + change makes unproved reads agree with proved reads instead of disagreeing. I consider that a + fix, but it is a returned-value change in a corrupt state and the spec says "cost only", so it + needs an explicit owner decision. +2. **Corrupt keys in the skipped region stop being detected.** Today every skipped key is decoded and + a malformed one raises `corrupted_secondary_key_error`. A counted skip never looks at them, so + that error becomes `Ok`. This is inherent to option A — validating the skipped region *is* the + linear scan. Returned keys are still validated. Documented, not fixed. +3. **`NonCounted`-wrapped entry in a secondary.** Cannot happen (§2), but if it did, counted skip and + positional skip would disagree — permanently and invisibly. Proposal: the reader returns + `Error::CorruptedState` on an in-range node with `own_struct == 0`, mirroring the prover's refusal + rather than silently dropping the entry. Fail loud over a silent divergence. +4. **Cost profile of the *collect* phase.** The skip gets much cheaper; the k-window changes from a + sequential `raw_iter` scan to k node fetches through `RefWalker::walk`. For small k (the ranked + surface's actual use) this is noise; for large k on a cold cache it could regress. Mitigation: + measure at `k ∈ {1, 10, 100, 1000}` before/after. If it regresses, the fallback is a **hybrid** — + use a counted descent only to *rank-select* the key at position `offset`, then hand that key to + the existing `KVIterator` as a directional range start and collect k exactly as today. That keeps + the collect phase byte-identical to current behaviour and shrinks the diff, at the cost of + sharing less with the prover. I am not proposing it first because the full walk is what makes + unproved reads structurally match the proved path. +5. **`value_defined_cost_fn`.** `emit` passes `None` to `walker.walk(..)` even though the secondary + is opened with `Some(&Element::value_defined_cost_for_serialized_value)`. That affects `KV` cost + accounting only, not values. I will mirror the prover (pass `None`) so the two walks stay + comparable, and note it. +6. **Resident memory.** `RefWalker::walk` upgrades `Link::Reference` → `Link::Loaded`, so walked + nodes stay attached to the in-memory tree. Bounded here at `O(log n + k)` nodes, and the + secondary `Merk` is opened per query and dropped — same profile as the prover. +7. **Version gating.** `prove_count_offset_on_range` is gated via + `merk_versions.proof.prove_count_offset_on_range`. A read is not consensus-visible, and adding a + field to `MerkProofVersions` means touching `v1.rs`–`v4.rs`. My inclination is **not** to gate a + read-only method and to say so in its doc comment; happy to gate it if the convention is meant to + be absolute. Owner call — it is cheap either way. +8. **`k = 0` / `offset` past the end / empty secondary.** All three return an empty vec today; the + walker must too (`limit = Some(0)` ⇒ immediate stop; exhausted population ⇒ empty entries with + `offset_remaining > 0`; `use_tree` `None` ⇒ empty). Covered by the equality grid. +9. **`u64` offset vs `u16` k.** Unchanged from today; `limit` becomes `Some(k as u64)` exactly as the + prover already does at `generate.rs:911`. + +## 6. Open questions — unanswered, must be settled before implementing + +These were put to the owner and overtaken by the decision to ship option C first. Whoever picks this +up needs answers to 1–3 before writing code; 4 is a scope confirmation. + +1. **Ghost-row divergence (risk 1)** — accept it (unproved reads start matching proved reads), or + gate the change so behaviour in a drifted state is bit-preserved? +2. **`emit.rs` edit** — extract the pure decision functions (the recommendation, §1), or zero-diff on + the emitter with duplicated decision logic plus an agreement test (§3.2 fallback)? +3. **Version gating (risk 7)** — gate `read_count_offset_on_range` behind a new `MerkVersions` + field, or leave a read-only method ungated? +4. **Scope** — this fixes the paginated read only. `indexed_axis_top_k_generic` and + `indexed_axis_range_generic` take no offset and are untouched. Confirm they should stay that way + (recommendation: yes). + +### One thing that changed since this was written + +By the time this lands, Platform's unproved ranked reads will already be going through +prove-then-verify (option C). So the pre/post equality grid in §4 has a **third** baseline available +and should use it: new counted read == old linear read == prove-then-verify result, same entries, +same order. Three-way agreement is a stronger check than the two-way one originally planned. It is +also worth confirming at that point whether `indexed_axis_top_k_paginated_generic` still has a live +caller, or whether the win is to point Platform's unproved path back at it and retire the +prove-then-verify detour. + +## 7. Implementation plan, if picked up + +Sequenced so the byte-neutrality of step 2 is provable in isolation: + +1. Generate and commit the golden proof-byte digests **on the base commit**, before any change. + Do not skip this or fold it into step 2 — the digests are only trustworthy if they were produced + by a binary that predates the extraction. +2. Extract `decide.rs`; rewire `emit.rs`'s three call sites; run the whole merk + grovedb proof + suite and the golden digests. This step alone must be provably byte-neutral. +3. Add `read.rs` + `Merk::read_count_offset_on_range` with merk-level tests. +4. Rewire `indexed_axis_top_k_paginated_generic`. +5. Add the cost regression test (record its red run) and the pre/post equality grid. +6. Multi-agent review of the diff per the repo's pipeline. From e79f618603edd5bbb6f1b317911d880127f59319 Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Thu, 13 Aug 2026 14:48:09 +0700 Subject: [PATCH 02/11] feat: counted offset skip for unproved ranked paginated reads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the linear offset walk in indexed_axis_top_k_paginated_generic (one storage-iterator step per skipped entry, Θ(min(offset, N))) with a counted descent over the secondary merk through the public Merk::walk: whole subtrees are consumed from their parents' link aggregate counts without ever being fetched, so a positive offset costs one root-to-position path (O(log n) node loads) plus the k-collect. The offset == 0 path keeps the raw storage iterator, now shared structurally with the plain top_k core (collect_top_k_via_iterator), so the common shape is untouched by construction. offset >= population is answered from the root aggregate alone with zero fetches. All three axes (count, sum, avg) funnel through the one changed generic. Proof bytes are unchanged by construction: no file under merk/ and no proof module is touched; the new code calls only read-only public traversal APIs, and every existing proof suite passes with zero edits. Hardening from review: strict provable-count aggregate matching (never as_count_u64's silent 0), own-count == 1 payload check, link-vs-child aggregate cross-check on every descent, present-but-zero-count link rejection, and a 128-level depth ceiling so cyclic link corruption errors instead of overflowing the stack. All corruption paths return Error::CorruptedData; no panic, no u64 wrap. Behavioral delta, deliberate: skipped rows are no longer decoded, so a malformed key inside the skipped region no longer errors the read (it still occupies its counted position; returned rows are still validated, and verify_grovedb still flags the state). The drift-suite assertion pinning the old decode-during-skip behavior was updated to pin the new contract — the only edited existing test. Test would have caught this in CI: ✖ before fix, ✔ after. paginated_offset_skip_is_counted_not_linear failed on the pre-change code with "seek_count 604 at offset 595 vs 9 at offset 0 (depth bound 14)" and passes after; equality grids (3 axes x both directions x offset/k boundaries x tie-heavy fixtures) pass before and after, and paginated_offset_zero_costs_exactly_plain_top_k pins offset-0 cost equality with plain top-k in both directions. Measured (release, measure_paginated_costs harness; k=1): offset 0 is identical to the old read at every N (5 seeks / 625 B / ~6 us); deep offset at N=1e6 is 22 seeks / 3.7 KB / 32 us vs 1,000,004 seeks / 316 MB / 326 ms linear; past-the-end is flat 3 seeks / 366 B / 4 us at every N. Known accepted corner: offset=1 k=100 costs ~150 us vs the old ~30 us (point-gets vs sequential iteration; near-identical counters), crossing over to counted-wins around offset ≈ a few hundred. --- docs/COUNTED_SKIP_DESIGN.md | 613 +++++++---------- grovedb/src/operations/indexed_tree.rs | 514 +++++++++++++-- .../indexed_axis_paginated_cost_tests.rs | 624 ++++++++++++++++++ .../indexed_tree_secondary_drift_tests.rs | 36 +- grovedb/src/tests/mod.rs | 1 + 5 files changed, 1323 insertions(+), 465 deletions(-) create mode 100644 grovedb/src/tests/indexed_axis_paginated_cost_tests.rs diff --git a/docs/COUNTED_SKIP_DESIGN.md b/docs/COUNTED_SKIP_DESIGN.md index e692c431e..4798afbab 100644 --- a/docs/COUNTED_SKIP_DESIGN.md +++ b/docs/COUNTED_SKIP_DESIGN.md @@ -1,394 +1,223 @@ -# Counted skip for unproved ranked reads — design note +# Counted skip for unproved ranked reads — agreed design (v2) -**Status:** design only — **deferred, not implemented**. No code was written. +**Status:** agreed, implemented on this branch. **Branch:** `fix/counted-skip-unproved-read`, based on `a2791bb` (`develop`). -**Problem statement:** `platform.test-indexes/docs/ranked-index-testing/FIX_SPEC_OFFSET.md`. -**Line numbers below are as of `a2791bb`** — if they have drifted, search by function name. - -### Why this is on the shelf - -**This design was deliberately not implemented. It was superseded by a cheaper mitigation, not -abandoned as unworkable.** Nothing below was found to be wrong, infeasible, or blocked — the work -simply stopped being necessary before it started, and the note was kept as the record. - -The spec's option A (this document) was the owner's first choice. It was then superseded as the -*immediate* remedy by option C: Platform serves unproved ranked reads through the prover internally -and verifies its own proof to recover the entries. That removes the linear offset walk with no -grovedb change, no query-grammar change and no cross-repo pin bump, so it shipped first. Measured: -78–129 µs round trip, with the deep-offset lever — the whole point of the exercise — dropping from -440 ms to a flat 48 µs. That is a complete fix for the denial-of-service problem, which is why this -one stopped being ship-blocking. - -This design stays the correct long-term shape. What it would buy once picked up: the unproved read -stops paying prove-then-verify overhead (proof construction, serialization, and full verification on -every read) and instead does the counted descent directly — the same `O(log n + k)` work with none of -the proof machinery. Against the 78–129 µs baseline above, that is an optimization, not a fix; treat -it as such when deciding whether it earns the risk of touching the proof emitter. It also makes the -code comment at `mode_detection.rs:302-313` true for both paths rather than only the proved one. - -The finding that reshaped the decision, recorded here so nobody re-derives it: **there is no -non-proof counted-skip primitive in merk.** The counted descent exists only inside the proof -emitter. Verified independently by an adversarial review. That is why option A is an extraction plus -a new entry point, not a wiring job — and why it was worth deferring rather than rushing. - -## 0. What the problem is, restated against this repo - -`indexed_axis_top_k_paginated_generic` (`grovedb/src/operations/indexed_tree.rs:1297`) skips the -offset by stepping a **storage** iterator once per skipped entry: - -```rust -let mut iter = KVIterator::new(secondary_merk.storage.raw_iter(), &all_query)…; -while skipped < offset { - match iter.next_kv() { Some((secondary_key, _)) => { decode(&secondary_key)?; skipped += 1 } … } -} -``` - -That is `Θ(min(offset, N))`. All three axes funnel through this one function -(`indexed_count_top_k_paginated:1483`, `indexed_sum_top_k_paginated:1677`, -`indexed_avg_top_k_paginated:1862`), so one fix covers the whole family. - -The proved path for the same query shape is -`build_indexed_axis_paginated_proof` → `Merk::prove_count_offset_on_range(RangeFull, offset, -Some(k), !descending)` (`grovedb/src/operations/proof/indexed_axis/generate.rs:904-918`). It walks -the **merk tree** and collapses any wholly-in-range subtree whose aggregate count fits inside the -remaining offset into one step — `O(log n + k)`. - -Confirmed against the tree: there is no non-proof counted-skip / rank-select primitive anywhere in -`merk`. `merk/src/proofs/query/count_offset/` is `emit / mod / prove / tests / verify`; every entry -point produces `Op`s. The independent reviewer reached the same conclusion. - -## 1. Exactly what can be shared, and what cannot - -I went through `emit_count_offset_proof` (`count_offset/emit.rs:95-503`) line by line. It does seven -things; only two of them are the counted-skip logic: - -| # | Step | Shareable with a plain read? | -|---|---|---| -| 1 | `classify_subtree(lo, hi, range)` | **Already shared** — lives in `aggregate_common.rs`, used by aggregate-count/sum too. Reuse as-is. | -| 2 | Collapse decision (Disjoint / count ≤ offset / past-limit / descend) — `emit.rs:136-148` | **Yes.** Pure function of `(class, subtree_count, offset_remaining, limit_remaining)`. | -| 3 | `own_struct = count − left_link_count − right_link_count` — `emit.rs:223-244` | **Yes.** Three lines of arithmetic on link aggregates. | -| 4 | Direction ordering (`first_dir`/`second_dir`) — `emit.rs:324-328` | **Yes**, but it is two lines; sharing it is not worth an abstraction. | -| 5 | Per-node disposition (path / offset-skipped / returned / past-limit) — `emit.rs:412-430` | **Yes.** Pure function of `(is_in_range, own_struct, offset_remaining, limit_remaining)`. | -| 6 | Op emission: `Node::HashWithCount…`, `Push`/`PushInverted`, `Parent`/`Child`, `emit_returned_node` | **No** — proof-only. Must not be touched. | -| 7 | Rejection of unsupported in-range shapes (NonCounted / Reference / non-empty tree) — `emit.rs:276-319` | **No** — those are constraints of the *wire format*, not of the traversal. The reader has its own (different) rule; see §4.3. | - -There is also one thing the reader must **not** share: the prover cannot stop early. Once the limit -is exhausted it still has to walk the rest of the tree and emit a `HashWithCount` per remaining -subtree, because the verifier reconstructs the root hash from the whole op stream. A plain read has -no such obligation and returns the moment `limit_remaining == Some(0)`. So the *traversal* is not -literally shared; the *decisions* are. - -### Proposed extraction - -New file `merk/src/proofs/query/count_offset/decide.rs`, holding only pure code moved verbatim out -of `emit.rs`: - -```rust -pub(super) enum CollapseAction { Disjoint, SkippedByOffset, PastLimit } // moved from emit.rs:511 -pub(super) enum NodeDisposition { Path, SkippedByOffset, PastLimit, Returned } - -/// Whole-subtree decision. `None` = must descend per-element. -pub(super) fn collapse_action( - class: SubtreeClassification, subtree_count: u64, - offset_remaining: u64, limit_remaining: Option, -) -> Option; - -/// Per-node decision at a descended node. -pub(super) fn node_disposition( - is_in_range: bool, own_struct: u64, - offset_remaining: u64, limit_remaining: Option, -) -> NodeDisposition; - -/// own_count = aggregate − left_link_count − right_link_count (saturating, as today). -pub(super) fn own_structural_count(node_count: u64, left: u64, right: u64) -> u64; -``` - -Both decision functions are **total, pure, allocation-free, and take no `&mut` state**. State -mutation (`offset_remaining -= 1`, `limit -= 1`, `returned += 1`) stays at each call site, exactly -where it is today. `emit.rs` shrinks by ~25 lines and gains three call sites; nothing else in it -moves. - -**Why not a visitor/callback trait over the whole recursion?** It would force the prover's -walk shape onto the reader (no early exit), thread a lifetime-heavy sink through the hot path, and — -decisively — it would rewrite the function that produces consensus-frozen bytes. The -decision-function extraction touches the emitter in three mechanical spots and can be argued -bit-identical (§3); a visitor rewrite cannot. - -## 2. The new read-only entry point - -Two new files, mirroring the prover's own split exactly: - -**`merk/src/proofs/query/count_offset/read.rs`** — the recursion: - -```rust -pub(super) struct ReadState { offset_remaining: u64, limit_remaining: Option, - left_to_right: bool, done: bool } - -pub(super) fn read_count_offset( - walker: &mut RefWalker<'_, S>, range: &QueryItem, - subtree_lo_excl: Option<&[u8]>, subtree_hi_excl: Option<&[u8]>, - state: &mut ReadState, out: &mut Vec<(Vec, Vec)>, - grove_version: &GroveVersion, -) -> CostResult<(), Error>; -``` - -Same bound-tightening (`walk left → (lo, node_key)`, `walk right → (node_key, hi)`), same direction -ordering, same `classify_subtree`, same `collapse_action` / `node_disposition`. Differences from -`emit`: no `ops`, no `Node` construction, no `tree_type` parameter (nothing is hashed, so the -single-axis vs dual-axis PCPS split is irrelevant here), returns `()` instead of a structural count -(only the verifier needs that), and returns early once `state.done`. - -**`merk/src/merk/read_count_offset.rs`** — the entry point, sibling of -`merk/src/merk/prove_count_offset.rs`: - -```rust -impl<'db, S: StorageContext<'db>> Merk { - pub fn read_count_offset_on_range( - &self, inner_range: &QueryItem, offset: u64, limit: Option, - left_to_right: bool, grove_version: &GroveVersion, - ) -> CostResult; -} - -pub struct CountOffsetReadResult { - pub entries: Vec<(Vec, Vec)>, // (key, value) in directional order - pub offset_remaining: u64, // > 0 ⇒ offset ran past the end of the population -} -``` - -It has to live under `merk/src/merk/` because `Merk::use_tree` is `pub(crate)` and `Merk::source()` -is `pub(in crate::merk)` — the same reason `prove_count_offset.rs` lives there. Same tree-type guard -as the prover (`ProvableCountTree` / `ProvableCountSumTree` / `ProvableCountProvableSumTree`), same -empty-merk behaviour (`use_tree_mut` → `None` ⇒ empty result, full offset remaining). - -Values come for free — the node is loaded either way — so the primitive returns them, even though -the current grovedb caller discards them (`Some((secondary_key, _))`). - -**GroveDB caller.** `indexed_axis_top_k_paginated_generic` replaces both loops with: - -```rust -let read = cost_return_on_error!(&mut cost, - secondary_merk.read_count_offset_on_range( - &QueryItem::RangeFull(..), offset, Some(k as u64), !descending, grove_version)); -let mut results = Vec::with_capacity(read.entries.len()); -for (secondary_key, _) in read.entries { - match decode(&secondary_key) { - Some(d) => results.push(d), - None => return Err(corrupted_secondary_key_error(axis, &secondary_key)).wrap_with_cost(cost), - } -} -``` - -`Query`/`KVIterator` disappear from this function. `indexed_axis_top_k_generic` and -`indexed_axis_range_generic` are **untouched** — neither takes an offset, so neither has the bug. - -### Ordering and tie-equivalence (the "identical entries, identical ordering" constraint) - -- The secondary's merk key **is** the sort key: `axis_sort_bytes ‖ item_key` - (`make_axis_secondary_key`, `indexed_tree.rs:78`). In-order traversal of the merk = lexicographic - order = exactly the order `raw_iter` produces. Reverse traversal = `raw_iter` backwards. -- Keys are unique (the item key is a suffix of every secondary key), so the order is **total**. - "Ties on the axis value" are broken by `item_key` inside the key bytes themselves — there is no - comparator, no stability question, and nothing for a different traversal to reorder. -- `left_to_right = !descending` in both the old read and the prover, so the offset counts in the - same directional order in all three paths. - -### Why counted-skip == positional-skip here - -The counted skip advances by *aggregate count*; the old loop advances by *iteration position*. They -agree iff every entry contributes exactly 1 to the count. In these secondaries it does, by -construction: - -- Every secondary is count-bearing: `axis_secondary_tree_type` → `ProvableCountTree` (count axis) or - `ProvableCountProvableSumTree` (sum, avg) — `indexed_tree.rs:57-70`. -- The only writer is `mirror_indexed_axis_to_secondary` (`indexed_tree.rs:2213`), which writes - exactly one of `Item(∅)` / `SumItem(sum)` / `ItemWithSumItem(∅, sum)` — never `NonCounted`, never - a tree, never a reference. Each contributes count = 1. -- `verify_grovedb`'s `verify_indexed_axis_content` (`grovedb/src/lib.rs:1614`) enforces exactly one - secondary row per primary entry, with that payload, and nothing else in the secondary. - -So on any state the DB considers valid, rank == position. §4 covers what happens when it isn't. - -## 3. Proof bytes must not move — the argument, and how to test it - -This is the load-bearing claim, so I want it argued *and* pinned by a test, not asserted. - -### 3.1 The argument - -1. Proof bytes are produced by exactly two things: the `ops.push_back(…)` calls in `emit.rs` and the - `Node` values they carry. The extraction moves **no** `push_back` and constructs **no** `Node`. - `emit_returned_node`, the `HashWithCount` / `HashWithCountAndSum` / `KVDigestCount` / - `KVDigestCountSum` construction, and the `Push`/`PushInverted`/`Parent`/`Child` sequencing are - untouched, character for character. -2. The three extracted functions are pure: no I/O, no interior mutability, no allocation, total over - their input domain. Their bodies are the existing expressions moved verbatim. -3. Therefore, for every call site, the emitted op is a pure function of (the decision returned, the - node data read from the tree). If the decision agrees with the branch the old code would have - taken, the op is identical. -4. So bit-identity reduces to a single finite claim: **the extracted functions agree with the - pre-extraction branch logic on every input.** That claim is testable exhaustively (§3.2). -5. Verifier, envelope (`proof/indexed_axis/envelope.rs`), and - `merk_versions.proof.prove_count_offset_on_range` are not touched at all. No version bump. - -### 3.2 How we test it - -Four layers, cheapest first: - -1. **Decision-table equivalence.** In `decide.rs`'s test module, keep a `reference_*` copy of the - pre-extraction branch logic (literally the `match` blocks as they exist at `a2791bb`) and assert - equality over the full behavioural domain: `class ∈ {Disjoint, Contained, Boundary}` × - `subtree_count ∈ {0,1,2,3}` × `offset_remaining ∈ {0,1,2,3}` × `limit_remaining ∈ {None, Some(0), - Some(1)}`, and the same for `node_disposition`. A few hundred cases, exhaustive over every - comparison boundary that matters. Optionally a `proptest` sweep over full `u64`s on top. -2. **Golden proof-byte digests — the real guarantee.** Before touching anything, run a generator on - the base commit that builds a deterministic fixture corpus and prints `blake3(encode_into(ops))` - per case; commit those digests as constants and assert them after. Corpus: the existing - `make_15_key_provable_count_tree` fixture plus a PCPS fixture and a ~1000-key fixture, crossed - with `offset ∈ {0, 1, 7, 14, 15, 1000}`, `limit ∈ {None, Some(0), Some(1), Some(5)}`, - `left_to_right ∈ {true, false}`, and each of the three host tree types. `count_offset/tests.rs` - already has the `encode_proof` helper, so this is a small addition in the right place. - A digest mismatch fails loudly and names the exact case. -3. **Existing suites unchanged.** `count_offset/tests.rs` (1490 lines) plus - `grovedb/src/tests/indexed_axis_offset_proof_tests.rs` and `indexed_axis_proof_tests.rs` must pass - with **zero edits**. Any test needing an edit is a red flag, not a rebase. -4. **Cross-check against the reader.** A differential test asserting the proved path and the new - unproved path return the same keys in the same order for a randomized corpus — this is what - catches a divergence in the *decisions* even if the bytes are stable. - -If review still judges any edit to `emit.rs` unacceptable, there is a strictly-safer fallback: -**do not touch `emit.rs` at all**, duplicate the two decision blocks in `read.rs`, and add the §3.2.1 -table test asserting the two copies agree. Bit-identity then holds by construction (the compiler -sees an unchanged emitter) at the cost of ~25 duplicated lines. I lean to the extraction because -duplicated decision logic silently drifts, but the fallback is a one-line change of plan and I will -take it if the owner prefers it. - -## 4. Testable assertion that the skip is counted, not linear - -Noted that the executors discard `CostContext` (`cost: _` at `execute_top_k.rs:66/84/102/182`), so -this cannot be asserted Platform-side. It has to be asserted here, and it can be — every grovedb -indexed-tree API returns `CostResult`, so a test can read `OperationCost` directly. - -The intent to encode is "**the offset costs at most one descent**", not "it is fast". Proposed -permanent regression test in `grovedb/src/tests/` (new -`indexed_axis_paginated_cost_tests.rs`, matching the existing per-topic test-file convention): - -```rust -// N entries, k results. AVL depth is bounded by 1.44·log2(N+2). -let CostContext { value: at_0, cost: cost_0 } = - db.indexed_count_top_k_paginated(path, k, 0, false, tx, v); -let CostContext { value: at_far, cost: cost_far } = - db.indexed_count_top_k_paginated(path, k, N - k as u64, false, tx, v); -at_0.expect("offset 0"); at_far.expect("deep offset"); - -let depth_bound = (1.44 * ((N + 2) as f64).log2()).ceil() as u64; - -// The counted skip pays one root-to-leaf descent for the offset — nothing per skipped entry. -assert!(cost_far.seek_count <= cost_0.seek_count + 2 * depth_bound, - "offset skip is walking entries: seek_count {} at offset {} vs {} at offset 0 (depth bound {})", - cost_far.seek_count, N - k as u64, cost_0.seek_count, depth_bound); -assert!(cost_far.storage_loaded_bytes <= cost_0.storage_loaded_bytes.saturating_mul(3) + 4096, …); -``` - -Properties that make this a real regression test rather than a snapshot: - -- **It fails today.** At `N = 10_000, k = 10`, the current loop does ≈ 10_000 iterator steps - (`seek_count` ≈ N) against a bound of ≈ `cost_0.seek_count + 2·20`. Red before, green after — the - red run is worth recording in the commit message. -- **It is machine-independent.** `OperationCost` counters only, never wall-clock. E1's own plan says - the same, for the same reason. -- **It encodes why.** The bound is stated as "offset ≤ one descent", so it keeps failing if someone - reintroduces any per-entry work in the skip, even a cheap one. - -Plus a merk-level twin in `count_offset/tests.rs`: for a 1000-key fixture, -`read_count_offset_on_range(RangeFull, 999, Some(1), …)` must return the last key and touch -`seek_count ≤ some small multiple of the depth` — this pins the primitive independently of grovedb's -wrapper. - -And, per the spec's verification plan, an **equality** test across the offset grid × all three axes × -both directions × tie-heavy populations (many entries sharing an axis value, so the item-key -tiebreak is exercised), asserting the new results equal the pre-fix results element-for-element. - -## 5. What could go wrong - -Ordered by how much I want the owner to look at it. - -1. **Storage rows that are not in the tree ("ghost rows"), and vice versa.** The old read iterates - *storage*; the new read walks the *tree*. In a drifted secondary these differ — a ghost row is - returned today and would not be after. This state is corruption by definition - (`verify_grovedb` flags it; `indexed_tree_secondary_drift_tests.rs` builds it deliberately by - injecting rows directly into storage), and the **proved** path already walks the tree, so the - change makes unproved reads agree with proved reads instead of disagreeing. I consider that a - fix, but it is a returned-value change in a corrupt state and the spec says "cost only", so it - needs an explicit owner decision. -2. **Corrupt keys in the skipped region stop being detected.** Today every skipped key is decoded and - a malformed one raises `corrupted_secondary_key_error`. A counted skip never looks at them, so - that error becomes `Ok`. This is inherent to option A — validating the skipped region *is* the - linear scan. Returned keys are still validated. Documented, not fixed. -3. **`NonCounted`-wrapped entry in a secondary.** Cannot happen (§2), but if it did, counted skip and - positional skip would disagree — permanently and invisibly. Proposal: the reader returns - `Error::CorruptedState` on an in-range node with `own_struct == 0`, mirroring the prover's refusal - rather than silently dropping the entry. Fail loud over a silent divergence. -4. **Cost profile of the *collect* phase.** The skip gets much cheaper; the k-window changes from a - sequential `raw_iter` scan to k node fetches through `RefWalker::walk`. For small k (the ranked - surface's actual use) this is noise; for large k on a cold cache it could regress. Mitigation: - measure at `k ∈ {1, 10, 100, 1000}` before/after. If it regresses, the fallback is a **hybrid** — - use a counted descent only to *rank-select* the key at position `offset`, then hand that key to - the existing `KVIterator` as a directional range start and collect k exactly as today. That keeps - the collect phase byte-identical to current behaviour and shrinks the diff, at the cost of - sharing less with the prover. I am not proposing it first because the full walk is what makes - unproved reads structurally match the proved path. -5. **`value_defined_cost_fn`.** `emit` passes `None` to `walker.walk(..)` even though the secondary - is opened with `Some(&Element::value_defined_cost_for_serialized_value)`. That affects `KV` cost - accounting only, not values. I will mirror the prover (pass `None`) so the two walks stay - comparable, and note it. -6. **Resident memory.** `RefWalker::walk` upgrades `Link::Reference` → `Link::Loaded`, so walked - nodes stay attached to the in-memory tree. Bounded here at `O(log n + k)` nodes, and the - secondary `Merk` is opened per query and dropped — same profile as the prover. -7. **Version gating.** `prove_count_offset_on_range` is gated via - `merk_versions.proof.prove_count_offset_on_range`. A read is not consensus-visible, and adding a - field to `MerkProofVersions` means touching `v1.rs`–`v4.rs`. My inclination is **not** to gate a - read-only method and to say so in its doc comment; happy to gate it if the convention is meant to - be absolute. Owner call — it is cheap either way. -8. **`k = 0` / `offset` past the end / empty secondary.** All three return an empty vec today; the - walker must too (`limit = Some(0)` ⇒ immediate stop; exhausted population ⇒ empty entries with - `offset_remaining > 0`; `use_tree` `None` ⇒ empty). Covered by the equality grid. -9. **`u64` offset vs `u16` k.** Unchanged from today; `limit` becomes `Some(k as u64)` exactly as the - prover already does at `generate.rs:911`. - -## 6. Open questions — unanswered, must be settled before implementing - -These were put to the owner and overtaken by the decision to ship option C first. Whoever picks this -up needs answers to 1–3 before writing code; 4 is a scope confirmation. - -1. **Ghost-row divergence (risk 1)** — accept it (unproved reads start matching proved reads), or - gate the change so behaviour in a drifted state is bit-preserved? -2. **`emit.rs` edit** — extract the pure decision functions (the recommendation, §1), or zero-diff on - the emitter with duplicated decision logic plus an agreement test (§3.2 fallback)? -3. **Version gating (risk 7)** — gate `read_count_offset_on_range` behind a new `MerkVersions` - field, or leave a read-only method ungated? -4. **Scope** — this fixes the paginated read only. `indexed_axis_top_k_generic` and - `indexed_axis_range_generic` take no offset and are untouched. Confirm they should stay that way - (recommendation: yes). - -### One thing that changed since this was written - -By the time this lands, Platform's unproved ranked reads will already be going through -prove-then-verify (option C). So the pre/post equality grid in §4 has a **third** baseline available -and should use it: new counted read == old linear read == prove-then-verify result, same entries, -same order. Three-way agreement is a stronger check than the two-way one originally planned. It is -also worth confirming at that point whether `indexed_axis_top_k_paginated_generic` still has a live -caller, or whether the win is to point Platform's unproved path back at it and retire the -prove-then-verify detour. - -## 7. Implementation plan, if picked up - -Sequenced so the byte-neutrality of step 2 is provable in isolation: - -1. Generate and commit the golden proof-byte digests **on the base commit**, before any change. - Do not skip this or fold it into step 2 — the digests are only trustworthy if they were produced - by a binary that predates the extraction. -2. Extract `decide.rs`; rewire `emit.rs`'s three call sites; run the whole merk + grovedb proof - suite and the golden digests. This step alone must be provably byte-neutral. -3. Add `read.rs` + `Merk::read_count_offset_on_range` with merk-level tests. -4. Rewire `indexed_axis_top_k_paginated_generic`. -5. Add the cost regression test (record its red run) and the pre/post equality grid. -6. Multi-agent review of the diff per the repo's pipeline. +**Supersedes:** the v1 emitter-extraction proposal, preserved in git history at `356af1e8` +(`git show 356af1e8:docs/COUNTED_SKIP_DESIGN.md`). Nothing in v1 was found unsound; v2 is +strictly smaller and was reached independently by two research passes. + +## 1. History, and why the unproved route is the fix rather than an optimization + +`indexed_axis_top_k_paginated_generic` (`grovedb/src/operations/indexed_tree.rs`) skipped the +offset by stepping a storage iterator once per skipped entry — `Θ(min(offset, N))`, a +consensus-reachable linear walk (~440 ms at deep offsets on the measured Platform fixture). All +three axes (Count u64 / Sum i64 / Avg i128 fixed-point) funnel through this one function. + +The first shipped mitigation (Platform PR #4382) routed the unproved read through +prove-then-verify internally: flat ~130 µs at any offset. It then drew a **blocking review** for a +structural reason: its handler-local retry re-executed the Drive request while still holding the +`platform_state` captured before the query began, so in the window where new state is published +but the block-height guard has not yet updated, a retry could attach the *old* block's signature +and metadata to *new*-state proof bytes. A counted unproved walk has no proof envelope and needs +no retry, so that failure mode ceases to exist — this design does not work around that review, it +removes the thing the review objected to. + +v1 of this document proposed extracting the emitter's decision logic (`decide.rs`) and adding a +merk-level `read.rs` + `Merk::read_count_offset_on_range`. Two later research passes (run +independently, then compared) both rejected that shape as unnecessarily risky and converged on the +design below; the owner confirmed the direction. + +## 2. Facts about `a2791bb` the design rests on (verified by both passes) + +1. **No unproved counted-skip / rank-select primitive exists.** v1's negative claim holds: + `merk/src/proofs/query/count_offset/` is `emit / mod / prove / tests / verify`, every entry + point produces proof ops, and nothing else in the workspace descends by count. +2. **But no-proof aggregate-aware walks are established precedent.** + `Merk::count_aggregate_on_range` (`merk/src/merk/get.rs:377`), `sum_aggregate_on_range` + (`:431`) and `count_and_sum_aggregate_on_range` (`:491`) walk the tree using link aggregates + with no proof machinery, and every `aggregate_*` proof module has a no-proof `walk.rs` sibling. + `count_offset/` is the only one missing its own — and grovedb's own proved rank-of-key + generator already trusts this family (`proof/indexed_axis/generate.rs:571` computes the rank + with the *unproved* count walk). +3. **Everything a counted descent needs is public.** `Merk::walk` (`merk/src/merk/mod.rs:523`) + yields a `RefWalker`; `RefWalker::{tree, walk}`, `TreeNode::{link, key, aggregate_data}` and + `Link::aggregate_data` are all public. The load-bearing fact: **`Link::aggregate_data()` + reads off `Link::Reference`** (`merk/src/tree/link.rs:209`), so a child subtree's population + is known *before* paying a fetch for it. (It panics on `Link::Modified`, which a freshly + opened read-only merk never holds — same exposure as merk's own walkers.) The implementation + deliberately does **not** use the public `AggregateData::as_count_u64`, which silently + returns 0 for non-count variants; it uses a strict matcher that treats any non-provable-count + aggregate as corruption. v1's claim that a new entry point had to live inside merk (because + `use_tree` is `pub(crate)`) was wrong. +4. **Counted rank equals iterator position on any valid secondary.** The secondary's merk key is + the complete sort key — order-preserving axis encoding ‖ original item key + (`make_axis_secondary_key`), so in-order traversal = lexicographic order = exactly what + `raw_iter` produces, ties broken by item key inside the key bytes, total order, both + directions. Every valid row contributes structural count exactly 1: the secondaries are + `ProvableCountTree` (count axis) / `ProvableCountProvableSumTree` (sum, avg), the only writer + (`mirror_indexed_axis_to_secondary`) writes exactly one count-1 payload per primary entry, and + `verify_indexed_axis_content` enforces it. + +## 3. The agreed design + +A grovedb-local counted traversal in `operations/indexed_tree.rs`, reached through the public +`Merk::walk`. No merk change of any kind. Specialized to `RangeFull` — the paginated caller only +ever scans the whole secondary, so `classify_subtree`, inherited bounds, and the proof flow's +shape-rejection rules are all unnecessary here. + +- **`offset == 0` keeps the raw-iterator path unchanged** — it is the overwhelmingly common + shape, and one iterator seek plus `k` sequential steps is cheaper than loading a root-to-leaf + merk path. The counted traversal serves `offset > 0` only. +- **Whole-population shortcut:** the root node is already loaded by the open, and its aggregate + is the population `N`. `offset ≥ N` returns empty with **zero** additional fetches — the + past-the-end case, which is the original DoS lever, costs the open and nothing else. +- **Counted descent**, per loaded node, children visited in direction order (ascending: left + first; descending: right first): + 1. Read the first child's count from its **link**, without fetching the child. + 2. If that count ≤ remaining offset: subtract and skip the child entirely (no fetch). This is + the counted skip. + 3. Otherwise fetch (`RefWalker::walk`) and recurse. + 4. Consume the node itself: its own structural count (aggregate − left link − right link, via + `checked_sub`) must be exactly 1 — anything else is corruption and fails loud + (`Error::CorruptedData`) rather than silently diverging from positional order. If offset + remains, burn it; else emit the node's key into the page. + 5. Recurse into the second child only while the page is not full; return the moment it is — + a reader has no obligation to keep walking, unlike the prover, which must hash-bind the + rest of the tree. +- The page holds secondary **keys** only; the caller decodes them exactly as before (the old + loop also discarded values). Cost is `O(depth + k)` node fetches for `offset > 0`. + +### Where the two research passes differed (recorded per the consolidation request) + +Both passes agreed on: the negative primitive claim; the walk-family precedent; link-count skip +without fetching; the root shortcut; own-count==1 fail-loud; the `offset == 0` fast path; the +`RangeFull` specialization; O(depth + k) cost; and proof-byte neutrality by construction. They +differed on two points: + +1. **Placement.** One pass preferred a merk-level `count_offset/walk.rs` twin with an entry in + `get.rs` (maximal conformance to the walk-family precedent, merk-level testability); the other + preferred the grovedb-local specialization (no new public merk API, no cross-crate release + coordination, no general-range machinery on the hot path). The owner chose grovedb-local. If a + general-range or cross-crate primitive is ever wanted, the merk-level twin is the shape — and + it must live under `merk::proofs::query`, because `classify_subtree` / + `SubtreeClassification` are `pub(super)` there. +2. **Open-cost constant** (≈2 vs ≈3–5 seeks before the descent). Settled empirically by the cost + regression test rather than argued. + +## 4. Behavior in corrupt or concurrent states (unchanged posture from v1, re-confirmed) + +- **Ghost rows** (storage rows absent from the tree, or vice versa): the old read iterates + storage, the new read walks the tree, so results in a *drifted* secondary differ. That state + is corruption by definition (`verify_grovedb` flags it; the drift tests build it deliberately), + and the proved path already walks the tree — the change makes unproved reads agree with proved + reads instead of with the raw-storage accident. +- **Corrupt keys inside the skipped region** are no longer decoded (not visiting them is the + point); returned keys are still validated, and a visited node with own-count ≠ 1 fails loud. +- **No snapshot isolation** (`start_transaction` takes no snapshot at this rev): a multi-fetch + walk can observe a concurrent commit between fetches. For *this function specifically* that is + a real (if narrow) regression, stated plainly: the old implementation ran skip and collect + through a single RocksDB iterator, which pins a consistent view for the whole page, so the page + was internally consistent even under concurrent writes; the counted descent is independent + point-gets with no read snapshot — the same exposure as merk's existing unproved aggregate + walks, but new to this read. A torn walk surfaces as `CorruptedData` (the checked count + arithmetic and the link/child cross-check both trip on mixed-version nodes) or a + stale-but-ordered page. The fix is a storage-layer read snapshot, outside this scope. +- **One function, two views.** `offset == 0` serves the storage-iterator view; `offset > 0` + serves the merk-tree view. Identical on any clean secondary; in a drifted one, a caller paging + `offset = 0, k, 2k, …` could see a discontinuity between the first page and the rest. Accepted: + the drifted state is corruption, and the pages a drifted state produces are not consistent + under the old code either (they include rows `verify_grovedb` rejects). + +## 5. Proof-byte neutrality — how it is proven rather than asserted + +The consensus-frozen envelope must not move. Three lines of proof, strongest first: + +1. **By construction:** the diff touches only `grovedb/src/operations/indexed_tree.rs` and test + files. No file under `merk/` and no grovedb proof module is modified — `git diff --stat` + against the base commit is checkable in review. The new code calls only read-only public + traversal APIs (`Merk::walk` / `RefWalker::walk`) and shares zero code with proof emission, + so no code path exists through which emitted bytes could change. +2. **Empirically:** the entire existing **proof** suites — `merk` `count_offset` tests, grovedb + `indexed_axis_proof_tests`, `indexed_axis_offset_proof_tests`, `count_offset_paginated_tests` + — pass with **zero edits**. Any proof-test edit would be a red flag, not a rebase. (One + non-proof test *was* deliberately edited: a drift-suite assertion that pinned the linear + skip's decode-during-skip behavior — see §7.4.) +3. **Root-hash pinning:** existing tests assert verified proofs against the live root hash, so + even an indirect state-shape change would surface as hash mismatches. + +This is why v1's golden-digest machinery is not needed: it existed to police an edit to +`emit.rs`, and v2 makes no such edit. + +## 6. Measured costs + +Measured with the in-repo harness (`indexed_axis_paginated_cost_tests::measure_paginated_costs`, +release build, N-row Count-axis secondaries, `k ∈ {1, 100}`, wall-clock = min of 3 runs on a +loaded shared machine — the seek/byte counters are the machine-independent signal). "linear" is +the pre-change implementation, kept verbatim as a test-only baseline; the harness also asserts +both paths return identical rows at every point. Selected rows (`k = 1`; full grid in the +harness output): + +| N | offset | counted (seeks / bytes / µs) | linear (seeks / bytes / µs) | +|---:|---:|---:|---:| +| 1e3 | 0 | 5 / 625 / 6 | 5 / 625 / 6 | +| 1e3 | N−1 | 11 / 1,727 / 15 | 1,004 / 316 KB / 268 | +| 1e3 | ≥ N | 3 / 362 / 4 | 1,004 / 316 KB / 259 | +| 1e6 | 0 | 5 / 629 / 8 | 5 / 629 / 7 | +| 1e6 | N−1 | 22 / 3,710 / 32 | 1,000,004 / 316 MB / 326,226 | +| 1e6 | 4×10⁹ | 3 / 366 / 4 | 1,000,004 / 316 MB / 339,806 | + +What the numbers establish: + +- **Offset 0 has zero regression** — identical seeks, bytes, and wall-clock in both directions + and at both `k` values, at every N (the fast path *is* the old code, and the always-on + `paginated_offset_zero_costs_exactly_plain_top_k` test pins the equality). +- **The deep-offset seek count is the tree depth**: 11 → 15 → 18 → 22 across N = 1e3 → 1e6, + logarithmic exactly as designed; wall-clock 15–36 µs against the linear walk's 268 µs–378 ms. +- **Past-the-end — the DoS lever — is flat 3 seeks / 366 B / 4 µs at every N**: the root + aggregate answers it with zero descent. +- Against the previously measured prove-then-verify route (78–129 µs flat, plus its proof + construction/serialization/verification CPU), the counted read measures 4–36 µs at `k = 1` — + the "unproved is even faster" claim is now measured, not modeled. +- **The one corner where the old path was faster, measured and accepted:** tiny positive offsets. + At `offset = 1, k = 100` the counted path costs 107–112 seeks / ~14 KB / 141–155 µs against the + linear read's 105 seeks / ~32 KB / ~30 µs — near-identical counters, ~5× wall-clock, because k + tree-node point-gets are slower than k sequential iterator steps. The crossover to + counted-wins sits around offset ≈ a few hundred rows. Absolute worst measured cost is ~155 µs + (the same order as prove-then-verify's flat floor), the shape decays as offset grows, and the + alternative — a threshold hybrid falling back to the linear skip for small offsets — would make + skipped-region error semantics depend on the offset value. Accepted as-is; §8 keeps the hybrid + on record if max-cost-per-request ever matters more than semantic uniformity. + +## 7. Test plan + +1. **Equality grid** (pins behavior; green before and after): the unchanged `indexed_*_top_k` + iterator path is the oracle — `paginated(k, offset) == top_k(offset + k)[offset..]` across all + three axes, both directions, offsets at and around subtree boundaries / population − 1 / + population / past-end, `k ∈ {0, 1, small, larger-than-remainder}`, and tie-heavy populations + exercising the item-key tiebreak. +2. **Offset-0 non-regression pin:** `paginated(k, 0)` must cost exactly what `top_k(k)` costs in + `seek_count` — the fast path is the old code, and this assertion keeps it that way. +3. **The counted-skip assertion** — the caller-usable proof that the skip is counted rather than + linear. Platform's executors discard `CostContext`, so this must live grovedb-side, and it + can: every indexed API returns `CostResult`. On an N-row fixture, deep-offset `seek_count` + must stay within `offset-0 seek_count + small multiple of the AVL depth bound + (1.44·log2(N+2))`, and past-the-end must not exceed the offset-0 cost. **Red before this + change** (the linear walk pays ~N seeks), green after — the red run is recorded in the commit + message. The bound encodes *why* ("the offset costs at most one descent"), so it fails again + if anyone reintroduces per-entry skip work. +4. **Existing suites** — every proof suite passes with zero edits (§5). Exactly one existing + test changed, deliberately: the drift-suite case that asserted the linear skip decodes every + skipped row (`indexed_tree_secondary_drift_tests`, the `offset = 1` assertion). Under the + counted skip, a malformed-but-tree-resident row still occupies its position (it is counted) + but its key is never decoded, so the read serves the positionally identical page instead of + erroring; the assertion now pins that contract. This is §4's second bullet made concrete — + detection of skipped-region corruption belongs to `verify_grovedb` and to the shapes that + actually decode the row. + +## 8. Explicitly out of scope + +- Surfacing `skipped = min(offset, population)` to callers — the traversal computes it for free, + but returning it changes the public API shape; deferred to an owner decision. +- Snapshot isolation for unproved reads (pre-existing, storage-layer). +- A general-range merk-level walker and the large-`k` iterator-hybrid collect — neither is needed + by the only caller; both are recorded above should the need appear. diff --git a/grovedb/src/operations/indexed_tree.rs b/grovedb/src/operations/indexed_tree.rs index 1da2cc91c..099b5c9e3 100644 --- a/grovedb/src/operations/indexed_tree.rs +++ b/grovedb/src/operations/indexed_tree.rs @@ -41,6 +41,7 @@ use grovedb_merk::{ }, merk::KVIterator, proofs::Query, + tree::{kv::ValueDefinedCostType, AggregateData, Fetch, RefWalker}, Merk, TreeType, }; use grovedb_path::SubtreePath; @@ -1269,28 +1270,7 @@ impl GroveDb { self.open_validated_axis_secondary(path, axis, tx_ref, grove_version) ); - let mut all_query = Query::new(); - all_query.left_to_right = !descending; - all_query.insert_all(); - - let mut iter = KVIterator::new(secondary_merk.storage.raw_iter(), &all_query) - .unwrap_add_cost(&mut cost); - - let mut results = Vec::with_capacity(k as usize); - while results.len() < k as usize { - match iter.next_kv().unwrap_add_cost(&mut cost) { - Some((secondary_key, _)) => match decode(&secondary_key) { - Some(decoded) => results.push(decoded), - None => { - return Err(corrupted_secondary_key_error(axis, &secondary_key)) - .wrap_with_cost(cost); - } - }, - None => break, - } - } - - Ok(results).wrap_with_cost(cost) + collect_top_k_via_iterator(&secondary_merk, axis, k, descending, &decode).add_cost(cost) } /// One implementation of the `indexed__top_k_paginated` shape. @@ -1317,42 +1297,43 @@ impl GroveDb { self.open_validated_axis_secondary(path, axis, tx_ref, grove_version) ); - let mut all_query = Query::new(); - all_query.left_to_right = !descending; - all_query.insert_all(); - - let mut iter = KVIterator::new(secondary_merk.storage.raw_iter(), &all_query) - .unwrap_add_cost(&mut cost); - - // Skip `offset` entries; surface corruption defensively. - let mut skipped: u64 = 0; - while skipped < offset { - match iter.next_kv().unwrap_add_cost(&mut cost) { - Some((secondary_key, _)) => { - if decode(&secondary_key).is_none() { - return Err(corrupted_secondary_key_error(axis, &secondary_key)) - .wrap_with_cost(cost); - } - skipped += 1; - } - None => return Ok(Vec::new()).wrap_with_cost(cost), - } + // `offset == 0` is the overwhelmingly common shape, and the raw + // iterator is the cheapest way to serve it: one directional seek + // plus `k` sequential steps, no tree-path loads. It shares the + // `top_k` core's implementation, so "offset 0 costs exactly what + // plain top-k costs" is structural, not coincidental. + if offset == 0 { + return collect_top_k_via_iterator(&secondary_merk, axis, k, descending, &decode) + .add_cost(cost); } - let mut results = Vec::with_capacity(k as usize); - while results.len() < k as usize { - match iter.next_kv().unwrap_add_cost(&mut cost) { - Some((secondary_key, _)) => match decode(&secondary_key) { - Some(decoded) => results.push(decoded), - None => { - return Err(corrupted_secondary_key_error(axis, &secondary_key)) - .wrap_with_cost(cost); - } - }, - None => break, + // `offset > 0`: counted skip. Walk the secondary merk itself, + // consuming the offset through link aggregate counts — a subtree + // whose whole population fits inside the remaining offset is + // skipped without ever being fetched — then collect up to `k` + // keys in directional order. One root-to-position descent plus + // `k` node loads, instead of one storage-iterator step per + // skipped entry. + let secondary_keys = cost_return_on_error!( + &mut cost, + counted_skip_page( + &secondary_merk, + offset, + u64::from(k), + !descending, + grove_version + ) + ); + let mut results = Vec::with_capacity(secondary_keys.len()); + for secondary_key in secondary_keys { + match decode(&secondary_key) { + Some(decoded) => results.push(decoded), + None => { + return Err(corrupted_secondary_key_error(axis, &secondary_key)) + .wrap_with_cost(cost); + } } } - Ok(results).wrap_with_cost(cost) } @@ -1460,10 +1441,13 @@ impl GroveDb { /// entries in the directional scan before collecting up to `k` /// results. /// - /// `offset = 0` is equivalent to plain `indexed_count_top_k`. The - /// skip is performed at the secondary's storage iterator level — - /// this is not a verifiable / proof-bounded skip; for the provable - /// variant use + /// `offset = 0` is equivalent to plain `indexed_count_top_k` and is + /// served by the same storage-iterator scan. A positive `offset` is + /// skipped by counted descent over the secondary merk — subtrees are + /// consumed from their aggregate counts without loading their + /// entries, so the skip costs `O(log n)` node loads rather than + /// `O(offset)` iterator steps. Neither shape is a verifiable / + /// proof-bounded read; for the provable variant use /// [`Self::prove_indexed_count_top_k_paginated`] which relies on the /// merk-level count-offset proof to commit the skipped count via /// `HashWithCount`. @@ -1658,9 +1642,10 @@ impl GroveDb { /// Paginated form of [`Self::indexed_sum_top_k`]. Skips `offset` /// entries in the directional scan before collecting up to `k` - /// results. `offset = 0` is equivalent to plain - /// `indexed_sum_top_k`; the skip is iterator-level only (not - /// proof-bounded). + /// results. `offset = 0` is equivalent to plain `indexed_sum_top_k`; + /// a positive `offset` is skipped by counted descent over the + /// secondary merk in `O(log n)` node loads. Not a verifiable / + /// proof-bounded read. pub fn indexed_sum_top_k_paginated<'b, B, P>( &self, path: P, @@ -1845,7 +1830,10 @@ impl GroveDb { /// Paginated form of [`Self::indexed_avg_top_k`]. Skips `offset` /// entries in the directional scan before collecting up to `k` - /// results. + /// results. `offset = 0` is equivalent to plain `indexed_avg_top_k`; + /// a positive `offset` is skipped by counted descent over the + /// secondary merk in `O(log n)` node loads. Not a verifiable / + /// proof-bounded read. pub fn indexed_avg_top_k_paginated<'b, B, P>( &self, path: P, @@ -2414,6 +2402,408 @@ fn corrupted_secondary_key_error(axis: IndexAxis, secondary_key: &[u8]) -> Error )) } +/// Directional top-`k` collect over the secondary's storage iterator. +/// Shared by the `top_k` core and the paginated core's `offset == 0` fast +/// path, so "offset 0 costs exactly what plain top-k costs" is a +/// structural fact rather than two implementations kept in sync. +fn collect_top_k_via_iterator<'db, S: StorageContext<'db>, T>( + secondary_merk: &Merk, + axis: IndexAxis, + k: u16, + descending: bool, + decode: &impl Fn(&[u8]) -> Option<(T, Vec)>, +) -> CostResult)>, Error> { + let mut cost = OperationCost::default(); + + let mut all_query = Query::new(); + all_query.left_to_right = !descending; + all_query.insert_all(); + + let mut iter = + KVIterator::new(secondary_merk.storage.raw_iter(), &all_query).unwrap_add_cost(&mut cost); + + let mut results = Vec::with_capacity(k as usize); + while results.len() < k as usize { + match iter.next_kv().unwrap_add_cost(&mut cost) { + Some((secondary_key, _)) => match decode(&secondary_key) { + Some(decoded) => results.push(decoded), + None => { + return Err(corrupted_secondary_key_error(axis, &secondary_key)) + .wrap_with_cost(cost); + } + }, + None => break, + } + } + + Ok(results).wrap_with_cost(cost) +} + +/// Strict provable-count read of an aggregate. The counted skip only ever +/// runs against axis secondaries, whose tree types (`ProvableCountTree` / +/// `ProvableCountProvableSumTree`) bind a provable count into every node; +/// any other aggregate shape here is corruption, never a fallback. +/// Deliberately not `AggregateData::as_count_u64`, which returns 0 for +/// non-count variants — silently, which is exactly what this matcher +/// exists to prevent. Mirrors merk's `pub(super)` +/// `provable_count_from_aggregate` (unreachable from here). +#[inline] +fn provable_count_from_aggregate(aggregate: AggregateData) -> Result { + match aggregate { + AggregateData::ProvableCount(c) + | AggregateData::ProvableCountAndSum(c, _) + | AggregateData::ProvableCountAndProvableSum(c, _) => Ok(c), + other => Err(Error::CorruptedData(format!( + "indexed secondary node carries a non-provable-count aggregate: {:?}", + other + ))), + } +} + +/// Mutable state threaded through the counted descent. +struct CountedPageState { + /// In-range entries still to skip before returning starts. + offset_remaining: u64, + /// Page slots still to fill. The recursion is only ever entered while + /// this is non-zero and unwinds the moment it reaches zero. + limit_remaining: u64, + /// `true` = ascending (left child first), `false` = descending. + left_to_right: bool, +} + +/// Serve one page of secondary keys at `offset` in directional order by +/// counted descent over the secondary merk: subtrees whose whole +/// population fits inside the remaining offset are consumed from their +/// parent's link aggregate without being fetched, so the skip costs one +/// root-to-position path instead of one step per skipped entry. Returns +/// the raw secondary keys (`sort_key ‖ item_key`); values are never +/// needed — the caller decodes keys exactly as the iterator path does. +/// +/// The root node is already resident from the merk open, so an offset at +/// or past the whole population is answered from its aggregate alone, +/// with zero storage fetches. +fn counted_skip_page<'db, S: StorageContext<'db>>( + merk: &Merk, + offset: u64, + limit: u64, + left_to_right: bool, + grove_version: &GroveVersion, +) -> CostResult>, Error> { + merk.walk(|maybe_walker| { + let mut cost = OperationCost::default(); + let Some(mut walker) = maybe_walker else { + // Empty secondary: nothing to skip, nothing to return. + return Ok(Vec::new()).wrap_with_cost(cost); + }; + let population = cost_return_on_error_no_add!( + cost, + walker + .tree() + .aggregate_data() + .map_err(|e| Error::CorruptedData(format!("secondary aggregate_data: {e}"))) + .and_then(provable_count_from_aggregate) + ); + if limit == 0 || offset >= population { + return Ok(Vec::new()).wrap_with_cost(cost); + } + let mut state = CountedPageState { + offset_remaining: offset, + limit_remaining: limit, + left_to_right, + }; + let page_len = (population - offset).min(limit) as usize; + let mut out = Vec::with_capacity(page_len); + cost_return_on_error!( + &mut cost, + counted_skip_collect(&mut walker, &mut state, &mut out, 0, grove_version) + ); + Ok(out).wrap_with_cost(cost) + }) +} + +/// Recursion ceiling for the counted descent. An AVL tree cannot exceed +/// 1.44·64 ≈ 93 levels even at the u64 population limit, so anything +/// deeper means a corrupt link structure (e.g. a cyclic link) — fail with +/// an error instead of overflowing the stack. +const COUNTED_SKIP_MAX_DEPTH: u32 = 128; + +/// Read a child link's provable count. A present link points at a +/// non-empty subtree, whose count is therefore at least 1 — a zero is +/// corruption, not an empty side (that is `None`). +fn provable_count_from_link(link: Option<&grovedb_merk::tree::Link>) -> Result { + match link { + None => Ok(0), + Some(link) => match provable_count_from_aggregate(link.aggregate_data())? { + 0 => Err(Error::CorruptedData( + "secondary link is present but carries aggregate count 0".to_string(), + )), + count => Ok(count), + }, + } +} + +/// Recursive counted descent. Entered only on subtrees whose population +/// exceeds the remaining offset (the caller and both descend sites +/// guarantee it) and only while the page has room. +fn counted_skip_collect( + walker: &mut RefWalker, + state: &mut CountedPageState, + out: &mut Vec>, + depth: u32, + grove_version: &GroveVersion, +) -> CostResult<(), Error> +where + S: Fetch + Sized + Clone, +{ + let mut cost = OperationCost::default(); + + if depth > COUNTED_SKIP_MAX_DEPTH { + return Err(Error::CorruptedData(format!( + "secondary tree exceeds the maximum plausible depth {COUNTED_SKIP_MAX_DEPTH} — link \ + structure is corrupt" + ))) + .wrap_with_cost(cost); + } + + let node_count = cost_return_on_error_no_add!( + cost, + walker + .tree() + .aggregate_data() + .map_err(|e| Error::CorruptedData(format!("secondary aggregate_data: {e}"))) + .and_then(provable_count_from_aggregate) + ); + let left_count = + cost_return_on_error_no_add!(cost, provable_count_from_link(walker.tree().link(true))); + let right_count = + cost_return_on_error_no_add!(cost, provable_count_from_link(walker.tree().link(false))); + + // Every valid secondary row contributes structural count exactly 1 + // (`mirror_indexed_axis_to_secondary` writes nothing else, and + // `verify_indexed_axis_content` enforces it). This is a payload check + // on the node's own count value — a link whose cached count disagrees + // with its child's real subtree is caught separately, by the + // cross-check in `counted_skip_descend`. + let own_count = node_count + .checked_sub(left_count) + .and_then(|n| n.checked_sub(right_count)); + if own_count != Some(1) { + return Err(Error::CorruptedData(format!( + "indexed secondary node must have own structural count 1: aggregate {} with child \ + counts {} + {}", + node_count, left_count, right_count + ))) + .wrap_with_cost(cost); + } + + let (first_is_left, first_count, second_count) = if state.left_to_right { + (true, left_count, right_count) + } else { + (false, right_count, left_count) + }; + + // First child in directional order: consumed wholesale from the link + // aggregate (no fetch) when its entire population fits inside the + // remaining offset; descended into otherwise. + if first_count > 0 { + if first_count <= state.offset_remaining { + state.offset_remaining -= first_count; + } else { + cost_return_on_error!( + &mut cost, + counted_skip_descend( + walker, + first_is_left, + first_count, + state, + out, + depth, + grove_version + ) + ); + if state.limit_remaining == 0 { + return Ok(()).wrap_with_cost(cost); + } + } + } + + // The node itself: burn one unit of offset, or emit its key. The + // entry invariant (population > offset, page not full) makes the + // emit branch safe without re-checking `limit_remaining`. + if state.offset_remaining > 0 { + state.offset_remaining -= 1; + } else { + out.push(walker.tree().key().to_vec()); + state.limit_remaining -= 1; + if state.limit_remaining == 0 { + return Ok(()).wrap_with_cost(cost); + } + } + + // Second child. With consistent aggregates the offset can never + // swallow it whole (this frame was entered because its subtree + // outlasts the offset), but the count arithmetic keeps the skip + // branch as the safe symmetric action. + if second_count > 0 { + if second_count <= state.offset_remaining { + state.offset_remaining -= second_count; + } else { + cost_return_on_error!( + &mut cost, + counted_skip_descend( + walker, + !first_is_left, + second_count, + state, + out, + depth, + grove_version + ) + ); + } + } + + Ok(()).wrap_with_cost(cost) +} + +/// Fetch one child (upgrading its link) and recurse into it. +/// +/// `link_count` is the aggregate count read off the parent's link — the +/// number that authorized this descent (and that whole-subtree skips +/// trust without fetching). It is cross-checked against the loaded +/// child's own aggregate, so a link whose cached count disagrees with +/// its subtree fails loud instead of shifting every position after it. +fn counted_skip_descend( + walker: &mut RefWalker, + left: bool, + link_count: u64, + state: &mut CountedPageState, + out: &mut Vec>, + depth: u32, + grove_version: &GroveVersion, +) -> CostResult<(), Error> +where + S: Fetch + Sized + Clone, +{ + let mut cost = OperationCost::default(); + let walked = cost_return_on_error!( + &mut cost, + walker + .walk( + left, + None::<&fn(&[u8], &GroveVersion) -> Option>, + grove_version, + ) + .map_err(Into::into) + ); + let mut child = match walked { + Some(child) => child, + None => { + // The caller only descends where the link (and its non-zero + // count) was just read, so a missing child is corruption. + return Err(Error::CorruptedData( + "secondary link is present but its child failed to load".to_string(), + )) + .wrap_with_cost(cost); + } + }; + let child_count = cost_return_on_error_no_add!( + cost, + child + .tree() + .aggregate_data() + .map_err(|e| Error::CorruptedData(format!("secondary aggregate_data: {e}"))) + .and_then(provable_count_from_aggregate) + ); + if child_count != link_count { + return Err(Error::CorruptedData(format!( + "secondary link claims aggregate count {link_count} but its subtree carries \ + {child_count}" + ))) + .wrap_with_cost(cost); + } + counted_skip_collect(&mut child, state, out, depth + 1, grove_version).add_cost(cost) +} + +#[cfg(test)] +impl GroveDb { + /// The pre-counted-skip linear implementation, specialized to the + /// Count axis (the generic `decode`/`axis` parameters are the only + /// change from the replaced code). Kept as the measurement baseline + /// for `indexed_axis_paginated_cost_tests::measure_paginated_costs`; + /// compiled only for tests, never reachable in production builds. + pub(crate) fn legacy_linear_indexed_count_top_k_paginated<'b, B, P>( + &self, + path: P, + k: u16, + offset: u64, + descending: bool, + transaction: TransactionArg, + grove_version: &GroveVersion, + ) -> CostResult)>, Error> + where + B: AsRef<[u8]> + 'b, + P: Into>, + { + let mut cost = OperationCost::default(); + let tx = TxRef::new(&self.db, transaction); + let tx_ref = tx.as_ref(); + + let secondary_merk = cost_return_on_error!( + &mut cost, + self.open_validated_axis_secondary( + path.into(), + IndexAxis::Count, + tx_ref, + grove_version + ) + ); + + let mut all_query = Query::new(); + all_query.left_to_right = !descending; + all_query.insert_all(); + + let mut iter = KVIterator::new(secondary_merk.storage.raw_iter(), &all_query) + .unwrap_add_cost(&mut cost); + + let mut skipped: u64 = 0; + while skipped < offset { + match iter.next_kv().unwrap_add_cost(&mut cost) { + Some((secondary_key, _)) => { + if decode_secondary_key(&secondary_key).is_none() { + return Err(corrupted_secondary_key_error( + IndexAxis::Count, + &secondary_key, + )) + .wrap_with_cost(cost); + } + skipped += 1; + } + None => return Ok(Vec::new()).wrap_with_cost(cost), + } + } + + let mut results = Vec::with_capacity(k as usize); + while results.len() < k as usize { + match iter.next_kv().unwrap_add_cost(&mut cost) { + Some((secondary_key, _)) => match decode_secondary_key(&secondary_key) { + Some(decoded) => results.push(decoded), + None => { + return Err(corrupted_secondary_key_error( + IndexAxis::Count, + &secondary_key, + )) + .wrap_with_cost(cost); + } + }, + None => break, + } + } + + Ok(results).wrap_with_cost(cost) + } +} + #[cfg(test)] mod secondary_key_codec_tests { //! The secondary keyspace is `sort_key ‖ item_key`, and every per-axis diff --git a/grovedb/src/tests/indexed_axis_paginated_cost_tests.rs b/grovedb/src/tests/indexed_axis_paginated_cost_tests.rs new file mode 100644 index 000000000..683665c68 --- /dev/null +++ b/grovedb/src/tests/indexed_axis_paginated_cost_tests.rs @@ -0,0 +1,624 @@ +//! Cost and equality pins for the unproved paginated ranked reads. +//! +//! The offset skip in `indexed__top_k_paginated` must be *counted* — +//! one descent through the secondary merk, skipping whole subtrees via link +//! aggregate counts — rather than *linear* (one storage-iterator step per +//! skipped entry). Platform-side executors discard `CostContext`, so the +//! assertion that the skip is counted has to live here, where +//! `OperationCost` is visible: seek counts at a deep offset are bounded by +//! the tree depth, not by the offset. +//! +//! The equality tests pin that the counted path returns byte-identical +//! entries in identical order to the storage-iterator order, using the +//! unchanged `indexed__top_k` iterator path as the oracle. + +#[cfg(test)] +mod tests { + use grovedb_costs::CostContext; + use grovedb_version::version::GroveVersion; + + use crate::{ + batch::QualifiedGroveDbOp, + tests::{make_test_grovedb, TempGroveDb, TEST_LEAF}, + Element, + }; + + /// Rows in the large tie-heavy fixture. Big enough that a linear skip + /// (≈ one seek per skipped row) is orders of magnitude over the + /// depth-bounded budget the cost test allows. + const N: u64 = 600; + const K: u16 = 5; + + /// AVL depth bound for a tree of `n` keys: 1.44·log2(n + 2). + fn avl_depth_bound(n: u64) -> u32 { + (1.44 * ((n + 2) as f64).log2()).ceil() as u32 + } + + /// PCIT with `n` Item children inserted via chunked batches. Every + /// Item child derives count = 1, so the count axis is maximally + /// tie-heavy and the secondary order is exactly the item-key tiebreak. + fn make_pcit_with_rows(n: u64, grove_version: &GroveVersion) -> TempGroveDb { + let db = make_test_grovedb(grove_version); + db.apply_batch( + vec![QualifiedGroveDbOp::insert_or_replace_op( + vec![TEST_LEAF.to_vec()], + b"cidx".to_vec(), + Element::empty_provable_count_indexed_tree(), + )], + None, + None, + grove_version, + ) + .unwrap() + .expect("create PCIT"); + const CHUNK: u64 = 50_000; + let mut next = 0u64; + while next < n { + let end = (next + CHUNK).min(n); + let ops = (next..end) + .map(|i| { + QualifiedGroveDbOp::insert_or_replace_op( + vec![TEST_LEAF.to_vec(), b"cidx".to_vec()], + format!("k{i:07}").into_bytes(), + Element::new_item(vec![]), + ) + }) + .collect(); + db.apply_batch(ops, None, None, grove_version) + .unwrap() + .expect("populate PCIT batch chunk"); + next = end; + } + db + } + + fn make_pcit_with_n_tied_rows(grove_version: &GroveVersion) -> TempGroveDb { + make_pcit_with_rows(N, grove_version) + } + + /// PCIT with count-tree children whose derived counts contain both + /// distinct values and ties (3×3 and 2×9), so the axis-value ordering + /// and the item-key tiebreak are both exercised. + fn make_pcit_with_mixed_counts(grove_version: &GroveVersion) -> (TempGroveDb, usize) { + let dataset: &[(&[u8], u64)] = &[ + (b"a", 5), + (b"b", 12), + (b"c", 1), + (b"d", 7), + (b"e", 20), + (b"f", 3), + (b"g", 3), + (b"h", 3), + (b"i", 9), + (b"j", 9), + ]; + let db = make_test_grovedb(grove_version); + db.insert( + [TEST_LEAF].as_ref(), + b"cidx", + Element::empty_provable_count_indexed_tree(), + None, + None, + grove_version, + ) + .unwrap() + .expect("insert PCIT"); + for (key, count) in dataset { + db.insert_into_count_indexed_tree( + [TEST_LEAF, b"cidx"].as_ref(), + key, + Element::empty_count_tree(), + None, + grove_version, + ) + .unwrap() + .expect("insert count-tree child"); + for i in 0..*count { + db.insert( + [TEST_LEAF, b"cidx", key].as_ref(), + &i.to_be_bytes(), + Element::new_item(vec![]), + None, + None, + grove_version, + ) + .unwrap() + .expect("populate count-tree child"); + } + } + (db, dataset.len()) + } + + /// PCPSIT (all three axes) with count-sum-tree children whose derived + /// sums include a tie group (10 appears three times), so the sum and + /// avg axes exercise the item-key tiebreak too. + fn make_pcpsit_with_mixed_sums(grove_version: &GroveVersion) -> (TempGroveDb, usize) { + let dataset: &[(&[u8], u64, i64)] = &[ + (b"a", 2, 10), + (b"b", 4, 100), + (b"c", 5, -25), + (b"d", 1, 0), + (b"e", 3, 9), + (b"f", 1, 10), + (b"g", 2, 10), + ]; + let db = make_test_grovedb(grove_version); + db.insert( + [TEST_LEAF].as_ref(), + b"pcpsit", + Element::empty_provable_count_provable_sum_indexed_tree(vec![ + (0u8, None), + (1u8, None), + (2u8, None), + ]) + .expect("canonical axes"), + None, + None, + grove_version, + ) + .unwrap() + .expect("insert PCPSIT"); + for (key, count, sum) in dataset { + db.insert_into_provable_count_provable_sum_indexed_tree( + [TEST_LEAF, b"pcpsit"].as_ref(), + key, + Element::empty_count_sum_tree(), + None, + grove_version, + ) + .unwrap() + .expect("insert count-sum-tree child"); + for i in 0..*count { + db.insert( + [TEST_LEAF, b"pcpsit", key].as_ref(), + &i.to_be_bytes(), + Element::new_sum_item(if i == 0 { *sum } else { 0 }), + None, + None, + grove_version, + ) + .unwrap() + .expect("populate count-sum-tree child"); + } + } + (db, dataset.len()) + } + + // ----------------------------------------------------------------- + // The counted-skip assertion: offset cost is bounded by tree depth, + // never by the offset itself. + // ----------------------------------------------------------------- + + #[test] + fn paginated_offset_skip_is_counted_not_linear() { + let grove_version = GroveVersion::latest(); + let db = make_pcit_with_n_tied_rows(grove_version); + let path = [TEST_LEAF, b"cidx"]; + + for descending in [false, true] { + let CostContext { + value: page0, + cost: cost_0, + } = db.indexed_count_top_k_paginated( + path.as_ref(), + K, + 0, + descending, + None, + grove_version, + ); + let page0 = page0.expect("offset 0"); + assert_eq!(page0.len(), K as usize); + + let far_offset = N - K as u64; + let CostContext { + value: page_far, + cost: cost_far, + } = db.indexed_count_top_k_paginated( + path.as_ref(), + K, + far_offset, + descending, + None, + grove_version, + ); + let page_far = page_far.expect("deep offset"); + assert_eq!(page_far.len(), K as usize, "last full page must exist"); + + let depth_bound = avl_depth_bound(N); + + // A counted skip pays at most one root-to-position descent (plus + // the k-collect walk) for the offset — never per-skipped-entry + // work. A linear skip pays ≈ one seek per skipped row (~595 + // here), which blows this budget by an order of magnitude. + assert!( + cost_far.seek_count <= cost_0.seek_count + 3 * depth_bound, + "offset skip is walking entries (descending={descending}): seek_count {} at \ + offset {} vs {} at offset 0 (depth bound {})", + cost_far.seek_count, + far_offset, + cost_0.seek_count, + depth_bound, + ); + // ~300 B per fetched node; 512 B/level keeps 2x headroom while + // staying an order of magnitude under the skipped region's bytes. + assert!( + cost_far.storage_loaded_bytes + <= cost_0.storage_loaded_bytes + u64::from(depth_bound) * 512, + "offset skip is loading the skipped region (descending={descending}): {} bytes \ + at offset {} vs {} at offset 0", + cost_far.storage_loaded_bytes, + far_offset, + cost_0.storage_loaded_bytes, + ); + + // Past the end: the root aggregate (resident from the open) + // alone proves the offset exceeds the population — no descent at + // all. Offset 0 costs open + iterator seek + K collect steps, so + // "no descent" pins as: past-end + the collect work still fits + // inside the offset-0 budget. + let CostContext { + value: past, + cost: cost_past, + } = db.indexed_count_top_k_paginated( + path.as_ref(), + K, + N + 100, + descending, + None, + grove_version, + ); + assert!( + past.expect("past-end offset").is_empty(), + "past-end offset must return an empty page" + ); + assert!( + cost_past.seek_count + u32::from(K) <= cost_0.seek_count, + "past-end offset must not descend (descending={descending}): seek_count {} vs \ + {} at offset 0 (k = {K})", + cost_past.seek_count, + cost_0.seek_count, + ); + } + } + + /// An empty secondary has no root to descend into: any offset must + /// return an empty page without erroring, on both the iterator path + /// (offset 0) and the counted path (offset > 0). + #[test] + fn paginated_on_empty_secondary_returns_empty_at_any_offset() { + let grove_version = GroveVersion::latest(); + let db = make_pcit_with_rows(0, grove_version); + let path = [TEST_LEAF, b"cidx"]; + + for descending in [false, true] { + for offset in [0u64, 1, 1_000_000] { + let page = db + .indexed_count_top_k_paginated( + path.as_ref(), + 3, + offset, + descending, + None, + grove_version, + ) + .unwrap() + .expect("empty secondary must serve an empty page"); + assert!( + page.is_empty(), + "offset={offset} descending={descending} must be empty" + ); + } + } + } + + // ----------------------------------------------------------------- + // Offset-0 non-regression: the common shape stays on the iterator + // path, at exactly the plain top-k cost. + // ----------------------------------------------------------------- + + #[test] + fn paginated_offset_zero_costs_exactly_plain_top_k() { + let grove_version = GroveVersion::latest(); + let db = make_pcit_with_n_tied_rows(grove_version); + let path = [TEST_LEAF, b"cidx"]; + + for descending in [false, true] { + let CostContext { + value: plain, + cost: cost_plain, + } = db.indexed_count_top_k(path.as_ref(), K, descending, None, grove_version); + let CostContext { + value: paginated, + cost: cost_paginated, + } = db.indexed_count_top_k_paginated( + path.as_ref(), + K, + 0, + descending, + None, + grove_version, + ); + assert_eq!( + plain.expect("plain top-k"), + paginated.expect("paginated offset 0"), + "offset 0 must return exactly the top-k page (descending={descending})" + ); + assert_eq!( + cost_plain.seek_count, cost_paginated.seek_count, + "offset 0 must stay on the iterator path: seek_count diverged \ + (descending={descending})" + ); + assert_eq!( + cost_plain.storage_loaded_bytes, cost_paginated.storage_loaded_bytes, + "offset 0 must stay on the iterator path: loaded bytes diverged \ + (descending={descending})" + ); + } + } + + // ----------------------------------------------------------------- + // Equality grids: the counted path must return byte-identical pages + // in identical order to the iterator order. Oracle: the unchanged + // `indexed__top_k` iterator path, sliced. + // ----------------------------------------------------------------- + + #[test] + fn counted_paginated_matches_iterator_oracle_count_axis() { + let grove_version = GroveVersion::latest(); + let (db, population) = make_pcit_with_mixed_counts(grove_version); + let path = [TEST_LEAF, b"cidx"]; + + for descending in [false, true] { + let full = db + .indexed_count_top_k( + path.as_ref(), + population as u16, + descending, + None, + grove_version, + ) + .unwrap() + .expect("full ordered scan (oracle)"); + assert_eq!(full.len(), population); + + for offset in [0u64, 1, 2, 3, 4, 5, 8, 9, 10, 11, 15] { + for k in [0u16, 1, 3, population as u16] { + let page = db + .indexed_count_top_k_paginated( + path.as_ref(), + k, + offset, + descending, + None, + grove_version, + ) + .unwrap() + .expect("paginated page"); + let start = (offset as usize).min(population); + let end = (start + k as usize).min(population); + assert_eq!( + page, + full[start..end], + "count axis page mismatch at offset={offset} k={k} \ + descending={descending}" + ); + } + } + } + } + + #[test] + fn counted_paginated_matches_iterator_oracle_sum_and_avg_axes() { + let grove_version = GroveVersion::latest(); + let (db, population) = make_pcpsit_with_mixed_sums(grove_version); + let path = [TEST_LEAF, b"pcpsit"]; + + for descending in [false, true] { + let full_sum = db + .indexed_sum_top_k( + path.as_ref(), + population as u16, + descending, + None, + grove_version, + ) + .unwrap() + .expect("full sum scan (oracle)"); + let full_avg = db + .indexed_avg_top_k( + path.as_ref(), + population as u16, + descending, + None, + grove_version, + ) + .unwrap() + .expect("full avg scan (oracle)"); + assert_eq!(full_sum.len(), population); + assert_eq!(full_avg.len(), population); + + for offset in [0u64, 1, 2, 3, 5, 6, 7, 9] { + for k in [0u16, 1, 2, population as u16] { + let start = (offset as usize).min(population); + let end = (start + k as usize).min(population); + + let sum_page = db + .indexed_sum_top_k_paginated( + path.as_ref(), + k, + offset, + descending, + None, + grove_version, + ) + .unwrap() + .expect("sum page"); + assert_eq!( + sum_page, + full_sum[start..end], + "sum axis page mismatch at offset={offset} k={k} \ + descending={descending}" + ); + + let avg_page = db + .indexed_avg_top_k_paginated( + path.as_ref(), + k, + offset, + descending, + None, + grove_version, + ) + .unwrap() + .expect("avg page"); + assert_eq!( + avg_page, + full_avg[start..end], + "avg axis page mismatch at offset={offset} k={k} \ + descending={descending}" + ); + } + } + } + } + + #[test] + fn counted_paginated_matches_iterator_oracle_across_subtree_boundaries() { + let grove_version = GroveVersion::latest(); + let db = make_pcit_with_n_tied_rows(grove_version); + let path = [TEST_LEAF, b"cidx"]; + + // Offsets chosen to land inside, at, and across internal subtree + // boundaries of a ~600-key AVL tree, plus the exact-population and + // past-end edges. + for descending in [false, true] { + let full = db + .indexed_count_top_k(path.as_ref(), N as u16, descending, None, grove_version) + .unwrap() + .expect("full ordered scan (oracle)"); + assert_eq!(full.len(), N as usize); + + for offset in [1u64, 7, 63, 64, 65, 250, 511, N - 6, N - 1, N, N + 17] { + let page = db + .indexed_count_top_k_paginated( + path.as_ref(), + K, + offset, + descending, + None, + grove_version, + ) + .unwrap() + .expect("paginated page"); + let start = (offset as usize).min(N as usize); + let end = (start + K as usize).min(N as usize); + assert_eq!( + page, + full[start..end], + "page mismatch at offset={offset} descending={descending}" + ); + } + } + } + + // ----------------------------------------------------------------- + // Manual measurement harness — not run in CI. Run with: + // cargo test -p grovedb measure_paginated_costs -- --ignored --nocapture + // ----------------------------------------------------------------- + + /// Measures the counted paginated read against the pre-change linear + /// loop (kept verbatim as + /// `GroveDb::legacy_linear_indexed_count_top_k_paginated`) on the + /// N-group Count-axis secondary shape, and prints `OperationCost` + /// counters plus wall-clock. The counters are the trustworthy, + /// machine-independent signal; wall-clock is reported because it is + /// what a node operator feels, but it is noisy on a loaded machine — + /// check the load average before quoting it. + #[test] + #[ignore] + fn measure_paginated_costs() { + use std::time::Instant; + + let grove_version = GroveVersion::latest(); + println!(); + println!( + "wall-clock is min of 3 runs on a shared machine — treat seeks/bytes as the signal" + ); + println!("| n | k | offset | path | seeks | loaded bytes | rows | wall µs |"); + println!("|---|---|---|---|---|---|---|---|"); + + for n in [1_000u64, 10_000, 100_000, 1_000_000] { + let db = make_pcit_with_rows(n, grove_version); + let path = [TEST_LEAF, b"cidx"]; + for k in [1u16, 100] { + // `offset = 1` is the worst case for the counted collect + // phase: the whole page is gathered through tree-node + // point-gets instead of sequential iterator steps. + for offset in [0u64, 1, n - 1, n, 4_000_000_000] { + let mut counted_wall = u128::MAX; + let mut counted = None; + for _ in 0..3 { + let started = Instant::now(); + let run = db.indexed_count_top_k_paginated( + path.as_ref(), + k, + offset, + false, + None, + grove_version, + ); + counted_wall = counted_wall.min(started.elapsed().as_micros()); + counted = Some(run); + } + let CostContext { + value: counted_rows, + cost: counted_cost, + } = counted.expect("three runs happened"); + let counted_rows = counted_rows.expect("counted read"); + + let mut linear_wall = u128::MAX; + let mut linear = None; + for _ in 0..3 { + let started = Instant::now(); + let run = db.legacy_linear_indexed_count_top_k_paginated( + path.as_ref(), + k, + offset, + false, + None, + grove_version, + ); + linear_wall = linear_wall.min(started.elapsed().as_micros()); + linear = Some(run); + } + let CostContext { + value: linear_rows, + cost: linear_cost, + } = linear.expect("three runs happened"); + let linear_rows = linear_rows.expect("legacy linear read"); + + assert_eq!( + counted_rows, linear_rows, + "counted and linear paths diverged at n={n} k={k} offset={offset}" + ); + + println!( + "| {n} | {k} | {offset} | counted | {} | {} | {} | {} |", + counted_cost.seek_count, + counted_cost.storage_loaded_bytes, + counted_rows.len(), + counted_wall, + ); + println!( + "| {n} | {k} | {offset} | linear | {} | {} | {} | {} |", + linear_cost.seek_count, + linear_cost.storage_loaded_bytes, + linear_rows.len(), + linear_wall, + ); + } + } + } + } +} diff --git a/grovedb/src/tests/indexed_tree_secondary_drift_tests.rs b/grovedb/src/tests/indexed_tree_secondary_drift_tests.rs index eb06a33fc..790a2a729 100644 --- a/grovedb/src/tests/indexed_tree_secondary_drift_tests.rs +++ b/grovedb/src/tests/indexed_tree_secondary_drift_tests.rs @@ -795,16 +795,19 @@ mod tests { } /// A secondary row whose key is shorter than the axis's 8-byte sort prefix - /// carries no recoverable `(count, original_key)` pair. Every direct count - /// query must raise `CorruptedData` naming the axis and the expected width - /// — dropping the row instead would let a short-key row silently shrink a - /// top-k page or a range listing while the caller sees a successful result. + /// carries no recoverable `(count, original_key)` pair. Every place that + /// decodes a row must raise `CorruptedData` naming the axis and the + /// expected width — dropping the row instead would let a short-key row + /// silently shrink a top-k page or a range listing while the caller sees a + /// successful result. /// - /// Each of the three query shapes decodes in a different place — the plain - /// walk, the pagination *skip* loop, the pagination *collect* loop, and the - /// range walk — so all four are driven here. + /// The decode sites driven here: the plain top-k walk, the paginated + /// collect at `offset == 0`, and the range walk. A paginated read with + /// `offset > 0` skips by counted descent and decodes only *returned* rows, + /// so its skip phase deliberately does not error — that contract is + /// asserted here too. #[test] - fn a_short_secondary_key_is_reported_by_every_count_query_shape() { + fn a_short_secondary_key_is_reported_by_every_shape_that_decodes_it() { let gv = GroveVersion::latest(); let db = make_test_grovedb(gv); make_pcit_with(&db, b"pcit", &[b"a", b"b"], gv); @@ -833,11 +836,22 @@ mod tests { .expect_err("paginated collect must surface the malformed row"), "paginated (offset 0, collect loop)", ); - assert_short_key_corruption( + // A paginated read with `offset > 0` skips by counted descent over + // the secondary merk: skipped rows are consumed from aggregate + // counts and their keys are never decoded. The malformed row is a + // real tree row here, so it still occupies its position (descending + // order is [0xff…, b, a] and offset 1 lands on b — the same + // position the storage iterator would assign), but only *returned* + // rows are decoded, so the skip no longer surfaces it as an error. + // Detecting malformed rows in the skipped region is + // `verify_grovedb`'s job (asserted above) and the collect loops' + // (asserted below for every shape that reads the row itself). + assert_eq!( db.indexed_count_top_k_paginated([TEST_LEAF, b"pcit"].as_ref(), 1, 1, true, None, gv) .unwrap() - .expect_err("paginated skip must surface the malformed row"), - "paginated (offset 1, skip loop)", + .expect("counted skip passes the malformed row without decoding it"), + vec![(1u64, b"b".to_vec())], + "descending order is [0xff…, b, a]; the malformed row is counted at offset 0", ); assert_short_key_corruption( db.indexed_count_range( diff --git a/grovedb/src/tests/mod.rs b/grovedb/src/tests/mod.rs index 7f130cb0a..71f015ed4 100644 --- a/grovedb/src/tests/mod.rs +++ b/grovedb/src/tests/mod.rs @@ -56,6 +56,7 @@ mod get_cost_estimator_tests; mod grove_query_result_tests; mod indexed_axis_nested_and_bounds_tests; mod indexed_axis_offset_proof_tests; +mod indexed_axis_paginated_cost_tests; mod indexed_axis_proof_tests; mod indexed_tree_secondary_drift_tests; mod indexed_tree_security_regression_tests; From e41d57e08b36225a52c8a6730a5cc9a824876eee Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Thu, 13 Aug 2026 20:56:29 +0700 Subject: [PATCH 03/11] feat: report the true skipped count from unproved paginated ranked reads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three indexed__top_k_paginated APIs now return IndexedTopKPage { entries, skipped } instead of a bare Vec, where skipped = min(offset, population) — read from the secondary's root aggregate at zero extra cost. The old linear read structurally could not report this (an offset past the end just exhausted the iterator and the caller could only echo the request); the proved path already attests exactly this quantity through its count commitments (its verifier derives skipped = offset - offset_remaining over the same provable-count aggregates), so unproved and proved reads now agree on it. Like the entries, the unproved value is the local tree's claim, not independently verifiable. offset = 0 reports skipped = 0 without touching the tree, keeping the fast path fast; empty secondaries report 0; k = 0 and past-end offsets report min(offset, population). Pinned across the equality grids, the cost tests, the empty-secondary test, and the measurement harness (skipped == min(offset, n) asserted at every n/k/offset point, including offset = 4e9 over 1e6 rows). Callers updated mechanically (.entries); the only consumer of the old shape was the test suite. --- docs/COUNTED_SKIP_DESIGN.md | 8 +- grovedb/src/lib.rs | 2 + grovedb/src/operations/indexed_tree.rs | 75 +++++++++++++------ .../indexed_axis_paginated_cost_tests.rs | 58 +++++++++++--- .../indexed_tree_secondary_drift_tests.rs | 3 +- .../provable_count_indexed_tree_tests.rs | 10 +-- ...e_count_provable_sum_indexed_tree_tests.rs | 6 +- .../tests/provable_sum_indexed_tree_tests.rs | 8 +- 8 files changed, 119 insertions(+), 51 deletions(-) diff --git a/docs/COUNTED_SKIP_DESIGN.md b/docs/COUNTED_SKIP_DESIGN.md index 4798afbab..f9f786584 100644 --- a/docs/COUNTED_SKIP_DESIGN.md +++ b/docs/COUNTED_SKIP_DESIGN.md @@ -86,6 +86,12 @@ shape-rejection rules are all unnecessary here. rest of the tree. - The page holds secondary **keys** only; the caller decodes them exactly as before (the old loop also discarded values). Cost is `O(depth + k)` node fetches for `offset > 0`. +- The three paginated APIs return `IndexedTopKPage { entries, skipped }`, where `skipped` is the + **true** skipped count `min(offset, population)` — read from the root aggregate at zero extra + cost. The old linear read structurally could not report this (an offset past the end just + exhausted the iterator); the proved path attests the same quantity through its count + commitments, so unproved and proved reads now agree on it. Like the entries, the unproved + value is the local tree's claim, not independently verifiable. ### Where the two research passes differed (recorded per the consolidation request) @@ -216,8 +222,6 @@ What the numbers establish: ## 8. Explicitly out of scope -- Surfacing `skipped = min(offset, population)` to callers — the traversal computes it for free, - but returning it changes the public API shape; deferred to an owner decision. - Snapshot isolation for unproved reads (pre-existing, storage-layer). - A general-range merk-level walker and the large-`k` iterator-hybrid collect — neither is needed by the only caller; both are recorded above should the need appear. diff --git a/grovedb/src/lib.rs b/grovedb/src/lib.rs index 79e02e394..f541016a8 100644 --- a/grovedb/src/lib.rs +++ b/grovedb/src/lib.rs @@ -242,6 +242,8 @@ use grovedb_version::version::GroveVersion; #[cfg(feature = "minimal")] use grovedb_visualize::DebugByteVectors; #[cfg(any(feature = "minimal", feature = "verify"))] +pub use operations::indexed_tree::IndexedTopKPage; +#[cfg(any(feature = "minimal", feature = "verify"))] pub use query::{ aggregate_sum_path_query::AggregateSumPathQuery, GroveBranchQueryResult, GroveTrunkQueryResult, LeafInfo, PathBranchChunkQuery, PathQuery, PathTrunkChunkQuery, SizedQuery, diff --git a/grovedb/src/operations/indexed_tree.rs b/grovedb/src/operations/indexed_tree.rs index 099b5c9e3..05a55d9a6 100644 --- a/grovedb/src/operations/indexed_tree.rs +++ b/grovedb/src/operations/indexed_tree.rs @@ -1284,7 +1284,7 @@ impl GroveDb { transaction: TransactionArg, grove_version: &GroveVersion, decode: impl Fn(&[u8]) -> Option<(T, Vec)>, - ) -> CostResult)>, Error> + ) -> CostResult, Error> where B: AsRef<[u8]> + 'b, { @@ -1301,9 +1301,15 @@ impl GroveDb { // iterator is the cheapest way to serve it: one directional seek // plus `k` sequential steps, no tree-path loads. It shares the // `top_k` core's implementation, so "offset 0 costs exactly what - // plain top-k costs" is structural, not coincidental. + // plain top-k costs" is structural, not coincidental. Zero offset + // skips zero entries, so `skipped = min(0, population) = 0` needs + // no tree read. if offset == 0 { return collect_top_k_via_iterator(&secondary_merk, axis, k, descending, &decode) + .map_ok(|entries| IndexedTopKPage { + entries, + skipped: 0, + }) .add_cost(cost); } @@ -1314,7 +1320,7 @@ impl GroveDb { // keys in directional order. One root-to-position descent plus // `k` node loads, instead of one storage-iterator step per // skipped entry. - let secondary_keys = cost_return_on_error!( + let (secondary_keys, skipped) = cost_return_on_error!( &mut cost, counted_skip_page( &secondary_merk, @@ -1324,17 +1330,17 @@ impl GroveDb { grove_version ) ); - let mut results = Vec::with_capacity(secondary_keys.len()); + let mut entries = Vec::with_capacity(secondary_keys.len()); for secondary_key in secondary_keys { match decode(&secondary_key) { - Some(decoded) => results.push(decoded), + Some(decoded) => entries.push(decoded), None => { return Err(corrupted_secondary_key_error(axis, &secondary_key)) .wrap_with_cost(cost); } } } - Ok(results).wrap_with_cost(cost) + Ok(IndexedTopKPage { entries, skipped }).wrap_with_cost(cost) } /// One implementation of the `indexed__range` shape. The @@ -1446,11 +1452,14 @@ impl GroveDb { /// skipped by counted descent over the secondary merk — subtrees are /// consumed from their aggregate counts without loading their /// entries, so the skip costs `O(log n)` node loads rather than - /// `O(offset)` iterator steps. Neither shape is a verifiable / - /// proof-bounded read; for the provable variant use - /// [`Self::prove_indexed_count_top_k_paginated`] which relies on the - /// merk-level count-offset proof to commit the skipped count via - /// `HashWithCount`. + /// `O(offset)` iterator steps. The returned + /// [`IndexedTopKPage::skipped`] is the true skipped count, + /// `min(offset, population)` — an offset past the end reports the + /// secondary's population rather than echoing the request. Neither + /// shape is a verifiable / proof-bounded read; for the provable + /// variant use [`Self::prove_indexed_count_top_k_paginated`] which + /// relies on the merk-level count-offset proof to commit the skipped + /// count via `HashWithCount`. pub fn indexed_count_top_k_paginated<'b, B, P>( &self, path: P, @@ -1459,7 +1468,7 @@ impl GroveDb { descending: bool, transaction: TransactionArg, grove_version: &GroveVersion, - ) -> CostResult)>, Error> + ) -> CostResult, Error> where B: AsRef<[u8]> + 'b, P: Into>, @@ -1644,8 +1653,9 @@ impl GroveDb { /// entries in the directional scan before collecting up to `k` /// results. `offset = 0` is equivalent to plain `indexed_sum_top_k`; /// a positive `offset` is skipped by counted descent over the - /// secondary merk in `O(log n)` node loads. Not a verifiable / - /// proof-bounded read. + /// secondary merk in `O(log n)` node loads, and + /// [`IndexedTopKPage::skipped`] reports the true + /// `min(offset, population)`. Not a verifiable / proof-bounded read. pub fn indexed_sum_top_k_paginated<'b, B, P>( &self, path: P, @@ -1654,7 +1664,7 @@ impl GroveDb { descending: bool, transaction: TransactionArg, grove_version: &GroveVersion, - ) -> CostResult)>, Error> + ) -> CostResult, Error> where B: AsRef<[u8]> + 'b, P: Into>, @@ -1832,8 +1842,9 @@ impl GroveDb { /// entries in the directional scan before collecting up to `k` /// results. `offset = 0` is equivalent to plain `indexed_avg_top_k`; /// a positive `offset` is skipped by counted descent over the - /// secondary merk in `O(log n)` node loads. Not a verifiable / - /// proof-bounded read. + /// secondary merk in `O(log n)` node loads, and + /// [`IndexedTopKPage::skipped`] reports the true + /// `min(offset, population)`. Not a verifiable / proof-bounded read. pub fn indexed_avg_top_k_paginated<'b, B, P>( &self, path: P, @@ -1842,7 +1853,7 @@ impl GroveDb { descending: bool, transaction: TransactionArg, grove_version: &GroveVersion, - ) -> CostResult)>, Error> + ) -> CostResult, Error> where B: AsRef<[u8]> + 'b, P: Into>, @@ -2460,6 +2471,20 @@ fn provable_count_from_aggregate(aggregate: AggregateData) -> Result } } +/// One page of an `indexed__top_k_paginated` read. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct IndexedTopKPage { + /// Page entries, `(axis_value, original_key)`, in directional order. + pub entries: Vec<(T, Vec)>, + /// How many entries the offset actually skipped: + /// `min(offset, population)`. When the offset runs past the end this + /// reports the secondary's true population instead of echoing the + /// request — the same quantity the proved path attests through its + /// count commitments, though here it is the local tree's unverified + /// claim, like the entries themselves. + pub skipped: u64, +} + /// Mutable state threaded through the counted descent. struct CountedPageState { /// In-range entries still to skip before returning starts. @@ -2476,8 +2501,9 @@ struct CountedPageState { /// population fits inside the remaining offset are consumed from their /// parent's link aggregate without being fetched, so the skip costs one /// root-to-position path instead of one step per skipped entry. Returns -/// the raw secondary keys (`sort_key ‖ item_key`); values are never -/// needed — the caller decodes keys exactly as the iterator path does. +/// the raw secondary keys (`sort_key ‖ item_key`) plus the true skipped +/// count, `min(offset, population)`; values are never needed — the +/// caller decodes keys exactly as the iterator path does. /// /// The root node is already resident from the merk open, so an offset at /// or past the whole population is answered from its aggregate alone, @@ -2488,12 +2514,12 @@ fn counted_skip_page<'db, S: StorageContext<'db>>( limit: u64, left_to_right: bool, grove_version: &GroveVersion, -) -> CostResult>, Error> { +) -> CostResult<(Vec>, u64), Error> { merk.walk(|maybe_walker| { let mut cost = OperationCost::default(); let Some(mut walker) = maybe_walker else { // Empty secondary: nothing to skip, nothing to return. - return Ok(Vec::new()).wrap_with_cost(cost); + return Ok((Vec::new(), 0)).wrap_with_cost(cost); }; let population = cost_return_on_error_no_add!( cost, @@ -2503,8 +2529,9 @@ fn counted_skip_page<'db, S: StorageContext<'db>>( .map_err(|e| Error::CorruptedData(format!("secondary aggregate_data: {e}"))) .and_then(provable_count_from_aggregate) ); + let skipped = offset.min(population); if limit == 0 || offset >= population { - return Ok(Vec::new()).wrap_with_cost(cost); + return Ok((Vec::new(), skipped)).wrap_with_cost(cost); } let mut state = CountedPageState { offset_remaining: offset, @@ -2517,7 +2544,7 @@ fn counted_skip_page<'db, S: StorageContext<'db>>( &mut cost, counted_skip_collect(&mut walker, &mut state, &mut out, 0, grove_version) ); - Ok(out).wrap_with_cost(cost) + Ok((out, skipped)).wrap_with_cost(cost) }) } diff --git a/grovedb/src/tests/indexed_axis_paginated_cost_tests.rs b/grovedb/src/tests/indexed_axis_paginated_cost_tests.rs index 683665c68..3ce7d326e 100644 --- a/grovedb/src/tests/indexed_axis_paginated_cost_tests.rs +++ b/grovedb/src/tests/indexed_axis_paginated_cost_tests.rs @@ -208,7 +208,8 @@ mod tests { grove_version, ); let page0 = page0.expect("offset 0"); - assert_eq!(page0.len(), K as usize); + assert_eq!(page0.entries.len(), K as usize); + assert_eq!(page0.skipped, 0, "offset 0 skips nothing"); let far_offset = N - K as u64; let CostContext { @@ -223,7 +224,15 @@ mod tests { grove_version, ); let page_far = page_far.expect("deep offset"); - assert_eq!(page_far.len(), K as usize, "last full page must exist"); + assert_eq!( + page_far.entries.len(), + K as usize, + "last full page must exist" + ); + assert_eq!( + page_far.skipped, far_offset, + "true skipped at a deep offset" + ); let depth_bound = avl_depth_bound(N); @@ -268,10 +277,15 @@ mod tests { None, grove_version, ); + let past = past.expect("past-end offset"); assert!( - past.expect("past-end offset").is_empty(), + past.entries.is_empty(), "past-end offset must return an empty page" ); + assert_eq!( + past.skipped, N, + "past-end must report the true population, not echo the request" + ); assert!( cost_past.seek_count + u32::from(K) <= cost_0.seek_count, "past-end offset must not descend (descending={descending}): seek_count {} vs \ @@ -305,9 +319,10 @@ mod tests { .unwrap() .expect("empty secondary must serve an empty page"); assert!( - page.is_empty(), + page.entries.is_empty(), "offset={offset} descending={descending} must be empty" ); + assert_eq!(page.skipped, 0, "an empty secondary has nothing to skip"); } } } @@ -339,11 +354,13 @@ mod tests { None, grove_version, ); + let paginated = paginated.expect("paginated offset 0"); assert_eq!( plain.expect("plain top-k"), - paginated.expect("paginated offset 0"), + paginated.entries, "offset 0 must return exactly the top-k page (descending={descending})" ); + assert_eq!(paginated.skipped, 0); assert_eq!( cost_plain.seek_count, cost_paginated.seek_count, "offset 0 must stay on the iterator path: seek_count diverged \ @@ -398,11 +415,16 @@ mod tests { let start = (offset as usize).min(population); let end = (start + k as usize).min(population); assert_eq!( - page, + page.entries, full[start..end], "count axis page mismatch at offset={offset} k={k} \ descending={descending}" ); + assert_eq!( + page.skipped, + offset.min(population as u64), + "true skipped at offset={offset} k={k}" + ); } } } @@ -455,11 +477,12 @@ mod tests { .unwrap() .expect("sum page"); assert_eq!( - sum_page, + sum_page.entries, full_sum[start..end], "sum axis page mismatch at offset={offset} k={k} \ descending={descending}" ); + assert_eq!(sum_page.skipped, (offset).min(population as u64)); let avg_page = db .indexed_avg_top_k_paginated( @@ -473,11 +496,12 @@ mod tests { .unwrap() .expect("avg page"); assert_eq!( - avg_page, + avg_page.entries, full_avg[start..end], "avg axis page mismatch at offset={offset} k={k} \ descending={descending}" ); + assert_eq!(avg_page.skipped, (offset).min(population as u64)); } } } @@ -514,10 +538,15 @@ mod tests { let start = (offset as usize).min(N as usize); let end = (start + K as usize).min(N as usize); assert_eq!( - page, + page.entries, full[start..end], "page mismatch at offset={offset} descending={descending}" ); + assert_eq!( + page.skipped, + offset.min(N), + "true skipped at offset={offset}" + ); } } } @@ -575,7 +604,7 @@ mod tests { value: counted_rows, cost: counted_cost, } = counted.expect("three runs happened"); - let counted_rows = counted_rows.expect("counted read"); + let counted_page = counted_rows.expect("counted read"); let mut linear_wall = u128::MAX; let mut linear = None; @@ -599,15 +628,20 @@ mod tests { let linear_rows = linear_rows.expect("legacy linear read"); assert_eq!( - counted_rows, linear_rows, + counted_page.entries, linear_rows, "counted and linear paths diverged at n={n} k={k} offset={offset}" ); + assert_eq!( + counted_page.skipped, + offset.min(n), + "true skipped at n={n} k={k} offset={offset}" + ); println!( "| {n} | {k} | {offset} | counted | {} | {} | {} | {} |", counted_cost.seek_count, counted_cost.storage_loaded_bytes, - counted_rows.len(), + counted_page.entries.len(), counted_wall, ); println!( diff --git a/grovedb/src/tests/indexed_tree_secondary_drift_tests.rs b/grovedb/src/tests/indexed_tree_secondary_drift_tests.rs index 790a2a729..70b7af334 100644 --- a/grovedb/src/tests/indexed_tree_secondary_drift_tests.rs +++ b/grovedb/src/tests/indexed_tree_secondary_drift_tests.rs @@ -849,7 +849,8 @@ mod tests { assert_eq!( db.indexed_count_top_k_paginated([TEST_LEAF, b"pcit"].as_ref(), 1, 1, true, None, gv) .unwrap() - .expect("counted skip passes the malformed row without decoding it"), + .expect("counted skip passes the malformed row without decoding it") + .entries, vec![(1u64, b"b".to_vec())], "descending order is [0xff…, b, a]; the malformed row is counted at offset 0", ); diff --git a/grovedb/src/tests/provable_count_indexed_tree_tests.rs b/grovedb/src/tests/provable_count_indexed_tree_tests.rs index 35e7cab92..7c180af38 100644 --- a/grovedb/src/tests/provable_count_indexed_tree_tests.rs +++ b/grovedb/src/tests/provable_count_indexed_tree_tests.rs @@ -1040,7 +1040,7 @@ mod tests { .unwrap() .expect("page 1"); assert_eq!( - page1, + page1.entries, vec![(20u64, b"eve".to_vec()), (12u64, b"bob".to_vec())] ); @@ -1057,7 +1057,7 @@ mod tests { .unwrap() .expect("page 2"); assert_eq!( - page2, + page2.entries, vec![(7u64, b"dave".to_vec()), (5u64, b"alice".to_vec())] ); @@ -1073,7 +1073,7 @@ mod tests { ) .unwrap() .expect("page 3"); - assert_eq!(page3, vec![(1u64, b"carol".to_vec())]); + assert_eq!(page3.entries, vec![(1u64, b"carol".to_vec())]); // Offset beyond total → empty. let beyond = db @@ -1087,7 +1087,7 @@ mod tests { ) .unwrap() .expect("offset beyond"); - assert!(beyond.is_empty()); + assert!(beyond.entries.is_empty()); // offset=0 ≡ plain top_k. let plain = db @@ -1105,7 +1105,7 @@ mod tests { ) .unwrap() .expect("paginated offset 0"); - assert_eq!(plain, paginated); + assert_eq!(plain, paginated.entries); } #[test] diff --git a/grovedb/src/tests/provable_count_provable_sum_indexed_tree_tests.rs b/grovedb/src/tests/provable_count_provable_sum_indexed_tree_tests.rs index 5cd76a458..88acc35a5 100644 --- a/grovedb/src/tests/provable_count_provable_sum_indexed_tree_tests.rs +++ b/grovedb/src/tests/provable_count_provable_sum_indexed_tree_tests.rs @@ -1395,7 +1395,7 @@ mod tests { .unwrap() .expect("page 1"); assert_eq!( - page1, + page1.entries, vec![ (25 * AVG_SCALE, b"bob".to_vec()), (5 * AVG_SCALE, b"alice".to_vec()), @@ -1415,7 +1415,7 @@ mod tests { .unwrap() .expect("page 2"); assert_eq!( - page2, + page2.entries, vec![(3 * AVG_SCALE, b"eve".to_vec()), (0, b"dave".to_vec())] ); @@ -1431,7 +1431,7 @@ mod tests { ) .unwrap() .expect("beyond"); - assert!(beyond.is_empty()); + assert!(beyond.entries.is_empty()); } #[test] diff --git a/grovedb/src/tests/provable_sum_indexed_tree_tests.rs b/grovedb/src/tests/provable_sum_indexed_tree_tests.rs index b168ea635..6ff0ee83e 100644 --- a/grovedb/src/tests/provable_sum_indexed_tree_tests.rs +++ b/grovedb/src/tests/provable_sum_indexed_tree_tests.rs @@ -918,7 +918,7 @@ mod tests { .unwrap() .expect("page 1"); assert_eq!( - page1, + page1.entries, vec![(100i64, b"frank".to_vec()), (12, b"bob".to_vec())] ); @@ -934,7 +934,7 @@ mod tests { .unwrap() .expect("page 2"); assert_eq!( - page2, + page2.entries, vec![(5i64, b"alice".to_vec()), (0, b"dave".to_vec())] ); @@ -950,7 +950,7 @@ mod tests { ) .unwrap() .expect("beyond"); - assert!(beyond.is_empty()); + assert!(beyond.entries.is_empty()); // offset=0 ≡ plain top_k. let plain = db @@ -968,7 +968,7 @@ mod tests { ) .unwrap() .expect("pag offset 0"); - assert_eq!(plain, pag); + assert_eq!(plain, pag.entries); } #[test] From c37729e621f6b6b978fa037026981f122802ec8e Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Fri, 14 Aug 2026 00:24:37 +0700 Subject: [PATCH 04/11] fix: gate the IndexedTopKPage re-export on minimal, not minimal-or-verify MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The re-export at lib.rs was gated on any(minimal, verify) while the module it names, operations::indexed_tree, is gated on minimal alone, so a verify-without-minimal build failed to compile: error[E0432]: unresolved import `operations::indexed_tree` note: found an item that was configured out That cut is drive's verifier-only build in Platform (cargo check -p drive --no-default-features --features verify), which is how it surfaced. Narrow the export to match the module rather than widening the module to match the export: the only APIs that produce an IndexedTopKPage are the three paginated indexed-axis reads, which need storage and are therefore minimal-only. A verify build consumes proofs and can never name the type. Red before / green after with the feature cut that reproduces it: cargo check -p grovedb --no-default-features --features verify failed with the E0432 above and now compiles. No permanent test is added because the guard already exists and is not a unit test — .github/ workflows/grovedb.yml runs 'cargo build --no-default-features --features verify -p grovedb' for exactly this. It did not catch this commit because the branch has never been pushed, so CI has never run on it. Co-Authored-By: Claude Opus 5 (1M context) --- grovedb/src/lib.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/grovedb/src/lib.rs b/grovedb/src/lib.rs index f541016a8..42856c8e2 100644 --- a/grovedb/src/lib.rs +++ b/grovedb/src/lib.rs @@ -241,7 +241,12 @@ use grovedb_storage::{Storage, StorageContext}; use grovedb_version::version::GroveVersion; #[cfg(feature = "minimal")] use grovedb_visualize::DebugByteVectors; -#[cfg(any(feature = "minimal", feature = "verify"))] +// Gated on `minimal` alone, matching `operations::indexed_tree` itself: the +// only APIs that produce an `IndexedTopKPage` are the paginated indexed-axis +// reads, which need storage. A verify-only build consumes proofs and can +// never name this type, so widening the gate here to `any(minimal, verify)` +// only breaks that cut on an unresolved import. +#[cfg(feature = "minimal")] pub use operations::indexed_tree::IndexedTopKPage; #[cfg(any(feature = "minimal", feature = "verify"))] pub use query::{ From cc7b3997be5819c60eb09251f963c1ed1a89306f Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Fri, 14 Aug 2026 00:24:46 +0700 Subject: [PATCH 05/11] test: pin that the unproved skipped equals the proved path's attested skipped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Platform exposes RankedPage::skipped on the wire from both the proved and unproved paths, and a client cannot tell which one served it, so the two must report the same quantity for the same request. Nothing structural held them in step: the proved side re-derives skipped from the counted subtree commitments in the proof bytes, the unproved side reads the secondary's root aggregate. Assert across offsets {0, 1, 5, pop-1, pop, pop+1, 4e9} x k {0, 1, 3} x both directions that the two skipped values match, that both equal min(offset, population) — equality alone would be satisfied by two identically wrong values — and that the entries match too. Co-Authored-By: Claude Opus 5 (1M context) --- .../indexed_axis_paginated_cost_tests.rs | 88 ++++++++++++++++++- 1 file changed, 87 insertions(+), 1 deletion(-) diff --git a/grovedb/src/tests/indexed_axis_paginated_cost_tests.rs b/grovedb/src/tests/indexed_axis_paginated_cost_tests.rs index 3ce7d326e..81bfa844c 100644 --- a/grovedb/src/tests/indexed_axis_paginated_cost_tests.rs +++ b/grovedb/src/tests/indexed_axis_paginated_cost_tests.rs @@ -19,8 +19,9 @@ mod tests { use crate::{ batch::QualifiedGroveDbOp, + operations::proof::indexed_axis::AxisEntries, tests::{make_test_grovedb, TempGroveDb, TEST_LEAF}, - Element, + Element, GroveDb, }; /// Rows in the large tie-heavy fixture. Big enough that a linear skip @@ -551,6 +552,91 @@ mod tests { } } + // ----------------------------------------------------------------- + // Cross-path parity: the unproved `skipped` is the same quantity the + // proved path attests. + // + // A client reading `skipped` off the wire cannot tell which path + // served it, so the two must report the same number for the same + // request — including the past-the-end shape, where both must report + // the population rather than echoing the request. The proved side + // re-derives its value from the counted subtree commitments in the + // proof bytes; the unproved side reads the secondary's root + // aggregate. Nothing structural keeps those in step, so it is pinned + // here rather than assumed. + // ----------------------------------------------------------------- + + #[test] + fn unproved_skipped_equals_the_proved_paths_attested_skipped() { + let grove_version = GroveVersion::latest(); + let (db, population) = make_pcit_with_mixed_counts(grove_version); + let path: &[&[u8]] = &[TEST_LEAF, b"cidx"]; + let pop = population as u64; + + for descending in [false, true] { + // Offsets spanning: nothing skipped, mid-page, the last row, + // exactly the population, just past it, and absurdly past it + // (the original denial-of-service lever). + for offset in [0u64, 1, 5, pop - 1, pop, pop + 1, 4_000_000_000] { + for k in [0u16, 1, 3] { + let unproved = db + .indexed_count_top_k_paginated( + path, + k, + offset, + descending, + None, + grove_version, + ) + .unwrap() + .expect("unproved page"); + + let proof = db + .prove_indexed_count_top_k_paginated( + path, + k, + offset, + descending, + None, + grove_version, + ) + .unwrap() + .expect("prove page"); + let proved = GroveDb::verify_indexed_count_top_k_paginated( + &proof, path, k, offset, descending, + ) + .expect("verify page"); + + assert_eq!( + unproved.skipped, proved.skipped, + "skipped disagrees between paths at offset={offset} k={k} \ + descending={descending}" + ); + // Both paths must also agree that `min(offset, + // population)` is what that number means — equality + // alone would be satisfied by two identically wrong + // values. + assert_eq!( + unproved.skipped, + offset.min(pop), + "skipped is not min(offset, population) at offset={offset} k={k} \ + descending={descending}" + ); + + let proved_entries = match &proved.entries { + AxisEntries::Count(v) => v.clone(), + other => panic!("count axis proof returned {other:?}"), + }; + assert_eq!( + unproved.entries, proved_entries, + "entries disagree between paths at offset={offset} k={k} \ + descending={descending}" + ); + } + } + } + } + // ----------------------------------------------------------------- // Manual measurement harness — not run in CI. Run with: // cargo test -p grovedb measure_paginated_costs -- --ignored --nocapture From c4ceac676630727d01b532314a7e488edbeaf115 Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Fri, 14 Aug 2026 00:30:46 +0700 Subject: [PATCH 06/11] test: cover the legacy baseline and the counted path's returned-row decode refusal Two additions closing the honest part of the codecov patch gap: - An always-on differential test pinning that the counted path returns identical entries to the pre-change linear implementation (kept verbatim as the test-only measurement baseline, previously exercised only by the ignored release harness) across offsets, k values, and both directions at a CI-affordable size. - A drift-suite case making the malformed row the RETURNED position of a counted read (ascending, offset 2), asserting CorruptedData: skipped rows go undecoded by design, returned rows never do. This pins the "returned rows are still validated" half of the counted-skip contract, which was previously asserted only through the iterator path. The remaining uncovered patch lines are corruption fail-loud guards (zero-count link, depth cap, link-vs-child count mismatch, walk-None, non-provable-count aggregate, defensive second-child skip) that an audit verified are not constructible through any supported write path; they stay uncovered rather than deleted, weakened, or reached by forging states no writer can produce. No coverage exclusions added. --- .../indexed_axis_paginated_cost_tests.rs | 54 +++++++++++++++++++ .../indexed_tree_secondary_drift_tests.rs | 11 ++++ 2 files changed, 65 insertions(+) diff --git a/grovedb/src/tests/indexed_axis_paginated_cost_tests.rs b/grovedb/src/tests/indexed_axis_paginated_cost_tests.rs index 81bfa844c..f9a42adc0 100644 --- a/grovedb/src/tests/indexed_axis_paginated_cost_tests.rs +++ b/grovedb/src/tests/indexed_axis_paginated_cost_tests.rs @@ -637,6 +637,60 @@ mod tests { } } + /// Always-on differential against the pre-change implementation + /// (`legacy_linear_indexed_count_top_k_paginated`, kept verbatim as a + /// test-only baseline): identical entries in identical order at every + /// probed offset/k/direction, plus the `skipped` semantics the legacy + /// shape could not report. The release-mode measurement harness runs + /// the same comparison at 1e3–1e6 rows; this pins it in CI at a size + /// CI can afford. + #[test] + fn counted_path_agrees_with_the_pre_change_linear_implementation() { + let grove_version = GroveVersion::latest(); + const ROWS: u64 = 120; + let db = make_pcit_with_rows(ROWS, grove_version); + let path = [TEST_LEAF, b"cidx"]; + + for descending in [false, true] { + for offset in [0u64, 1, 3, 60, ROWS - 1, ROWS, ROWS + 80] { + for k in [1u16, 7] { + let counted = db + .indexed_count_top_k_paginated( + path.as_ref(), + k, + offset, + descending, + None, + grove_version, + ) + .unwrap() + .expect("counted read"); + let legacy = db + .legacy_linear_indexed_count_top_k_paginated( + path.as_ref(), + k, + offset, + descending, + None, + grove_version, + ) + .unwrap() + .expect("legacy linear read"); + assert_eq!( + counted.entries, legacy, + "counted and legacy diverge at offset={offset} k={k} \ + descending={descending}" + ); + assert_eq!( + counted.skipped, + offset.min(ROWS), + "true skipped at offset={offset} k={k} descending={descending}" + ); + } + } + } + } + // ----------------------------------------------------------------- // Manual measurement harness — not run in CI. Run with: // cargo test -p grovedb measure_paginated_costs -- --ignored --nocapture diff --git a/grovedb/src/tests/indexed_tree_secondary_drift_tests.rs b/grovedb/src/tests/indexed_tree_secondary_drift_tests.rs index 70b7af334..3745f1490 100644 --- a/grovedb/src/tests/indexed_tree_secondary_drift_tests.rs +++ b/grovedb/src/tests/indexed_tree_secondary_drift_tests.rs @@ -869,6 +869,17 @@ mod tests { "range", ); + // When the malformed row is the RETURNED position of a counted read + // (ascending order is [a, b, 0xff…]; offset 2 selects it), the + // counted path must decode-and-refuse it exactly as the iterator + // paths do — skipped rows go undecoded, returned rows never do. + assert_short_key_corruption( + db.indexed_count_top_k_paginated([TEST_LEAF, b"pcit"].as_ref(), 1, 2, false, None, gv) + .unwrap() + .expect_err("a returned malformed row must surface through the counted path"), + "paginated (offset 2, counted path, returned row)", + ); + // Ascending top-k stops before reaching the malformed row, so the // well-formed prefix of the index still reads cleanly — the error above // is the decoder refusing a specific row, not the query failing wholesale. From 0100cb833075621659a68ddd3696baecc98e55b8 Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Fri, 14 Aug 2026 03:06:48 +0700 Subject: [PATCH 07/11] fix: serve the whole counted page from one pinned iterator view MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Blocking Platform review finding, independently confirmed: the counted descent fetched children through successive RefWalker point-gets on a snapshotless transaction (start_transaction is a bare db.transaction()), so a block committing mid-descent could hand back a child from a newer state than its resident parent. Merk's child loads never verify the fetched child against the parent's recorded link hash, and the aggregate count cross-checks cannot see a same-population update, so the result was a silently mixed page — where the replaced linear scan, driven by a single KVIterator, pinned one consistent view for its whole page. The proved path survives the same torn reads only because verification's ancestor-chain reconciliation rejects them; the unproved read has no such check. The counted walk now fetches every node — root re-read, descent, and collect — through one raw iterator over the secondary's storage context (seek by node key + decode via the public TreeNode::decode). A RocksDB transaction iterator pins an implicit snapshot of committed state plus the transaction's own uncommitted writes: the same guarantee, from the same mechanism, the pre-change implementation had. RefWalker leaves the walk entirely; the decision logic, fail-loud guards (own-count, link/child cross-check, zero-count link, depth cap), and cost accounting are unchanged in substance. The open-to-iterator window can at worst produce a loud CorruptedData (root key moved), never a mixed page. The transaction-overlay half of the property is pinned by a new always-on test (rows inserted in an open transaction are visible and counted through it, invisible outside it). The commit-interleaving half is not deterministically testable — the read is one synchronous call with no way to pause between fetches — and rests on RocksDB's iterator-snapshot contract, exactly as the replaced implementation's guarantee did; stated in the test and design doc rather than implied. Cost impact, re-measured in release at 1e6 rows: offset 0 unchanged (5 seeks / 629 B); deep offset 23 seeks / 32 us (one extra seek for the root re-read); past-the-end 4 seeks / 7 us, still flat at every N. --- docs/COUNTED_SKIP_DESIGN.md | 38 +-- grovedb/src/operations/indexed_tree.rs | 244 +++++++++++------- .../indexed_axis_paginated_cost_tests.rs | 62 +++++ 3 files changed, 236 insertions(+), 108 deletions(-) diff --git a/docs/COUNTED_SKIP_DESIGN.md b/docs/COUNTED_SKIP_DESIGN.md index f9f786584..dcb49b5ef 100644 --- a/docs/COUNTED_SKIP_DESIGN.md +++ b/docs/COUNTED_SKIP_DESIGN.md @@ -119,15 +119,22 @@ differed on two points: reads instead of with the raw-storage accident. - **Corrupt keys inside the skipped region** are no longer decoded (not visiting them is the point); returned keys are still validated, and a visited node with own-count ≠ 1 fails loud. -- **No snapshot isolation** (`start_transaction` takes no snapshot at this rev): a multi-fetch - walk can observe a concurrent commit between fetches. For *this function specifically* that is - a real (if narrow) regression, stated plainly: the old implementation ran skip and collect - through a single RocksDB iterator, which pins a consistent view for the whole page, so the page - was internally consistent even under concurrent writes; the counted descent is independent - point-gets with no read snapshot — the same exposure as merk's existing unproved aggregate - walks, but new to this read. A torn walk surfaces as `CorruptedData` (the checked count - arithmetic and the link/child cross-check both trip on mixed-version nodes) or a - stale-but-ordered page. The fix is a storage-layer read snapshot, outside this scope. +- **Snapshot consistency: restored after a blocking Platform review finding.** The first + implementation fetched nodes through independent `RefWalker` point-gets on a snapshotless + transaction; a commit landing mid-descent could hand back a child from a newer state than its + resident parent, and because merk's child loads never verify the fetched child against the + parent's recorded link hash — and the count cross-checks cannot see a same-population update — + the result was a *silently mixed page*. (The proved path survives the same torn reads because + verification's ancestor-chain reconciliation rejects them; the unproved read has no such + check, which is exactly why it needed the snapshot.) The counted walk now serves **every** + node fetch — root re-read, descent, and collect — through one raw iterator, whose RocksDB + transaction iterator pins an implicit snapshot of committed state plus the transaction's own + uncommitted writes: the same guarantee, from the same mechanism, that the replaced + single-`KVIterator` linear scan had. The transaction-overlay half is pinned by an always-on + test; the commit-interleaving half is not deterministically testable (no way to pause a + synchronous read between fetches) and rests on RocksDB's iterator-snapshot contract, as the + old code's guarantee did. The open→iterator window can at worst produce a loud + `CorruptedData` (root key moved), never a mixed page. - **One function, two views.** `offset == 0` serves the storage-iterator view; `offset > 0` serves the merk-tree view. Identical on any clean secondary; in a drifted one, a caller paging `offset = 0, k, 2k, …` could see a discontinuity between the first page and the rest. Accepted: @@ -165,12 +172,13 @@ harness output): | N | offset | counted (seeks / bytes / µs) | linear (seeks / bytes / µs) | |---:|---:|---:|---:| -| 1e3 | 0 | 5 / 625 / 6 | 5 / 625 / 6 | -| 1e3 | N−1 | 11 / 1,727 / 15 | 1,004 / 316 KB / 268 | -| 1e3 | ≥ N | 3 / 362 / 4 | 1,004 / 316 KB / 259 | -| 1e6 | 0 | 5 / 629 / 8 | 5 / 629 / 7 | -| 1e6 | N−1 | 22 / 3,710 / 32 | 1,000,004 / 316 MB / 326,226 | -| 1e6 | 4×10⁹ | 3 / 366 / 4 | 1,000,004 / 316 MB / 339,806 | +| 1e6 | 0 | 5 / 629 / 7 | 5 / 629 / 6 | +| 1e6 | N−1 | 23 / 5,811 / 32 | 1,000,004 / 316 MB / 316,420 | +| 1e6 | ≥ N | 4 / 643 / 7 | 1,000,004 / 316 MB / 308,299 | + +(Numbers are from the final snapshot-consistent implementation; relative to the pre-snapshot +point-get walk, the pinned-view fetches cost one extra seek — the root re-read through the +iterator — and charge full prefixed key bytes per fetch, leaving wall-clock unchanged.) What the numbers establish: diff --git a/grovedb/src/operations/indexed_tree.rs b/grovedb/src/operations/indexed_tree.rs index 05a55d9a6..e3d7d6f6b 100644 --- a/grovedb/src/operations/indexed_tree.rs +++ b/grovedb/src/operations/indexed_tree.rs @@ -41,13 +41,13 @@ use grovedb_merk::{ }, merk::KVIterator, proofs::Query, - tree::{kv::ValueDefinedCostType, AggregateData, Fetch, RefWalker}, + tree::{kv::ValueDefinedCostType, AggregateData, TreeNode}, Merk, TreeType, }; use grovedb_path::SubtreePath; use grovedb_storage::{ rocksdb_storage::{PrefixedRocksDbTransactionContext, RocksDbStorage}, - Storage, StorageBatch, StorageContext, + RawIterator, Storage, StorageBatch, StorageContext, }; use grovedb_version::version::GroveVersion; @@ -2496,6 +2496,27 @@ struct CountedPageState { left_to_right: bool, } +/// Recursion ceiling for the counted descent. An AVL tree cannot exceed +/// 1.44·64 ≈ 93 levels even at the u64 population limit, so anything +/// deeper means a corrupt link structure (e.g. a cyclic link) — fail with +/// an error instead of overflowing the stack. +const COUNTED_SKIP_MAX_DEPTH: u32 = 128; + +/// Read a child link's provable count. A present link points at a +/// non-empty subtree, whose count is therefore at least 1 — a zero is +/// corruption, not an empty side (that is `None`). +fn provable_count_from_link(link: Option<&grovedb_merk::tree::Link>) -> Result { + match link { + None => Ok(0), + Some(link) => match provable_count_from_aggregate(link.aggregate_data())? { + 0 => Err(Error::CorruptedData( + "secondary link is present but carries aggregate count 0".to_string(), + )), + count => Ok(count), + }, + } +} + /// Serve one page of secondary keys at `offset` in directional order by /// counted descent over the secondary merk: subtrees whose whole /// population fits inside the remaining offset are consumed from their @@ -2505,9 +2526,17 @@ struct CountedPageState { /// count, `min(offset, population)`; values are never needed — the /// caller decodes keys exactly as the iterator path does. /// -/// The root node is already resident from the merk open, so an offset at -/// or past the whole population is answered from its aggregate alone, -/// with zero storage fetches. +/// **Every node in the page comes from one pinned view.** All fetches — +/// the root re-read, the descent, and the collect — go through a single +/// raw iterator, and a RocksDB transaction iterator pins an implicit +/// snapshot of the committed state at creation plus the transaction's +/// own uncommitted writes. That is the same consistency guarantee the +/// replaced linear scan had from its single `KVIterator`. Independent +/// point-gets through the (snapshotless) transaction would not have it: +/// a commit landing mid-descent could hand back a child from a newer +/// state than its resident parent, and merk's child loads do not verify +/// the child against the parent's recorded link hash, so the result +/// would be a silently mixed page rather than an error. fn counted_skip_page<'db, S: StorageContext<'db>>( merk: &Merk, offset: u64, @@ -2515,73 +2544,98 @@ fn counted_skip_page<'db, S: StorageContext<'db>>( left_to_right: bool, grove_version: &GroveVersion, ) -> CostResult<(Vec>, u64), Error> { - merk.walk(|maybe_walker| { - let mut cost = OperationCost::default(); - let Some(mut walker) = maybe_walker else { - // Empty secondary: nothing to skip, nothing to return. - return Ok((Vec::new(), 0)).wrap_with_cost(cost); - }; - let population = cost_return_on_error_no_add!( - cost, - walker - .tree() - .aggregate_data() - .map_err(|e| Error::CorruptedData(format!("secondary aggregate_data: {e}"))) - .and_then(provable_count_from_aggregate) - ); - let skipped = offset.min(population); - if limit == 0 || offset >= population { - return Ok((Vec::new(), skipped)).wrap_with_cost(cost); + let mut cost = OperationCost::default(); + + let Some(root_key) = merk.root_key() else { + // Empty secondary: nothing to skip, nothing to return. + return Ok((Vec::new(), 0)).wrap_with_cost(cost); + }; + + // The pinned view for the whole read. + let mut iter = merk.storage.raw_iter(); + + // Re-fetch the root through the iterator so the root itself belongs + // to the pinned view rather than to the earlier open. A miss means + // the tree's root key moved between the open and iterator creation — + // fail loud instead of mixing the two states. + let root = match cost_return_on_error!( + &mut cost, + snapshot_fetch_node(&mut iter, &root_key, grove_version) + ) { + Some(root) => root, + None => { + return Err(Error::CorruptedData( + "secondary root node is absent from the read snapshot — the tree moved \ + between open and read" + .to_string(), + )) + .wrap_with_cost(cost); } - let mut state = CountedPageState { - offset_remaining: offset, - limit_remaining: limit, - left_to_right, - }; - let page_len = (population - offset).min(limit) as usize; - let mut out = Vec::with_capacity(page_len); - cost_return_on_error!( - &mut cost, - counted_skip_collect(&mut walker, &mut state, &mut out, 0, grove_version) - ); - Ok((out, skipped)).wrap_with_cost(cost) - }) -} + }; -/// Recursion ceiling for the counted descent. An AVL tree cannot exceed -/// 1.44·64 ≈ 93 levels even at the u64 population limit, so anything -/// deeper means a corrupt link structure (e.g. a cyclic link) — fail with -/// an error instead of overflowing the stack. -const COUNTED_SKIP_MAX_DEPTH: u32 = 128; + let population = cost_return_on_error_no_add!( + cost, + root.aggregate_data() + .map_err(|e| Error::CorruptedData(format!("secondary aggregate_data: {e}"))) + .and_then(provable_count_from_aggregate) + ); + let skipped = offset.min(population); + if limit == 0 || offset >= population { + return Ok((Vec::new(), skipped)).wrap_with_cost(cost); + } + let mut state = CountedPageState { + offset_remaining: offset, + limit_remaining: limit, + left_to_right, + }; + let page_len = (population - offset).min(limit) as usize; + let mut out = Vec::with_capacity(page_len); + cost_return_on_error!( + &mut cost, + counted_skip_collect(&mut iter, &root, &mut state, &mut out, 0, grove_version) + ); + Ok((out, skipped)).wrap_with_cost(cost) +} -/// Read a child link's provable count. A present link points at a -/// non-empty subtree, whose count is therefore at least 1 — a zero is -/// corruption, not an empty side (that is `None`). -fn provable_count_from_link(link: Option<&grovedb_merk::tree::Link>) -> Result { - match link { - None => Ok(0), - Some(link) => match provable_count_from_aggregate(link.aggregate_data())? { - 0 => Err(Error::CorruptedData( - "secondary link is present but carries aggregate count 0".to_string(), - )), - count => Ok(count), - }, +/// Fetch and decode one merk node from the pinned iterator view. Returns +/// `Ok(None)` when the key is absent from that view. +fn snapshot_fetch_node( + iter: &mut I, + key: &[u8], + grove_version: &GroveVersion, +) -> CostResult, Error> { + let mut cost = OperationCost::default(); + iter.seek(key).unwrap_add_cost(&mut cost); + match iter.key().unwrap_add_cost(&mut cost) { + Some(found) if found == key => {} + _ => return Ok(None).wrap_with_cost(cost), } + let Some(bytes) = iter.value().unwrap_add_cost(&mut cost) else { + return Ok(None).wrap_with_cost(cost); + }; + TreeNode::decode( + key.to_vec(), + bytes, + None:: Option>, + grove_version, + ) + .map(Some) + .map_err(|e| Error::CorruptedData(format!("secondary node failed to decode: {e}"))) + .wrap_with_cost(cost) } -/// Recursive counted descent. Entered only on subtrees whose population -/// exceeds the remaining offset (the caller and both descend sites -/// guarantee it) and only while the page has room. -fn counted_skip_collect( - walker: &mut RefWalker, +/// Recursive counted descent over nodes served by the pinned iterator. +/// Entered only on subtrees whose population exceeds the remaining +/// offset (the caller and both descend sites guarantee it) and only +/// while the page has room. +fn counted_skip_collect( + iter: &mut I, + node: &TreeNode, state: &mut CountedPageState, out: &mut Vec>, depth: u32, grove_version: &GroveVersion, -) -> CostResult<(), Error> -where - S: Fetch + Sized + Clone, -{ +) -> CostResult<(), Error> { let mut cost = OperationCost::default(); if depth > COUNTED_SKIP_MAX_DEPTH { @@ -2594,16 +2648,13 @@ where let node_count = cost_return_on_error_no_add!( cost, - walker - .tree() - .aggregate_data() + node.aggregate_data() .map_err(|e| Error::CorruptedData(format!("secondary aggregate_data: {e}"))) .and_then(provable_count_from_aggregate) ); - let left_count = - cost_return_on_error_no_add!(cost, provable_count_from_link(walker.tree().link(true))); + let left_count = cost_return_on_error_no_add!(cost, provable_count_from_link(node.link(true))); let right_count = - cost_return_on_error_no_add!(cost, provable_count_from_link(walker.tree().link(false))); + cost_return_on_error_no_add!(cost, provable_count_from_link(node.link(false))); // Every valid secondary row contributes structural count exactly 1 // (`mirror_indexed_axis_to_secondary` writes nothing else, and @@ -2639,7 +2690,8 @@ where cost_return_on_error!( &mut cost, counted_skip_descend( - walker, + iter, + node, first_is_left, first_count, state, @@ -2660,7 +2712,7 @@ where if state.offset_remaining > 0 { state.offset_remaining -= 1; } else { - out.push(walker.tree().key().to_vec()); + out.push(node.key().to_vec()); state.limit_remaining -= 1; if state.limit_remaining == 0 { return Ok(()).wrap_with_cost(cost); @@ -2678,7 +2730,8 @@ where cost_return_on_error!( &mut cost, counted_skip_descend( - walker, + iter, + node, !first_is_left, second_count, state, @@ -2693,43 +2746,49 @@ where Ok(()).wrap_with_cost(cost) } -/// Fetch one child (upgrading its link) and recurse into it. +/// Fetch one child from the pinned view and recurse into it. /// /// `link_count` is the aggregate count read off the parent's link — the /// number that authorized this descent (and that whole-subtree skips /// trust without fetching). It is cross-checked against the loaded /// child's own aggregate, so a link whose cached count disagrees with /// its subtree fails loud instead of shifting every position after it. -fn counted_skip_descend( - walker: &mut RefWalker, +#[allow(clippy::too_many_arguments)] +fn counted_skip_descend( + iter: &mut I, + parent: &TreeNode, left: bool, link_count: u64, state: &mut CountedPageState, out: &mut Vec>, depth: u32, grove_version: &GroveVersion, -) -> CostResult<(), Error> -where - S: Fetch + Sized + Clone, -{ +) -> CostResult<(), Error> { let mut cost = OperationCost::default(); - let walked = cost_return_on_error!( + let child_key = match parent.link(left) { + Some(link) => link.key().to_vec(), + None => { + // The caller only descends where the link (and its non-zero + // count) was just read, so this is unreachable short of a + // logic error — fail loud regardless. + return Err(Error::CorruptedData( + "secondary descend without a link".to_string(), + )) + .wrap_with_cost(cost); + } + }; + let child = match cost_return_on_error!( &mut cost, - walker - .walk( - left, - None::<&fn(&[u8], &GroveVersion) -> Option>, - grove_version, - ) - .map_err(Into::into) - ); - let mut child = match walked { + snapshot_fetch_node(iter, &child_key, grove_version) + ) { Some(child) => child, None => { - // The caller only descends where the link (and its non-zero - // count) was just read, so a missing child is corruption. + // The parent's link names a key the pinned view does not + // contain: corruption (or a view predating the parent, which + // a single snapshot rules out). return Err(Error::CorruptedData( - "secondary link is present but its child failed to load".to_string(), + "secondary link is present but its child is absent from the read snapshot" + .to_string(), )) .wrap_with_cost(cost); } @@ -2737,7 +2796,6 @@ where let child_count = cost_return_on_error_no_add!( cost, child - .tree() .aggregate_data() .map_err(|e| Error::CorruptedData(format!("secondary aggregate_data: {e}"))) .and_then(provable_count_from_aggregate) @@ -2749,7 +2807,7 @@ where ))) .wrap_with_cost(cost); } - counted_skip_collect(&mut child, state, out, depth + 1, grove_version).add_cost(cost) + counted_skip_collect(iter, &child, state, out, depth + 1, grove_version).add_cost(cost) } #[cfg(test)] diff --git a/grovedb/src/tests/indexed_axis_paginated_cost_tests.rs b/grovedb/src/tests/indexed_axis_paginated_cost_tests.rs index f9a42adc0..e8ff3fc80 100644 --- a/grovedb/src/tests/indexed_axis_paginated_cost_tests.rs +++ b/grovedb/src/tests/indexed_axis_paginated_cost_tests.rs @@ -691,6 +691,68 @@ mod tests { } } + /// The counted read's pinned view must be the *transaction's* view — + /// snapshot of committed state plus the transaction's own uncommitted + /// writes — not a bare DB snapshot. Rows inserted inside an open + /// transaction must be visible (and counted) by a paginated read + /// through that same transaction before commit, exactly as they were + /// through the old single-iterator scan. + /// + /// The other half of the consistency property — that a commit landing + /// mid-descent cannot produce a page mixing two states — is not + /// deterministically testable from here: the read is one synchronous + /// call with no way to pause between node fetches. It rests on + /// RocksDB's iterator-snapshot contract, the same contract the + /// replaced implementation relied on for the same guarantee. + #[test] + fn counted_read_sees_the_transactions_uncommitted_writes() { + let grove_version = GroveVersion::latest(); + let db = make_pcit_with_rows(20, grove_version); + let path = [TEST_LEAF, b"cidx"]; + + let tx = db.start_transaction(); + for i in 20..30u64 { + db.insert_into_count_indexed_tree( + [TEST_LEAF, b"cidx"].as_ref(), + format!("k{i:07}").as_bytes(), + Element::new_item(vec![]), + Some(&tx), + grove_version, + ) + .unwrap() + .expect("insert within tx"); + } + + // Through the transaction, all 30 rows exist: a counted read at + // offset 25 must see the uncommitted tail. + let in_tx = db + .indexed_count_top_k_paginated(path.as_ref(), 3, 25, false, Some(&tx), grove_version) + .unwrap() + .expect("counted read inside the transaction"); + assert_eq!(in_tx.skipped, 25, "population through the tx is 30"); + assert_eq!( + in_tx + .entries + .iter() + .map(|(_, key)| key.clone()) + .collect::>(), + vec![ + b"k0000025".to_vec(), + b"k0000026".to_vec(), + b"k0000027".to_vec() + ], + ); + + // Without the transaction, the committed view still has 20 rows: + // the same offset is past the end. + let outside = db + .indexed_count_top_k_paginated(path.as_ref(), 3, 25, false, None, grove_version) + .unwrap() + .expect("counted read outside the transaction"); + assert!(outside.entries.is_empty()); + assert_eq!(outside.skipped, 20, "committed population is 20"); + } + // ----------------------------------------------------------------- // Manual measurement harness — not run in CI. Run with: // cargo test -p grovedb measure_paginated_costs -- --ignored --nocapture From 63df14c27c4b9ad47881141a213af6d92ef8d10f Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Fri, 14 Aug 2026 10:05:18 +0700 Subject: [PATCH 08/11] fix: discover the secondary root key inside the counted read's pinned view MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second-round review finding on the snapshot fix, verified: root-key DISCOVERY still ran outside the pinned view. The validated open read secondary_root_key from the parent element through the snapshotless transaction, and only afterwards did the counted read create its pinned iterator and re-fetch the node at that key. A commit rotating the secondary AVL root in that window could leave the old root key resolving in the newer snapshot as a DEMOTED CHILD — an internally consistent subtree that passes every count check, so the traversal would serve it as the complete ranking: entries and skipped silently truncated. The absent-root guard only caught the case where the old key vanished entirely; a wrong answer that verifies is exactly the failure class this read must not produce. The read is now pinned end to end. One raw iterator is created under the parent merk's prefix, re-reads the indexed element to obtain the authoritative secondary root key (re-validating the element variant and axis in-view via the extraction helper now shared with the merk-backed reader), is retargeted to the secondary's prefix — a new consuming PrefixedRocksDbRawIterator::retarget that keeps the underlying iterator and therefore its snapshot — and then serves the root fetch, descent, and collect. Nothing the page is built from is read outside that one view. The ordinary validated open still runs first, purely for validation and the offset-0 fast path; none of its loads are trusted as page data. Cost, re-measured in release at 1e6 rows, k=1: offset 0 unchanged (5 seeks / 629 B / 7 us); deep offset 24 seeks / 32 us (+1 seek for the in-view element re-read); past-the-end 5 seeks / 9 us, flat at every N. The cost test's past-end bound now budgets the two pinned-view discovery reads explicitly. --- docs/COUNTED_SKIP_DESIGN.md | 40 ++-- grovedb/src/operations/indexed_tree.rs | 186 ++++++++++++------ .../indexed_axis_paginated_cost_tests.rs | 18 +- .../storage_context/raw_iterator.rs | 16 ++ 4 files changed, 178 insertions(+), 82 deletions(-) diff --git a/docs/COUNTED_SKIP_DESIGN.md b/docs/COUNTED_SKIP_DESIGN.md index dcb49b5ef..dbc0570c5 100644 --- a/docs/COUNTED_SKIP_DESIGN.md +++ b/docs/COUNTED_SKIP_DESIGN.md @@ -126,15 +126,23 @@ differed on two points: parent's recorded link hash — and the count cross-checks cannot see a same-population update — the result was a *silently mixed page*. (The proved path survives the same torn reads because verification's ancestor-chain reconciliation rejects them; the unproved read has no such - check, which is exactly why it needed the snapshot.) The counted walk now serves **every** - node fetch — root re-read, descent, and collect — through one raw iterator, whose RocksDB - transaction iterator pins an implicit snapshot of committed state plus the transaction's own - uncommitted writes: the same guarantee, from the same mechanism, that the replaced - single-`KVIterator` linear scan had. The transaction-overlay half is pinned by an always-on - test; the commit-interleaving half is not deterministically testable (no way to pause a - synchronous read between fetches) and rests on RocksDB's iterator-snapshot contract, as the - old code's guarantee did. The open→iterator window can at worst produce a loud - `CorruptedData` (root key moved), never a mixed page. + check, which is exactly why it needed the snapshot.) A second review round tightened the + boundary further: pinning only the traversal still left **root-key discovery** outside the + view, and a commit rotating the secondary root between discovery and traversal could leave the + old root key resolving to a *demoted child* in the newer view — an internally consistent + subtree that every count check accepts, silently truncating the page. The final shape + therefore pins the *entire read*: one raw iterator (implicit RocksDB snapshot at creation plus + the transaction's uncommitted-write overlay) is created under the parent merk's prefix, + re-reads the indexed element to obtain the authoritative secondary root key, is retargeted to + the secondary's prefix (`PrefixedRocksDbRawIterator::retarget`, same underlying iterator, same + snapshot), and then serves the root fetch, the descent, and the collect. **Nothing the page is + built from is read outside that one view**; the ordinary validated open still runs first, but + purely for validation and the offset-0 fast path — none of its loads are trusted as page data. + This restores, and slightly exceeds, the guarantee the replaced single-`KVIterator` linear + scan had. The transaction-overlay half is pinned by an always-on test; the + commit-interleaving half is not deterministically testable (no way to pause a synchronous + read between fetches) and rests on RocksDB's iterator-snapshot contract, as the old code's + guarantee did. - **One function, two views.** `offset == 0` serves the storage-iterator view; `offset > 0` serves the merk-tree view. Identical on any clean secondary; in a drifted one, a caller paging `offset = 0, k, 2k, …` could see a discontinuity between the first page and the rest. Accepted: @@ -173,12 +181,14 @@ harness output): | N | offset | counted (seeks / bytes / µs) | linear (seeks / bytes / µs) | |---:|---:|---:|---:| | 1e6 | 0 | 5 / 629 / 7 | 5 / 629 / 6 | -| 1e6 | N−1 | 23 / 5,811 / 32 | 1,000,004 / 316 MB / 316,420 | -| 1e6 | ≥ N | 4 / 643 / 7 | 1,000,004 / 316 MB / 308,299 | - -(Numbers are from the final snapshot-consistent implementation; relative to the pre-snapshot -point-get walk, the pinned-view fetches cost one extra seek — the root re-read through the -iterator — and charge full prefixed key bytes per fetch, leaving wall-clock unchanged.) +| 1e6 | N−1 | 24 / 5,985 / 32 | 1,000,004 / 316 MB / 306,699 | +| 1e6 | ≥ N | 5 / 817 / 9 | 1,000,004 / 316 MB / 312,300 | + +(Numbers are from the final fully-pinned implementation. Relative to the original point-get +walk, the pinned view costs two extra seeks — the in-view re-read of the indexed element that +carries the authoritative secondary root key, and the root node fetch — and charges full +prefixed key bytes per fetch. Wall-clock is unchanged; the past-the-end shape stays flat at +every N.) What the numbers establish: diff --git a/grovedb/src/operations/indexed_tree.rs b/grovedb/src/operations/indexed_tree.rs index e3d7d6f6b..da2d6f72e 100644 --- a/grovedb/src/operations/indexed_tree.rs +++ b/grovedb/src/operations/indexed_tree.rs @@ -1294,7 +1294,7 @@ impl GroveDb { let secondary_merk = cost_return_on_error!( &mut cost, - self.open_validated_axis_secondary(path, axis, tx_ref, grove_version) + self.open_validated_axis_secondary(path.clone(), axis, tx_ref, grove_version) ); // `offset == 0` is the overwhelmingly common shape, and the raw @@ -1312,18 +1312,76 @@ impl GroveDb { }) .add_cost(cost); } + // The open above serves validation (path shape, element variant, + // axis compatibility) and the offset-0 fast path only. For the + // counted read, nothing it loaded is trusted as page data. + drop(secondary_merk); + + // `offset > 0`: counted skip. NOTHING THE PAGE IS BUILT FROM IS + // READ OUTSIDE ONE PINNED VIEW: a single raw iterator (implicit + // RocksDB snapshot at creation, plus the transaction's own + // uncommitted writes) serves the indexed element's re-read — the + // authoritative secondary root key — and then, retargeted to the + // secondary's prefix, the root node and every descent and collect + // fetch. Discovering the root key outside the view would let a + // commit that rotates the secondary root between discovery and + // traversal leave the old root key resolving to a *demoted child* + // in the newer view: an internally consistent subtree that every + // count check accepts, silently truncating the page. Re-reading + // the element inside the view closes that hole. + let Some((parent_path, indexed_key)) = path.derive_parent() else { + // Unreachable: the validated open above already rejected the + // root path. + return Err(Error::InvalidPath( + "cannot query an indexed tree at the root path".to_string(), + )) + .wrap_with_cost(cost); + }; + let parent_prefix = + RocksDbStorage::build_prefix(parent_path.clone()).unwrap_add_cost(&mut cost); + let primary_prefix = RocksDbStorage::build_prefix(path).unwrap_add_cost(&mut cost); + let secondary_prefix = RocksDbStorage::secondary_prefix_for(&primary_prefix, axis.tag()) + .unwrap_add_cost(&mut cost); + let parent_ctx = self + .db + .get_transactional_storage_context_by_subtree_prefix(parent_prefix, None, tx_ref) + .unwrap_add_cost(&mut cost); + + // The pinned view. Created under the parent merk's prefix to read + // the indexed element, then retargeted to the secondary's prefix + // for the traversal — same underlying iterator, same snapshot. + let mut view = parent_ctx.raw_iter(); + let parent_node = match cost_return_on_error!( + &mut cost, + snapshot_fetch_node(&mut view, indexed_key, grove_version) + ) { + Some(node) => node, + None => { + return Err(Error::CorruptedData( + "indexed-tree element is absent from the read snapshot — the tree was \ + removed between validation and read" + .to_string(), + )) + .wrap_with_cost(cost); + } + }; + let element = cost_return_on_error_no_add!( + cost, + Element::deserialize(parent_node.value_as_slice(), grove_version).map_err(|e| { + Error::CorruptedData(format!("indexed-tree element failed to deserialize: {e}")) + }) + ); + let secondary_root_key = cost_return_on_error_no_add!( + cost, + axis_secondary_root_key_from_element(axis, &element) + ); + let view = view.retarget(secondary_prefix); - // `offset > 0`: counted skip. Walk the secondary merk itself, - // consuming the offset through link aggregate counts — a subtree - // whose whole population fits inside the remaining offset is - // skipped without ever being fetched — then collect up to `k` - // keys in directional order. One root-to-position descent plus - // `k` node loads, instead of one storage-iterator step per - // skipped entry. let (secondary_keys, skipped) = cost_return_on_error!( &mut cost, counted_skip_page( - &secondary_merk, + view, + secondary_root_key, offset, u64::from(k), !descending, @@ -2018,34 +2076,7 @@ impl GroveDb { &mut cost, Element::get(&parent_merk, indexed_key, true, grove_version).map_err(Error::MerkError) ); - match (axis, element.underlying()) { - // PCIT carries a single secondary; only Count axis is valid. - (IndexAxis::Count, Element::ProvableCountIndexedTree(_, secondary, ..)) => { - Ok(secondary.clone()).wrap_with_cost(cost) - } - // PSIT carries a single secondary; only Sum axis is valid. - (IndexAxis::Sum, Element::ProvableSumIndexedTree(_, secondary, ..)) => { - Ok(secondary.clone()).wrap_with_cost(cost) - } - // PCPSIT carries a TLV of 1..=3 axis-tagged secondaries; the - // requested axis must appear in the TLV. - (_, Element::ProvableCountProvableSumIndexedTree(_, _, _, axes, _)) => { - let want_tag = axis.tag(); - match axes.iter().find(|(t, _)| *t == want_tag) { - Some((_, sec)) => Ok(sec.clone()).wrap_with_cost(cost), - None => Err(Error::InvalidPath(format!( - "{:?} axis not indexed at this path", - axis - ))) - .wrap_with_cost(cost), - } - } - _ => Err(Error::InvalidPath(format!( - "{:?} axis not indexed at this path", - axis - ))) - .wrap_with_cost(cost), - } + axis_secondary_root_key_from_element(axis, &element).wrap_with_cost(cost) } /// Delete an item from a `CountIndexedTree` element. Removes the @@ -2496,6 +2527,43 @@ struct CountedPageState { left_to_right: bool, } +/// Pure extraction of the per-axis `secondary_root_key` from an +/// indexed-tree element. Shared by the merk-backed reader (which drives +/// every per-axis query API's validation) and by the counted paginated +/// path's pinned-view re-read, so the two agree on axis-compatibility by +/// construction. +fn axis_secondary_root_key_from_element( + axis: IndexAxis, + element: &Element, +) -> Result>, Error> { + match (axis, element.underlying()) { + // PCIT carries a single secondary; only Count axis is valid. + (IndexAxis::Count, Element::ProvableCountIndexedTree(_, secondary, ..)) => { + Ok(secondary.clone()) + } + // PSIT carries a single secondary; only Sum axis is valid. + (IndexAxis::Sum, Element::ProvableSumIndexedTree(_, secondary, ..)) => { + Ok(secondary.clone()) + } + // PCPSIT carries a TLV of 1..=3 axis-tagged secondaries; the + // requested axis must appear in the TLV. + (_, Element::ProvableCountProvableSumIndexedTree(_, _, _, axes, _)) => { + let want_tag = axis.tag(); + match axes.iter().find(|(t, _)| *t == want_tag) { + Some((_, sec)) => Ok(sec.clone()), + None => Err(Error::InvalidPath(format!( + "{:?} axis not indexed at this path", + axis + ))), + } + } + _ => Err(Error::InvalidPath(format!( + "{:?} axis not indexed at this path", + axis + ))), + } +} + /// Recursion ceiling for the counted descent. An AVL tree cannot exceed /// 1.44·64 ≈ 93 levels even at the u64 population limit, so anything /// deeper means a corrupt link structure (e.g. a cyclic link) — fail with @@ -2526,19 +2594,22 @@ fn provable_count_from_link(link: Option<&grovedb_merk::tree::Link>) -> Result>( - merk: &Merk, +/// **Every node in the page comes from one pinned view.** The caller +/// hands in the raw iterator already carrying the view that the +/// secondary root key was discovered in (retargeted to the secondary's +/// prefix), and the root fetch, descent, and collect all go through it. +/// A RocksDB transaction iterator pins an implicit snapshot of the +/// committed state at creation plus the transaction's own uncommitted +/// writes — the same consistency guarantee the replaced linear scan had +/// from its single `KVIterator`. Independent point-gets through the +/// (snapshotless) transaction would not have it: a commit landing +/// mid-descent could hand back a child from a newer state than its +/// resident parent, and merk's child loads do not verify the child +/// against the parent's recorded link hash, so the result would be a +/// silently mixed page rather than an error. +fn counted_skip_page( + mut iter: I, + root_key: Option>, offset: u64, limit: u64, left_to_right: bool, @@ -2546,18 +2617,13 @@ fn counted_skip_page<'db, S: StorageContext<'db>>( ) -> CostResult<(Vec>, u64), Error> { let mut cost = OperationCost::default(); - let Some(root_key) = merk.root_key() else { + let Some(root_key) = root_key else { // Empty secondary: nothing to skip, nothing to return. return Ok((Vec::new(), 0)).wrap_with_cost(cost); }; - // The pinned view for the whole read. - let mut iter = merk.storage.raw_iter(); - - // Re-fetch the root through the iterator so the root itself belongs - // to the pinned view rather than to the earlier open. A miss means - // the tree's root key moved between the open and iterator creation — - // fail loud instead of mixing the two states. + // The root key was read from the indexed element inside this same + // view, so a miss here is corruption, not a race. let root = match cost_return_on_error!( &mut cost, snapshot_fetch_node(&mut iter, &root_key, grove_version) @@ -2565,8 +2631,8 @@ fn counted_skip_page<'db, S: StorageContext<'db>>( Some(root) => root, None => { return Err(Error::CorruptedData( - "secondary root node is absent from the read snapshot — the tree moved \ - between open and read" + "secondary root node named by the indexed element is absent from the same \ + read snapshot" .to_string(), )) .wrap_with_cost(cost); diff --git a/grovedb/src/tests/indexed_axis_paginated_cost_tests.rs b/grovedb/src/tests/indexed_axis_paginated_cost_tests.rs index e8ff3fc80..52974f618 100644 --- a/grovedb/src/tests/indexed_axis_paginated_cost_tests.rs +++ b/grovedb/src/tests/indexed_axis_paginated_cost_tests.rs @@ -262,11 +262,15 @@ mod tests { cost_0.storage_loaded_bytes, ); - // Past the end: the root aggregate (resident from the open) - // alone proves the offset exceeds the population — no descent at - // all. Offset 0 costs open + iterator seek + K collect steps, so - // "no descent" pins as: past-end + the collect work still fits - // inside the offset-0 budget. + // Past the end: the root aggregate alone proves the offset + // exceeds the population — no descent at all. The counted path + // pays the open plus exactly two pinned-view discovery reads + // (the indexed element carrying the authoritative secondary + // root key, then the root node itself, both through the one + // snapshot the whole page is served from). Offset 0 costs open + // + iterator seek + K collect steps, so "no descent" pins as: + // past-end + the collect work fits inside the offset-0 budget + // plus those two discovery reads. let CostContext { value: past, cost: cost_past, @@ -288,9 +292,9 @@ mod tests { "past-end must report the true population, not echo the request" ); assert!( - cost_past.seek_count + u32::from(K) <= cost_0.seek_count, + cost_past.seek_count + u32::from(K) <= cost_0.seek_count + 2, "past-end offset must not descend (descending={descending}): seek_count {} vs \ - {} at offset 0 (k = {K})", + {} at offset 0 (k = {K}, 2 pinned-view discovery reads allowed)", cost_past.seek_count, cost_0.seek_count, ); diff --git a/storage/src/rocksdb_storage/storage_context/raw_iterator.rs b/storage/src/rocksdb_storage/storage_context/raw_iterator.rs index c6593746f..bb3559329 100644 --- a/storage/src/rocksdb_storage/storage_context/raw_iterator.rs +++ b/storage/src/rocksdb_storage/storage_context/raw_iterator.rs @@ -46,6 +46,22 @@ pub struct PrefixedRocksDbRawIterator { pub(super) raw_iterator: I, } +impl PrefixedRocksDbRawIterator { + /// Re-scope this iterator to a different subtree prefix while keeping + /// the underlying RocksDB iterator — and therefore its pinned snapshot + /// view — alive. A multi-subtree read that must be internally + /// consistent (e.g. discovering a subtree's root key in one subtree + /// and then traversing that subtree) can hop scopes without giving up + /// the view; consuming `self` makes the hand-off explicit, so one + /// pinned view serves one subtree at a time, in sequence. + pub fn retarget(self, prefix: SubtreePrefix) -> Self { + Self { + prefix, + raw_iterator: self.raw_iterator, + } + } +} + impl RawIterator for PrefixedRocksDbRawIterator> { fn seek_to_first(&mut self) -> CostContext<()> { self.raw_iterator.seek(self.prefix); From 5085dd84b7ab9201f91f516babe8f94c6653d358 Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Fri, 14 Aug 2026 13:57:45 +0700 Subject: [PATCH 09/11] fix: clamp the counted page's pre-allocation; align doc prose with measured table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings. The page vector's capacity derived from caller-supplied limit and the on-disk root aggregate, so a huge k (or a forged aggregate) could reserve memory the page can never fill on a public read path — the capacity hint is now clamped at 1024 entries and the vector grows only by actually being filled. The design doc's prose bullets still quoted pre-snapshot figures; they now carry the measured numbers of the fully-pinned implementation (deep-offset seeks 13/17/20/24 across 1e3..1e6 rows, past-the-end flat at 5 seeks at every N), matching the table. --- docs/COUNTED_SKIP_DESIGN.md | 11 ++++++----- grovedb/src/operations/indexed_tree.rs | 8 +++++++- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/docs/COUNTED_SKIP_DESIGN.md b/docs/COUNTED_SKIP_DESIGN.md index dbc0570c5..be0d2edb9 100644 --- a/docs/COUNTED_SKIP_DESIGN.md +++ b/docs/COUNTED_SKIP_DESIGN.md @@ -195,12 +195,13 @@ What the numbers establish: - **Offset 0 has zero regression** — identical seeks, bytes, and wall-clock in both directions and at both `k` values, at every N (the fast path *is* the old code, and the always-on `paginated_offset_zero_costs_exactly_plain_top_k` test pins the equality). -- **The deep-offset seek count is the tree depth**: 11 → 15 → 18 → 22 across N = 1e3 → 1e6, - logarithmic exactly as designed; wall-clock 15–36 µs against the linear walk's 268 µs–378 ms. -- **Past-the-end — the DoS lever — is flat 3 seeks / 366 B / 4 µs at every N**: the root - aggregate answers it with zero descent. +- **The deep-offset seek count is the tree depth plus the two pinned-view discovery reads**: + 13 → 17 → 20 → 24 across N = 1e3 → 1e6, logarithmic exactly as designed; wall-clock 14–37 µs + against the linear walk's 268 µs–312 ms. +- **Past-the-end — the DoS lever — is flat 5 seeks / ~817 B / ≤ 11 µs at every N**: the + in-view element and root reads alone answer it with zero descent. - Against the previously measured prove-then-verify route (78–129 µs flat, plus its proof - construction/serialization/verification CPU), the counted read measures 4–36 µs at `k = 1` — + construction/serialization/verification CPU), the counted read measures 6–37 µs at `k = 1` — the "unproved is even faster" claim is now measured, not modeled. - **The one corner where the old path was faster, measured and accepted:** tiny positive offsets. At `offset = 1, k = 100` the counted path costs 107–112 seeks / ~14 KB / 141–155 µs against the diff --git a/grovedb/src/operations/indexed_tree.rs b/grovedb/src/operations/indexed_tree.rs index da2d6f72e..de456cd89 100644 --- a/grovedb/src/operations/indexed_tree.rs +++ b/grovedb/src/operations/indexed_tree.rs @@ -2654,8 +2654,14 @@ fn counted_skip_page( limit_remaining: limit, left_to_right, }; + // Pre-allocation is a hint, not a promise: `limit` is caller-supplied + // and `population` comes from an on-disk aggregate, so an unclamped + // capacity would let a huge limit (or a forged aggregate) reserve + // memory the page can never fill. The vector grows past the clamp + // only by actually being filled, one visited node at a time. + const PAGE_CAPACITY_CLAMP: usize = 1024; let page_len = (population - offset).min(limit) as usize; - let mut out = Vec::with_capacity(page_len); + let mut out = Vec::with_capacity(page_len.min(PAGE_CAPACITY_CLAMP)); cost_return_on_error!( &mut cost, counted_skip_collect(&mut iter, &root, &mut state, &mut out, 0, grove_version) From 91490a46331c098185ef39cf60698be27a8923dd Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Fri, 14 Aug 2026 14:05:56 +0700 Subject: [PATCH 10/11] test: pin that a dangling secondary root key fails loud through the counted path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A parent element whose secondary_root_key names a node that does not exist is corruption the counted read must refuse, never serve as an empty or truncated page — the pinned view read the element itself, so the dangling key is not a race to retry through. Constructed honestly via the drift suite's rebind machinery (element rebound with a bogus root key, rows and hashes otherwise intact); also pins the contrast that the offset-0 iterator path, which never consults the root key, still reads the physical rows. This is the one remaining counted-path guard that drift surgery can reach; it also carries the indexed-tree codecov patch component over its 82% gate (the pinned-view rewrite had landed at 81.99%). --- .../indexed_tree_secondary_drift_tests.rs | 129 ++++++++++++++++++ 1 file changed, 129 insertions(+) diff --git a/grovedb/src/tests/indexed_tree_secondary_drift_tests.rs b/grovedb/src/tests/indexed_tree_secondary_drift_tests.rs index 3745f1490..93233b88b 100644 --- a/grovedb/src/tests/indexed_tree_secondary_drift_tests.rs +++ b/grovedb/src/tests/indexed_tree_secondary_drift_tests.rs @@ -1204,6 +1204,135 @@ mod tests { tx.commit().expect("tx commit"); } + /// Rebind a PCIT's parent element so its `secondary_root_key` names a + /// key that does not exist in the count secondary, leaving every row + /// and hash otherwise intact. Produces the state a counted read must + /// treat as corruption: the element is readable and valid, but the + /// root it names resolves to nothing. + fn rebind_count_secondary_root_key_to( + db: &TempGroveDb, + pcit_path: &[&[u8]], + bogus_key: Vec, + gv: &GroveVersion, + ) { + use grovedb_merk::element::reconstruct::ElementReconstructExtensions; + + let tx = db.start_transaction(); + let batch = StorageBatch::new(); + let owned: Vec<&[u8]> = pcit_path.to_vec(); + let path: SubtreePath<&[u8]> = owned.as_slice().into(); + let (parent_path, pcit_key) = path.derive_parent().expect("non-root PCIT"); + + let (primary_root_hash, primary_root_key, primary_aggregate) = { + let primary = db + .open_transactional_merk_at_path(path.clone(), &tx, Some(&batch), gv) + .unwrap() + .expect("open PCIT primary"); + primary + .root_hash_key_and_aggregate_data() + .unwrap() + .expect("primary root state") + }; + + let mut parent_merk = db + .open_transactional_merk_at_path(parent_path.clone(), &tx, Some(&batch), gv) + .unwrap() + .expect("open parent merk"); + let pcit_element = Element::get(&parent_merk, pcit_key, true, gv) + .unwrap() + .expect("PCIT element"); + let secondary_root_key = match pcit_element.underlying() { + Element::ProvableCountIndexedTree(_, s, ..) => s.clone(), + other => panic!("not a PCIT element: {other:?}"), + }; + let secondary_root_hash = { + let secondary = db + .open_indexed_secondary_at_path( + path.clone(), + IndexAxis::Count, + secondary_root_key, + &tx, + Some(&batch), + gv, + ) + .unwrap() + .expect("open count secondary"); + let (hash, _, _) = secondary + .root_hash_key_and_aggregate_data() + .unwrap() + .expect("secondary root state"); + hash + }; + + let rebound = pcit_element + .reconstruct_with_two_root_keys(primary_root_key, Some(bogus_key), primary_aggregate) + .expect("reconstruct PCIT element"); + rebound + .insert_count_indexed_subtree( + &mut parent_merk, + pcit_key, + primary_root_hash, + secondary_root_hash, + None, + gv, + ) + .unwrap() + .expect("rebind PCIT element"); + + let mut merk_cache = std::collections::HashMap::new(); + merk_cache.insert(parent_path.clone(), parent_merk); + db.propagate_changes_with_transaction(merk_cache, parent_path, &tx, &batch, gv) + .unwrap() + .expect("propagate rebind"); + db.db + .commit_multi_context_batch(batch, Some(&tx)) + .unwrap() + .expect("commit rebind"); + tx.commit().expect("tx commit"); + } + + /// A parent element whose `secondary_root_key` names a node that does + /// not exist must fail loud through the counted path. The pinned view + /// read the element itself, so a dangling root key is corruption, not + /// a race to retry through — and it must never be silently served as + /// an empty or truncated page. The offset-0 iterator path, which + /// never consults the root key, still reads the physical rows: the + /// contrast is the two-views posture the design doc records. + #[test] + fn a_dangling_secondary_root_key_fails_loud_through_the_counted_path() { + let gv = GroveVersion::latest(); + let db = make_test_grovedb(gv); + make_pcit_with(&db, b"pcit", &[b"a", b"b"], gv); + rebind_count_secondary_root_key_to( + &db, + &[TEST_LEAF, b"pcit"], + b"no-such-node".to_vec(), + gv, + ); + + let err = db + .indexed_count_top_k_paginated([TEST_LEAF, b"pcit"].as_ref(), 1, 1, false, None, gv) + .unwrap() + .expect_err("a dangling secondary root key must error, not serve a page"); + match err { + Error::CorruptedData(message) => assert!( + message.contains("absent from the same read snapshot"), + "unexpected corruption message: {message}" + ), + other => panic!("expected CorruptedData, got {other:?}"), + } + + // The iterator path reads the physical keyspace and is untouched + // by the dangling root key. + assert_eq!( + db.indexed_count_top_k_paginated([TEST_LEAF, b"pcit"].as_ref(), 2, 0, false, None, gv) + .unwrap() + .expect("offset-0 path reads physical rows") + .entries, + vec![(1u64, b"a".to_vec()), (1u64, b"b".to_vec())], + ); + } + /// PSIT: an orphan row in the sum secondary is removed and the root /// returns to the pristine twin's — the repair covers the sum axis, not /// just count. From 63ad09ed82b4d108a51cd037c9833696e1a07332 Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Sat, 15 Aug 2026 06:46:15 +0700 Subject: [PATCH 11/11] fix: adapt the unified-stack test suites to IndexedTopKPage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The merge compiled the lib but not the test binaries — the paginated readers' new page shape reaches nine call sites across axis_descent_proof_tests, run_path_query_tests, and the paginated cost suite (which also needed #801's GroveVersion param on the verify side). Bounded/range reader call sites are untouched: those still return plain vecs. Co-Authored-By: Claude Opus 5 --- grovedb/src/tests/axis_descent_proof_tests.rs | 10 +++++----- grovedb/src/tests/indexed_axis_paginated_cost_tests.rs | 7 ++++++- grovedb/src/tests/run_path_query_tests.rs | 8 ++++---- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/grovedb/src/tests/axis_descent_proof_tests.rs b/grovedb/src/tests/axis_descent_proof_tests.rs index d4e976015..06722e0ca 100644 --- a/grovedb/src/tests/axis_descent_proof_tests.rs +++ b/grovedb/src/tests/axis_descent_proof_tests.rs @@ -182,7 +182,7 @@ mod tests { ) .unwrap() .expect("direct read"); - assert_eq!(entries_as_sum(&entries), direct.as_slice()); + assert_eq!(entries_as_sum(&entries), direct.entries.as_slice()); // ...and to the standalone envelope over the same state. let standalone_bytes = db @@ -234,7 +234,7 @@ mod tests { ) .unwrap() .expect("direct"); - assert_eq!(entries_as_sum(&entries), direct.as_slice()); + assert_eq!(entries_as_sum(&entries), direct.entries.as_slice()); } other => panic!("expected AxisEntries, got {other:?}"), } @@ -358,7 +358,7 @@ mod tests { ) .unwrap() .expect("direct"); - assert_eq!(entries_as_sum(&entries), direct.as_slice()); + assert_eq!(entries_as_sum(&entries), direct.entries.as_slice()); } other => panic!("expected AxisEntries, got {other:?}"), } @@ -411,7 +411,7 @@ mod tests { .expect("direct"); assert_eq!( entries_as_sum(entries.as_ref().expect("present")), - direct.as_slice() + direct.entries.as_slice() ); } } @@ -906,7 +906,7 @@ mod tests { .indexed_count_top_k_paginated(path.as_ref(), 2, 0, true, None, grove_version) .unwrap() .expect("direct count top-k"); - assert_eq!(entries, AxisEntries::Count(direct)); + assert_eq!(entries, AxisEntries::Count(direct.entries)); } other => panic!("expected AxisEntries, got {other:?}"), } diff --git a/grovedb/src/tests/indexed_axis_paginated_cost_tests.rs b/grovedb/src/tests/indexed_axis_paginated_cost_tests.rs index 52974f618..793953362 100644 --- a/grovedb/src/tests/indexed_axis_paginated_cost_tests.rs +++ b/grovedb/src/tests/indexed_axis_paginated_cost_tests.rs @@ -607,7 +607,12 @@ mod tests { .unwrap() .expect("prove page"); let proved = GroveDb::verify_indexed_count_top_k_paginated( - &proof, path, k, offset, descending, + &proof, + path, + k, + offset, + descending, + grove_version, ) .expect("verify page"); diff --git a/grovedb/src/tests/run_path_query_tests.rs b/grovedb/src/tests/run_path_query_tests.rs index d29b8c0e4..efaae33ec 100644 --- a/grovedb/src/tests/run_path_query_tests.rs +++ b/grovedb/src/tests/run_path_query_tests.rs @@ -207,7 +207,7 @@ mod tests { .expect("unified top-k"); assert_eq!( run_entries(run), - AxisEntries::Sum(direct), + AxisEntries::Sum(direct.entries), "top-k k={k} offset={offset} descending={descending}" ); } @@ -475,7 +475,7 @@ mod tests { .expect("direct branch read"); assert_eq!( entries.as_ref().expect("present branch"), - &AxisEntries::Sum(direct), + &AxisEntries::Sum(direct.entries), "branch {}", String::from_utf8_lossy(present) ); @@ -735,7 +735,7 @@ mod tests { ) .unwrap() .expect("unified count top-k"); - assert_eq!(run_entries(run), AxisEntries::Count(direct)); + assert_eq!(run_entries(run), AxisEntries::Count(direct.entries)); // Bounded on the count axis, with bounds deliberately below and // above the u64 domain so the count clamp is exercised (the sum @@ -842,7 +842,7 @@ mod tests { ) .unwrap() .expect("unified avg top-k"); - assert_eq!(run_entries(run), AxisEntries::Avg(direct)); + assert_eq!(run_entries(run), AxisEntries::Avg(direct.entries)); // Bounded on the avg axis takes the i128 bounds unclamped — the // avg domain is the whole i128 range.