fix: V1 trunk proofs + count==0 hash verification bypass - #646
Merged
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #646 +/- ##
===========================================
- Coverage 90.83% 90.69% -0.14%
===========================================
Files 182 182
Lines 51909 52193 +284
===========================================
+ Hits 47150 47339 +189
- Misses 4759 4854 +95
🚀 New features to boost your workflow:
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTrunk-chunk proof generation and verification were versioned: generators and verifiers now dispatch between V0 and V1. GROVE_V3 flags for proving trunk chunks were enabled. V1 uses ProofBytes::Merk and enforces a stricter bottom-up combine_hash validation across layers; tests updated to exercise V1 semantics. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Generator as Proof Generator
participant Dispatcher as Version Dispatcher
participant ImplV0 as V0 Impl\n(MerkOnlyLayerProof)
participant ImplV1 as V1 Impl\n(LayerProof + ProofBytes::Merk)
participant Verifier as Proof Verifier
participant Validator as Proof Variant Dispatcher
Client->>Generator: Request trunk-chunk proof
Generator->>Dispatcher: check grove_version
alt version == 0
Dispatcher->>ImplV0: Build MerkOnlyLayerProof(s)
ImplV0-->>Generator: V0 proof
else version == 1
Dispatcher->>ImplV1: Build LayerProof(s) with ProofBytes::Merk
ImplV1-->>Generator: V1 proof
else unknown
Dispatcher-->>Generator: VersionError [0,1]
end
Generator-->>Client: Return proof
Client->>Verifier: Submit proof for verification
Verifier->>Validator: Inspect GroveDBProof variant
alt GroveDBProof::V0
Validator->>ImplV0: Verify V0 proof (existing flow)
ImplV0-->>Verifier: result
else GroveDBProof::V1
Validator->>ImplV1: Verify V1 proof
ImplV1->>ImplV1: Traverse layers bottom-up, validate combine_hash chain and layer hashes
ImplV1-->>Verifier: GroveTrunkQueryResult / error
end
Verifier-->>Client: Verification outcome
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
grovedb/src/operations/proof/verify.rs (1)
1940-1946:⚠️ Potential issue | 🔴 CriticalThe vulnerable V0 trunk verifier is still reachable from the public API.
verify_trunk_chunk_proofstill acceptsGroveDBProof::V0and routes it toverify_trunk_chunk_proof_v0. The new regression ingrovedb/src/tests/trunk_proof_tests.rs:776-833shows that path still accepts the forgedcount == 0proof, so an attacker can bypass the fix by sending a V0-encoded trunk proof. Either hard-reject V0 trunk proofs here or port the sameNULL_HASH/always-runcombine_hashvalidation into the V0 verifier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/operations/proof/verify.rs` around lines 1940 - 1946, The public verifier still accepts GroveDBProof::V0 and routes to verify_trunk_chunk_proof_v0, leaving the vulnerable V0 path available; update verify_trunk_chunk_proof so that when grovedb_proof is GroveDBProof::V0 it immediately returns a validation error (hard-reject V0 trunk proofs) instead of calling verify_trunk_chunk_proof_v0, or alternatively port the V1 safeguards into verify_trunk_chunk_proof_v0 by enforcing the NULL_HASH check and always-running combine_hash validation (matching the logic in verify_trunk_chunk_proof_v1); reference the functions verify_trunk_chunk_proof, verify_trunk_chunk_proof_v0, verify_trunk_chunk_proof_v1 and the NULL_HASH/combine_hash validation when making the change.
🧹 Nitpick comments (2)
grovedb/src/operations/proof/generate.rs (1)
784-789: Add trunk-proof context to the new V1trunk_queryerror path.This branch currently forwards the raw Merk error, so failures in the new V1 generator lose the operation context that will matter during debugging. Please wrap it with a message that names trunk proof generation before returning it.
As per coding guidelines, "Wrap errors with context using
.map_err(|e| Error::CorruptedData(format!("context: {}", e)))pattern in Rust source files"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/operations/proof/generate.rs` around lines 784 - 789, The trunk-proof path currently maps Merk errors directly when calling target_tree.trunk_query inside the cost_return_on_error! for trunk_result; change the map_err to wrap the error with context before converting to Error::MerkError (e.g., use .map_err(|e| Error::CorruptedData(format!("trunk proof generation: {}", e))).map_err(Error::MerkError) or equivalent) so the returned error names trunk proof generation and preserves the original error details; update the trunk_result expression to apply this contextual map_err to target_tree.trunk_query(query.max_depth, query.min_depth, grove_version).grovedb/src/tests/trunk_proof_tests.rs (1)
153-170: Pin proof-format assertions to a fixed Grove version, notlatest().This test now requires
GroveDBProof::V1, but it still builds the proof withGroveVersion::latest()above. The next protocol bump will make this fail even if trunk proofs legitimately move forward again. Use the concrete version that is supposed to emit V1 here, and keeplatest()coverage in a separate smoke test. The same pattern repeats in the other V1-only tests added below.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/tests/trunk_proof_tests.rs` around lines 153 - 170, The test is asserting GroveDBProof::V1 (pattern matching decoded_proof into proof_v1) but earlier constructs the proof using GroveVersion::latest(), which will break when the default advances; change the proof construction in this test (and the other V1-only tests nearby) to use the concrete GroveVersion constant that is known to emit V1 (the specific version your project expects) instead of GroveVersion::latest(), so decoded_proof will reliably be a V1; keep a separate smoke test that exercises GroveVersion::latest() behavior. Ensure you update any references around decoded_proof/proof_v1 and the lower_layers lookup (count_sum_tree, merk_proof) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@grovedb/src/operations/proof/verify.rs`:
- Around line 1940-1946: The public verifier still accepts GroveDBProof::V0 and
routes to verify_trunk_chunk_proof_v0, leaving the vulnerable V0 path available;
update verify_trunk_chunk_proof so that when grovedb_proof is GroveDBProof::V0
it immediately returns a validation error (hard-reject V0 trunk proofs) instead
of calling verify_trunk_chunk_proof_v0, or alternatively port the V1 safeguards
into verify_trunk_chunk_proof_v0 by enforcing the NULL_HASH check and
always-running combine_hash validation (matching the logic in
verify_trunk_chunk_proof_v1); reference the functions verify_trunk_chunk_proof,
verify_trunk_chunk_proof_v0, verify_trunk_chunk_proof_v1 and the
NULL_HASH/combine_hash validation when making the change.
---
Nitpick comments:
In `@grovedb/src/operations/proof/generate.rs`:
- Around line 784-789: The trunk-proof path currently maps Merk errors directly
when calling target_tree.trunk_query inside the cost_return_on_error! for
trunk_result; change the map_err to wrap the error with context before
converting to Error::MerkError (e.g., use .map_err(|e|
Error::CorruptedData(format!("trunk proof generation: {}",
e))).map_err(Error::MerkError) or equivalent) so the returned error names trunk
proof generation and preserves the original error details; update the
trunk_result expression to apply this contextual map_err to
target_tree.trunk_query(query.max_depth, query.min_depth, grove_version).
In `@grovedb/src/tests/trunk_proof_tests.rs`:
- Around line 153-170: The test is asserting GroveDBProof::V1 (pattern matching
decoded_proof into proof_v1) but earlier constructs the proof using
GroveVersion::latest(), which will break when the default advances; change the
proof construction in this test (and the other V1-only tests nearby) to use the
concrete GroveVersion constant that is known to emit V1 (the specific version
your project expects) instead of GroveVersion::latest(), so decoded_proof will
reliably be a V1; keep a separate smoke test that exercises
GroveVersion::latest() behavior. Ensure you update any references around
decoded_proof/proof_v1 and the lower_layers lookup (count_sum_tree, merk_proof)
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a5a1409-5141-4128-94d7-5971c66ffc13
📒 Files selected for processing (4)
grovedb-version/src/version/v3.rsgrovedb/src/operations/proof/generate.rsgrovedb/src/operations/proof/verify.rsgrovedb/src/tests/trunk_proof_tests.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
grovedb/src/tests/trunk_proof_tests.rs (1)
596-597: Pin the V1 assertions to an explicit GroveVersion.These cases are validating V1-specific wire-format behavior, so coupling them to
GroveVersion::latest()will make the file fail as soon aslatest()moves past V1. Please use the concrete version constant that introduced trunk-proof V1 instead of a floating latest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/tests/trunk_proof_tests.rs` around lines 596 - 597, The test currently uses a floating GroveVersion::latest() which will break once latest() advances; replace the dynamic call with the explicit V1 constant used for trunk-proof V1 (e.g., assign grove_version = GroveVersion::V1) so the V1-specific wire-format assertions remain pinned; update the line that sets grove_version (near make_empty_grovedb()) to use the concrete GroveVersion V1 constant rather than GroveVersion::latest().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@grovedb/src/tests/trunk_proof_tests.rs`:
- Around line 1357-1379: The test test_trunk_proof_v1_rejects_empty_path
currently allows either "not a count tree" or "empty path" which is too loose;
change the assertion after calling GroveDb::verify_trunk_chunk_proof
(constructed with PathTrunkChunkQuery::new(vec![], 3)) to require the error
specifically indicates the empty-path rejection (e.g., assert the error string
contains "empty path" or, better, match the specific error variant returned by
verify_trunk_chunk_proof) so only the empty-path branch is considered correct.
---
Nitpick comments:
In `@grovedb/src/tests/trunk_proof_tests.rs`:
- Around line 596-597: The test currently uses a floating GroveVersion::latest()
which will break once latest() advances; replace the dynamic call with the
explicit V1 constant used for trunk-proof V1 (e.g., assign grove_version =
GroveVersion::V1) so the V1-specific wire-format assertions remain pinned;
update the line that sets grove_version (near make_empty_grovedb()) to use the
concrete GroveVersion V1 constant rather than GroveVersion::latest().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9bb693ef-dd21-4f46-8982-ca45f46be1a2
📒 Files selected for processing (1)
grovedb/src/tests/trunk_proof_tests.rs
…pass V0 trunk proof verification has a security flaw: when count==0, the verifier returns early without running the combine_hash(value_hash, lower_hash) check. Since merk KVValueHash nodes store value_hash separately from value bytes, an attacker can replace element bytes (setting count to 0) while keeping the original value_hash, making the verifier report an empty tree for a non-empty one. V1 trunk proofs fix this by always running the combine_hash verification chain, using NULL_HASH as the lower hash for empty trees. Forged element bytes produce a different value_hash, so the combined hash won't match. Changes: - Bump prove_trunk_chunk version to 1 in v3 - Add V1 trunk proof generation using LayerProof/ProofBytes::Merk - Add V1 trunk proof verification with mandatory combine_hash check - Add security regression test proving V1 rejects and V0 accepts the forged count==0 attack Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Multi-level path (2 layers): exercises combine_hash loop across layers - V1 trailing bytes rejection: error path for corrupted target layer - Non-count-tree target: error path when element has no count - Empty tree multi-level: count==0 with combine_hash across 2 layers - Different count tree types: CountTree and ProvableCountSumTree - Forged count on multi-level path: security test with deeper nesting Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
7 new tests covering previously untested error branches in verify_trunk_chunk_proof_v1: - Non-Merk ProofBytes at path layer - Corrupted merk proof bytes at path layer - Wrong path segment (not found in proof result) - Missing lower layer for path segment - Non-Merk ProofBytes at target layer - Empty path (no root hash computed) - Invalid target proof ops (decode OK, execute fails) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
QuantumExplorer
force-pushed
the
fix/trunk-proof-count-zero-bypass
branch
from
March 11, 2026 03:14
b4c3de1 to
b58e300
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
grovedb/src/tests/trunk_proof_tests.rs (1)
1372-1379:⚠️ Potential issue | 🟡 MinorTighten this to the specific empty-path failure.
As written, this still passes if verification fails for some unrelated reason, so it doesn't really prove the empty-path branch. Either make
verify_trunk_chunk_proofreject empty paths explicitly and assert that exact error here, or assert the exact current failure mode instead ofnot a count tree || empty path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/tests/trunk_proof_tests.rs` around lines 1372 - 1379, The test currently allows any failure by checking for "not a count tree" OR "empty path", which masks unrelated errors; change the assertion to verify the specific empty-path failure from GroveDb::verify_trunk_chunk_proof. Locate the call to verify_trunk_chunk_proof (the result variable) in trunk_proof_tests.rs and either: 1) match the returned Err against the concrete empty-path error variant/type that verify_trunk_chunk_proof produces (e.g., compare to the specific Error enum variant or pattern-match the error) or 2) if only text matching is possible, assert that the debug/message string contains exactly the "empty path" text (remove the alternative "not a count tree" branch) so the test only passes for the intended empty-path failure. Ensure you reference the verify_trunk_chunk_proof call and the result.unwrap_err() when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@grovedb/src/operations/proof/verify.rs`:
- Around line 2240-2242: The call to key_query.execute_proof currently passes
PROOF_VERSION_LATEST which lets future grovedb_merk releases change verification
semantics for existing GroveDBProof::V1 proofs; replace that argument with a
pinned constant (e.g. MERK_PROOF_VERSION_FOR_GROVEDB_V1) that is set to the
explicit merk proof version that GroveDB V1 expects and use that constant in the
execute_proof call (referencing key_query.execute_proof and GroveDBProof::V1),
so V1 verification remains stable until you intentionally bump the GroveDB proof
version.
In `@grovedb/src/tests/trunk_proof_tests.rs`:
- Around line 1407-1411: The test currently only checks result.is_err(); change
it to assert the error is the specific "Failed to execute V1 trunk proof"
failure so the test targets the intended execution-failure branch: call
GroveDb::verify_trunk_chunk_proof(&tampered, &query, grove_version), unwrap the
error (e.g. let err = result.unwrap_err()), and assert that err (or
err.to_string()) matches or contains "Failed to execute V1 trunk proof" (or
pattern-match the specific error variant if a concrete error enum is returned)
so the assertion ties to that exact failure path.
---
Duplicate comments:
In `@grovedb/src/tests/trunk_proof_tests.rs`:
- Around line 1372-1379: The test currently allows any failure by checking for
"not a count tree" OR "empty path", which masks unrelated errors; change the
assertion to verify the specific empty-path failure from
GroveDb::verify_trunk_chunk_proof. Locate the call to verify_trunk_chunk_proof
(the result variable) in trunk_proof_tests.rs and either: 1) match the returned
Err against the concrete empty-path error variant/type that
verify_trunk_chunk_proof produces (e.g., compare to the specific Error enum
variant or pattern-match the error) or 2) if only text matching is possible,
assert that the debug/message string contains exactly the "empty path" text
(remove the alternative "not a count tree" branch) so the test only passes for
the intended empty-path failure. Ensure you reference the
verify_trunk_chunk_proof call and the result.unwrap_err() when making the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 101e1ad9-c9b4-4afc-90f9-96a400864f7e
📒 Files selected for processing (4)
grovedb-version/src/version/v3.rsgrovedb/src/operations/proof/generate.rsgrovedb/src/operations/proof/verify.rsgrovedb/src/tests/trunk_proof_tests.rs
The empty-path error at the end of verify_trunk_chunk_proof_v1 was unreachable dead code: with an empty path the for-loop never runs, so count stays None and the "not a count tree" check fires first. Move the empty-path guard to the top of the function so it fires immediately, remove the now-redundant Option wrapper on grovedb_root_hash, and tighten the test assertion to require specifically "empty path" in the error message. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
grovedb/src/tests/trunk_proof_tests.rs (1)
1407-1411:⚠️ Potential issue | 🟡 MinorAssert the
execute()failure branch explicitly.
is_err()also passes if verification fails earlier for some unrelated reason, so this can stop covering theFailed to execute V1 trunk proofbranch without the test noticing.💡 Tighten the assertion
- let result = GroveDb::verify_trunk_chunk_proof(&tampered, &query, grove_version); - assert!( - result.is_err(), - "should reject proof with invalid op sequence" - ); + let err = GroveDb::verify_trunk_chunk_proof(&tampered, &query, grove_version) + .expect_err("should reject proof with invalid op sequence"); + assert!( + format!("{:?}", err).contains("Failed to execute V1 trunk proof"), + "expected execute() failure, got: {:?}", + err + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/tests/trunk_proof_tests.rs` around lines 1407 - 1411, The test currently only asserts result.is_err() after calling GroveDb::verify_trunk_chunk_proof(&tampered, &query, grove_version); tighten this to explicitly assert the failure is the execute() branch by matching the error returned (e.g., unwrap_err() or pattern-match) and checking it equals or contains the specific variant/message expected for "Failed to execute V1 trunk proof"; update the assertion to inspect the error from verify_trunk_chunk_proof (using tampered, query, grove_version) and verify it is the execute-failure error rather than any generic error.grovedb/src/operations/proof/verify.rs (1)
2247-2248:⚠️ Potential issue | 🟠 MajorPin the Merk proof version instead of
PROOF_VERSION_LATEST.Line 2248 makes GroveDB V1 verification follow whatever
grovedb_merklater defines as “latest”, so already-issued V1 proofs can change semantics without a GroveDB proof-version bump. Please pin a GroveDB-owned Merk proof version constant for the V1 contract here instead of using the moving alias.🔎 Read-only verification
Expected result: this V1 verifier should depend on a pinned GroveDB constant, not on the upstream
PROOF_VERSION_LATESTalias.#!/bin/bash set -euo pipefail echo "Call sites using PROOF_VERSION_LATEST in proof verification:" rg -n -A2 -B2 "PROOF_VERSION_LATEST|execute_proof\\(" grovedb/src/operations/proof/verify.rs echo echo "Definition sites for PROOF_VERSION_LATEST in the workspace:" rg -n "PROOF_VERSION_LATEST" --glob '*.rs'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/operations/proof/verify.rs` around lines 2247 - 2248, The code currently calls key_query.execute_proof(..., PROOF_VERSION_LATEST) which ties GroveDB V1 verification to an upstream moving alias; define a GroveDB-owned constant (e.g. PROOF_VERSION_MERK_V1 or MERK_PROOF_VERSION_V1) in this module or a nearby constants module and replace PROOF_VERSION_LATEST with that constant in the key_query.execute_proof call; ensure the new constant value matches the intended pinned Merk proof version for GroveDB V1 and update any module exports/imports so the constant is visible where verify.rs uses it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@grovedb/src/operations/proof/verify.rs`:
- Around line 2365-2436: The extractor currently only rechecks embedded hashes
for KVValueHashFeatureTypeWithChildHash nodes, so modify
extract_elements_and_leaf_keys (and/or get_key_value_from_node) to validate
embedded hashes for KVValueHash, KVValueHashFeatureType, and KVRefValueHash
variants as well: for each of these node kinds compute
value_hash(&value_bytes).value(), use combine_hash(value_hash,
&child_or_lower_hash) when appropriate (same pattern as the existing
combine_hash usage), compare the resulting bytes to the node's stored hash (or
layer_expected_hash) and return Error::InvalidProof on mismatch; reuse the
existing combine_hash and value_hash helpers and the same error format used
elsewhere in verify.rs to ensure tampered value bytes are rejected.
---
Duplicate comments:
In `@grovedb/src/operations/proof/verify.rs`:
- Around line 2247-2248: The code currently calls key_query.execute_proof(...,
PROOF_VERSION_LATEST) which ties GroveDB V1 verification to an upstream moving
alias; define a GroveDB-owned constant (e.g. PROOF_VERSION_MERK_V1 or
MERK_PROOF_VERSION_V1) in this module or a nearby constants module and replace
PROOF_VERSION_LATEST with that constant in the key_query.execute_proof call;
ensure the new constant value matches the intended pinned Merk proof version for
GroveDB V1 and update any module exports/imports so the constant is visible
where verify.rs uses it.
In `@grovedb/src/tests/trunk_proof_tests.rs`:
- Around line 1407-1411: The test currently only asserts result.is_err() after
calling GroveDb::verify_trunk_chunk_proof(&tampered, &query, grove_version);
tighten this to explicitly assert the failure is the execute() branch by
matching the error returned (e.g., unwrap_err() or pattern-match) and checking
it equals or contains the specific variant/message expected for "Failed to
execute V1 trunk proof"; update the assertion to inspect the error from
verify_trunk_chunk_proof (using tampered, query, grove_version) and verify it is
the execute-failure error rather than any generic error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d2025cec-9df1-407c-95ee-52f5b8b52289
📒 Files selected for processing (2)
grovedb/src/operations/proof/verify.rsgrovedb/src/tests/trunk_proof_tests.rs
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
grovedb/src/tests/trunk_proof_tests.rs (3)
1510-1545: Consider extracting node-tampering logic to reduce duplication.The pattern for tampering with
KVValueHash,KVValueHashFeatureType, andKVValueHashFeatureTypeWithChildHashnodes is duplicated between this test andtest_trunk_proof_v1_rejects_forged_count_zero(lines 762-805).♻️ Optional helper to reduce duplication
/// Replace value bytes for a target key in merk proof ops while preserving value_hash. fn tamper_node_value_bytes( ops: &[Op], target_key: &[u8], forged_bytes: Vec<u8>, ) -> (Vec<Op>, bool) { let mut tampered_ops = Vec::new(); let mut found = false; for op in ops { match op { Op::Push(Node::KVValueHash(key, _value, vh)) if key == target_key => { tampered_ops.push(Op::Push(Node::KVValueHash( key.clone(), forged_bytes.clone(), *vh, ))); found = true; } Op::Push(Node::KVValueHashFeatureType(key, _value, vh, ft)) if key == target_key => { tampered_ops.push(Op::Push(Node::KVValueHashFeatureType( key.clone(), forged_bytes.clone(), *vh, ft.clone(), ))); found = true; } Op::Push(Node::KVValueHashFeatureTypeWithChildHash(key, _value, vh, ft, ch)) if key == target_key => { tampered_ops.push(Op::Push(Node::KVValueHashFeatureTypeWithChildHash( key.clone(), forged_bytes.clone(), *vh, ft.clone(), *ch, ))); found = true; } other => tampered_ops.push(other.clone()), } } (tampered_ops, found) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/tests/trunk_proof_tests.rs` around lines 1510 - 1545, The test duplicates logic that replaces value bytes while preserving value hashes for Node::KVValueHash, Node::KVValueHashFeatureType, and Node::KVValueHashFeatureTypeWithChildHash; extract that logic into a helper like tamper_node_value_bytes(ops: &[Op], target_key: &[u8], forged_bytes: Vec<u8>) -> (Vec<Op>, bool) which iterates over ops, matches the three Node variants under Op::Push, replaces the value bytes with forged_bytes.clone() while keeping the original value_hash/feature/child_hash, sets found=true when matched, and returns (tampered_ops, found); then replace the inline loop in this test and in test_trunk_proof_v1_rejects_forged_count_zero to call tamper_node_value_bytes and use its returned tampered_ops and found.
1220-1355: Good error-branch coverage, but consider expanding variant coverage.The error-branch tests effectively exercise key rejection paths. However, per the
ProofBytesenum definition, only two of the five non-Merk variants are tested:
- Path layer (line 1232): Uses
MMRvariant- Target layer (line 1337): Uses
DenseTreevariantThe
BulkAppendTreeandCommitmentTreevariants are not tested at either layer. While the verification logic rejects all non-Merk variants identically via the wildcard match, adding at least one test for these variants would confirm they're handled correctly.🧪 Optional: Add variant coverage tests
/// Optional: Test BulkAppendTree rejection at path layer. #[test] fn test_trunk_proof_v1_rejects_bulk_append_tree_at_path_layer() { let grove_version = GroveVersion::latest(); let (mut proof_v1, query, _) = make_single_level_v1_proof(); proof_v1.root_layer.merk_proof = ProofBytes::BulkAppendTree(vec![0x01, 0x02]); let config = bincode::config::standard() .with_big_endian() .with_no_limit(); let tampered = bincode::encode_to_vec(&GroveDBProof::V1(proof_v1), config).expect("encode"); let result = GroveDb::verify_trunk_chunk_proof(&tampered, &query, grove_version); assert!(result.is_err()); let err_msg = format!("{:?}", result.unwrap_err()); assert!(err_msg.contains("expected Merk proof bytes at path layer")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/tests/trunk_proof_tests.rs` around lines 1220 - 1355, Add tests that cover the remaining ProofBytes variants by duplicating the existing non-Merk rejection tests and swapping the ProofBytes variant; e.g., create new tests alongside test_trunk_proof_v1_rejects_non_merk_at_path_layer and test_trunk_proof_v1_rejects_non_merk_at_target_layer that set proof_v1.root_layer.merk_proof = ProofBytes::BulkAppendTree(...) and ProofBytes::CommitmentTree(...) (and similarly set target_layer.merk_proof to those variants for target-layer coverage), re-encode with bincode, call GroveDb::verify_trunk_chunk_proof(&tampered, &query, grove_version) and assert it returns Err with the same error messages ("expected Merk proof bytes at path layer" or "expected Merk proof bytes at target layer") as the existing tests.
1071-1083: Test assertion could be more explicit about expected behavior.The current pattern accepts either a prove-time or verify-time failure without distinguishing which path is exercised. If
prove_trunk_chunkbehavior changes to succeed where it previously failed, this test would silently pass without verifying the intended rejection path.Consider making the expected behavior explicit:
💡 Option 1: Assert prove fails (if that's the expected behavior)
- let result = db.prove_trunk_chunk(&query, grove_version).unwrap(); - - // Proving or verifying should fail because plain Tree has no count - // Either the prover rejects it or the verifier sees no count - if let Ok(proof) = result { - let verify_result = GroveDb::verify_trunk_chunk_proof(&proof, &query, grove_version); - assert!( - verify_result.is_err(), - "trunk proof for non-count tree should fail verification" - ); - } - // If prove itself failed, that's also acceptable + let result = db.prove_trunk_chunk(&query, grove_version).unwrap(); + assert!( + result.is_err(), + "prove_trunk_chunk should fail for non-count tree, got: {:?}", + result + );💡 Option 2: Assert verify fails (if prove is expected to succeed)
- let result = db.prove_trunk_chunk(&query, grove_version).unwrap(); - - // Proving or verifying should fail because plain Tree has no count - // Either the prover rejects it or the verifier sees no count - if let Ok(proof) = result { - let verify_result = GroveDb::verify_trunk_chunk_proof(&proof, &query, grove_version); - assert!( - verify_result.is_err(), - "trunk proof for non-count tree should fail verification" - ); - } - // If prove itself failed, that's also acceptable + let proof = db + .prove_trunk_chunk(&query, grove_version) + .unwrap() + .expect("prove should succeed for plain tree"); + let verify_result = GroveDb::verify_trunk_chunk_proof(&proof, &query, grove_version); + assert!( + verify_result.is_err(), + "trunk proof verification should fail for non-count tree" + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/tests/trunk_proof_tests.rs` around lines 1071 - 1083, The test currently accepts either prove_trunk_chunk(&query, grove_version) failing or its proof failing verification; make the expected path explicit: if you expect the prover to reject non-count trees, replace the conditional with assert!(db.prove_trunk_chunk(&query, grove_version).is_err(), "prove_trunk_chunk should fail for non-count tree"); if instead you expect the prover to produce a proof but verification to fail, unwrap the proof (let proof = db.prove_trunk_chunk(&query, grove_version).unwrap();) and then assert that GroveDb::verify_trunk_chunk_proof(&proof, &query, grove_version).is_err() with a clear message; update the test around prove_trunk_chunk and GroveDb::verify_trunk_chunk_proof accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@grovedb/src/tests/trunk_proof_tests.rs`:
- Around line 1510-1545: The test duplicates logic that replaces value bytes
while preserving value hashes for Node::KVValueHash,
Node::KVValueHashFeatureType, and Node::KVValueHashFeatureTypeWithChildHash;
extract that logic into a helper like tamper_node_value_bytes(ops: &[Op],
target_key: &[u8], forged_bytes: Vec<u8>) -> (Vec<Op>, bool) which iterates over
ops, matches the three Node variants under Op::Push, replaces the value bytes
with forged_bytes.clone() while keeping the original
value_hash/feature/child_hash, sets found=true when matched, and returns
(tampered_ops, found); then replace the inline loop in this test and in
test_trunk_proof_v1_rejects_forged_count_zero to call tamper_node_value_bytes
and use its returned tampered_ops and found.
- Around line 1220-1355: Add tests that cover the remaining ProofBytes variants
by duplicating the existing non-Merk rejection tests and swapping the ProofBytes
variant; e.g., create new tests alongside
test_trunk_proof_v1_rejects_non_merk_at_path_layer and
test_trunk_proof_v1_rejects_non_merk_at_target_layer that set
proof_v1.root_layer.merk_proof = ProofBytes::BulkAppendTree(...) and
ProofBytes::CommitmentTree(...) (and similarly set target_layer.merk_proof to
those variants for target-layer coverage), re-encode with bincode, call
GroveDb::verify_trunk_chunk_proof(&tampered, &query, grove_version) and assert
it returns Err with the same error messages ("expected Merk proof bytes at path
layer" or "expected Merk proof bytes at target layer") as the existing tests.
- Around line 1071-1083: The test currently accepts either
prove_trunk_chunk(&query, grove_version) failing or its proof failing
verification; make the expected path explicit: if you expect the prover to
reject non-count trees, replace the conditional with
assert!(db.prove_trunk_chunk(&query, grove_version).is_err(), "prove_trunk_chunk
should fail for non-count tree"); if instead you expect the prover to produce a
proof but verification to fail, unwrap the proof (let proof =
db.prove_trunk_chunk(&query, grove_version).unwrap();) and then assert that
GroveDb::verify_trunk_chunk_proof(&proof, &query, grove_version).is_err() with a
clear message; update the test around prove_trunk_chunk and
GroveDb::verify_trunk_chunk_proof accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 63c685e1-6db3-4f23-8baf-1675c75c4d40
📒 Files selected for processing (1)
grovedb/src/tests/trunk_proof_tests.rs
Extends extract_elements_and_leaf_keys to verify H(value) == value_hash for KVValueHash and KVValueHashFeatureType nodes, and rejects KVRefValueHash* nodes in trunk/branch proofs. Without this, an attacker could substitute KV(key, real_value) with KVValueHash(key, forged_value, real_value_hash) and the forged value would pass verification. Adds two attack tests that craft forged proofs and assert rejection. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When count is 0, the target layer's merk proof bytes are unused since lower_hash is hardcoded to NULL_HASH. Without validation, an attacker could embed arbitrary data in the target layer for bandwidth amplification. Now rejects proofs with non-empty target bytes when count==0. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ement validation - Fix value_hash validation for tree elements: tree elements have value_hash = combine_hash(H(value), child_hash), so H(value) alone won't match. Skip the mismatch for tree elements since the hash chain verification already covers them. - Move Element::deserialize before value_hash check so tree detection works and invalid bytes are caught by deserialization. - Use valid serialized Elements in forgery tests so the hash check path is properly exercised. - Add tests: unknown version in prove_trunk_chunk_non_serialized, KVRefValueHash node rejection, KVValueHashFeatureTypeWithChildHash forgery, Hash node in target layer, inconsistent depth, and V1 proof with subtree elements. Co-Authored-By: Claude Opus 4.6 <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.
Summary
LayerProof/ProofBytes::Merk(previously only V0MerkOnlyLayerProofwas supported)Vulnerability (V0)
In
verify_trunk_chunk_proof_v0, whencount == 0the function returns early without running thecombine_hash(value_hash(value_bytes), lower_hash)verification loop. Since merkKVValueHashnodes storevalue_hashseparately from value bytes, an attacker can:count = 0value_hashintact in theKVValueHashnodevalue_hash)count == 0fast-path returns an empty result, skipping hash verificationFix (V1)
The V1 verifier always runs the
combine_hashverification chain, usingNULL_HASHas the lower hash for empty trees. Forged element bytes produce a differentvalue_hash, socombine_hash(H(forged_bytes), NULL_HASH) != expected_hash, and the proof is rejected.Test plan
cargo test -p grovedb --lib trunk_proof_tests— all 7 tests passcargo test -p grovedb --lib chunk_branch_proof— all 3 tests passcargo clippy -p grovedb -- -D warnings— cleantest_trunk_proof_v1_rejects_forged_count_zero) proves V1 rejects and V0 accepts the attack🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests