feat: serve per-instance query limits in V1 proofs and lift them through merges - #845
Merged
Merged
Conversation
Contributor
|
Warning Review limit reachedNext included review available in 19 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds ChangesPer-instance query limits
Estimated code review effort: 5 (Critical) | ~90+ minutes Merge Risk: 🔵 Low · up to Per-instance limits are enforced for Merk-backed queries, but several non-Merk proof types still construct or verify the full proof before capping returned rows. Large proofs could therefore consume more processing and memory than the configured limit; the change is mergeable with explicit owner awareness and follow-up on bounded proof processing. Sequence Diagram(s)sequenceDiagram
participant QueryCaller
participant PathQuery
participant V1Prover
participant V1LimitState
participant V1Verifier
QueryCaller->>PathQuery: provide Query::limit
PathQuery->>V1Prover: generate V1 proof
V1Prover->>V1LimitState: derive and charge frame budgets
V1Prover-->>QueryCaller: return proof bytes
QueryCaller->>V1Verifier: verify proof with same limit
V1Verifier->>V1LimitState: replay row and descent charges
V1Verifier-->>QueryCaller: return verified rows
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 82.22% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 45 functions across 11 files. (4 skipped: 4 unsupported.) ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ugh merges Teaches the V1 prover and verifier the per-instance limit accounting (Query::limit, introduced in #844), and makes PathQuery::merge lift merged inputs' global limits into per-instance caps. Proofs: the bare overall_limit/limit_left counters become a shared V1LimitState { global, consumed_rows, consumed_total } threaded through both walks, plus a frame-local instance budget per layer (min(inherited, the layer query's Query::limit), resolved through SinglePathSubquery::instance_limit). Each layer's merk walk runs under min(global, instance); rows charge every budget, descents settle the enclosing frame from the consumed_rows delta, and the empty-layer charge stays global-only. The consumed_total counter also replaces the old detect-consumption-by-diffing-the-Option trick, which went blind when the global limit was None. Non-Merk layers (MMR / BulkAppend / Dense / CommitmentTree) and the count-offset dispatch keep their helper signatures and are bounded through an effective-limit temp whose delta is absorbed back. Behavior for every query without per-instance limits is byte-for-byte unchanged; the V0 prover/verifier stay frozen and reject limit-carrying queries at dispatch, as do absence-proof assembly and pre-GROVE_V4 versions. The public verification wrappers run the version-aware gate before decoding the proof envelope, with the absence-mode blanket kept pre-decode as well. Merging (path_query_methods.merge = 2, GROVE_V4): an input's global SizedQuery::limit is lifted onto its merged branch's Query::limit — exact, because the branch instance executes exactly once — and authored per-instance limits ride along on their branches. Limits merge only as exclusive grafts: a limited input landing at the merged root, or two branches colliding on a key, are refused with typed errors; limit-free inputs keep the v1 merge behavior identically. prove_query_many therefore now serves limited path queries, and the verifier re-derives the same merged query at the same grove version. Deliberately NOT included: counting add_parent_tree_on_subquery rows against the limit (the M6 known limitation). Making the prover charge exactly where the verifier pushes needs a per-descent-arm audit (including the aggregate arms) that deserves its own change; the documented limitation stands. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
force-pushed
the
claude/query-instance-limit-proofs
branch
from
August 31, 2026 18:36
8456217 to
3d17adc
Compare
Adds the Per-Instance Query Limits chapter (semantics, min composition, budget-vs-traversal charges, the merge lift, proof behavior, the fail-closed table, and the version-3 wire encoding), links it from the Query System chapter, and refreshes the stale Query struct listings in the book and the grovedb crate doc (both predated read_mode). The add_parent_tree_on_subquery known-limitation note now points at the remaining prover/verifier charging work instead of promising the per-level redesign that has since shipped. Part 3 of 3. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #845 +/- ##
===========================================
- Coverage 92.55% 92.55% -0.01%
===========================================
Files 293 296 +3
Lines 91035 91344 +309
===========================================
+ Hits 84256 84540 +284
- Misses 6779 6804 +25
🚀 New features to boost your workflow:
|
…pattern
The merge slot's behavior was branched inline throughout one function
body (if merge_version >= 1 / < 2 / match merge_version), against the
repo convention that version gates live in <op>/{mod.rs,v0.rs,...}
sub-files with a match dispatcher. PathQuery::merge now keeps only the
version-independent prelude (unknown-slot fail-closed validation,
empty-input rejection, the read-mode refusal, the single-input
shortcut) and dispatches to query/merge/{v0,v1,v2}.rs:
- v0: frozen GROVE_V1..V3 behavior — directions silently dropped, all
limits and offsets refused;
- v1: direction agreement + propagation, limits still refused (kept as
its own arm — unlike the folded path_query_push intermediate, the
0/1/2 numbering is spelled out in the version-table docs);
- v2 (GROVE_V4): v1's direction rules plus the per-instance limit lift
with its exclusive-graft refusals.
Pure restructure — every version's behavior is byte-for-byte what the
inline branches produced, pinned by the untouched merge test suite.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Same reasoning as the path_query_push fold: the direction-agreement merge landed gated to GROVE_V4 as slot 1, V4 has not been released, and the per-instance lift took the V4 slot as 2 — leaving slot 1 a dead dispatch arm no version table selects. Since the slot values are internal until V4 ships, collapse: the direction rules + limit lift ARE merge v1 now, GROVE_V4 maps path_query_methods.merge back to 1, and the dispatcher (and its fail-closed validator) know [0, 1]. Table docs, tests, and the book chapter renumbered to match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
5 tasks
QuantumExplorer
commented
Aug 31, 2026
QuantumExplorer
left a comment
QuantumExplorer
left a comment
Member
Author
There was a problem hiding this comment.
Reviewed the final merged head ed50f67755c00c65ec8142a027a4b5e5105a4bbf. Documentation note (P3): grovedb-version/src/version/v4.rs:91-99 still says per-instance limits are served only on trusted reads and that proofs reject them. That contradicts this PR's V1 prover/verifier support and the updated generic capability documentation; please update the V4 capability summary as well.
QuantumExplorer
added a commit
that referenced
this pull request
Aug 31, 2026
* fix: address maintainer P1 review on per-instance limits in proofs - A layer with subquery branches no longer runs its merk walk under the instance budget: the cap counts descendant ROWS, and an empty first child consumes none of it, so truncating the child enumeration discarded later populated children (proofs returned 0 rows where the trusted read returned 1). Subquery layers now walk under the global budget only, pure terminal layers keep min(global, instance), and the verifier derives the identical per-layer value. - That exposed a truncation-masked prover/verifier asymmetry on subquery-matched EMPTY trees: the prover charged them as result rows (instance included), the verifier charged nothing. Both sides now charge them as empty children — global budget only, mirroring the trusted read's empty-subquery charge — while genuinely-terminal empty trees keep their row charge. - The four non-Merk adapters (MMR / BulkAppend / Dense / CommitmentTree) min-compose the lower query's own Query::limit on both prover and verifier; they bypass recursive frame creation, so a child cap was ignored entirely (3 rows verified under a cap of 1). - The BulkAppend layer accounting saturates on an empty child range (positions at or past total_count clamped end below start — debug-panic underflow, ~u16::MAX charged in release). - The merge lift treats overlap with the merged root's own selection as a collision: a grafted conditional overrides the root query's default/terminal semantics for that key and silently dropped the root-landing input's contribution. Regression tests: empty-then-populated sibling parity under an instance cap, child caps enforced across Dense/Bulk/MMR proofs, the empty-range underflow, and the root-overlap merge refusal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: tidy merge v1 comments after the fold Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: follow-ups from the per-instance limits reviews (#844/#845) Carries the review fixes the #845 squash-merge raced past, plus the follow-ups from the post-merge #844 review. Proof/merge fixes (reviewed on #845, missed by its merge): - subquery layers no longer run their merk walk under the instance budget (an empty first child discarded later populated ones); pure terminal layers keep min(global, instance); verifier mirrors the rule - prover and verifier both charge subquery-matched EMPTY trees as empty children (global budget only) — truncation had masked their row-vs-nothing asymmetry - the non-Merk adapters (MMR/Bulk/Dense/CT) min-compose the lower query's own Query::limit on both sides - the BulkAppend layer accounting saturates on an empty child range (debug-panic underflow) - the merge lift treats overlap with the merged root's own selection as a collision #844 follow-ups: - Query's serde representation is versioned and hand-written: fixed always-present positional layout with a leading version; flat base-compatible JSON for versions 1-2; the limit-bearing version 3 nests under `body` so pre-limit readers hard-fail instead of silently decoding an unlimited query; unknown flat keys are refused; canonical version claims enforced; cross-generation writer/reader tests both directions in both format families - Element::query_item runs the recursive whole-query preflight (an unmatched limited conditional made acceptance data-dependent) - the internal walk helpers validate their subordinate method-version slots (query_item / get_query_apply_function / get_path_query), which the wrapper-only checks no longer covered - both coherence gates exact-match the path_query_push selector, so an unknown engine value is a typed VersionError independent of data Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: address the six Codex findings on the follow-ups PR - Serde: the positional layout is now FRAMED — a leading magic sentinel before the version, because a bare version byte was consumed by the released unframed reader as its items-vector length and the payload reinterpreted (full-consumption repro); the framed reader requires the magic exactly. The human-readable form nests version 2 as well as 3 under `body`: the actual released v5.0.1 derive (no read_mode) ignores unknown flat keys, so a flat read-mode payload silently decoded as plain key selection. Flat maps smuggling read_mode/limit are refused; cross-generation tests now target the released layout in both directions and both format families. - Merge lift is order-independent: sub-level branches merge in a canonical partition — limit-free branches first through the full machinery (still limit-free at that point), limited grafts last, with collisions judged only against the structures they actually touch; permutation regression added. - The specialized adapters (MMR / Bulk / Dense / CT) order rows by the query direction before the cap applies, so a descending cap keeps the last positions; exact-key descending tests for all four (including a CommitmentTree fixture). - The BulkAppend prover charges semantically matched rows (the same expand_query_to_u64_positions set the verifier uses) instead of the bounding span, which desynchronized the shared budget on sparse queries and made the verifier reject honest proofs; regression with positions {0,100} under a global limit plus a later sibling layer. - The empty-child verifier arm reports the matched parent when add_parent_tree_on_subquery requests it, so empty and non-empty parents behave alike (parent rows stay uncharged per the documented flag limitation); mixed empty/non-empty regression added. - The 8-site lower-layer cap derivation is one shared helper (V1LimitState::effective_lower_layer_limit) so prover and verifier cannot silently diverge (CodeRabbit). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part 2 of 3, stacked on #844 (merge that first; this PR's base is its branch). Part 3 is the book chapter.
V1 prover/verifier accounting
The bare
overall_limit: &mut Option<u16>/limit_leftcounters become a sharedV1LimitState { global, consumed_rows, consumed_total }threaded through both walks, plus a frame-local instance budget per layer:min(inherited remaining, the layer query's own Query::limit), resolved through the newSinglePathSubquery::instance_limit(so synthesized path-component layers pass the chain through untouched, and the chain starts at the query root automatically).min(global, instance)— merk's per-layer "proof returns more data than limit" check is what enforces per-instance over-delivery on the verifier side (pinned by an adversarial test: a proof built for cap 4 must not verify under cap 2).consumed_rowsdelta; the empty-layer charge (decrease_limit_on_empty_sub_query_result) stays global-only — it bounds traversal work, instance caps bound rows. (No newProveOptionsfield: V0 proofs bincode-embedProveOptions, so a new field there would break the frozen wire format, and the V1 verifier always runsProveOptions::default()anyway.)consumed_totalalso replaces the old detect-consumption-by-diffing-the-Optiontrick, which went blind when the global limit wasNone— a real case once instance caps can exist without a global one.Behavior for every query without per-instance limits is unchanged — the full workspace suite (5125 tests) passes untouched, which matters because the V1 proof format is live under GROVE_V3. The V0 prover/verifier stay frozen and reject limit-carrying queries at dispatch; absence-proof assembly and pre-V4 versions keep failing closed.
Merge lifting (
path_query_methods.merge = 2, GROVE_V4)PathQuery::mergenow merges limited path queries by lifting: an input's globalSizedQuery::limitbecomes its merged branch'sQuery::limit. The lift is semantically exact — the branch instance executes exactly once (its path is a concrete key chain), so "at most N rows from this input" and "at most N per instance" coincide. Authored per-instance limits ride along on their branches. Per the refuse-on-collision decision:limits merge only as exclusive grafts);prove_query_manytherefore now serves limited path queries, and the verifier re-derives the identical merged query at the same grove version.Deliberately out of scope
Counting
add_parent_tree_on_subqueryrows against the limit (the M6 known limitation) is not included. Making the prover charge exactly where the verifier pushes parent rows requires auditing every descent arm (ordinary, cidx-family, the four non-Merk arms, and the aggregate descents, where the pairing is unclear) — an asymmetry there breaks honest proofs. It deserves its own focused change; the documented limitation stands.Tests
per_instance_limit_tests.rsgrows to 21: proof round-trips differentially checked against the trusted read (top-k per parent, global∧instance composition, conditional-branch caps, ancestor caps, reverse direction), the wide-cap-vs-tight-cap adversarial rejection, absence-mode rejection, pre-V4 prover rejection, merge lift round-trips (instance-capped and global-limited inputs through prove/verify), and the collision/root-landing refusals.merge_refuses_limits_and_offsets_at_every_merge_versionbecomesmerge_refuses_offsets_at_every_version_and_lifts_limits_from_v2, pinning the pre-v2 refusal and the v2 lifted structure.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation