Skip to content

fix(dense): ignore inclusive queries on empty trees#745

Open
thepastaclaw wants to merge 2 commits into
dashpay:developfrom
thepastaclaw:fix-dense-inclusive-empty-tree
Open

fix(dense): ignore inclusive queries on empty trees#745
thepastaclaw wants to merge 2 commits into
dashpay:developfrom
thepastaclaw:fix-dense-inclusive-empty-tree

Conversation

@thepastaclaw

@thepastaclaw thepastaclaw commented May 23, 2026

Copy link
Copy Markdown
Contributor

Summary

  • return no dense proof positions for empty-tree inclusive queries
  • cover RangeInclusive, RangeToInclusive, and
    RangeAfterToInclusive empty-tree conversions
  • add end-to-end empty-tree proof generation/verification regressions

Fixes #713.

Validation

  • cargo test --offline -p grovedb-dense-fixed-sized-merkle-tree --lib
    177 passed
  • pre-PR code review gate: ship

Summary by CodeRabbit

Bug Fixes

  • Fixed range query handling on empty datasets for inclusive range variants, ensuring results remain empty when the requested count is zero while still validating malformed range bounds.

Tests

  • Added additional test cases for inclusive range queries on empty trees, covering both empty-result behavior and proper invalid-data errors for malformed inputs.
  • Extended query verification tests to confirm empty entries are returned on empty datasets for these query types.

@coderabbitai

coderabbitai Bot commented May 23, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 25cee562-3677-4e76-bfa4-bb84754b777a

📥 Commits

Reviewing files that changed from the base of the PR and between b5926a2 and 4b859cb.

📒 Files selected for processing (2)
  • grovedb-dense-fixed-sized-merkle-tree/src/proof/mod.rs
  • grovedb-dense-fixed-sized-merkle-tree/src/proof/tests.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • grovedb-dense-fixed-sized-merkle-tree/src/proof/mod.rs
  • grovedb-dense-fixed-sized-merkle-tree/src/proof/tests.rs

📝 Walkthrough

Walkthrough

This PR fixes incorrect empty tree handling in proof query conversion. Three inclusive range query types now short-circuit immediately when count == 0, preventing faulty position generation. Test coverage validates the fix at both unit and integration levels across all three range variants.

Changes

Empty tree inclusive range query fix

Layer / File(s) Summary
Empty tree short-circuit guards in query_to_positions
grovedb-dense-fixed-sized-merkle-tree/src/proof/mod.rs
Three match arms for RangeInclusive, RangeToInclusive, and RangeAfterToInclusive each add an explicit count == 0 check that immediately continues, preventing position decoding and clamping logic from executing on empty trees. RangeInclusive validates bounds before the short-circuit to ensure malformed encodings still error on empty trees.
Test coverage for empty tree inclusive range queries
grovedb-dense-fixed-sized-merkle-tree/src/proof/tests.rs
New unit tests verify query_to_positions returns empty positions when count is zero for all three inclusive range types, and confirm that malformed bounds still fail with DenseMerkleError::InvalidData even on empty trees. Integration tests in verify_for_query_edges verify that verify_for_query returns an empty entries vector for empty-tree queries across range_inclusive, range_to_inclusive, and range_after_to_inclusive.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A tree so sparse it holds no leaves,
Yet still we must prove it believes.
Count zero now short-circuits well,
Empty queries have tales to tell!
Tests guard this fix, precise and neat. 🌳✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely describes the main change: adding handling for inclusive queries on empty trees.
Linked Issues check ✅ Passed The PR fully addresses issue #713 by implementing the suggested fix for inclusive range queries on empty trees and adding comprehensive test coverage.
Out of Scope Changes check ✅ Passed All changes are directly related to fixing inclusive query handling on empty trees; no out-of-scope modifications detected.
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 unit tests (beta)
  • Create PR with unit tests

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.

@thepastaclaw

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 23, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@codecov

codecov Bot commented May 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.47%. Comparing base (9f67e8c) to head (4b859cb).
⚠️ Report is 10 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #745      +/-   ##
===========================================
+ Coverage    91.43%   91.47%   +0.04%     
===========================================
  Files          236      240       +4     
  Lines        67111    67621     +510     
===========================================
+ Hits         61364    61858     +494     
- Misses        5747     5763      +16     
Components Coverage Δ
grovedb-core 89.07% <ø> (+0.12%) ⬆️
merk 92.26% <ø> (ø)
storage 86.20% <ø> (-0.17%) ⬇️
commitment-tree 96.05% <ø> (-0.38%) ⬇️
mmr 96.79% <ø> (ø)
bulk-append-tree 89.82% <ø> (+0.42%) ⬆️
element 97.38% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@thepastaclaw thepastaclaw force-pushed the fix-dense-inclusive-empty-tree branch from b5926a2 to 2e9b179 Compare May 23, 2026 17:19
@thepastaclaw

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 23, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@thepastaclaw

thepastaclaw commented Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

✅ Review complete (commit 2e9b179)

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Targeted fix correctly addresses the empty-tree inclusive-range bug and adds regression coverage. One suggestion-level concern: the new count == 0 short-circuits in the three inclusive arms run before bytes_to_position, so malformed bounds are now silently accepted on empty trees while still being rejected on non-empty trees — a behavioral inconsistency that also propagates through verify_for_query.

🟡 1 suggestion(s)

1 additional finding(s) omitted (not in diff).

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `grovedb-dense-fixed-sized-merkle-tree/src/proof/mod.rs`:
- [SUGGESTION] grovedb-dense-fixed-sized-merkle-tree/src/proof/mod.rs:66-127: Validate inclusive range bounds before the empty-tree short-circuit
  The new `if count == 0 { continue; }` guards in `RangeInclusive`, `RangeToInclusive`, and `RangeAfterToInclusive` run before `bytes_to_position` is called. Every other `QueryItem` arm decodes bounds first and returns `InvalidData` for malformed encodings (non-1/2-byte keys). With this PR, a malformed inclusive bound (e.g., a 0-byte or 3-byte key) returns `Ok(empty)` when `count == 0` but `Err(InvalidData)` when `count > 0`. This makes the `Result` contract depend on the runtime tree state rather than query validity, and because `verify_for_query` reuses `query_to_positions` (verify.rs:97), a malformed inclusive query can verify successfully against an empty-tree proof instead of being rejected. Move `bytes_to_position` calls above the `count == 0` short-circuit in each of the three arms so bound validation is uniform across query items and tree states.

The count == 0 short-circuits for RangeInclusive, RangeToInclusive, and
RangeAfterToInclusive previously ran before bytes_to_position, so
malformed inclusive bounds (byte length != 1 or 2) silently passed on
empty trees instead of returning InvalidData. Decode the bounds first,
then short-circuit, so validation is consistent regardless of count.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@thepastaclaw

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@thepastaclaw

Copy link
Copy Markdown
Contributor Author

✅ Review complete (commit 4b859cb)

Follow-up validation after the automated review suggestion:

  • Inclusive empty-tree query bounds are now decoded/validated before the count == 0 short-circuit.
  • Added malformed-bound regression coverage for RangeInclusive, RangeToInclusive, and RangeAfterToInclusive.
  • Targeted local test passed: cargo test -p grovedb-dense-fixed-sized-merkle-tree proof::tests::proof_tests (121 passed).
  • GitHub CI is green and CodeRabbit completed.

@thepastaclaw thepastaclaw marked this pull request as ready for review June 16, 2026 05:58
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.

[audit][low] Dense inclusive query conversion mishandles empty trees

1 participant