feat: add Element::NonCounted wrapper to opt-out of count propagation#654
Merged
Conversation
A new `Element::NonCounted(Box<Element>)` variant that behaves identically to its inner element except it contributes 0 to the parent count tree's aggregate. Sums still propagate. May only be inserted into count-bearing trees (CountTree, ProvableCountTree, CountSumTree, ProvableCountSumTree). Use case: insert housekeeping rows, schema markers, or auxiliary indexes inside a count tree without inflating its count. Single-variant wrapper design (instead of 15+ explicit variants) keeps the diff surface and audit footprint small. Existing `Element` predicates and helpers look through the wrapper via a new `underlying()` accessor; cost, proof, and pattern-match sites dispatch on the underlying element. The wrapper byte is included in the serialized value so the value hash distinguishes wrapped from unwrapped at the cryptographic layer. Nested `NonCounted(NonCounted(...))` is rejected at construction, serialization, and deserialization to close a stack-overflow vector. Tests: 10 in grovedb-element (constructor, helpers, bincode round-trip, nested-rejection), 5 in grovedb-merk (insert validation, aggregate_data end-to-end). Full workspace test suite still passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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 (12)
📝 WalkthroughWalkthroughAdds Element::NonCounted wrapper (non-nestable) that preserves sums but suppresses parent counts; integrates wrapper handling across serialization, element helpers, cost accounting, insert/batch propagation, queries, proofs, verification, visualization, and tests. ChangesNonCounted Wrapper Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
grovedb/src/batch/mod.rs (1)
1444-1453:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDon't unwrap before computing the base element hash.
into_underlying()drops theNonCounteddiscriminant, so a multi-hop reference that resolves to an on-diskNonCounted(Item|SumItem|ItemWithSumItem)will hashXinstead ofNonCounted(X). That makes the reference value hash diverge from the serialized bytes actually stored on disk.Suggested fix
- let element = element.into_underlying(); - match element { + match element.underlying() { Element::Item(..) | Element::SumItem(..) | Element::ItemWithSumItem(..) => { let serialized = cost_return_on_error_into_no_add!(cost, element.serialize(grove_version)); let val_hash = value_hash(&serialized).unwrap_add_cost(&mut cost); Ok(val_hash).wrap_with_cost(cost) } Element::Reference(path, ..) => { let path = cost_return_on_error_into_no_add!( cost, - path_from_reference_qualified_path_type(path, qualified_path) + path_from_reference_qualified_path_type(path.clone(), qualified_path) ); self.follow_reference_get_value_hash( path.as_slice(), ops_by_qualified_paths,🤖 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/batch/mod.rs` around lines 1444 - 1453, The bug is that into_underlying() is called before hashing so NonCounted's wrapper byte is lost; stop dropping the NonCounted discriminant before computing the stored value hash. Specifically, in the block handling Element::Item/::SumItem/::ItemWithSumItem, compute the hash from element.serialize(grove_version) (or otherwise serialize the original element including the NonCounted wrapper) and pass that serialized bytes into value_hash(...) before calling into_underlying() or any wrapper-stripping; ensure into_underlying() is only used for dispatch logic, not for deriving the value_hash.grovedb/src/operations/insert/mod.rs (1)
248-351:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace
unreachable!with a typed error for nested wrappers.
element.underlying()unwraps a single level. If a nestedElement::NonCounted(Box::new(Element::NonCounted(...)))is passed, Line 350 can panic on a public insert path.Suggested fix
- Element::NonCounted(_) => unreachable!("unwrapped above"), + Element::NonCounted(_) => { + return Err(Error::InvalidInput( + "nested non-counted wrappers are not allowed", + )) + .wrap_with_cost(cost); + }🤖 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/insert/mod.rs` around lines 248 - 351, The match arm currently uses unreachable!() for Element::NonCounted(_) which can panic if element.underlying() only unwraps one level and a nested NonCounted wrapper is passed; change this to return a typed error instead (e.g., Err(Error::InvalidCodeExecution("nested Element::NonCounted wrapper encountered")).wrap_with_cost(cost)) so the public insert path won't panic; update the match arm for Element::NonCounted(_) to produce that error (keeping use of wrap_with_cost(cost) consistent with surrounding error handling) and reference element.underlying() and Element::NonCounted in the message to make the cause clear.
🧹 Nitpick comments (1)
merk/src/tree_type/mod.rs (1)
215-400: ⚡ Quick winAdd a unit test for
is_count_bearingto mirror the existing predicate tests.The test module already has exhaustive variant coverage for
uses_non_merk_data_storageandallows_sum_item. A matching test foris_count_bearingwould lock the contract behind which the newElement::NonCountedinsertion gate atmerk/src/element/insert.rsoperates.🧪 Proposed test addition
+ #[test] + fn is_count_bearing() { + assert!(!TreeType::NormalTree.is_count_bearing()); + assert!(!TreeType::SumTree.is_count_bearing()); + assert!(!TreeType::BigSumTree.is_count_bearing()); + assert!(TreeType::CountTree.is_count_bearing()); + assert!(TreeType::CountSumTree.is_count_bearing()); + assert!(TreeType::ProvableCountTree.is_count_bearing()); + assert!(TreeType::ProvableCountSumTree.is_count_bearing()); + assert!(!TreeType::CommitmentTree(0).is_count_bearing()); + assert!(!TreeType::MmrTree.is_count_bearing()); + assert!(!TreeType::BulkAppendTree(0).is_count_bearing()); + assert!(!TreeType::DenseAppendOnlyFixedSizeTree(0).is_count_bearing()); + }🤖 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 `@merk/src/tree_type/mod.rs` around lines 215 - 400, Add a unit test in the existing tests mod that mirrors the pattern used for uses_non_merk_data_storage and allows_sum_item: iterate/assert the expected boolean for every TreeType variant against TreeType::is_count_bearing() (including parameterized variants like CommitmentTree(n), BulkAppendTree(n), DenseAppendOnlyFixedSizeTree(n)) so all variants are covered and the contract behind Element::NonCounted insertion is locked in.
🤖 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.
Inline comments:
In `@grovedb/src/batch/mod.rs`:
- Around line 1867-1871: The current bug: wrapped (NonCounted(Tree)) subtrees
are handled by execute_ops_on_path() but apply_batch_structure() still matches
on the outer Element and thus treats writes under a wrapped tree as "insertion
under a non tree"; to fix, change apply_batch_structure() to check the element's
underlying variant (use element.underlying() or the same wrapper-aware helper
used elsewhere) when matching occupied parent ops and, when encountering
NonCounted(Tree) or equivalent wrappers with children-to-be-written, convert the
parent insertion op into the corresponding tree insertion ops
(InsertTreeWithRootHash or InsertNonMerkTree) instead of emitting the non-tree
insertion error; update the matching logic around Element/occupied parent ops in
apply_batch_structure() to mirror execute_ops_on_path()'s wrapper-aware behavior
so subtree insertions propagate correctly.
In `@grovedb/src/operations/get/query.rs`:
- Around line 212-215: The follow_element path currently only calls
element.into_underlying() on the value at the query edge, leaving cases where a
resolved Reference yields a NonCounted(Item) wrapped value inconsistent; update
follow_element so that after resolving a Reference you also normalize/unwrap any
NonCounted wrapper from the resolved result (i.e. ensure the value returned by
follow_element always calls into_underlying()/equivalent unwrapping on resolved
references), and apply the same fix to the other branch handling (the code
around the 225-237 block) so both direct and referenced resolutions return the
same unwrapped Item.
In `@grovedb/src/operations/proof/verify.rs`:
- Around line 485-489: verify_trunk_chunk_proof_v0 and
verify_trunk_chunk_proof_v1 still pass wrapped NonCounted elements straight into
extract_count_from_element causing "target is not a count tree element" for
trunks rooted in NonCounted; after deserializing the element with
Element::deserialize(...) and before calling extract_count_from_element, unwrap
the NonCounted wrapper (e.g., call .into_underlying() like the other
verify_layer_proof* paths do) so the count-extraction logic receives the
underlying CountTree/CountSumTree/ProvableCount element; apply the same unwrap
fix in both verify_trunk_chunk_proof_v0 and verify_trunk_chunk_proof_v1 (also
mirror the change at the other occurrence referenced around the later block).
In `@merk/src/element/costs.rs`:
- Around line 99-105: The NonCounted wrapper loses its discriminant when calling
into_underlying(), causing constant-size branches in the cost match to omit the
wrapper byte; update the match arms that compute value_len for the tree/sum-item
specialized branches to add the wrapper_overhead (i.e., add "+ wrapper_overhead"
to those constant-based value_len calculations) so that replacement-cost
accounting includes the 1-byte wrapper discriminant while keeping the other
paths unchanged (locate the logic where element = element.into_underlying() and
the subsequent match on element).
In `@merk/src/element/get.rs`:
- Around line 411-416: The tree cost arm currently uses element_for_cost =
element.as_ref().map(|e| e.underlying()) and then relies on
tree_type().cost_size(), which misses the 1-byte NonCounted discriminant and
undercounts storage_loaded_bytes; change the sizing for the tree arm to use the
actual serialized value length (e.g., value.as_ref().unwrap().len()) rather than
tree_type().cost_size() so the logged/read bytes include the wrapper byte.
Update the same pattern wherever tree sizing is computed (the other occurrences
referenced around the element_for_cost usage at the later spots) to consistently
use the serialized value length for tree arms instead of cost_size().
In `@merk/src/element/insert.rs`:
- Around line 166-171: The validation that NonCounted elements may only be
inserted into count-bearing trees is currently only in insert(); add the same
guard at the start of insert_reference and insert_subtree to reject NonCounted
elements when merk.tree_type.is_count_bearing() is false, returning the same
Error::InvalidInputError wrapped with cost as in insert. Also ensure any batch
insertion paths that know the target tree type apply the same check before
delegating to insert_reference/insert_subtree so the restriction cannot be
bypassed via batch APIs.
---
Outside diff comments:
In `@grovedb/src/batch/mod.rs`:
- Around line 1444-1453: The bug is that into_underlying() is called before
hashing so NonCounted's wrapper byte is lost; stop dropping the NonCounted
discriminant before computing the stored value hash. Specifically, in the block
handling Element::Item/::SumItem/::ItemWithSumItem, compute the hash from
element.serialize(grove_version) (or otherwise serialize the original element
including the NonCounted wrapper) and pass that serialized bytes into
value_hash(...) before calling into_underlying() or any wrapper-stripping;
ensure into_underlying() is only used for dispatch logic, not for deriving the
value_hash.
In `@grovedb/src/operations/insert/mod.rs`:
- Around line 248-351: The match arm currently uses unreachable!() for
Element::NonCounted(_) which can panic if element.underlying() only unwraps one
level and a nested NonCounted wrapper is passed; change this to return a typed
error instead (e.g., Err(Error::InvalidCodeExecution("nested Element::NonCounted
wrapper encountered")).wrap_with_cost(cost)) so the public insert path won't
panic; update the match arm for Element::NonCounted(_) to produce that error
(keeping use of wrap_with_cost(cost) consistent with surrounding error handling)
and reference element.underlying() and Element::NonCounted in the message to
make the cause clear.
---
Nitpick comments:
In `@merk/src/tree_type/mod.rs`:
- Around line 215-400: Add a unit test in the existing tests mod that mirrors
the pattern used for uses_non_merk_data_storage and allows_sum_item:
iterate/assert the expected boolean for every TreeType variant against
TreeType::is_count_bearing() (including parameterized variants like
CommitmentTree(n), BulkAppendTree(n), DenseAppendOnlyFixedSizeTree(n)) so all
variants are covered and the contract behind Element::NonCounted insertion is
locked in.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2c38ba03-458e-42c2-b5c8-b85471cec79d
📒 Files selected for processing (18)
grovedb-element/src/element/constructor.rsgrovedb-element/src/element/helpers.rsgrovedb-element/src/element/mod.rsgrovedb-element/src/element/serialize.rsgrovedb-element/src/element/visualize.rsgrovedb-element/src/element_type.rsgrovedb-query/src/proofs/tree_feature_type.rsgrovedb/src/batch/mod.rsgrovedb/src/lib.rsgrovedb/src/operations/get/query.rsgrovedb/src/operations/insert/mod.rsgrovedb/src/operations/proof/generate.rsgrovedb/src/operations/proof/verify.rsmerk/src/element/costs.rsmerk/src/element/get.rsmerk/src/element/insert.rsmerk/src/element/tree_type.rsmerk/src/tree_type/mod.rs
CodeRabbit review findings on PR #654: - batch/mod.rs (process_reference_with_hop_count_greater_than_one): hash must be over the WRAPPED bytes (which is what's on disk), not the underlying bytes. Now matches on element.underlying() for dispatch but serializes the outer element for value_hash. - batch/mod.rs (apply_batch_structure): the long if/else-if chain that rewrites parent insert ops into InsertTreeWithRootHash / InsertNonMerkTree was matching on the outer Element, so a NonCounted-wrapped tree fell through to the "non tree" error path. Now rebinds via element.underlying() before the chain. - query.rs (follow_element): a Reference resolving to NonCounted(Item) returned the wrapper while a directly-queried NonCounted(Item) was unwrapped. Normalize the resolved value via into_underlying() too. - proof/verify.rs (extract_count_from_element): trunk proofs rooted at a NonCounted(CountTree/...) failed with "target is not a count tree element". Look through the wrapper. - merk/element/costs.rs (specialized_costs_for_key_value): the +1 wrapper byte was a deliberate under-count; replace with explicit wrapper_overhead added to all constant-size value_len computations so storage cost accounting matches on-disk bytes exactly. - merk/element/get.rs (V0 + V1 cost paths): same wrapper_overhead fix for storage_loaded_bytes on tree and sum-item arms. - merk/element/insert.rs: the NonCounted insert guard only ran in insert(); add the same check at the start of insert_reference and insert_subtree so NonCounted(Reference) and NonCounted(Tree) cannot be inserted into non-count-bearing trees through those paths. - grovedb/operations/insert/mod.rs: replace unreachable!() for Element::NonCounted(_) on the public insert path with a typed Error::InvalidInput so a hand-built nested wrapper returns an error rather than panicking. Tests: - merk/tree_type/mod.rs: add is_count_bearing test for all variants (CodeRabbit nitpick). - merk/element/insert.rs: add insert_subtree path validation test. Full workspace tests still pass (1449 grovedb, 362 grovedb-merk, 41 grovedb-element). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Old orchard rev pulled core2 ^0.3, all of whose published versions are yanked from crates.io, breaking fresh `cargo update` runs. The new dashified branch is rebased on orchard 0.13.1 which uses corez instead. This unblocks CI dependency resolution. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drops the flat ElementType::NonCounted = 15 variant and replaces it with 15 NonCountedXxx variants whose discriminants are the base type's discriminant with the high bit set (e.g. NonCountedItem = 128, NonCountedTree = 130, ...). Bit 7 cleanly encodes "is this wrapped?" and bits 0-6 encode the underlying base type. The on-disk format is unchanged: Element::NonCounted still serializes as [15, ...inner bytes]. ElementType::from_serialized_value detects this by peeking byte 1 when byte 0 is the new NON_COUNTED_WRAPPER_DISCRIMINANT (15) constant, then synthesizes the NonCountedXxx variant. TryFrom<u8> on byte 15 alone is rejected (it needs the inner byte to disambiguate) — every other base byte and the new 128..=142 range round-trip normally. New helpers on ElementType: - is_non_counted(self) -> bool: (self as u8) & 0x80 != 0 - base(self) -> ElementType: strips the high bit, returns underlying base type (or self if already a base) is_tree, is_item, is_reference, has_simple_value_hash, and proof_node_type all dispatch via base(), so a NonCountedTree is still a tree, a NonCountedItem still uses Kv proof nodes, etc. Element::element_type() returns the synthetic NonCountedXxx for a wrapped element, so callers get both "what kind" and "is wrapped" from a single ElementType value. Tests: - test_element_type_from_discriminant updated: TryFrom(15) is now Err; added 128, 129, 142 mappings and out-of-range 127, 143. - test_non_counted_helpers: is_non_counted and base() correctness + the discriminant relationship (twin = base | 0x80). - test_simple_vs_combined_hash: NonCounted twins inherit base hash kind. - test_proof_node_type_through_non_counted_wrapper: wrapper transparent for proof-node-type selection. - test_from_serialized_value: 2-byte peek for byte-15 case, including truncated/nested/unknown-inner rejections. - test_is_tree: covers NonCountedXxx + spot checks is_item/is_reference. - test_non_counted_wrapper_discriminant_pinned: pins the bincode discriminant for Element::NonCounted to 15 and verifies the synthetic ElementType resolution end-to-end. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ebugger CI runs `cargo clippy --workspace --all-features -- -D warnings`. Three pre-existing warnings in untouched files were already promoted to errors by a recent clippy update and were blocking CI: - merk/src/proofs/query/verify.rs:693: collapsible-match — fold the inner `if k.as_slice() == key` into the outer match arm guard. - merk/src/test_utils/mod.rs:268: explicit-counter-loop — replace the hand-rolled `seed += 1` counter with `for seed in initial_seed..end`. - grovedb/src/replication.rs:163: useless-conversion — drop the redundant `.into_iter()` call. Also covers a non-exhaustive match in `grovedb/src/debugger.rs` (only compiled with the `grovedbg` feature, hence missed earlier): the visualizer wire format has no NonCounted variant, so we render the inner element transparently — the wrapper is invisible in the debug UI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #654 +/- ##
===========================================
- Coverage 90.63% 90.54% -0.09%
===========================================
Files 182 182
Lines 52304 52873 +569
===========================================
+ Hits 47405 47875 +470
- Misses 4899 4998 +99
🚀 New features to boost your workflow:
|
Member
Author
|
Codex review findings:
Verification performed:
|
Codex review of PR #654 surfaced five real bugs in the NonCounted implementation. All fixed; regression tests added. P1 #1 — serialize.rs: stack-overflow vector still open The nested-wrapper guard ran AFTER bincode::decode_from_slice had already recursed through the Box<Element> chain. A payload of repeated [15, 15, 15, ...] bytes could blow the stack before any check fired. Fix: pre-check the leading two bytes for a wrapper-of- wrapper before invoking bincode. The post-check stays as defense in depth. Test: deserialize_rejects_long_nested_wrapper_chain_without_recursion feeds 1024 leading wrapper bytes — pre-check stops it on byte 1. P1 #2 — batch propagation dropped the wrapper apply_batch_structure converted a NonCounted(Tree) into the internal InsertTreeWithRootHash op but stored no information that the original was wrapped. On execution the reconstructed element was bare, so the on-disk parent element lost the wrapper byte AND the parent count tree's aggregate started counting the subtree. Fix: add `non_counted: bool` to both InsertTreeWithRootHash and InsertNonMerkTree (#[non_exhaustive]). Set it from element.is_non_counted() during apply_batch_structure; re-wrap the reconstructed element via into_non_counted() during execution. Test: batch_propagation_preserves_non_counted_wrapper_on_subtree inserts a NonCounted(CountTree) plus a child under it in one batch and asserts get_raw on the parent still returns NonCounted. P1 #3 — reconstruct_with_root_key dropped the wrapper No NonCounted arm meant existing wrapped trees couldn't propagate. Any subtree mutation that flowed through update_tree_item_preserve_flag reached the helper and got None, failing the operation. Fix: add a NonCounted arm that recurses on the inner element and re-wraps the reconstructed result. Test: reconstruct_preserves_non_counted_wrapper. P2 #4 — batch insert path bypassed the parent-type guard Direct Merk insertion already rejected NonCounted children outside count-bearing parents, but the batch path skipped the same check. Users could persist NonCounted into Normal/Sum/BigSum trees via batch. Fix: mirror the per-merk guard with the same predicate (element.is_non_counted() && !in_tree_type.is_count_bearing()). Test: batch_insert_rejects_non_counted_into_normal_tree. P2 #5 — get path didn't follow NonCounted(Reference) get_caching_optional matched only the bare Reference variant; a NonCounted-wrapped reference fell through and was returned as a value, contradicting the wrapper-transparency contract. Fix: into_underlying() on the get result before dispatching, and same in the follow_reference inner loop so reference chains hop through wrapped intermediate references. Test: get_follows_non_counted_reference inserts a NonCounted-wrapped reference inside a count tree and asserts get() returns the referenced item. Test counts (with --features minimal,test_utils,full): - grovedb-element: 45 (was 44, +1 stack-overflow regression) - grovedb-merk: 364 (was 362, +2: reconstruct + reconstruct-non-tree) - grovedb: 1452 (was 1449, +3: NonCounted regressions) Build paths touched: GroveOp::InsertTreeWithRootHash and GroveOp::InsertNonMerkTree gained a non_counted: bool field. The struct literal construction sites in tests/cost-estimation modules get non_counted: false (they predate the wrapper feature). Match-only sites (e.g. cost estimators destructuring with `..`) are unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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/get/mod.rs (1)
73-85:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrapper transparency is still incomplete for subtree validation.
get_caching_optional()now unwrapsNonCounted, butcheck_subtree_exists()later in this file still only accepts bare tree variants. Any API that validates its path through that helper will keep rejecting parents stored asNonCounted(Tree), which breaks the “wrapper is transparent” behavior this change is introducing.🤖 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/get/mod.rs` around lines 73 - 85, The subtree-validation helper check_subtree_exists still only handles bare tree variants while get_caching_optional/get_raw_caching_optional now return wrapped NonCounted variants; update validation to treat the wrapper as transparent by unwrapping NonCounted before checking. Specifically, locate where check_subtree_exists is invoked after get_caching_optional/get_raw_caching_optional and either: 1) change the caller to call .into_underlying() (or unwrap the NonCounted) before passing the value to check_subtree_exists, or 2) modify check_subtree_exists itself to match on NonCounted(...) and recurse into the inner tree variant so parents stored as NonCounted(Tree) validate correctly. Ensure you reference NonCounted, get_caching_optional/get_raw_caching_optional, and check_subtree_exists when making the change.
🧹 Nitpick comments (2)
grovedb/src/tests/non_counted_tests.rs (2)
101-105: ⚡ Quick winTighten this assertion to validate rejection reason, not just “some error”.
Right now Line 103 can pass on unrelated failures. Prefer asserting the specific NonCounted/parent-type validation failure signal.Proposed assertion hardening
- let result = db.apply_batch(vec![op], None, None, grove_version).unwrap(); - assert!( - result.is_err(), - "batch insert of NonCounted into NormalTree must fail" - ); + let err = db + .apply_batch(vec![op], None, None, grove_version) + .unwrap() + .expect_err("batch insert of NonCounted into NormalTree must fail"); + assert!( + format!("{err:?}").contains("NonCounted"), + "expected NonCounted validation error, got: {err:?}" + );🤖 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/non_counted_tests.rs` around lines 101 - 105, The test currently only checks that db.apply_batch(vec![op], None, None, grove_version) returns an error; tighten it to assert the specific validation failure for inserting a NonCounted into a NormalTree by extracting the error (e.g., via result.unwrap_err() or pattern matching) and asserting its variant/message corresponds to the NonCounted/parent-type validation failure (reference apply_batch, op, NonCounted and NormalTree) so unrelated errors don't make the test pass.
109-115: ⚡ Quick winTest intent mentions count exclusion, but the assertion only checks wrapper persistence.
Please also assert the outer count remains1(onlyplaincounted) after the batch, so this test protects both invariants it documents.Also applies to: 164-179
🤖 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/non_counted_tests.rs` around lines 109 - 115, The test batch_propagation_preserves_non_counted_wrapper_on_subtree currently only checks that the NonCounted wrapper is preserved; add an assertion after the batch commit that the outer count-tree aggregate equals 1 (i.e., only the plain counted child is counted) to ensure the count exclusion invariant; locate the code around the batch application and the on-disk parent retrieval in function batch_propagation_preserves_non_counted_wrapper_on_subtree and assert the parent count/aggregate is 1. Apply the same additional assertion to the other test in this file that similarly verifies wrapper persistence so both wrapper and count-exclusion invariants are covered.
🤖 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.
Inline comments:
In `@grovedb-element/src/element_type.rs`:
- Around line 200-215: The code currently masks inner_byte with NON_COUNTED_FLAG
and calls Self::try_from, which allows inner discriminants with the high bit set
to slip through; instead validate the raw inner_byte is a base discriminant
(0..=14) before wrapping. In the branch handling
NON_COUNTED_WRAPPER_DISCRIMINANT, after reading inner_byte from
serialized_value, explicitly check that inner_byte <= 14 (and also reject
inner_byte == 15), returning ElementError::CorruptedData with an appropriate
message if out of range or if inner_byte == NON_COUNTED_WRAPPER_DISCRIMINANT;
only then call Self::try_from(NON_COUNTED_FLAG | inner_byte).
In `@grovedb/src/batch/mod.rs`:
- Around line 1887-1891: The RefreshReference path in execute_ops_on_path
currently matches the element directly as Element::Reference and thus rejects
wrapped references; update the GroveOp::RefreshReference arm to inspect
element.underlying() (or otherwise unwrap wrapper variants) when checking for
Element::Reference so NonCounted(Reference) and similar wrappers are treated as
their inner Reference—i.e., replace the direct pattern match on `element` with a
match/if-let on `element.underlying()` and then proceed with the same refresh
logic.
---
Outside diff comments:
In `@grovedb/src/operations/get/mod.rs`:
- Around line 73-85: The subtree-validation helper check_subtree_exists still
only handles bare tree variants while
get_caching_optional/get_raw_caching_optional now return wrapped NonCounted
variants; update validation to treat the wrapper as transparent by unwrapping
NonCounted before checking. Specifically, locate where check_subtree_exists is
invoked after get_caching_optional/get_raw_caching_optional and either: 1)
change the caller to call .into_underlying() (or unwrap the NonCounted) before
passing the value to check_subtree_exists, or 2) modify check_subtree_exists
itself to match on NonCounted(...) and recurse into the inner tree variant so
parents stored as NonCounted(Tree) validate correctly. Ensure you reference
NonCounted, get_caching_optional/get_raw_caching_optional, and
check_subtree_exists when making the change.
---
Nitpick comments:
In `@grovedb/src/tests/non_counted_tests.rs`:
- Around line 101-105: The test currently only checks that
db.apply_batch(vec![op], None, None, grove_version) returns an error; tighten it
to assert the specific validation failure for inserting a NonCounted into a
NormalTree by extracting the error (e.g., via result.unwrap_err() or pattern
matching) and asserting its variant/message corresponds to the
NonCounted/parent-type validation failure (reference apply_batch, op, NonCounted
and NormalTree) so unrelated errors don't make the test pass.
- Around line 109-115: The test
batch_propagation_preserves_non_counted_wrapper_on_subtree currently only checks
that the NonCounted wrapper is preserved; add an assertion after the batch
commit that the outer count-tree aggregate equals 1 (i.e., only the plain
counted child is counted) to ensure the count exclusion invariant; locate the
code around the batch application and the on-disk parent retrieval in function
batch_propagation_preserves_non_counted_wrapper_on_subtree and assert the parent
count/aggregate is 1. Apply the same additional assertion to the other test in
this file that similarly verifies wrapper persistence so both wrapper and
count-exclusion invariants are covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8bd81d79-a0dc-4474-888c-1b56c6f2905c
📒 Files selected for processing (18)
grovedb-element/src/element/helpers.rsgrovedb-element/src/element/mod.rsgrovedb-element/src/element/serialize.rsgrovedb-element/src/element_type.rsgrovedb/src/batch/estimated_costs/average_case_costs.rsgrovedb/src/batch/estimated_costs/worst_case_costs.rsgrovedb/src/batch/mod.rsgrovedb/src/debugger.rsgrovedb/src/operations/get/mod.rsgrovedb/src/replication.rsgrovedb/src/tests/batch_coverage_tests.rsgrovedb/src/tests/batch_rejection_tests.rsgrovedb/src/tests/batch_unit_tests.rsgrovedb/src/tests/mod.rsgrovedb/src/tests/non_counted_tests.rsmerk/src/element/reconstruct.rsmerk/src/proofs/query/verify.rsmerk/src/test_utils/mod.rs
✅ Files skipped from review due to trivial changes (5)
- merk/src/proofs/query/verify.rs
- grovedb/src/debugger.rs
- grovedb/src/replication.rs
- grovedb/src/tests/batch_coverage_tests.rs
- grovedb/src/batch/estimated_costs/worst_case_costs.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- grovedb-element/src/element/serialize.rs
- grovedb-element/src/element/mod.rs
- grovedb-element/src/element/helpers.rs
… tests) CodeRabbit follow-up review on 10d3821 surfaced three more issues: 1. ElementType::from_serialized_value accepted invalid inner discriminants. The bitwise OR `Self::try_from(NON_COUNTED_FLAG | inner_byte)` is a no-op when `inner_byte` already has bit 7 set. A payload like `[15, 128, ...]` silently parsed as `NonCountedItem` (since 0x80 | 0x80 == 0x80) instead of being rejected. Fix: explicitly require `inner_byte < 15` (i.e. a real base discriminant 0..=14) before applying the bit. New rejection tests cover [15, 128], [15, 142], [15, 16], [15, 100]. 2. GroveOp::RefreshReference rejected wrapped references. The arm matched `&element` directly as `Element::Reference`, so a stored `NonCounted(Reference)` failed refresh as "not a reference". Fix: match `element.underlying()` so the wrapper is transparent for refresh — the inner reference's path is what matters. 3. check_subtree_exists rejected wrapped tree parents. Although get_caching_optional now unwraps NonCounted, the subtree validation helper still only accepted bare tree variants. Any path through a `NonCounted(Tree)` parent failed validation, breaking the wrapper-transparency contract. Fix: `element.map(|e| e.into_underlying())` before matching tree variants. Regression test `check_subtree_exists_through_non_counted_wrapper` inserts a child into a wrapped subtree and asserts it succeeds. Also tightened existing tests per nitpicks: - batch_insert_rejects_non_counted_into_normal_tree now asserts the error message contains "non-counted" (catches the parent-type guard specifically, not any unrelated batch failure). - batch_propagation_preserves_non_counted_wrapper_on_subtree now also reads the outer count tree's aggregate and asserts it equals 1 (i.e. the NonCounted subtree did NOT contribute to the count). This protects both invariants the test documents. Test counts (with --features minimal,test_utils,full): - grovedb-element: 45 (unchanged; from_serialized_value rejection cases added to existing test) - grovedb-merk: 364 (unchanged) - grovedb: 1453 (was 1452, +1: check_subtree_exists regression) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
Codex follow-up review: I re-fetched the updated PR at
Focused verification run:
All passed. The |
Targeted audit (security-auditor agent) found 5 categories of catchall match arms over Element where NonCounted was silently rejected or mis-handled instead of dispatching through to the inner element. HIGH: typed non-Merk tree APIs rejected wrapped trees The mmr_tree_*, bulk_*, commitment_tree_*, and dense_tree_* methods all matched bare variants like `Element::MmrTree(..)` and treated `NonCounted(MmrTree)` as "not the expected tree", returning InvalidInput. NonCounted-wrapped non-Merk trees are a perfectly valid use case (insert a wrapped MmrTree inside a CountTree to suppress its count contribution), so this broke every read/write path. - operations/mmr_tree.rs (5 sites) - operations/bulk_append_tree.rs (7 sites) - operations/commitment_tree.rs (5 sites) - operations/dense_tree.rs (5 sites) Fix: dispatch via element.underlying() / into_underlying() everywhere. Test: typed_mmr_api_works_through_non_counted_wrapper exercises a wrapped MmrTree end-to-end via mmr_tree_leaf_count. HIGH: query_encoded_many rejected wrapped references and resolved items operations/get/query.rs:90 matched QueryResultElement::ElementResultItem(Element::Reference(..)) directly, so any wrapped reference fell into the catchall as "path_queries can only refer to references". The resolved value at line 106 had the same problem. Fix: extract the element first then match .into_underlying(), mirroring the pattern already used elsewhere in the file. MEDIUM: aggregate_sum_query rejected NonCounted-wrapped sum items element/aggregate_sum_query/mod.rs:775 — exactly the kind of element the wrapper exists for (suppress count, keep sum). Now matches .into_underlying(). MEDIUM: batch flag-update returned wrong/None cost for wrapped trees batch/mod.rs:2443 — the just-in-time flag-update path dispatched on bare new_element and routed `NonCounted(Tree)` through the `_ => None` arm, dropping the layered cost. Fix: capture wrapper_overhead, then match on .underlying() and add wrapper_overhead to value_len in each constant-size branch (parallel to merk/src/element/costs.rs). MEDIUM: estimated_costs replace paths inconsistent with insert paths estimated_costs/average_case_costs.rs:329 (replace_element) and estimated_costs/worst_case_costs.rs:189, 244 (insert_element + replace_element) all routed `NonCounted(Tree)` through the serialized_size fallback instead of the layered cost arm. Average's insert path was already correct (uses tree_flags_and_type) — the replace paths and worst_case insert/replace were inconsistent. Fix: same wrapper_overhead pattern + match .underlying(). Test counts (with --features minimal,test_utils,full): - grovedb-element: 45 (unchanged) - grovedb-merk: 364 (unchanged) - grovedb: 1454 (+1: typed_mmr_api_works_through_non_counted_wrapper) Clippy clean with --workspace --all-features. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
Self Reviewed |
QuantumExplorer
added a commit
to dashpay/platform
that referenced
this pull request
May 9, 2026
…ange) Brings in dashpay/grovedb#654 (Element::NonCounted wrapper) and #656 (QueryItem::AggregateCountOnRange + Node::HashWithCount). Both are prerequisites for the `range_countable` index property that the parallel design work in `book/src/drive/indexes.md` depends on: - `Element::NonCounted(Box<Element>)` — wrapper variant whose count contributes 0 to a parent count tree's aggregate. Lets a count tree hold housekeeping rows / sibling sub-property continuations without polluting the count. Only insertable into count-bearing trees; nested wrappers rejected at construction / serialize / deserialize. - `QueryItem::AggregateCountOnRange(Box<QueryItem>)` — count-only proof shape returning `(CryptoHash, u64)` in O(log n) bytes. Backed by a new self-verifying `Node::HashWithCount(kv_hash, l, r, count)` proof node so the count is bound by the proof, not trusted on faith. Restricted to `ProvableCountTree` / `ProvableCountSumTree` (and their `NonCounted*` wrappers) at proof time. Verified via `GroveDb::verify_aggregate_count_query`. Together these unblock implementing `range_countable` indexes (per- node counts on the property-name tree, NonCounted wrappers for sibling continuations) and `return_distinct_counts_in_range` / range count queries on the no-prove and prove paths — both currently gated as "not yet supported" in the unified count handler. Workspace fixups required by the bump: - `wasm-drive-verify` JS shim: add a `QueryItem::AggregateCountOnRange` arm in `serialize_query_item` (descriptive type, no recursion into the inner range — the wasm verify path doesn't drive these queries today, but the variant must be matched for the workspace to compile). - `rs-sdk-ffi` path-elements display: add `Element::NonCounted(_)` arm reporting `"non_counted"` (placeholder display; we'll inflate it to describe the inner element when the wrapper is actually used in contracts). - `rs-drive-abci` shielded common: orchard's transitive bump made `Action::from_parts` return `Option<Action>`. Wrap with `.ok_or_else` surfacing `InvalidShieldedProofError("invalid action parts")` rather than panicking; otherwise behaviorally unchanged. Tests: 14 rs-drive count-query lib tests, 5 drive-abci handler tests, 3079 rs-drive lib tests, and 3435 dpp lib tests all still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
QuantumExplorer
added a commit
to dashpay/platform
that referenced
this pull request
May 10, 2026
…ing) Per-index `rangeCountable: bool` flag, additive on top of `countable`. When true, the index is laid out so that range-count queries on the indexed property can be answered in O(log n): - Property-name level: `ProvableCountTree` (per-node counts let a range query walk just the boundary path). - Each value tree under it: `CountTree` (count-bearing so the property-name aggregate sums per-value counts cleanly). - Sibling continuations inside a value tree: wrapped with `Element::NonCounted` so their counts don't pollute the value tree's count. Depends on the grovedb features bumped in the previous commit (`Element::NonCounted` + `QueryItem::AggregateCountOnRange` from dashpay/grovedb#654 + #656). This commit lands the schema-level plumbing only: - `Index.range_countable: bool` field + serde derive. - Index parser reads `"rangeCountable"` (boolean only — no enum form needed). - Cross-field validation in `Index::try_from`: `range_countable: true` requires `countable.is_countable()`. Without that, it would change layout of a non-count-bearing tree, which is meaningless. - v1 meta-schema schema entry under each index in `documentSchemas`. - Protocol-version gate in `try_from_schema/v1`: `range_countable: true` on protocol_version < 12 raises `UnsupportedFeatureError`. Pre-v12 nodes therefore reject the contract at validation time, before any state mutation. Mirrors the existing v12 gate on countable indexes. - `IndexLevelTypeInfo.range_countable` populated from the source index so the insert/delete walkers can reach it (used in a follow-up). - `random_index` default + ~16 IndexLevel test-init sites updated. Storage layout change (the actual `NonCounted` wrapping + `ProvableCountTree`/`CountTree` selection in the insert / delete walkers) is **deferred to a follow-up commit**. Until that lands, `IndexLevelTypeInfo.range_countable` is read but not yet acted on — the on-disk layout is unchanged, so the schema gate is the only gate in effect right now. Combined with the v12 protocol gate, no v11 node ever sees a `range_countable` contract, and no v12 node yet emits NonCounted-wrapped writes. Tests: 79 dpp index tests, 14 rs-drive count-query lib tests, 5 drive-abci handler tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
QuantumExplorer
added a commit
to dashpay/platform
that referenced
this pull request
May 10, 2026
Adds the foundational helper for the upcoming `range_countable` storage layout, plus a runtime guard that fails loudly if a v12+ contract with `range_countable: true` reaches the insert walker before the rest of the storage-layout work lands. - `LowLevelDriveOperation::for_known_path_key_empty_non_counted_normal_tree`: builds a `GroveOperation` that inserts `Element::NonCounted(empty_tree())` at the given path and key. The wrapper makes the inserted subtree contribute 0 to a parent count tree's aggregate (per dashpay/grovedb#654), which is what the index walker needs for sibling continuations under a `range_countable` value tree (e.g., the `'shape'` continuation under a `byColor` value tree, when `byColor` is range_countable but `byColorShape` shares its prefix). Construction is infallible by `new_non_counted`'s contract — the `expect` documents the invariant. - `add_indices_for_top_index_level_for_contract_operations_v0` and `add_indices_for_index_level_for_contract_operations_v0`: both now inspect `sub_level.has_index_with_type().range_countable` and return `DriveError::NotSupported` if true, with TODO comments pointing to the exact lines that need to switch tree types and the helper to use for NonCounted wrapping. Belt-and-suspenders alongside the rs-dpp v12 validation gate added in the previous commit — pre-v12 nodes already reject the contract; on v12+ the contract reaches here and we refuse rather than corrupt the count aggregation by writing a NormalTree where a CountTree / ProvableCountTree / NonCounted is required. Tests: 79 dpp + 14 rs-drive + 5 drive-abci tests still pass. Next chunks (still TODO on this PR — best as separate focused commits): - Insert walker: switch property-name tree to ProvableCountTree, value tree to CountTree, and wrap sibling continuations with NonCounted when `IndexLevelTypeInfo.range_countable` is true. Threads a `parent_value_tree_is_count_bearing` flag through recursion. - Same in cost-estimation paths (`EstimatedLayerInformation.tree_type`). - Mirror in delete (`remove_*_for_index_level_*`). - Count picker: accept `range_countable` indexes for range operators. - `DriveDocumentCountQuery::execute_no_proof` range mode via grovedb's `AggregateCountOnRange` query item. - Drive-abci handler: route `return_distinct_counts_in_range = true` to the new range-mode logic instead of erroring. - Drop the `u16::MAX` materialization cap on prove path for range counts via `verify_aggregate_count_query`. - Tests covering count-aggregation correctness with NonCounted siblings. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
QuantumExplorer
added a commit
that referenced
this pull request
May 10, 2026
) * feat: add Element::NotSummed wrapper to opt-out of sum propagation Symmetric counterpart to Element::NonCounted (#654). Where NonCounted opts a child out of count propagation in count-bearing trees, NotSummed opts a sum-bearing subtree out of sum propagation in sum-bearing trees. The wrapper is strictly typed: it can ONLY contain one of the four sum-tree variants (SumTree, BigSumTree, CountSumTree, ProvableCountSumTree), and may only be inserted into sum-bearing parent trees. Items, sum items, references, non-sum trees, and any wrapper nesting are rejected at construction, serialization, and deserialization. When a NotSummed-wrapped sum-tree is inserted into a sum-bearing parent, it contributes 0 to the parent's running sum. Counts (if any) still propagate. The inner sum-tree retains its own internal sum aggregate unchanged — only the outward propagation is suppressed. Discriminant scheme: - bincode 16 (Element variant) - ElementType twins at 0x40 | base: 68, 69, 71, 74 (only the four legal sum-tree base discriminants). The 0x40 flag bit is disjoint from NonCounted's 0x80, so wrapper status is independently testable. Stack-overflow defense: deserialize pre-checks the leading two bytes for any wrapper combination ([15,15], [15,16], [16,15], [16,16]) and rejects before bincode recurses through the Box<Element> chain. Wiring: - grovedb-element: enum + constructor whitelist + serialize/deserialize guards + helpers (sum_value_or_default → 0, count still propagates) + ElementType twins + visualize. - grovedb-query: TreeFeatureType::zero_sum() symmetric helper. - merk: TreeType::is_sum_bearing(), insert/insert_subtree/ insert_reference parent-type guards, reconstruct preserves wrapper, costs/get account for the wrapper byte. - grovedb: batch propagation tracks not_summed alongside non_counted in InsertTreeWithRootHash, parent-type guard, estimated cost overhead, debugger renders inner element transparently. Tests (725+ existing pass, plus new): - grovedb-element: constructor whitelist, helper passthrough, bincode round-trip, deserialize rejects nested chains and non-sum-tree inner. - grovedb-merk: insert rejected in NormalTree/CountTree, accepted in SumTree contributes 0, NotSummed(SumTree(_,100,_)) in ProvableCountSumTree → count=1 sum=0, reconstruct preserves wrapper. - grovedb: 5 end-to-end tests covering batch parent-type guard, batch propagation preserving the wrapper, check_subtree_exists through the wrapper, and outer sum aggregate excluding the subtree. Breaking changes: none for existing callers. The on-disk serialization format gains bincode discriminant 16 — older code reading data written with this variant fails to deserialize, matching the existing "ONLY APPEND TO THIS LIST" comment on Element. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: add coverage for zero_sum, NotSummed Display/element_type, and tree extensions CodeRabbit's Codecov pass on PR #659 flagged 86.42% patch coverage. Most gaps were unreachable!() arms (genuinely uncoverable), but several testable additions had no direct unit tests yet. - grovedb-query/src/proofs/tree_feature_type.rs: new tests module covering both new and existing helpers — `zero_sum` (each variant + no-op cases), `zero_count` (mirror), `count`. The file had no test module previously. - grovedb-element/src/element/mod.rs: new tests for `Display` of `NotSummed` and `NonCounted`, plus `element_type()` resolving to the correct `NotSummedXxx` / `NonCountedXxx` synthetic twin for each legal inner. - merk/src/element/tree_type.rs: tests asserting all `ElementTreeTypeExtensions` methods (`tree_type`, `maybe_tree_type`, `root_key_and_tree_type{,_owned}`, `tree_flags_and_type`, `tree_feature_type`) delegate through the `NotSummed` wrapper, plus `get_feature_type` zeros sum (and propagates count where applicable) for all four sum-bearing parent tree types. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(reference_path): look through wrappers when following reference chains `reference_path::follow_reference` matched on the resolved `element` directly, so a `NonCounted(Reference)` would have been returned as a target value rather than followed as a hop. The function is currently only used from tests (production reference resolution goes through `GroveDb::follow_reference` in `operations/get/mod.rs`, which already unwraps via `into_underlying()`), but the divergence was a latent trap for any future wiring. Symmetric to the existing get-path unwrap. The wrapper byte is part of `value_hash` (computed from the on-disk bytes earlier in the function), so dropping it from `target_element` does not affect cryptographic verification of subsequent hops. `NotSummed` cannot wrap a reference (constructor whitelist enforces sum-tree variants), so this fix is purely forward-safe and symmetric to the `NonCounted` handling. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(element): close cross-wrapper construction + serde validation gaps Addresses Codex's review of PR #659. **P1 — Reject cross-wrapper construction** `new_non_counted` previously rejected only `NonCounted` inners, so a caller could build `NonCounted(NotSummed(SumTree))` even though serialization rejects cross-wrapper nesting. Such a value would reach batch execution with `is_non_counted() == true` and `underlying() == NotSummed`, then hit the supposedly-unreachable wrapper arm. - `Element::new_non_counted` now rejects both `NonCounted` and `NotSummed` inners with `InvalidInput`. - `Element::into_non_counted` is now fallible (`Result<Self, ElementError>`) and returns `Err` for `NotSummed` input. Two in-tree callers in `grovedb/src/batch/mod.rs` already pass freshly-constructed bare tree elements; updated to `.expect(..)` with a comment documenting the precondition. - New regression in `grovedb/src/tests/not_summed_tests.rs` builds a hand-rolled `NonCounted(NotSummed(SumTree))` (bypassing the constructor) and asserts `apply_batch` rejects it as a typed error rather than panicking. - Two new unit tests in `grovedb-element/src/element/helpers.rs`: `into_non_counted_rejects_not_summed` and `new_non_counted_rejects_not_summed`. **P2 — Manual serde Deserialize with wrapper validation** The previous `derive(serde::Deserialize)` did not enforce the wrapper invariants. With the `serde` feature enabled, payloads could construct invalid `NotSummed(Item)` or cross-wrapper values, and deeply-nested wrapper payloads recursed before any check fired. - Replaced the derive on `Element` with a manual `Deserialize` impl in a `serde_impl` module, gated behind `feature = "serde"`. - The impl deserializes through a private `ElementShadow` mirror enum (`#[serde(rename = "Element")]` so the wire format is unchanged), converts to `Element`, then calls `Element::check_recursive_wrapper_invariants` to validate every level of the tree. - New public `Element::validate_wrapper_invariants` codifies the rules in one place (used by both the serde path and available to external callers). - `Serialize` derive is preserved — serialization of a valid Element is always safe. New serde-feature tests in `grovedb-element/src/element/mod.rs`: - `serde_round_trip_valid_elements` — JSON round-trip for Item, SumTree, NonCounted(Item), NotSummed(SumTree). - `serde_rejects_nested_non_counted` / `serde_rejects_nested_not_summed` — nested same-wrapper payloads. - `serde_rejects_cross_wrapper_nesting` — both `NonCounted(NotSummed(_))` and `NotSummed(NonCounted(_))`. - `serde_rejects_not_summed_with_non_sum_tree_inner` — six illegal inner types covering items, plain trees, count trees, MMR. - `serde_rejects_deeply_nested_wrapper_chain` — depth-64 chain rejected via the per-level check fired during recursive conversion. Adds `serde_json` as a dev-dependency for the JSON round-trip tests. Test counts: grovedb-element 63 lib (was 57 + 6 serde = 63), merk 418, grovedb 1496 (was 1495 + 1 regression). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(element): tighten NotSummed API + relocate twin discriminants Three follow-ups from review: **1. Replace `into_not_summed_unchecked` (panicking) with fallible `into_not_summed`.** The panic path was a latent foot-gun. The new helper mirrors `into_non_counted`'s API: returns `Result`, idempotent on `NotSummed`, returns `Err(InvalidInput)` for `NonCounted` (mutually exclusive) or any non-sum-tree variant. The single in-tree caller (batch propagation) now uses `.expect(..)` with a comment documenting that the input is always a freshly-constructed bare sum-tree element. **2. Move `NotSummed` twin discriminants from `0x40 | base` to `0xb0 | base`.** This places the four twins at 180, 181, 183, 186 — inside the high-bit-set range alongside `NonCounted` twins (128..142), rather than down at 64..78 mid-range. The two ranges are still distinguished by the upper nibble: `0x80` for `NonCounted`, `0xb0` for `NotSummed`. Detection is now an upper-nibble compare instead of a single-bit test: - `is_non_counted`: `disc & 0xf0 == 0x80` (was `disc & 0x80 != 0`). - `is_not_summed`: `disc & 0xf0 == 0xb0` (was `disc & 0x40 != 0`). The single-bit `is_non_counted` would have started returning true for `NotSummed` twins too once they shared bit 7, which would have been wrong. Upper-nibble compares keep the predicates disjoint. Renamed the now-misnamed `NOT_SUMMED_FLAG` constant to `NOT_SUMMED_TWIN_PREFIX` (`= 0xb0`) and updated `NOT_SUMMED_BASE_MASK` to `0x0F` (sufficient since base discriminants are 0..14). Updated the `try_from`, `from_serialized_value`, the discriminant-pinning test, and added a new `test_not_summed_helpers` mirroring `test_non_counted_helpers`. **3. Remove unused `TreeFeatureType::zero_sum` helper.** It was added preemptively for symmetry with `zero_count` but had no production callers — `Element::sum_value_or_default` already returns 0 for `NotSummed`, so `get_feature_type` produces the right zero-summed feature type without needing this helper. Removed the function and its unit test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(element): explain why the serde shadow enum is necessary The `serde_impl` module already had a comment describing the *approach* (shadow + From conversion + recursive validation), but not the *why* — which serde patterns fail and which alternatives exist. Codifies the trade-off so future maintainers can see at a glance that: - `#[serde(try_from)]` needs a separate source type (no self-pointing). - `#[serde(remote)]` is for foreign types only. - `#[serde(deserialize_with)]` is field-level. - `#[serde(transparent)]` / `flatten` are for single-field structs. Leaving three real options for derive-and-validate: shadow enum, manual Visitor, or drop the derive. We keep the derive (external tooling consumers may rely on it) and use the shadow as the shortest correct form. Doc-only change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(batch): replace .expect on wrapper conversions with typed errors The three `.expect(..)` calls added in commit 4a93a88 around the batch propagation wrapper-rewrap path would panic if a future change ever passed a pre-existing wrapper element through these branches. Replace them with `CorruptedCodeExecution` errors so the failure surfaces as a typed `Result` and bubbles up via the cost-aware return machinery instead of aborting the process. Sites: - `GroveOp::InsertTreeWithRootHash` propagation: `into_non_counted` and `into_not_summed` calls now `map_err(..)` to `CorruptedCodeExecution`, drained via `cost_return_on_error_no_add!`. - `GroveOp::InsertNonMerkTree` propagation: same treatment for the `into_non_counted` call. Behavior in the happy path is unchanged — the input elements are freshly built from `aggregate_data` / `meta.to_element(..)`, never wrapped, and the wrappers always succeed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <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
GroveDB count-bearing trees (
CountTree,ProvableCountTree,CountSumTree,ProvableCountSumTree) currently aggregate every child element into the parent's count. There's no way to insert a value that participates in storage and proofs but is exempt from the count — e.g. housekeeping rows, schema markers, or auxiliary indexes inside a count tree.This adds that escape hatch.
What was done?
Added a new
Element::NonCounted(Box<Element>)variant that:Design choice: single wrapper variant (vs. ~15 explicit variants)
A wrapper means one new discriminant and one dispatch point for the "is non-counted?" question. Existing
Elementpredicates (`is_tree`, `is_sum_item`, `is_any_item`, `is_non_empty_tree`, ...) keep working by looking through the wrapper via a new `underlying()` helper. Pattern-match sites in cost, proof, query, batch, and insert paths dispatch on the underlying element.Cryptographic notes
The wrapper byte is included in the serialized value, so the value hash distinguishes `NonCounted(X)` from a bare `X`. The parent's `TreeFeatureType` for a non-counted child is the inner element's feature type with its count zeroed (via a new `TreeFeatureType::zero_count()` helper); no new `TreeFeatureType` variants are introduced.
Nested `NonCounted(NonCounted(...))` is rejected at construction, serialization, and deserialization to close a stack-overflow vector.
Scope
Touched 18 files across `grovedb-element`, `grovedb-query`, `grovedb-merk`, and `grovedb`:
How Has This Been Tested?
New tests:
Full workspace test suite still passes:
`cargo clippy` is clean for the changed code (3 pre-existing warnings in unrelated files).
Breaking Changes
None for existing callers. The change adds a new `Element` variant and is additive at the API surface. The on-disk serialization format gains a new bincode discriminant (15) — older code reading data written with this variant would fail to deserialize, so producer/consumer pairs need to be upgraded together. This matches the existing "ONLY APPEND TO THIS LIST" comment on the `Element` enum.
Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Behavior Changes
Validation & Safety
Tests