fix: V0 proof backwards compatibility for KVValueHash item check#643
Conversation
…mpatibility The security check from da308f7 (PR #553) that rejects item elements in KVValueHash nodes is now gated on the proof version. V0 proofs pass `allow_items_in_value_hash_nodes = true` to skip the check, maintaining backwards compatibility with pre-existing proof fixtures. V1 proofs continue to enforce the check. This fixes 11 dash-sdk test failures caused by stale proof fixtures generated before the security fix was introduced. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughPropagates a flag/version through proof verification paths to control whether items inside value-hash nodes are allowed: grovedb passes a boolean for V0 (allow) vs V1 (reject), while merk's query verifier gained a proof_version parameter (0 vs 1) and tests updated to pass the new argument. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #643 +/- ##
===========================================
- Coverage 90.83% 90.83% -0.01%
===========================================
Files 182 182
Lines 51895 51911 +16
===========================================
+ Hits 47138 47151 +13
- Misses 4757 4760 +3
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
merk/src/proofs/query/verify.rs (1)
349-362: Extract the KVValueHash item-validation into one helper.The same element-type check now exists in three node arms. Pulling it into a shared helper will make future rule changes much less likely to drift across
KVValueHash*variants.Also applies to: 399-414, 435-452
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@merk/src/proofs/query/verify.rs` around lines 349 - 362, Extract the duplicated element-type check into a single helper (e.g., validate_kv_value_hash_no_item) that takes the serialized value and allow_items_in_value_hash_nodes and returns Result<(), Error>; inside the helper call ElementType::from_serialized_value(value).map_err(...) and if !allow_items_in_value_hash_nodes && element_type.has_simple_value_hash() return the existing Error::InvalidProofError message. Replace the three duplicated blocks in the KVValueHash* match arms with a call to this helper and propagate the Result (using ?), keeping the original error messages (including "cannot determine element type in KVValueHash node" and "KVValueHash node must not contain an item element") and preserving behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@merk/src/proofs/query/verify.rs`:
- Around line 84-90: The trait method execute_proof currently changes the public
signature by adding a bare bool (left_to_right /
allow_items_in_value_hash_nodes); revert the trait to preserve the original
public signature and introduce a non-breaking opt-in API: add either an options
enum/struct (e.g., ExecuteProofOptions) or a new method (e.g.,
execute_proof_with_options or execute_proof_v1) that takes the extra flags,
update implementations to implement the new method while providing a default
execute_proof that delegates to the new method with V0-compatible defaults, and
ensure the trait docs note the security-sensitive flags so callers must opt into
them explicitly.
---
Nitpick comments:
In `@merk/src/proofs/query/verify.rs`:
- Around line 349-362: Extract the duplicated element-type check into a single
helper (e.g., validate_kv_value_hash_no_item) that takes the serialized value
and allow_items_in_value_hash_nodes and returns Result<(), Error>; inside the
helper call ElementType::from_serialized_value(value).map_err(...) and if
!allow_items_in_value_hash_nodes && element_type.has_simple_value_hash() return
the existing Error::InvalidProofError message. Replace the three duplicated
blocks in the KVValueHash* match arms with a call to this helper and propagate
the Result (using ?), keeping the original error messages (including "cannot
determine element type in KVValueHash node" and "KVValueHash node must not
contain an item element") and preserving behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 09dd49ee-0eb8-4633-8e9a-c324472a59f1
📒 Files selected for processing (3)
grovedb/src/operations/proof/verify.rsmerk/src/proofs/query/merk_integration_tests.rsmerk/src/proofs/query/verify.rs
| fn execute_proof( | ||
| &self, | ||
| bytes: &[u8], | ||
| limit: Option<u16>, | ||
| left_to_right: bool, | ||
| allow_items_in_value_hash_nodes: bool, | ||
| ) -> CostResult<(MerkHash, ProofVerificationResult), Error>; |
There was a problem hiding this comment.
Avoid making execute_proof a breaking public API with a bare bool.
This changes a public trait method’s signature, so every downstream caller now has to update call sites, and the new last argument is security-sensitive but opaque (..., true, false). Please preserve the old signature and add an opt-in variant or options/enum-based API for the V0 compatibility path instead.
Also applies to: 114-120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@merk/src/proofs/query/verify.rs` around lines 84 - 90, The trait method
execute_proof currently changes the public signature by adding a bare bool
(left_to_right / allow_items_in_value_hash_nodes); revert the trait to preserve
the original public signature and introduce a non-breaking opt-in API: add
either an options enum/struct (e.g., ExecuteProofOptions) or a new method (e.g.,
execute_proof_with_options or execute_proof_v1) that takes the extra flags,
update implementations to implement the new method while providing a default
execute_proof that delegates to the new method with V0-compatible defaults, and
ensure the trait docs note the security-sensitive flags so callers must opt into
them explicitly.
There was a problem hiding this comment.
This is Claude. Disagree — execute_proof is an internal trait (QueryProofVerify) that lives in the grovedb-merk crate. It is not part of any public SDK API. The only callers are:
- 3 sites inside
grovedb/src/operations/proof/verify.rs(GroveDB's own proof verification) - 5 sites in
merk/src/proofs/query/merk_integration_tests.rs(tests) - 1 site in
verify_proofwhich delegates internally
There are no downstream/external consumers of this trait. Adding an options struct or a second method for a single boolean that only has 2 internal callers passing true would be over-engineering. The parameter is well-documented in the trait doc comment explaining exactly when to use true vs false.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if QueryProofVerify is re-exported from lib.rs or any public module
rg -n "QueryProofVerify" --type rust
# Check pub use in lib.rs files
rg -n "pub use" merk/src/lib.rs 2>/dev/null || true
rg -n "pub use" merk/src/proofs/mod.rs 2>/dev/null || true
rg -n "pub use" merk/src/proofs/query/mod.rs 2>/dev/null || true
Unrecoverable error: Failed to clone repository
@QuantumExplorer, that's a fair point. The method already accepted a bare bool for left_to_right, so the new allow_items_in_value_hash_nodes is consistent with the existing style, and with only ~9 internal call sites (no external consumers), introducing an options struct would genuinely be over-engineering. The trait doc comment covers the security intent clearly. I'll withdraw the concern.
✏️ Learnings added
Learnt from: QuantumExplorer
Repo: dashpay/grovedb PR: 643
File: merk/src/proofs/query/verify.rs:84-90
Timestamp: 2026-03-10T10:18:18.519Z
Learning: In the `grovedb-merk` crate, `QueryProofVerify::execute_proof` in `merk/src/proofs/query/verify.rs` is an internal trait with no downstream/external consumers. All callers are within `grovedb/src/operations/proof/verify.rs` (3 sites), `merk/src/proofs/query/merk_integration_tests.rs` (5 sites), and the internal `verify_proof` delegation. Adding options structs or extra methods for internal-only boolean flags on this trait is considered over-engineering.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
The test_verify_query_get_parent_tree_info test used a hardcoded V0 proof with items in KVValueHash nodes. Now that V0 proofs allow this for backwards compatibility, the test should expect success. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Change the `allow_items_in_value_hash_nodes: bool` parameter to `proof_version: u16` for clearer semantics: 0 = V0 lenient mode, ≥1 = strict mode that rejects items in value hash nodes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
merk/src/proofs/query/verify.rs (1)
350-363: Factor the V1 item-node rejection into one helper.The same
proof_version >= 1/ElementType::from_serialized_value/has_simple_value_hashblock now lives in three match arms. Centralizing it would reduce drift if the V1+ rules change again.♻️ Suggested cleanup
+fn reject_item_in_value_hash_node( + proof_version: u16, + value: &[u8], + node_name: &str, +) -> Result<(), Error> { + if proof_version >= 1 { + let element_type = ElementType::from_serialized_value(value).map_err(|e| { + Error::InvalidProofError(format!( + "cannot determine element type in {node_name} node: {e}" + )) + })?; + if element_type.has_simple_value_hash() { + return Err(Error::InvalidProofError(format!( + "{node_name} node must not contain an item element" + ))); + } + } + Ok(()) +} + Node::KVValueHash(key, value, value_hash) => { + reject_item_in_value_hash_node(proof_version, value, "KVValueHash")?; execute_node(key, Some(value), *value_hash, false)?; }Also applies to: 400-415, 436-453
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@merk/src/proofs/query/verify.rs` around lines 350 - 363, Duplicate logic checking proof_version >= 1, calling ElementType::from_serialized_value(value) and verifying element_type.has_simple_value_hash() is repeated across multiple match arms (KVValueHash and two others); extract this into a small helper function (e.g., fn reject_v1_item_node_if_needed(proof_version: u8, value: &[u8]) -> Result<(), Error>) that performs the ElementType::from_serialized_value(...) mapping and returns Err(Error::InvalidProofError(...)) when has_simple_value_hash() is true, then replace the three inline blocks with calls to that helper from the match arms (referencing proof_version, value, ElementType and has_simple_value_hash in the implementation).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@merk/src/proofs/query/verify.rs`:
- Around line 350-363: Duplicate logic checking proof_version >= 1, calling
ElementType::from_serialized_value(value) and verifying
element_type.has_simple_value_hash() is repeated across multiple match arms
(KVValueHash and two others); extract this into a small helper function (e.g.,
fn reject_v1_item_node_if_needed(proof_version: u8, value: &[u8]) -> Result<(),
Error>) that performs the ElementType::from_serialized_value(...) mapping and
returns Err(Error::InvalidProofError(...)) when has_simple_value_hash() is true,
then replace the three inline blocks with calls to that helper from the match
arms (referencing proof_version, value, ElementType and has_simple_value_hash in
the implementation).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e14d33b-d7f0-4118-b411-c9480605c9cb
📒 Files selected for processing (3)
grovedb/src/operations/proof/verify.rsmerk/src/proofs/query/merk_integration_tests.rsmerk/src/proofs/query/verify.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- grovedb/src/operations/proof/verify.rs
Replace magic number `1` with `PROOF_VERSION_LATEST` in all test and production code calling execute_proof. The constant is defined in merk::proofs::query::verify and re-exported from the query module. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
da308f7(PR fix: prevent KV-to-KVValueHash proof value forgery attack (C1) #553) that rejects item elements inKVValueHash/KVValueHashFeatureType/KVValueHashFeatureTypeWithChildHashproof nodes is now gated on proof versionallow_items_in_value_hash_nodes = true) for backwards compatibility with existing proof fixturesfalse) — the forgery protection remains activeProblem
11 dash-sdk tests fail with:
These tests use pre-recorded V0 proof fixtures generated before PR #553 added the security check. The proofs are valid but use the old format where items could appear in KVValueHash nodes.
Approach
Added
allow_items_in_value_hash_nodes: boolparameter toexecute_proofin theQueryProofVerifytrait:verify_layer_proof(V0 path)trueverify_trunk_chunk_proof_v0trueverify_layer_proof_v1(V1 path)falseverify_proof(merk-level)falsefalseThe
verify_proofmethod (71 test callers) is unchanged — it delegates toexecute_proofwithfalse.Test plan
cargo clippy -- -D warningsclean🤖 Generated with Claude Code
Summary by CodeRabbit