fix(grovedb): bind terminal non-Merk tree element bytes to the parent value_hash - #782
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.
… value_hash When a V1 proof reported an Element::CommitmentTree / MmrTree / BulkAppendTree / DenseAppendOnlyFixedSizeTree as a TERMINAL result — the query targets the tree element itself and the prover emits no lower layer — nothing tied the serialized element bytes to the value_hash its parent Merk commits to. Both existing binding mechanisms skipped these four types. The empty-tree combine_hash(H(value), NULL_HASH) check is gated on `!is_non_empty_tree()`, but `is_non_empty_tree()` returns true unconditionally for them. The `child_hash_verified` requirement was gated on `is_non_empty_merk_tree()`, which by construction excludes them. The prover, meanwhile, only rewrote regular non-empty Merk trees to KVValueHashFeatureTypeWithChildHash and left these emitting a bare KVValueHash, which hashes only (key, value_hash) — the value bytes never enter the node hash. A prover could therefore serve forged element bytes (an inflated or deflated CommitmentTree total_count, a different MMR size) alongside the genuine value_hash and still reconstruct the correct root hash. Dash Platform's GetShieldedNotesCount verifier reads total_count from exactly such a terminal element, so a malicious node could misreport a wallet's shielded sync denominator. Note this could NOT be fixed verifier-side by asserting hash == value_hash(value_bytes): these types are written through insert_subtree, so the parent commits combine_hash(H(value), state_root), not plain H(value), and the state root is not derivable from the element bytes. It has to travel in the proof. No new proof format is needed. KVValueHashFeatureTypeWithChildHash already verifies combine_hash(H(value), child_hash) == value_hash, which is exactly the composition these types commit — the prover simply was not using it here. Prover: the terminal arm now covers all four types (CommitmentTree moved out of the empty-trees arm, since it is bound whether or not it holds notes) and rewrites the node to carry the tree's state root, computed by the new `non_merk_tree_child_hash`. Each arm mirrors its write path: MmrTree, BulkAppendTree and DenseAppendOnlyFixedSizeTree are inserted with NULL_HASH while empty — note an empty BulkAppendTree's compute_current_state_root() is NOT NULL_HASH, so that case short-circuits — while CommitmentTree needs no special case because the sinsemilla/bulk composition already yields EMPTY_COMMITMENT_TREE_STATE_ROOT at count 0. A self-check fails loudly if a recomputed root does not reproduce the committed value hash, so any future convention drift surfaces as a prover error instead of an unverifiable proof. Verifier: the child-hash requirement widens from is_non_empty_merk_tree() to is_non_empty_tree(), which adds exactly these four types. This tightens verification: proofs from an un-upgraded prover are now rejected, so provers and verifiers must upgrade together. New proofs still verify under old verifiers, which simply do not enforce the check. Left ungated, matching the existing non-empty-Merk-tree requirement and e2168c1. V0 envelopes are deliberately untouched. The gap is broader there (a regular non-empty CountTree's count is forgeable the same way) and V0 documents the child-hash check as V1-only; it is a frozen wire format and Platform no longer accepts V0 proofs. Tests cover all four types plus empty instances, each forgery tried in two shapes: tampering the value bytes inside the honest child-hash node, and downgrading the node back to bare KVValueHash — the latter is what exercises the verifier's new requirement. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughV1 proof generation binds non-Merk tree elements to state roots. V1 verification authenticates lower layers for subset queries. Feature gates separate prover-only indexed-axis APIs from verifier builds. Tests cover forgery, tampering, empty trees, and subset verification. ChangesProof integrity and feature availability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Query
participant V1Verifier
participant MerkProof
participant NonMerkVerifier
Query->>V1Verifier: request tree element subset
V1Verifier->>MerkProof: derive authenticated lower-layer root
MerkProof-->>V1Verifier: return root hash
V1Verifier->>NonMerkVerifier: verify lower proof in root-only mode
NonMerkVerifier-->>V1Verifier: return authenticated state root
V1Verifier-->>Query: return parent tree element
Possibly related PRs
🚥 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 ❌ Your patch status has failed because the patch coverage (85.17%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #782 +/- ##
===========================================
- Coverage 92.14% 92.11% -0.03%
===========================================
Files 254 257 +3
Lines 77692 77936 +244
===========================================
+ Hits 71588 71794 +206
- Misses 6104 6142 +38
🚀 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 (3)
grovedb/src/operations/proof/generate.rs (2)
2647-2668: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the now-unreachable
CommitmentTreearm.The comment at Line 2647 states that
CommitmentTreeis not handled by the empty-tree arm. Line 2667 still listsOk(Element::CommitmentTree(..))in that arm. The new non-Merk child-hash arm at Lines 2355-2361 matches everyCommitmentTreeunder the same!done_with_resultsguard and appears earlier, so Line 2667 can never match. Delete the pattern so the code matches the comment.♻️ Proposed cleanup
| Ok(Element::ProvableCountProvableSumIndexedTree(None, ..)) - | Ok(Element::CommitmentTree(..)) if !done_with_results =>🤖 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/operations/proof/generate.rs` around lines 2647 - 2668, Remove the unreachable Ok(Element::CommitmentTree(..)) pattern from the empty-tree match arm, leaving the earlier non-Merk child-hash handling unchanged and keeping the remaining empty-tree variants intact.
2841-2857: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated Sinsemilla-root read.
Lines 2847-2857 duplicate the identical block in
generate_commitment_tree_layer_proofat Lines 3104-3114. Extract a small helper that takes the storage context and returns the Sinsemilla root. This keeps the two paths from diverging when the frontier encoding changes.🤖 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/operations/proof/generate.rs` around lines 2841 - 2857, Extract the duplicated Sinsemilla-root retrieval and deserialization logic from the CommitmentTree branch and generate_commitment_tree_layer_proof into a shared helper accepting the storage context. Replace both inline blocks with calls to that helper, preserving the existing empty-root fallback for missing data or deserialization errors.grovedb/src/tests/commitment_tree_tests.rs (1)
2742-2860: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a negative case for this exact shape.
The test covers the honest path.
succinctness_gap_test.rscovers lower-layer tampering for a Merk subtree, andproof_coverage_tests.rscovers terminal forgery with no lower layer. The combination exercised here — a forgedCommitmentTreeelement reported through subset verification while a real lower layer is present — is not covered. Add a tamper that swapstotal_countin the proof and assert rejection.🤖 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/commitment_tree_tests.rs` around lines 2742 - 2860, Extend test_commitment_tree_element_count_subset_query_against_note_fetch_proof with a negative case that tampers with the proof’s CommitmentTree element by replacing total_count while preserving the lower-layer data. Run GroveDb::verify_subset_query with the existing count_query and assert verification rejects the modified proof, covering forged terminal values when a real lower layer is present.
🤖 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/operations/proof/generate.rs`:
- Around line 2647-2668: Remove the unreachable Ok(Element::CommitmentTree(..))
pattern from the empty-tree match arm, leaving the earlier non-Merk child-hash
handling unchanged and keeping the remaining empty-tree variants intact.
- Around line 2841-2857: Extract the duplicated Sinsemilla-root retrieval and
deserialization logic from the CommitmentTree branch and
generate_commitment_tree_layer_proof into a shared helper accepting the storage
context. Replace both inline blocks with calls to that helper, preserving the
existing empty-root fallback for missing data or deserialization errors.
In `@grovedb/src/tests/commitment_tree_tests.rs`:
- Around line 2742-2860: Extend
test_commitment_tree_element_count_subset_query_against_note_fetch_proof with a
negative case that tampers with the proof’s CommitmentTree element by replacing
total_count while preserving the lower-layer data. Run
GroveDb::verify_subset_query with the existing count_query and assert
verification rejects the modified proof, covering forged terminal values when a
real lower layer is present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 212fcbd5-840a-4262-88e8-0ed144106254
📒 Files selected for processing (8)
grovedb/src/operations/proof/generate.rsgrovedb/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/proof_coverage_tests.rsgrovedb/src/tests/succinctness_gap_test.rs
The binding fix in the previous commit was applied unconditionally, which is
wrong: GROVE_V3 is live, and per v4.rs a fix that changes an accepted/rejected
outcome or a tracked cost cannot land on a released version without nodes
carrying it diverging from nodes that do not. This change does both. An
upgraded verifier rejects proofs a released one accepts, and deriving the
tree's state root costs the prover storage reads and hash calls that V1..V3
never paid — and cost feeds fees.
Adds `proof.terminal_non_merk_tree_child_hash`, 0 in V1..V3 and 1 in V4, and
branches both sides on it:
- Prover: under v0 the node is left exactly as it has always been emitted
(bare KVValueHash) and only the limit moves, so the released byte and cost
shape is untouched. Under v1 it computes the state root and rewrites the
node.
- Verifier: non-empty *Merk* trees keep requiring the child hash at every
version — that has been released behaviour since V3 and is unchanged.
Only the four non-Merk types are added, and only from V4, so a V4 verifier
never rejects an honest V3 proof and a V3 verifier never demands a node a
V3 prover does not emit.
The consequence worth being explicit about: the forgery stays exploitable on
V1..V3 and closes when protocol v4 activates. That is inherent to gating —
fixing it in place is the divergence v4.rs exists to prevent — and it matches
how the other fixes parked on V4 are being handled.
`terminal_non_merk_tree_child_hash_version_gate` pins both sides: it asserts
GROVE_V3 still emits a bare KVValueHash and still accepts the forged
total_count, and that GROVE_V4 emits the child-hash node and rejects it. The
V3 assertion is deliberately an assertion about a hole — if it starts failing,
the fix has leaked into a released version.
Also documents the gate in v4.rs's header alongside the two existing ones, as
that file asks.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
grovedb/src/tests/proof_coverage_tests.rs (1)
8766-8784: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAlso exercise
DowngradeToKvValueHashon the empty-tree forgery.This test only exercises
TerminalForgery::KeepChildHash. The empty-tree path is where a downgraded node matters most, because the committed child hash is a constant (NULL_HASH, orEMPTY_COMMITMENT_TREE_STATE_ROOTfor aCommitmentTree). A forger who drops the child hash entirely is then caught only by the verifier'schild_hash_verifiedrequirement, which is a different code path from thecombine_hashcheck. Loop over both modes so the empty-tree case pins both defenses.♻️ Proposed change to cover both forgery modes
// And a forgery on the empty tree is still caught. let fake_element_bytes = Element::empty_mmr_tree() .serialize(grove_version) .expect("serialize"); if fake_element_bytes != results[0].value { - let tampered = forge_terminal_tree_element( - &proof_bytes, - key, - &fake_element_bytes, - TerminalForgery::KeepChildHash, - ); - assert!( - GroveDb::verify_query_raw(&tampered, &path_query, grove_version).is_err(), - "type swap on empty {} must be rejected", - String::from_utf8_lossy(key) - ); + for forgery in [ + TerminalForgery::KeepChildHash, + TerminalForgery::DowngradeToKvValueHash, + ] { + let tampered = + forge_terminal_tree_element(&proof_bytes, key, &fake_element_bytes, forgery); + assert!( + GroveDb::verify_query_raw(&tampered, &path_query, grove_version).is_err(), + "type swap on empty {} must be rejected", + String::from_utf8_lossy(key) + ); + } }🤖 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/proof_coverage_tests.rs` around lines 8766 - 8784, Extend the empty-tree forgery test around forge_terminal_tree_element to exercise both TerminalForgery::KeepChildHash and TerminalForgery::DowngradeToKvValueHash. Iterate over both modes while preserving the existing conditional fake-element comparison and verification assertion, so the empty-tree case covers both combine_hash and child_hash_verified defenses.
🤖 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/proof_coverage_tests.rs`:
- Around line 8766-8784: Extend the empty-tree forgery test around
forge_terminal_tree_element to exercise both TerminalForgery::KeepChildHash and
TerminalForgery::DowngradeToKvValueHash. Iterate over both modes while
preserving the existing conditional fake-element comparison and verification
assertion, so the empty-tree case covers both combine_hash and
child_hash_verified defenses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ad9fe3fd-818d-4888-9690-d12c2fe8f883
📒 Files selected for processing (8)
grovedb-version/src/version/grovedb_versions.rsgrovedb-version/src/version/v1.rsgrovedb-version/src/version/v2.rsgrovedb-version/src/version/v3.rsgrovedb-version/src/version/v4.rsgrovedb/src/operations/proof/generate.rsgrovedb/src/operations/proof/verify.rsgrovedb/src/tests/proof_coverage_tests.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- grovedb/src/operations/proof/verify.rs
- grovedb/src/operations/proof/generate.rs
…ned function
The gate added in the previous commit was an inline
`if ...terminal_non_merk_tree_child_hash >= 1` branch inside
`prove_subqueries_v1`. That does not follow the versioning system's code
structure: every other gated behaviour in the repo is a module directory with
one file per version and a dispatching `mod.rs`, so each version's behaviour
reads on its own instead of being reconstructed from a conditional.
Restructures it into
`operations/proof/bind_terminal_non_merk_tree/{mod,v0,v1}.rs`, following
`operations/insert/add_element_on_transaction/`:
- `mod.rs` matches the slot and dispatches, with an `UnknownVersionMismatch`
arm, and carries the docs on what differs between versions.
- `v0.rs` is the released no-op. It takes the same arguments and ignores
them, and documents that the unbound element bytes are a known gap
preserved because GROVE_V3 is live — not an oversight.
- `v1.rs` holds the state-root derivation, the self-check and the node
rewrite. `non_merk_tree_child_hash` moves here from `generate.rs`, since
only v1 needs it.
The gated unit is the binding step rather than the whole enclosing function,
which keeps the per-version files small — duplicating the ~1000-line
`prove_subqueries_v1` would not have been reasonable. `generate.rs` now just
calls the dispatcher. Passing `&mut Node` and deriving key/value inside also
resolves the borrow that forced the old code to clone them up front.
Behaviour is unchanged: `terminal_non_merk_tree_child_hash_version_gate` still
pins GROVE_V3 to the bare KVValueHash (forgery accepted) and GROVE_V4 to the
child-hash node (forgery rejected). The module is `minimal`-gated, so
verify-only builds are unaffected.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nd hot path Two cleanups to `bind_terminal_non_merk_tree`, both on a path that runs per terminal non-Merk tree while serving a latency-sensitive proof. `grove_version` was threaded into v0 and v1 and used by neither — the version is consumed by the dispatch in `mod.rs`, which is the point of the split. Dropped from both implementations and from the call sites. The self-check then went from an unconditional runtime error to a `debug_assert`. It was costing two blake3 hashes on every terminal non-Merk tree: `H(value)` plus a `combine_hash`, purely to re-derive a value_hash the node already carries. That is small next to the storage reads `non_merk_tree_child_hash` does in the same function, but it bought nothing in production — it can fire only on a prover bug or corrupted storage, never on attacker input, and every arm of the derivation is pinned by tests across all four types, empty and populated. If it ever did fire in release the verifier would reject the proof anyway; the check only made the diagnosis nicer. The common path now does no hashing at all: the `value_hash` the node carries is the one the parent committed, so it is reused as-is. Deriving it is confined to node shapes that carry none (`KV` / `KVCount` / `KVSum` / `KVCountSum`), which trees are not proved with in practice. The debug block is deliberately uncosted (`.unwrap()`, not `unwrap_add_cost`) so `OperationCost` stays identical between debug and release builds — a cost that varied by build profile would be far worse than the two hashes. Net effect on tracked cost: two fewer `hash_node_calls` per terminal non-Merk tree than the previous commit charged. That only moves V4 numbers, which are unreleased, so nothing shifts for V1..V3. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
QuantumExplorer
commented
Aug 2, 2026
QuantumExplorer
left a comment
QuantumExplorer
left a comment
Member
Author
There was a problem hiding this comment.
This should be fine, as proof generation is not in consensus.
QuantumExplorer
added a commit
to dashpay/platform
that referenced
this pull request
Aug 2, 2026
Pulls in the terminal non-Merk proof-binding fix (dashpay/grovedb#782): CommitmentTree / MmrTree / BulkAppendTree / DenseTree elements reported as a query's final result are now bound to the parent value_hash, so a malicious node can no longer serve forged aggregates (e.g. a wrong shielded note count) under a genuine root hash. 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.
Issue being fixed or feature implemented
A V1 proof that reported an
Element::CommitmentTree/MmrTree/BulkAppendTree/DenseAppendOnlyFixedSizeTreeas a terminal result — the query targets the tree element itself and the prover emits no lower layer — left the serialized element bytes unbound to thevalue_hashits parent Merk commits to.Both existing binding mechanisms skipped these four types:
combine_hash(H(value), NULL_HASH)check is gated on!is_non_empty_tree(), butis_non_empty_tree()returnstrueunconditionally for them;child_hash_verifiedrequirement was gated onis_non_empty_merk_tree(), which by construction excludes them.Meanwhile the prover only rewrote regular non-empty Merk trees to
KVValueHashFeatureTypeWithChildHash, leaving these emitting a bareKVValueHash— a node that hashes only(key, value_hash), so the value bytes never enter the node hash.Impact. A prover could serve forged element bytes (an inflated or deflated
CommitmentTreetotal_count, a different MMR size) alongside the genuinevalue_hashand still reconstruct the correct root hash. Dash Platform'sGetShieldedNotesCountverifier readstotal_countfrom exactly such a terminal element, so a malicious node could misreport a wallet's shielded sync denominator.This is pre-existing, not a regression — it reproduces identically at tag
v5.0.1. It is also not what the recent subset-verification fix (e2168c1) addressed: that path has a lower layer available and does bind viacombine_hash(H(value), child_root). This gap is only on the no-lower-layer terminal path.What was done?
Worth stating up front, because it rules out the obvious shortcut: this could not be fixed verifier-side by asserting
hash == value_hash(value_bytes). These types are written throughinsert_subtree→KV::new_with_layered_value_hash, so the parent commitscombine_hash(H(value), state_root), not plainH(value)— and the state root is not derivable from the element bytes (aCommitmentTree's state root depends on the actual notes, not ontotal_count). The child hash has to travel in the proof.No new proof format is needed.
KVValueHashFeatureTypeWithChildHash(tag0x1c) already verifiescombine_hash(H(value), child_hash) == value_hash, which is exactly the composition these types commit — the prover simply was not using it here.Prover (
operations/proof/generate.rs+operations/proof/bind_terminal_non_merk_tree/): the terminal arm now covers all four types and rewrites the node to carry the tree's state root, computed by the newnon_merk_tree_child_hash.CommitmentTreemoved out of the empty-trees arm, since it is bound whether or not it holds notes. Each arm mirrors its write path exactly:MmrTree/BulkAppendTree/DenseAppendOnlyFixedSizeTreeare inserted withNULL_HASHwhile still empty. Note an emptyBulkAppendTree'scompute_current_state_root()is notNULL_HASH, so the zero-count case short-circuits.CommitmentTreeneeds no special case: the sinsemilla/bulk composition already yieldsEMPTY_COMMITMENT_TREE_STATE_ROOTat count 0.A self-check fails loudly if a recomputed root does not reproduce the committed value hash, so any future convention drift surfaces as a clear prover error instead of a proof that cannot verify.
Verifier (
operations/proof/verify.rs): the child-hash requirement widens fromis_non_empty_merk_tree()tois_non_empty_tree(), which adds exactly these four types.V0 envelopes are deliberately untouched. The gap is broader there (a regular non-empty
CountTree's count is forgeable the same way) and V0 documents the child-hash check as V1-only. It is a frozen wire format and Platform no longer accepts V0 proofs.How Has This Been Tested?
Five new tests in
grovedb/src/tests/proof_coverage_tests.rs. Each forgery is tried in two shapes: tampering the value bytes inside the honest child-hash node, and downgrading the node back to bareKVValueHash— the latter is what actually exercises the verifier's new requirement, since it is the shape the gap allowed.terminal_commitment_tree_count_forgery_is_detectedterminal_mmr_tree_size_forgery_is_detectedterminal_bulk_append_and_dense_tree_forgeries_are_detectedempty_non_merk_trees_still_prove_and_verify— empty instances of all four must still prove and verifyterminal_non_merk_tree_child_hash_version_gate— pins both sides of the gate:GROVE_V3still emits a bareKVValueHashand still accepts the forgedtotal_count;GROVE_V4emits the child-hash node and rejects it. The V3 assertion is deliberately an assertion about a hole — if it starts failing, the fix has leaked into a released version.Before the fix, the CommitmentTree test failed with
verification accepted it: Ok([Ok(commitment_tree: count: 999 chunk_power: 10)])against a real count of 3.cargo test -p grovedb --lib— 2540 passed, 0 failedcargo test -p grovedb-version --lib— 47 passed, including the*_unchanged_fields_remain_zeroguardscargo clippy --workspace --all-features -- -D warnings— cleancargo fmt --all— applied,--checkcleancargo check -p grovedb --no-default-features --features verify— the verifier change compiles in verify-only buildstest_commitment_tree_element_count_subset_query_against_note_fetch_proof,test_subset_mode_still_binds_element_bytes_to_lower_layer) still passBreaking Changes
None on released versions — the fix is gated behind
GROVE_V4.GROVE_V3is live, and perv4.rsa fix that changes an accepted/rejected outcome or a tracked cost cannot land on a released version without diverging nodes. This change does both: an upgraded verifier rejects proofs a released one accepts, and deriving the state root costs the prover extra storage reads and hash calls, which feeds fees. So it lands as a new slot,proof.terminal_non_merk_tree_child_hash—0in V1..V3,1in V4 — with both prover and verifier branching on it.The prover side follows the versioning system's code structure — a module directory with one file per version and a dispatching
mod.rs, modelled onoperations/insert/add_element_on_transaction/:The gated unit is the binding step rather than the whole enclosing function, so the per-version files stay small — duplicating the ~1000-line
prove_subqueries_v1would not have been reasonable.generate.rsjust calls the dispatcher, and the module isminimal-gated so verify-only builds are unaffected.KVValueHash), only the limit moves. Released byte and cost shape untouched.A V4 verifier therefore never rejects an honest V3 proof, and a V3 verifier never demands a node a V3 prover does not emit.
Worth being explicit: the forgery stays exploitable on V1..V3 and closes when protocol v4 activates. That is inherent to gating — fixing it in place is precisely the divergence
v4.rsexists to prevent — and it matches how the other fixes parked on V4 are being handled.Note for reviewers
This branch was cut from local work that is not yet on
develop, so the PR carries two pre-existing commits that are not part of this change:82726a0feat(grovedb): expose indexed-axis proof verification to verify-only buildse2168c1fix(grovedb): bind, don't reject, a lower layer with no query below itOnly the third commit (
ff77fc0) is this change. If those two are landing through their own PR, this one should be rebased once they do.Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code