Skip to content

fix: verify combine_hash for empty trees in proof result set#631

Merged
QuantumExplorer merged 1 commit into
developfrom
fix/C1-empty-tree-combine-hash-verify
Mar 9, 2026
Merged

fix: verify combine_hash for empty trees in proof result set#631
QuantumExplorer merged 1 commit into
developfrom
fix/C1-empty-tree-combine-hash-verify

Conversation

@QuantumExplorer
Copy link
Copy Markdown
Member

@QuantumExplorer QuantumExplorer commented Mar 9, 2026

Summary

  • Adds combine_hash(H(value_bytes), NULL_HASH) == proof_hash verification for empty trees in the proof result set (both V0 and V1 verifiers)
  • Prevents tree type swapping attacks on empty trees in KVValueHash/KVValueHashFeatureType proof nodes, where the value bytes are not part of the merk hash computation
  • Adds empty_tree_type_swap_is_detected regression test that proves a SumTree→Tree swap in the proof is now caught

Context

In KVValueHash nodes, the tree hash is kv_digest_to_kv_hash(key, value_hash) — the value bytes are NOT incorporated into the hash. For non-empty trees with lower layer proofs, GroveDB already catches value tampering via combine_hash(H(value), child_root). However, empty trees in the result set (no lower layer) had no such verification, allowing an attacker to swap serialized Element types without detection.

This is a companion fix to PR #553 (which blocks KV→KVValueHash substitution for items at the merk level).

Test plan

  • New empty_tree_type_swap_is_detected test — tampers a KVValueHash node's value bytes from SumTree to Tree, verifies rejection
  • All 237 proof tests pass
  • All 102 sum_tree tests pass
  • Builds with both full and verify features

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced V1 proof verification with additional validation checks for empty tree representations to ensure proof integrity.
  • Tests

    • Added new test coverage for empty tree handling and validation scenarios in the proof verification system.

In KVValueHash/KVValueHashFeatureType proof nodes, the value bytes are
NOT part of the merk hash computation. For non-empty trees with lower
layer proofs, GroveDB's combine_hash(H(value), child_root) check
catches value tampering. However, empty trees in the result set had no
such verification — an attacker could swap tree types (e.g. SumTree →
Tree) without detection.

Add combine_hash(H(value_bytes), NULL_HASH) verification for empty
trees in both V0 and V1 proof verifiers before adding them to the
result set.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 9, 2026

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

The PR adds validation to V1 proof verification to prevent empty-tree type swapping exploits. When a tree element lacks a lower-layer proof, the verifier now asserts that the value_hash matches the combined hash of the value and NULL_HASH. A corresponding test validates this protection by attempting to tamper with empty tree types in proofs.

Changes

Cohort / File(s) Summary
Proof Verification Logic
grovedb/src/operations/proof/verify.rs
Adds validation in two paths (verify_proof_v1_internal and verify_proof_v1_raw_internal) to detect empty-tree type swaps by verifying that value_hash equals combine_hash(H(value), NULL_HASH) when no lower-layer proof exists. Returns InvalidProof with descriptive error if validation fails.
Test Coverage
grovedb/src/tests/proof_coverage_tests.rs
Adds helper function tamper_kvvaluehash_value to locate and swap KVValueHash entries in proofs, and introduces test empty_tree_type_swap_is_detected to verify that tampering with empty tree types in proofs is correctly detected and rejected.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A bunny's burrow grows secure today,
Empty trees can't hide or twist away!
With hashes checked and tampering denied,
Our proofs stand tall with nothing left to hide! 🌳✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding verification for combine_hash of empty trees in proof verification, which is the core security fix implemented in verify.rs.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/C1-empty-tree-combine-hash-verify

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 9, 2026

Codecov Report

❌ Patch coverage is 55.55556% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.72%. Comparing base (319dea5) to head (ad6d21e).
⚠️ Report is 2 commits behind head on develop.

Files with missing lines Patch % Lines
grovedb/src/operations/proof/verify.rs 55.55% 16 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           develop     #631       +/-   ##
============================================
+ Coverage    59.95%   90.72%   +30.77%     
============================================
  Files          182      182               
  Lines        51082    51118       +36     
============================================
+ Hits         30624    46378    +15754     
+ Misses       20458     4740    -15718     
Components Coverage Δ
grovedb-core 88.87% <55.55%> (+31.35%) ⬆️
merk 92.05% <ø> (+30.02%) ⬆️
storage 86.36% <ø> (+15.95%) ⬆️
commitment-tree 96.41% <ø> (+26.16%) ⬆️
mmr 96.72% <ø> (+28.41%) ⬆️
bulk-append-tree 90.85% <ø> (+18.64%) ⬆️
element 97.55% <ø> (+48.24%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@QuantumExplorer QuantumExplorer merged commit e2fa568 into develop Mar 9, 2026
8 of 10 checks passed
@QuantumExplorer QuantumExplorer deleted the fix/C1-empty-tree-combine-hash-verify branch March 9, 2026 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant