fix: verify-only access to indexed-axis proofs + bind subset lower layers - #781
Merged
Merged
Conversation
…builds The verify-side entry points of the indexed-axis proof envelope (verify_indexed_axis_top_k and friends) were minimal-gated as a module, so a consumer compiling with --no-default-features --features verify could not reach them. Gate the prove-side items individually instead and open the module to both feature sets, mirroring how the rest of the proof code splits prover from verifier. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PR #657 added a hard rejection in `verify_layer_proof_v1`: if a V1 proof carries a lower layer for a tree whose key the query stops at, verification fails with "the element bytes would be unbound". The concern is real — a `KVValueHash`-family node hashes only `(key, value_hash)`, so reporting the element without a child-hash check would let a prover attach a dummy lower layer and swap in forged element bytes under a genuine root hash. At v5.0.1 that path pushed the element with no binding at all. But "the proof descends below the query" is the ordinary shape of a SUBSET verification, not an attack: `verify_subset_query` exists to run a narrower query against a proof generated for a wider one. Rejecting it broke every caller that reads a tree element out of a proof that descended into it. Dash Platform hit this on three shielded-notes tests, which pull a `CommitmentTree`'s total note count from the note-fetch proof they already hold via a single-key, no-subquery, limit-1 subset query. Bind the element instead of refusing it. The lower layer is now consumed for its root hash in both cases, and the existing `combine_hash(H(value), child_root)` chain check does the binding; only the reporting differs. With no query below, none of the lower layer's rows belong in the result set, so the tree element itself is reported — under its PARENT path, matching query_raw and the terminal arm. Every lower-layer flavour already derives its root independently of the query (the query only selects rows), so the existing MMR / BulkAppend / CommitmentTree / DenseTree verifiers take a `report_contents` flag and return the root early; Merk layers get their root from an empty query, which matches nothing and consumes no limit. Succinct mode (`verify_query`) still rejects outright: for that query the layer is data the caller never asked for. The result is strictly stronger than v5.0.1, which bound nothing here. Tests: subset verification of a tree element against a descending proof, for both a plain Tree and the exact Platform CommitmentTree note-count shape; two tamper tests proving the binding is load-bearing in subset mode — a dummy lower layer and a sibling subtree's real-but-wrong layer are both rejected. All three fail before this change with the reported error.
Contributor
📝 WalkthroughWalkthroughThe change separates proof generation from verification feature gates. V1 verification now supports parent-only queries over nested proofs by authenticating lower-layer roots without returning child rows. Regression tests cover result reporting, strictness, and proof binding. ChangesV1 proof verification and feature gating
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Query
participant verify_v1
participant LowerLayerVerifier
participant ParentResult
Query->>verify_v1: submit parent-only query and proof
verify_v1->>LowerLayerVerifier: validate lower layer without reporting contents
LowerLayerVerifier-->>verify_v1: return authenticated child root
verify_v1->>ParentResult: bind child root to parent element
ParentResult-->>Query: return parent result with limit handling
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #781 +/- ##
===========================================
- Coverage 92.14% 92.14% -0.01%
===========================================
Files 254 254
Lines 77692 77750 +58
===========================================
+ Hits 71588 71641 +53
- Misses 6104 6109 +5
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
grovedb/src/tests/succinctness_gap_test.rs (1)
405-428: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the error variant so the tampers prove the binding.
Both tampers assert only
is_err(). Tamper 1 can fail insideexecute_proofwhile decoding the empty proof bytes, which does not exercise thecombine_hashcheck. Tamper 2 supplies a structurally valid sibling proof, so it must fail at the lower-layer hash comparison. Match onError::InvalidProofand check for the mismatch message. The test then proves the binding rather than any rejection.♻️ Proposed assertion tightening for tamper 2
let swapped_bytes = bincode::encode_to_vec(&swapped, config).expect("re-encode"); - assert!( - GroveDb::verify_subset_query(&swapped_bytes, &narrow_query, grove_version).is_err(), - "the reported element must be bound to ITS OWN child root, not any valid subtree proof" - ); + match GroveDb::verify_subset_query(&swapped_bytes, &narrow_query, grove_version) { + Err(Error::InvalidProof(_, msg)) => assert!( + msg.contains("lower layer hash"), + "expected a lower-layer hash mismatch, got: {msg}" + ), + other => panic!( + "the reported element must be bound to ITS OWN child root; got {:?}", + other.map(|(root, _)| root) + ), + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@grovedb/src/tests/succinctness_gap_test.rs` around lines 405 - 428, Tighten both tamper assertions around GroveDb::verify_subset_query to require Error::InvalidProof and verify the error message identifies the child-root/hash mismatch. Ensure Tamper 1 reaches the combine_hash validation rather than being accepted merely because execute_proof rejects malformed bytes, while Tamper 2 confirms the structurally valid sibling proof fails specifically against the reported element’s own child root.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@grovedb/src/tests/succinctness_gap_test.rs`:
- Around line 405-428: Tighten both tamper assertions around
GroveDb::verify_subset_query to require Error::InvalidProof and verify the error
message identifies the child-root/hash mismatch. Ensure Tamper 1 reaches the
combine_hash validation rather than being accepted merely because execute_proof
rejects malformed bytes, while Tamper 2 confirms the structurally valid sibling
proof fails specifically against the reported element’s own child root.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f3045b7-88c0-49f6-bde1-6e68c12f88e0
📒 Files selected for processing (6)
grovedb/src/operations/proof/indexed_axis/axis_api.rsgrovedb/src/operations/proof/indexed_axis/mod.rsgrovedb/src/operations/proof/mod.rsgrovedb/src/operations/proof/verify.rsgrovedb/src/tests/commitment_tree_tests.rsgrovedb/src/tests/succinctness_gap_test.rs
Merged
6 tasks
5 tasks
This was referenced Aug 13, 2026
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.
Summary
Two follow-ups to the indexed-tree family (#657), found while integrating it into Dash Platform:
1. Expose indexed-axis proof verification to verify-only builds
The verify-side entry points of the indexed-axis proof envelope (
verify_indexed_axis_top_kand friends) wereminimal-gated at the module level, so a consumer compiling with--no-default-features --features verify(exactly what Dash Platform'sdrivecrate does for its proof-verification layer) could not reach them. The prove-side items are now gated individually and the module is open to both feature sets, mirroring how the rest of the proof code splits prover from verifier. Verified with a reachability probe: without the change,GroveDb::verify_indexed_count_top_kand the other verify entry points fail to resolve under a verify-only build.2. Bind, don't reject, a lower layer with no query below it
#657 added a hard rejection in
verify_layer_proof_v1: a V1 proof supplying a lower layer for a tree the query doesn't descend into errored with "the element bytes would be unbound". The underlying concern is real — aKVValueHash-family node hashes only(key, value_hash), so reporting the element without binding it would let a prover attach a dummy layer and swap in forged bytes under a genuine root hash.But the check conflates that attack with the normal shape of a subset verification:
verify_subset_queryexists precisely to run a narrow query against a proof generated for a wider one, and any such proof legitimately descends below the narrow query. The rejection broke previously-passing verifications downstream (Dash Platform extracts a commitment tree'stotal_countfrom the same proof bytes that prove a note fetch — a single-key, no-subquery subset query that stops at the tree element).The fix binds instead of rejecting: the lower layer is consumed for its root hash and the existing
combine_hash(H(value), child_root)chain check does the binding; only the reporting differs. Succinct mode still rejects outright with the original message. The result is strictly stronger than v5.0.1, which bound nothing on this path — pinned by two tamper tests (a dummy layer, and a sibling subtree's real-but-wrong layer, both rejected in subset mode).Tests
cargo test -p grovedb --lib: 2535 passed, 0 failedcargo test --workspace,cargo clippy --workspace --all-features -- -D warnings,cargo check -p grovedb --no-default-features --features verify: all cleanKnown related gap (not addressed here)
Terminally-reported
CommitmentTree/MmrTree/BulkAppendTree/DenseAppendOnlyFixedSizeTreeelements (no lower layer at all) are bound by nothing in V1 proofs — pre-existing at v5.0.1, independent of this change, and being worked separately since the fix touches the proof format.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Compatibility
Tests