fix: allow parent tree info count-offset proofs#749
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR addresses an audit finding by removing the early offset validation in parent-tree-info proof verification. The centralized envelope gate now uniformly applies V0 rejection and V1 acceptance of offset queries. Tests verify V0 still correctly rejects offset-bearing queries while V1 accepts and returns correctly paginated results. ChangesOffset validation centralization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
✅ Review complete (commit fe6cf44) |
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Targeted change correctly removes a redundant early offset reject from verify_query_get_parent_tree_info_with_options and delegates to the shared V0-rejects/V1-relaxes envelope gate. However, the newly-enabled V1 path inherits a latent bug: combining absence_proofs_for_non_existing_searched_keys: true with a non-zero offset on a V1 envelope causes query.terminal_keys(limit, ...) (offset-unaware) to rewrite valid results into false absences. The PR newly exposes this consensus-relevant misbehavior through the parent-tree-info entry point, and the fix belongs in the shared V1 absence-proof block so it covers all entry points.
🔴 1 blocking
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `grovedb/src/operations/proof/verify.rs`:
- [BLOCKING] grovedb/src/operations/proof/verify.rs:370-375: V1 absence-proof rewrite is offset-unaware and produces false absences
After this PR, `verify_query_get_parent_tree_info_with_options` can reach V1 count-offset verification, and `verify_query_with_options` already does. Both still accept `absence_proofs_for_non_existing_searched_keys: true` together with a non-zero offset. The V1 verifier returns the offset page (e.g. keys `f,g,h` for `offset=5, limit=3` on a narrow range), but the absence-proof block then calls `query.terminal_keys(max_results, ...)`, which is offset-unaware — `Query::terminal_keys_inner` (grovedb-query/src/query.rs:361) only enumerates the first `limit` searched keys from the range item and never consults `SizedQuery::offset`. The terminal keys it produces (e.g. `a,b,c`) miss every entry in the result set, so the final rewrite emits `(path, a, None), (path, b, None), (path, c, None)` — false absence proofs for keys that actually exist. Because absence-proof verification is consensus-critical, this needs to be rejected at the shared V1 absence-proof entry until terminal-key generation is made offset-aware. Centralizing the reject in `verify_proof_v1_internal` (rather than only in the parent-tree-info entry point) covers both call sites uniformly.
Fix parent-tree-info count-offset proof verification
Summary
verify_query_get_parent_tree_info_with_optionsto reach the sharedV0/V1 proof-envelope offset gate instead of rejecting offsets before proof
decoding.
succeeds for provable count trees.
Fixes #707.
Validation
cargo test -p grovedb verify_v1_get_parent_tree_info_with_offset_succeeds --libcargo test -p grovedb verify_v0_get_parent_tree_info_with_offset_errors --libcode-review dashpay/grovedb upstream/develop refs/remotes/local/fix-parent-tree-count-offset "Fix grovedb#707 by allowing parent-tree-info verification with offset-paginated V1 count proofs while preserving V0 rejection; adds V0 rejection and V1 success regressions"→ shipSummary by CodeRabbit
Bug Fixes
Tests