fix(proofs): V1 layer proofs must be encoded in the family of their walk direction (#863) - #917
Merged
Merged
Conversation
…lk direction (#863) `execute` checks per op that upright pushes ascend and inverted pushes descend, but it never ties the op family to the `left_to_right` the verifier walks with, and it lets the two families mix in one stream. Every bound-witness rule in `execute_proof` assumes the visit order is the tree's in-order for an ascending walk and its exact reverse for a descending one, which only holds for a homogeneous stream in the walk's own family. So an honest ascending proof of `{b, d}` handed to a descending `Key(c)` query verified `c` absent although it is the root, an honest ascending limit-1 proof handed to a descending "latest entry" page filled it with the smallest key, and a mixed stream that rebuilds the honest tree while visiting an abridged root after both children read an absence out of an authentic root hash. `Query::execute_proof` now refuses, for V1 proofs, any op that is not in the family of its walk direction. V0 is a locked wire format and is untouched. No GroveVersion gate: prover and verifier take each real level's direction from the same `query_items_at_path`, synthesized one-key levels keep reading it off the stream (#818), and the check only removes shapes no prover emits. `merk_layer_root_hash` (row-less root derivation for a subset query that stops at a tree element) reads the direction off the stream; the axis-descent secondary walk, the aggregate carrier and single-key layer walks, and the test-only indexed-axis oracles move from the lenient proof_version 0 to strict so the same forgery cannot fill a limited page from the wrong end. The count-offset and aggregate verifiers walk the reconstructed tree structurally and were never affected. Tests: the forgeries at the merk level and end-to-end through prove_query / verify_query against trusted reads, plus honest ascending/descending reads agreeing with trusted reads in both directions. All fail on develop with a false absence or the wrong page. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (9)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change enforces direction-consistent operation families for V1 Merk proofs. GroveDB verification uses the latest proof version and stream direction. New tests cover mismatched directions, mixed streams, limited queries, subset verification, and valid proofs. ChangesProof orientation enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The orientation fix has no substantiated merge-blocking risk at the current head. Sequence Diagram(s)sequenceDiagram
participant Query
participant execute_proof
participant ProofDecoder
participant execute
Query->>execute_proof: Execute proof with query direction
execute_proof->>ProofDecoder: Decode proof operations
execute_proof->>execute: Pass direction-validated operations
execute->>Query: Return verified result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #917 +/- ##
===========================================
- Coverage 92.32% 92.26% -0.06%
===========================================
Files 299 299
Lines 93205 93245 +40
===========================================
- Hits 86052 86034 -18
- Misses 7153 7211 +58
🚀 New features to boost your workflow:
|
…lower layer refused (#863) Covers `merk_layer_root_hash`'s stream-direction read: a wider descending proof re-verified by a narrower query that stops at the tree element derives the inverted lower layer's root (a fixed ascending walk would now reject it), and a mixed lower layer is refused by the orientation read before any op executes. Co-Authored-By: Claude Fable 5.1 <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.
Closes #863.
Verdict: real, reproduced at the public API
A V1 layer proof is a stream of ops in one of two families — upright (
Push/Parent/Child, visiting keys ascending) or inverted (PushInverted/ParentInverted/ChildInverted, descending).executechecks per op that upright pushes ascend and inverted pushes descend, but it never ties the family to theleft_to_rightthe verifier walks with, and it lets the two families mix in one stream.Every bound-witness rule in
execute_proof— "the previous push was key-bearing, so nothing lies between it and this key", "this is the first push, so it is the leftmost (rightmost) node", "the limit is met, so the abridged tail is fine" — assumes the visit order is the tree's in-order for an ascending walk and its exact reverse for a descending one. That only holds for a homogeneous stream in the walk's own family. Two ways to break it, both with an authentic root hash:{b, d}(rootcabridged to its kv-hash) handed to a descendingKey(c)query:bis met first, read as the rightmost node,Key(c)is consumed as "before this key", andcis verified absent although it is the root. Likewise the honest ascending limit-1 proof handed to a descending limit-1 range ("give me the latest entry") fills the page with the smallest key.Push(b) · Push(d) · PushInverted(KVHash c) · ParentInverted · Parentrebuilds the honest tree byte-for-byte at the root, passes every per-op key check, and visitsb,d, then the abridged root — so an ascending walk forKey(c)sees two adjacent key-bearing pushes and endorses the gap as an absence.All of this goes through
prove_query/verify_querywith no hand-crafted bytes needed for the first case: it is literally an honest proof of a different query. Ondevelopthe new tests fail withgot Ok([])(false absence) andgot Ok([[98]])(page filled withbwhere the trusted read saysd).Fix (V1 only, no version gate)
Query::execute_proofnow requires, forproof_version >= 1, that every op is in the family of the walk direction it was given. An op in the other family — or a mixed stream — is rejected before it reaches the bound-witness logic. V0 (proof_version == 0) is untouched: it is a locked wire format and its verifier is not changed.No
GroveVersiongate: honest V1 proofs are unaffected. Prover and verifier take each real level's direction from the samequery_items_at_path, synthesized one-key levels keep reading the direction off the stream (#818, unchanged), andproof_stream_directionalready refused mixed streams there. The check only removes forged shapes no prover emits, so nothing an honest GROVE_V3 node produces changes verdict.Callers aligned:
merk_layer_root_hash(row-less root derivation for a subset query that stops at a tree element) reads the direction off the stream, since it cannot know the generating query's direction and reports no rows. It still refuses a mixed stream.proof_version 0toPROOF_VERSION_LATESTso a wrong-family secondary stream cannot fill a limited page from the wrong end. Their honest streams already satisfied strict mode.#[cfg(test)]indexed-axis oracles do the same.Verified unaffected: the count-offset and aggregate verifiers walk the reconstructed tree structurally, not the visit order.
Tests
merk_integration_tests: the two opposite-family absences, the opposite-family limited page, the mixed-family absence (asserting the mixed stream really rebuilds the honest root), and honest homogeneous streams in both directions.grovedb/src/tests/proof_orientation_tests.rs: the same forgeries end-to-end throughprove_query/verify_queryagainst trusted reads, plus honest ascending/descending reads (unlimited, limit 1, limit 2, single keys, and an honest absence) agreeing with trusted reads in both directions.merged_descending_subset_bound_testscover the synthesized-path handling from fix: subset verification of a descending merged proof's branches (#815) #818 and keep passing.One existing test was widened:
combined_v1_envelope_non_tree_intermediate_rejectedforges an Item into a tree slot of the single-key layer and asserted on the aggregate chain gate's message; with that walk now strict, the merk verifier refuses the same forgery one step earlier (KVValueHash node must not contain an item element). The assertion accepts either gate.Local runs:
cargo test -p grovedb-merk(731 + 9 pass),cargo test -p grovedb --features full,verify,estimated_costs,unsafe-dump-load,serde,zk_client(3034 pass, 2 ignored;--all-featuresneeds thegrovedbgdownload, which was unavailable here),cargo fmt.cargo clippy --all-targets -D warningson the local stable 1.97 flags pre-existing lines in untouched files; the only lint in new code is fixed.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests