feat: add Element::NotSummed wrapper to opt-out of sum propagation#659
Conversation
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>
|
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 (29)
✨ 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 review This is Claude. The PR is intentionally in draft while we wait for the first review pass — please review it now. |
|
✅ Actions performedReview triggered.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #659 +/- ##
===========================================
+ Coverage 90.64% 90.74% +0.10%
===========================================
Files 184 184
Lines 54826 55532 +706
===========================================
+ Hits 49699 50395 +696
- Misses 5127 5137 +10
🚀 New features to boost your workflow:
|
…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>
QuantumExplorer
left a comment
There was a problem hiding this comment.
Codex review findings:
- [P1] Reject cross-wrapper construction (
grovedb-element/src/element/constructor.rs:393-407)
new_non_counted only rejects NonCounted, so it can still construct NonCounted(NotSummed(...)) even though serialization rejects cross-wrapper nesting. That value can reach batch execution with is_non_counted() == true and underlying() == NotSummed, then hit the supposedly-unreachable wrapper arm instead of returning a typed error.
Suggested fix: make new_non_counted reject both wrapper variants, and make into_non_counted avoid wrapping NotSummed as well, either by making it fallible or by adding a separate checked helper for external callers. Add a batch regression for NonCounted(NotSummed(SumTree)) to confirm it returns an InvalidInput / InvalidBatchOperation instead of panicking.
- [P2] Add serde-side wrapper validation (
grovedb-element/src/element/mod.rs:45-47)
The bincode serialize/deserialize paths now reject nested wrappers and NotSummed with a non-sum-tree inner, but the serde feature still derives recursive Deserialize for Element. With serde enabled, a payload can construct invalid NotSummed(Item) / cross-wrapper values, and deeply nested wrapper payloads still recurse before any validation runs.
Suggested fix: replace the derived serde deserialize path with a manual visitor or serde helper wrappers that enforce the same invariants as Element::deserialize: wrapper inners must not be wrappers, and NotSummed may only wrap SumTree, BigSumTree, CountSumTree, or ProvableCountSumTree. Add serde-feature tests for nested wrapper rejection and NotSummed(non_sum_tree) rejection.
…hains `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>
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>
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>
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>
…rors 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>
|
Reviewed |
Two PRs landed on develop while this PR was open: - #659: Element::NotSummed wrapper variant - #658: aggregate_count proof verification under verify feature Conflicts resolved in 8 files. The substantive resolution work: DISCRIMINANT COLLISION. The shipped cidx PR used ElementType discriminant 16 for CountIndexedTree and 17 for ProvableCountIndexedTree. Develop's NotSummed PR allocated byte 16 as NOT_SUMMED_WRAPPER_DISCRIMINANT. Both are still pre-shipping, so renumbering is safe — and since the NotSummed wrapper byte semantics need to round-trip across the wire, the wrapper byte stays at 16 and the cidx discriminants shift: CountIndexedTree: 16 → 17 ProvableCountIndexedTree: 17 → 18 NonCountedCountIndexedTree: 144 → 145 (= 0x80 | 17) NonCountedProvableCountIndexedTree: 145 → 146 (= 0x80 | 18) ENUM VARIANT ORDER. Bincode encodes Element variants by ENUM ORDER (not by ElementType discriminant). The Element enum has been reordered so NotSummed appears at variant index 16 (matching NOT_SUMMED_WRAPPER_DISCRIMINANT), and CountIndexedTree / ProvableCountIndexedTree appear AFTER NotSummed at indices 17/18 — matching their new ElementType discriminants. Without this reordering, bincode would still write 16 for CountIndexedTree but from_serialized_value would interpret 16 as the NotSummed wrapper. WRAPPER NESTING CHECK in `from_serialized_value`: now explicitly rejects both nested NonCounted and cross-nesting with NotSummed (inner_byte == 15 || inner_byte == 16), in addition to the existing reject-high-bit-twin guard. Other resolutions are straightforward additions: both branches added arms to match expressions across helpers.rs, tree_type.rs, visualize.rs; merged additively. The two `mod tests` blocks in grovedb-element/src/element/mod.rs (one from each PR) renamed the cidx-side block to `cidx_tests` to avoid duplicate name. All 1944+ workspace tests pass post-merge. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
[P1] 247-byte ceiling on cidx primary item keys (direct + batch). Secondary keys are (count_be ‖ item_key); Merk requires keys < 256 bytes. Generic batch validation only enforces 255-byte cap, so cidx primaries need an 8-byte stricter ceiling. Added MAX_CIDX_ITEM_KEY_LEN = 247 and validate_cidx_item_key_len. Enforced on insert_into_count_indexed_tree_on_transaction and on execute_ops_on_path when in_tree_type is cidx primary. [P1] insert_into_count_indexed_tree overwrite cleanup. The dedicated API read existing element only for count delta — didn't clean up old tree storage when replacing a tree entry. Mirrors batch safe-subset semantics: - existing tree, new non-tree: ALLOW + cleanup - existing tree, new empty tree (root_key=None): ALLOW + cleanup - existing cidx, new non-empty cidx: REJECT (ambiguous) - existing tree, new non-empty non-cidx tree: REJECT (ambiguous) Cleanup mirrors db.delete/batch DeleteTree: find_subtrees + clear, plus secondary namespace clear for existing-cidx. [P2] Batch consistency check for safe-subset overwrite descendants. When scheduling cidx-overwrite cleanup, scan ops_by_qualified_paths for descendants of the cidx path and reject as InvalidBatchOperation if found. Defense in depth; the audit's worry about silent cleanup-drop is already caught by other checks in most flavors, but the new check provides a clearer error message. [P2] verify_grovedb duplicate-secondary detection. Changed HashMap<Vec<u8>, u64> to HashMap<Vec<u8>, Vec<u64>> so duplicate secondary rows for the same primary key (real drift class) are flagged via __cidx_secondary_duplicate__ sentinel path. [P2] appendix-a.md: updated discriminants 16/17/144/145 → 17/18/ 145/146, added NotSummed wrapper byte row, added 247-byte ceiling note. Now matches code (post-merge with #659). All 1611 grovedb tests pass; 5 new audit-targeted tests added. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Issue being fixed or feature implemented
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.
Use case: a SumTree of category subtotals, where one inner sum-tree should be informational only — it tracks its own sum but does not bubble up to the parent total.
What was done?
Added a new
Element::NotSummed(Box<Element>)variant that:SumTree,BigSumTree,CountSumTree,ProvableCountSumTree),SumTree,BigSumTree,CountSumTree,ProvableCountSumTree).Design choice: stricter inner-type whitelist than NonCounted
NonCounted can wrap any element (including items, references, all tree types). NotSummed restricts the inner to the four sum-tree variants because that is where the wrapper is semantically meaningful — a sum-bearing subtree that retains its own internal sum but contributes nothing to the parent. Wrapping items or non-sum trees would have no effect distinct from using the bare type.
The constructor
Element::new_not_summedreturnsResultand rejects everything else withInvalidInput. Serialization and deserialization enforce the same whitelist, plus an O(1) two-byte pre-check that rejects all four wrapper-on-wrapper combinations ([15,15],[15,16],[16,15],[16,16]) before bincode can recurse — closing a stack-exhaustion vector.Discriminant scheme
Element)ElementTypeflag bit0x800x40The disjoint flag bits keep
is_non_counted()andis_not_summed()independently testable onElementType.base()strips whichever flag is set. The two wrappers are mutually exclusive in practice — constructors and (de)serializers reject any nesting, including cross-nestingNotSummed(NonCounted(_))andNonCounted(NotSummed(_)).Cryptographic notes
The wrapper byte is part of the serialized value, so the value hash distinguishes
NotSummed(X)from a bareX. The parent'sTreeFeatureTypefor a not-summed child is the inner element's feature type with its sum component zeroed (via a newTreeFeatureType::zero_sum()helper, symmetric tozero_count()); no newTreeFeatureTypevariants are introduced.Scope
Touched 27 files across
grovedb-element,grovedb-query,grovedb-merk, andgrovedb. Mirrors PR #654's wiring throughout the stack:TreeType::is_sum_bearing(), insert validation, feature-type dispatch, get/cost paths, reconstruct preserves wrappernot_summedflag alongsidenon_countedinInsertTreeWithRootHash, parent-type guard, estimated cost overhead, debugger transparencytests/{batch_unit,batch_coverage,batch_rejection}_tests.rsupdated for the new fieldHow Has This Been Tested?
New tests:
is_*,sum_value_or_default → 0,count_value_or_defaultstill propagates,count_sum_value_or_default → (count, 0), flag accessors); bincode round-trip; reject nested wrappers at deserialize and at serialize; long-chain pre-check stack-overflow guard.NOT_SUMMED_WRAPPER_DISCRIMINANT = 16and the four0x40 | basetwin discriminants; rejects all illegal inner bytes including the wrapper bytes themselves and the synthetic twin range.NormalTreeandCountTreeis rejected; insert intoSumTreesucceeds and contributes 0;NotSummed(SumTree(_,100,_))in aProvableCountSumTreeyields count=1 sum=0;reconstruct_with_root_keypreserves the wrapper through propagation; constructor rejects non-sum-tree inner.tests/not_summed_tests.rs, 5 tests): batch insert rejected into NormalTree and CountTree; direct insert in SumTree excludes subtree sum from parent aggregate; batch propagation preserves wrapper throughInsertTreeWithRootHash(with both wrapper insert AND child insert in the same batch);check_subtree_existsworks through the wrapper.Full workspace test suite passes:
cargo test -p grovedb-element— 74 passed (57 unit + 17 integration)cargo test -p grovedb-query— 235 passedcargo test -p grovedb-merk --features minimal,test_utils,full— 416 passedcargo test -p grovedb— 1495 passedBreaking Changes
None for existing callers. The change adds a new
Elementvariant and is additive at the API surface. The on-disk serialization format gains bincode discriminant 16 — 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 theElementenum.The internal
GroveOp::InsertTreeWithRootHashvariant gains anot_summed: boolfield; the variant is#[non_exhaustive]and only constructed inside this crate, but the four in-tree test sites that build it directly are updated.Checklist:
🤖 Generated with Claude Code