feat(grovedb,merk,element): add Element::NotCountedOrSummed wrapper for combined sum+count opt-out#666
Merged
Conversation
…opt-out Adds a third element wrapper that suppresses BOTH count AND sum contributions when inserted into a count-and-sum-bearing parent. The existing wrappers each cover one axis: NonCounted suppresses count, NotSummed suppresses sum. ## Element variant ```rust Element::NotCountedOrSummed(Box<Element>) ``` - Inner element MUST be one of the four sum-bearing tree variants: `SumTree`, `BigSumTree`, `CountSumTree`, `ProvableCountSumTree`. (Same allow-list as `NotSummed`; `SumTree` / `BigSumTree` have an implicit count of 1 that this wrapper can suppress.) - Parent at insert time MUST be `CountSumTree` or `ProvableCountSumTree` — the only tree types where suppressing both axes is meaningful. Any other parent (including `NormalTree`, `SumTree`, `BigSumTree`, `CountTree`, `ProvableCountTree`) rejects the wrapper at insert with `InvalidInputError` (per-merk) or `InvalidBatchOperation` (batch). - The three wrappers are mutually exclusive — constructors and (de)serializers reject any nesting in any direction. Aggregation suppression flows through the standard helpers: - `count_value_or_default` returns 0 - `sum_value_or_default` / `big_sum_value_or_default` return 0 - `count_sum_value_or_default` returns (0, 0) ## Discriminant layout - Wrapper byte: 17 (after `NonCounted`=15 and `NotSummed`=16) - Twin prefix: `0xc0` (so synthetic twins are `NotCountedOrSummedSumTree`=196, `NotCountedOrSummedBigSumTree`=197, `NotCountedOrSummedCountSumTree`=199, `NotCountedOrSummedProvableCountSumTree`=202). - Upper-nibble compare distinguishes the three wrapper-twin ranges (`0x80` / `0xb0` / `0xc0`); the deserialize pre-check rejects every ordered pair of wrapper bytes to bound bincode recursion. ## Wiring - `grovedb-element`: constructor (`new_not_counted_or_summed`, `into_not_counted_or_summed`), helpers (`is_not_counted_or_summed`, `is_wrapped`), display, type-twin mapping, serde shadow, and serialize/deserialize guards. - `merk`: `TreeType::is_count_and_sum_bearing()` predicate; insert guards on `insert`/`insert_reference`/`insert_subtree`; `ElementReconstructExtensions`, `tree_type` extensions, cost paths, and v0/v1 `get` paths look through the new wrapper. - `grovedb`: `GroveOp::InsertTreeWithRootHash` gains a `not_counted_or_summed: bool` field carried through propagation and re-applied at execution via `into_not_counted_or_summed()`. Batch validation, query/proof/get match arms, debugger and estimated-cost paths all see the new wrapper. ## Tests - `grovedb-element`: discriminant pinning, helper predicates, serde round-trip and rejection cases. - `merk`: `TreeType::is_count_and_sum_bearing` matrix; per-merk insert acceptance in `CountSumTree`, rejection in `NormalTree`/`SumTree`/`CountTree`; aggregate-suppression assertion. - `grovedb`: end-to-end constructor invariants, batch insert rejection in non-count-sum parents, direct + batch insert in `CountSumTree` and `ProvableCountSumTree` with both axes suppressed, batch propagation preserves the wrapper on disk, and `check_subtree_exists` traverses through the wrapper. 421 merk lib tests pass, 83 grovedb-element tests pass, 1552 grovedb lib tests pass; clippy clean on the touched crates. 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 (7)
📝 WalkthroughWalkthroughThis PR introduces a new ChangesElement NotCountedOrSummed Wrapper & Integration
Sequence DiagramssequenceDiagram
participant API as Insertion API
participant Validator as Tree Type Validator
participant Execution as Wrapper Execution
participant Storage as Storage Layer
participant Parent as Parent Propagation
API->>Validator: insert Element::NotCountedOrSummed(SumTree)
Validator->>Validator: is_count_and_sum_bearing()?
alt Tree is CountSumTree or ProvableCountSumTree
Validator->>Execution: permit execution
Execution->>Execution: store wrapped element
Execution->>Storage: compute cost with wrapper overhead
Storage->>Storage: wrapped subtree contributes (0, 0)
Storage->>Parent: propagate not_counted_or_summed flag
Parent->>Parent: add to parent GroveOp::InsertTreeWithRootHash
else Tree lacks both axes
Validator->>API: Error::InvalidInputError
end
sequenceDiagram
participant Batch as Batch Execution
participant Reconstruct as Wrapper Reconstruction
participant Store as Element Storage
participant Verify as Verification
Batch->>Batch: execute GroveOp::InsertTreeWithRootHash
Batch->>Batch: check not_counted_or_summed flag
Batch->>Reconstruct: into_not_counted_or_summed(element)
Reconstruct->>Reconstruct: rewrap inner with new wrapper
Reconstruct->>Store: persist wrapped element
Store->>Verify: verify invariants on deserialization
Verify->>Verify: validate inner is sum-tree variant
Verify->>Verify: reject invalid wrapper nesting
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #666 +/- ##
===========================================
+ Coverage 90.77% 90.87% +0.09%
===========================================
Files 189 189
Lines 56466 57139 +673
===========================================
+ Hits 51255 51923 +668
- Misses 5211 5216 +5
🚀 New features to boost your workflow:
|
![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: 2
🧹 Nitpick comments (4)
merk/src/element/reconstruct.rs (1)
81-83: ⚡ Quick winAdd a wrapper-preservation test for
NotCountedOrSummed.This new branch is untested in this file; adding a symmetric test would lock in behavior and prevent wrapper-loss regressions.
Proposed test addition
#[test] fn reconstruct_preserves_not_summed_wrapper() { @@ } +#[test] +fn reconstruct_preserves_not_counted_or_summed_wrapper() { + let inner = Element::new_count_sum_tree_with_flags_and_count_sum_value(None, 7, 100, None); + let wrapped = Element::new_not_counted_or_summed(inner).expect("wrap ok"); + let new_root = Some(b"new_root".to_vec()); + let reconstructed = wrapped + .reconstruct_with_root_key(new_root.clone(), AggregateData::CountAndSum(7, 100)) + .expect("reconstruct ok"); + assert!(matches!(reconstructed, Element::NotCountedOrSummed(_))); + if let Element::NotCountedOrSummed(boxed) = reconstructed { + assert!(matches!( + *boxed, + Element::CountSumTree(ref k, 7, 100, _) if k == &new_root + )); + } +}🤖 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/element/reconstruct.rs` around lines 81 - 83, Add a unit test that mirrors the existing wrapper-preservation tests but for Element::NotCountedOrSummed: construct an Element::NotCountedOrSummed containing a known inner element, call reconstruct_with_root_key(maybe_root_key, aggregate_data) on it, and assert the result is still an Element::NotCountedOrSummed wrapping the reconstructed inner (i.e. the outer variant is preserved and contains the expected reconstructed inner). Use the same test harness/location as the other reconstruct tests and name it clearly (e.g., preserves_not_counted_or_summed_wrapper).merk/src/element/insert.rs (1)
180-186: ⚡ Quick winUse cost-return macros for the new validation early-returns.
These new guard exits should follow the repo’s cost-accumulation convention (
cost_return_on_error!family) instead of directreturn Err(...).wrap_with_cost(...)branches, for consistency with the project’s error-cost flow.As per coding guidelines, "Use
cost_return_on_error!macro for early returns with cost accumulation in Rust source files".Also applies to: 466-472, 573-579
🤖 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/element/insert.rs` around lines 180 - 186, Replace the direct early-return error branches that call return Err(...).wrap_with_cost(...) with the repository's cost-return macros (e.g., cost_return_on_error!) so cost accumulation follows project conventions; specifically, where code checks self.is_not_counted_or_summed() && !merk.tree_type.is_count_and_sum_bearing() and constructs Error::InvalidInputError, wrap that error construction in the appropriate cost_return_on_error! macro invocation instead of returning directly, and apply the same replacement pattern for the other similar guard exits around the regions referenced (the blocks at the locations corresponding to lines 466-472 and 573-579) so all validation early-returns use the cost-return macro family.grovedb/src/batch/mod.rs (1)
291-297: ⚡ Quick winEncode wrapper propagation state as one enum, not three booleans.
InsertTreeWithRootHashcan now represent impossible combinations, and reconstruction resolves them by branch order. If a future change ever sets two flags together, the wrong wrapper wins silently. A single internal enum/Option<WrapperKind>would make the exclusivity invariant explicit and simplify both propagation and re-wrapping.Also applies to: 2342-2373
🤖 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 291 - 297, The struct currently uses multiple booleans (not_summed, not_counted, not_counted_or_summed) to represent mutually exclusive wrapper state which allows impossible combinations in InsertTreeWithRootHash; replace the three booleans with a single enum (e.g. enum WrapperKind { None, NotCounted, NotSummed, NotCountedOrSummed } or Option<WrapperKind>) and update all places that set, propagate and re-wrap the element (including the InsertTreeWithRootHash type and the reconstruction/propagation code paths referenced around lines 2342-2373) to use that enum so exclusivity is enforced and reconstruction logic uses the enum variant order instead of resolving boolean conflicts. Ensure serialization/deserialization and any pattern matches are updated to the new enum shape and that callers that previously checked booleans now match on the enum or Option<WrapperKind>.grovedb/src/tests/not_counted_or_summed_tests.rs (1)
65-73: ⚡ Quick winHarden error assertions to avoid debug-string brittleness.
On Line 69, Line 105, and Line 141, these tests rely on
format!("{err:?}")substring checks. Prefer matching the concrete error variant (Error::InvalidBatchOperation/ typed guard error) so harmless message wording changes don’t break tests.Also applies to: 101-109, 137-145
🤖 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/not_counted_or_summed_tests.rs` around lines 65 - 73, Replace brittle debug-string checks of err (produced by apply_batch(...).unwrap().expect_err(...)) with a concrete pattern match against the typed error variant instead of substring matching; e.g. use matches!(err, Error::InvalidBatchOperation(..)) and further match the inner guard/typed error to assert the NotCountedOrSummed guard (or the exact guard enum variant name used in your code) so the test asserts the error variant directly rather than relying on format!("{err:?}"); apply this change in the three test locations that currently inspect msg (the err variable after apply_batch).
🤖 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/constructor.rs`:
- Around line 415-424: The idempotent match arms in into_non_counted currently
return Ok(self) without re-validating wrapper rules; before returning an
already-wrapped value run validate_wrapper_invariants() and propagate any error.
Update the Element::NonCounted(_) arm in into_non_counted to call
self.validate_wrapper_invariants() (or &self.validate_wrapper_invariants() as
appropriate) and only return Ok(self) if validation succeeds; apply the same
pattern to the analogous methods/match-arms (the idempotent arms handling
Element::NotSummed, Element::NotCountedOrSummed) in the other constructors (the
similar blocks at the other noted ranges) so every early-returning wrapped path
re-runs validate_wrapper_invariants() first.
In `@grovedb/src/batch/mod.rs`:
- Around line 1915-1924: The current guard in the batch insert path only checks
the parent tree type via in_tree_type.is_count_and_sum_bearing() when
element.is_not_counted_or_summed(), but does not validate the wrapped inner-tree
type; update the same branch in grovedb::batch::mod (where
element.is_not_counted_or_summed() is checked) to also verify that the
wrapped/inner tree kind is one of the allowed types (SumTree, BigSumTree,
CountSumTree, ProvableCountSumTree) and if not, return the same
Error::InvalidBatchOperation(...) wrapped with .wrap_with_cost(cost); ensure you
locate the inner-tree kind via the batch path metadata used there (the variable
used to inspect the child/wrapped tree) and perform the membership check
alongside the existing is_count_and_sum_bearing() check so invalid
Element::NotCountedOrSummed inserts are rejected at insert time.
---
Nitpick comments:
In `@grovedb/src/batch/mod.rs`:
- Around line 291-297: The struct currently uses multiple booleans (not_summed,
not_counted, not_counted_or_summed) to represent mutually exclusive wrapper
state which allows impossible combinations in InsertTreeWithRootHash; replace
the three booleans with a single enum (e.g. enum WrapperKind { None, NotCounted,
NotSummed, NotCountedOrSummed } or Option<WrapperKind>) and update all places
that set, propagate and re-wrap the element (including the
InsertTreeWithRootHash type and the reconstruction/propagation code paths
referenced around lines 2342-2373) to use that enum so exclusivity is enforced
and reconstruction logic uses the enum variant order instead of resolving
boolean conflicts. Ensure serialization/deserialization and any pattern matches
are updated to the new enum shape and that callers that previously checked
booleans now match on the enum or Option<WrapperKind>.
In `@grovedb/src/tests/not_counted_or_summed_tests.rs`:
- Around line 65-73: Replace brittle debug-string checks of err (produced by
apply_batch(...).unwrap().expect_err(...)) with a concrete pattern match against
the typed error variant instead of substring matching; e.g. use matches!(err,
Error::InvalidBatchOperation(..)) and further match the inner guard/typed error
to assert the NotCountedOrSummed guard (or the exact guard enum variant name
used in your code) so the test asserts the error variant directly rather than
relying on format!("{err:?}"); apply this change in the three test locations
that currently inspect msg (the err variable after apply_batch).
In `@merk/src/element/insert.rs`:
- Around line 180-186: Replace the direct early-return error branches that call
return Err(...).wrap_with_cost(...) with the repository's cost-return macros
(e.g., cost_return_on_error!) so cost accumulation follows project conventions;
specifically, where code checks self.is_not_counted_or_summed() &&
!merk.tree_type.is_count_and_sum_bearing() and constructs
Error::InvalidInputError, wrap that error construction in the appropriate
cost_return_on_error! macro invocation instead of returning directly, and apply
the same replacement pattern for the other similar guard exits around the
regions referenced (the blocks at the locations corresponding to lines 466-472
and 573-579) so all validation early-returns use the cost-return macro family.
In `@merk/src/element/reconstruct.rs`:
- Around line 81-83: Add a unit test that mirrors the existing
wrapper-preservation tests but for Element::NotCountedOrSummed: construct an
Element::NotCountedOrSummed containing a known inner element, call
reconstruct_with_root_key(maybe_root_key, aggregate_data) on it, and assert the
result is still an Element::NotCountedOrSummed wrapping the reconstructed inner
(i.e. the outer variant is preserved and contains the expected reconstructed
inner). Use the same test harness/location as the other reconstruct tests and
name it clearly (e.g., preserves_not_counted_or_summed_wrapper).
🪄 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: 5dbdbb80-894c-463c-994c-6762cf98db00
📒 Files selected for processing (26)
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/src/batch/mod.rsgrovedb/src/debugger.rsgrovedb/src/estimated_costs/average_case_costs.rsgrovedb/src/estimated_costs/worst_case_costs.rsgrovedb/src/lib.rsgrovedb/src/operations/get/query.rsgrovedb/src/operations/insert/mod.rsgrovedb/src/operations/proof/generate.rsgrovedb/src/operations/proof/verify.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/not_counted_or_summed_tests.rsmerk/src/element/costs.rsmerk/src/element/get.rsmerk/src/element/insert.rsmerk/src/element/reconstruct.rsmerk/src/element/tree_type.rsmerk/src/tree_type/mod.rs
Comment on lines
+1915
to
+1924
| if element.is_not_counted_or_summed() | ||
| && !in_tree_type.is_count_and_sum_bearing() | ||
| { | ||
| return Err(Error::InvalidBatchOperation( | ||
| "not-counted-or-summed elements may only be inserted into trees \ | ||
| that bear BOTH count and sum (CountSumTree or \ | ||
| ProvableCountSumTree)", | ||
| )) | ||
| .wrap_with_cost(cost); | ||
| } |
Contributor
There was a problem hiding this comment.
Also enforce the wrapped inner-tree allow-list here.
This check only validates the parent. The new wrapper is also supposed to be limited to SumTree, BigSumTree, CountSumTree, and ProvableCountSumTree, but the batch path never verifies that at insert time. That means an invalid Element::NotCountedOrSummed(...) can slip past this guard and fail later in a less precise code path instead of being rejected as InvalidBatchOperation here.
Suggested fix
+ if element.is_not_counted_or_summed()
+ && !matches!(
+ element.underlying(),
+ Element::SumTree(..)
+ | Element::BigSumTree(..)
+ | Element::CountSumTree(..)
+ | Element::ProvableCountSumTree(..)
+ )
+ {
+ return Err(Error::InvalidBatchOperation(
+ "not-counted-or-summed may only wrap SumTree, BigSumTree, \
+ CountSumTree, or ProvableCountSumTree",
+ ))
+ .wrap_with_cost(cost);
+ }
if element.is_not_counted_or_summed()
&& !in_tree_type.is_count_and_sum_bearing()
{📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if element.is_not_counted_or_summed() | |
| && !in_tree_type.is_count_and_sum_bearing() | |
| { | |
| return Err(Error::InvalidBatchOperation( | |
| "not-counted-or-summed elements may only be inserted into trees \ | |
| that bear BOTH count and sum (CountSumTree or \ | |
| ProvableCountSumTree)", | |
| )) | |
| .wrap_with_cost(cost); | |
| } | |
| if element.is_not_counted_or_summed() | |
| && !matches!( | |
| element.underlying(), | |
| Element::SumTree(..) | |
| | Element::BigSumTree(..) | |
| | Element::CountSumTree(..) | |
| | Element::ProvableCountSumTree(..) | |
| ) | |
| { | |
| return Err(Error::InvalidBatchOperation( | |
| "not-counted-or-summed may only wrap SumTree, BigSumTree, \ | |
| CountSumTree, or ProvableCountSumTree", | |
| )) | |
| .wrap_with_cost(cost); | |
| } | |
| if element.is_not_counted_or_summed() | |
| && !in_tree_type.is_count_and_sum_bearing() | |
| { | |
| return Err(Error::InvalidBatchOperation( | |
| "not-counted-or-summed elements may only be inserted into trees \ | |
| that bear BOTH count and sum (CountSumTree or \ | |
| ProvableCountSumTree)", | |
| )) | |
| .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/batch/mod.rs` around lines 1915 - 1924, The current guard in the
batch insert path only checks the parent tree type via
in_tree_type.is_count_and_sum_bearing() when element.is_not_counted_or_summed(),
but does not validate the wrapped inner-tree type; update the same branch in
grovedb::batch::mod (where element.is_not_counted_or_summed() is checked) to
also verify that the wrapped/inner tree kind is one of the allowed types
(SumTree, BigSumTree, CountSumTree, ProvableCountSumTree) and if not, return the
same Error::InvalidBatchOperation(...) wrapped with .wrap_with_cost(cost);
ensure you locate the inner-tree kind via the batch path metadata used there
(the variable used to inspect the child/wrapped tree) and perform the membership
check alongside the existing is_count_and_sum_bearing() check so invalid
Element::NotCountedOrSummed inserts are rejected at insert time.
codecov/patch flagged 65 missing lines on #666 (83.29%). Adds focused tests for every new wrapper arm so each touchpoint is exercised: - grovedb-element/src/element/mod.rs: Display, element_type twin mapping for all four sum-tree inners, validate_wrapper_invariants on every arm (NonCounted / NotSummed / NotCountedOrSummed and their cross-nesting + non-sum-tree-inner failure modes). - grovedb-element/src/element/helpers.rs: full `not_counted_or_summed_tests` module — constructor accept/reject, idempotent `into_…`, cross-wrapper rejection in `into_non_counted` / `into_not_summed`, `is_*` and `is_wrapped` predicates, `underlying{,_mut,into_}`, flag accessors and `set_flags`, all four aggregation helpers zero out for the new wrapper, bincode round-trip, deserialize rejection of every cross-wrapper combo plus the long-chain pre-check, and serialize+deserialize rejection of non-sum-tree inners. - merk/src/element/tree_type.rs: ElementTreeTypeExtensions delegation through the new wrapper for all four sum-tree inners, plus `get_feature_type` zeroing both axes in CountSumTree and ProvableCountSumTree parents. - merk/src/element/reconstruct.rs: wrapper survives root-key propagation (matches the existing NonCounted / NotSummed tests). - merk/src/element/insert.rs: guards fire on the `insert` and `insert_reference` entry points (not just `insert_subtree`), ProvableCountSumTree parent acceptance with both axes suppressed, and a storage round-trip that exercises the v1 read path and the +1 wrapper-overhead accounting. - grovedb/src/tests/not_counted_or_summed_tests.rs: batch propagation through a ProvableCountSumTree parent — mirrors the existing CountSumTree batch test and exercises the second propagation arm. 431 merk lib tests pass, 100 grovedb-element tests pass, 1553 grovedb lib tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…r asserts) Addresses two items from CodeRabbit's review on #666: 1. constructor.rs (actionable A1): `into_non_counted`, `into_not_summed`, and `into_not_counted_or_summed` all return `Ok(self)` on the idempotent already-wrapped match arm. Add a `validate_wrapper_invariants()` re-check on each so a hand-built nested-wrapper value can't slip through this path (constructors validate at creation, but the idempotent arm takes `self` as-is). 2. not_counted_or_summed_tests.rs (nitpick N4): replace `format!("{err:?}").contains("...")` substring matching with a typed `Error::InvalidBatchOperation` variant match via a new local `assert_is_not_counted_or_summed_guard_error` helper. Test no longer depends on debug-format wording — only the typed variant and a stable error string. CodeRabbit's remaining suggestions were considered and skipped: - N1 (reconstruct.rs preservation test): already added in the prior commit. - N2 (cost_return_on_error! in insert.rs guards): the existing NonCounted and NotSummed guards in the same file use the identical `return Err(...).wrap_with_cost(Default::default())` pattern; keeping the three guards consistent. - N3 (collapse three booleans into an enum on InsertTreeWithRootHash): larger refactor that would touch all three wrapper propagation paths; better as a separate cleanup PR. - A2 (validate inner-tree type in batch guard): the constructor and serialize/deserialize both enforce the four-sum-bearing-tree whitelist, and an inner-validation check would need to be applied uniformly to NotSummed as well; out of scope for this PR. 83 grovedb-element tests pass, 9 NotCountedOrSummed grovedb tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
This is Claude. Addressed CodeRabbit's review on Applied:
Skipped, with rationale:
|
QuantumExplorer
added a commit
that referenced
this pull request
May 16, 2026
Resolves conflicts between this PR (CountIndexedTree / cidx) and develop's PR #666 (Element::NotCountedOrSummed wrapper). Both PRs add new Element variants and new arms to the same match statements throughout the element / merk / debugger layers; the resolution is to union both sets. **Critical: on-disk discriminant collision fixed.** Develop assigns `NOT_COUNTED_OR_SUMMED_WRAPPER_DISCRIMINANT = 17`; this PR had `ElementType::CountIndexedTree = 17`. Same on-disk byte, two incompatible meanings. Resolution: shift cidx discriminants by 1 and re-derive the NonCounted twin bytes: - `ElementType::CountIndexedTree` 17 → 18 - `ElementType::ProvableCountIndexedTree` 18 → 19 - `ElementType::NonCountedCountIndexedTree` 145 → 146 (`0x80 | 18`) - `ElementType::NonCountedProvableCountIndexedTree` 146 → 147 (`0x80 | 19`) The `Element` enum variant order is updated to match: `NotCountedOrSummed` keeps variant index 17, cidx variants move to 18 and 19. The bincode auto-generated tag bytes stay aligned with `ElementType`. Updated: - `try_from`, `from_serialized_value`, related doc-comments - assertion / range-check tests for the new wrapper slot at 145 - `is_non_counted` doc range `[128, 146]` → `[128, 147]` Other conflict resolutions union both sets of match arms in: - `grovedb-element/element/helpers.rs` (get/set flags) - `grovedb-element/element/mod.rs` (Element enum, ElementShadow serde) - `merk/element/costs.rs` (get_specialized_cost) - `merk/element/tree_type.rs` (6 wrapper-arm extensions) - `grovedb/debugger.rs` (element_to_grovedbg) And one non-exhaustive-match fix flagged by rustc post-merge: - `grovedb/operations/count_indexed_tree.rs:438` — add `NotCountedOrSummed(_)` to the wrapper-unreachable arm. Verified: - `cargo check -p grovedb` clean - `cargo test --workspace --lib` 2949 tests pass, 0 failures - `cargo fmt --all` applied Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
QuantumExplorer
added a commit
that referenced
this pull request
May 16, 2026
…SumTree Pulls in PR #666 which adds the `Element::NotCountedOrSummed` wrapper — the third aggregation-suppression wrapper, suppressing BOTH count and sum contributions when inserted into a CountSumTree / ProvableCountSumTree parent — alongside PRs #663 / #664 (aggregate-count carrier improvements) already on develop. PR #666's contract specified four sum-bearing inner types (SumTree, BigSumTree, CountSumTree, ProvableCountSumTree). Because this branch adds `Element::ProvableSumTree` (the new fifth sum-bearing variant), the wrapper's allow-list is widened here to include it, matching the NotSummed allow-list extension we already did on the sum side. ## Conflict resolution ### Element / ElementType discriminants - `Element::ProvableSumTree` stays at variant 17 (our branch). - `Element::NotCountedOrSummed` slides from 17 (develop) to 18. - `NOT_COUNTED_OR_SUMMED_WRAPPER_DISCRIMINANT` becomes 18. - New synthetic twin `NotCountedOrSummedProvableSumTree = 193` (`0xC1`), hand-assigned because base 17 overflows the low-nibble formula — same reason `NotSummedProvableSumTree = 177` is hand-assigned at `0xB1`. Other four twins (196/197/199/202) keep the `0xC0 | base` formula slots. - `ElementType::base()` falls back to a per-variant match for ALL hand-assigned wrapper twins (NotSummed + NotCountedOrSummed). The develop-side `disc & NOT_*_BASE_MASK` paths are replaced because they'd collide with the per-variant slot assignments. ### Allow-list extensions `ProvableSumTree` joins the inner allow-list in every gate: - `Element::new_not_counted_or_summed` constructor. - `Element::validate_wrapper_invariants` (deserialize post-check). - `Element::serialize` pre-check. - `Element::deserialize` post-check. - All matching test data sets and rejection bytes. ### Batch propagation `GroveOp::InsertTreeWithRootHash` carries a new `not_counted_or_summed: bool` field (added by #666). Our branch's `Element::ProvableSumTree` build site was updated to set it from the upstream `not_counted_or_summed` local, matching the other tree variants. ### Test wrapper-byte fix The `deserialize_rejects_long_nested_wrapper_chain_without_recursion` test fills 1024 bytes with the wrapper discriminant. PR #666 used `17`; in this branch it must be `18` because slot 17 is `ProvableSumTree`. ## New tests - `direct_insert_not_counted_or_summed_provable_sum_tree_in_count_sum_tree_excludes_both_axes`: wrapped ProvableSumTree inside a CountSumTree parent — outer aggregate must read `(count=1, sum=7)` (only the unwrapped sum item contributes). - `direct_insert_not_counted_or_summed_provable_sum_tree_in_provable_count_sum_tree_excludes_both_axes`: same scenario but with a ProvableCountSumTree parent. - `constructor_invariants` extended to also accept `new_not_counted_or_summed(new_provable_sum_tree(None))`. ## Verification - `cargo build --workspace`: clean - `cargo clippy --workspace --all-features`: clean - `cargo test --workspace`: 3138 / 0 fail (3136 → 3138, +2 new end-to-end ProvableSumTree+NotCountedOrSummed tests; remaining delta from the develop merge bringing in #663/#664/#666 tests). - `cargo test -p grovedb-element --lib`: 99 / 0 fail - `cargo test -p grovedb --lib not_counted_or_summed`: 11 / 0 fail Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
QuantumExplorer
added a commit
that referenced
this pull request
May 16, 2026
…rapper byte The previous commit added wrapper-byte accounting for InsertTreeWithRootHash but only threaded the non_counted and not_summed flags. The op also carries a third wrapper bit, not_counted_or_summed (introduced in #666 as the combined sum+count opt-out), which was still being silently dropped. Mechanics: - wrapper_overhead_for now takes three flags (non_counted, not_summed, not_counted_or_summed). All three are mutually exclusive on any element (the wrapper-invariant validator enforces this), so the result is still 0 or 1, but the helper now matches the full set of wrappers that InsertTreeWithRootHash actually carries. - The two InsertTreeWithRootHash cost arms (avg + worst) destructure the third field and feed it to the helper. - InsertNonMerkTree arms are unchanged (non-Merk trees are never sum-bearing, so not_summed / not_counted_or_summed cannot apply). Coverage tests added (4 in-file tests): - test_insert_tree_with_root_hash_wrapper_bits_average_case_cost_direct - test_insert_tree_with_root_hash_wrapper_bits_worst_case_cost_direct Both construct InsertTreeWithRootHash with each wrapper bit set in turn and assert the bare and wrapped costs differ. Pins the +1 byte delta per wrapper bit. - test_insert_non_merk_tree_non_counted_average_case_cost_direct - test_insert_non_merk_tree_non_counted_worst_case_cost_direct Same for InsertNonMerkTree's single wrapper bit. These tests also exercise the previously-uncovered true-branch of wrapper_overhead_for, lifting patch coverage above the 90% threshold. 1592 grovedb tests pass (was 1588, +4). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
QuantumExplorer
added a commit
that referenced
this pull request
May 17, 2026
…freshReferenceWithSumItem batch op (#667) * feat(element,grovedb): add Element::ReferenceWithSumItem variant + RefreshReferenceWithSumItem batch op Adds a new element variant that fuses Reference and SumItem semantics: a row that resolves like Element::Reference on get() (hop-limited, cycle-detected, combined value hash) AND contributes an explicit SumValue to a sum-bearing parent like SumItem / ItemWithSumItem. The sum is independent of the resolved target's value — it is a caller-supplied weight associated with the link itself, e.g. for ranked / sortable index entries where the key encodes the rank, the reference points to a canonical record elsewhere, and the sum is the entry's monetary weight that aggregates into the parent's total. Element variant signature: ReferenceWithSumItem(ReferencePathType, MaxReferenceHop, SumValue, Option<ElementFlags>) - Bincode discriminant: 17 (appended after NotSummed = 16). - ElementType::ReferenceWithSumItem = 17, NonCountedReferenceWithSumItem = 145 (= 0x80 | 17, NonCounted twin). - Permitted in any parent tree type — the sum simply doesn't propagate in non-sum parents (same rule ItemWithSumItem follows). - NonCounted wrapper compatibility: full. NotSummed: rejected — its whitelist only accepts sum-tree variants. - Version gate: reuses existing element.serialize / element.deserialize gates (same approach NotSummed took). Wrapper bit-encoding fix: The existing scheme assumed base discriminants ≤ 15 so that NonCounted twins fit in the 0x80..=0x8F upper nibble. Base discriminant 17 → twin 0x91 broke that assumption. Switched is_non_counted from an upper-nibble compare to a range check (0x80..=0xAF = NonCounted, 0xB0..=0xBF = NotSummed) so future base variants up through 0x2F work without further changes. NotSummed bases still must fit in the low nibble. Batch op GroveOp::RefreshReferenceWithSumItem (to_u8 = 17): First-class peer to RefreshReference. Carries the same fields plus sum_value, so refresh becomes deterministic: the on-disk variant and the parent's sum aggregate stay in sync without re-reading the previous element. Cross-type refresh (this op against a Reference on disk, or RefreshReference against a ReferenceWithSumItem) is rejected at apply time when trust_refresh_reference is false — silent coercion would corrupt parent aggregates. Public API: QualifiedGroveDbOp::refresh_reference_with_sum_item_op. Tests: - Discriminant-pinning tests updated (test_cases.len bumped from 15 to 16) and continue to pass — load-bearing safety net for the on-disk format. - 12 new end-to-end integration tests in grovedb/src/tests/reference_with_sum_item_tests.rs covering: insert into SumTree with sum aggregation, insertion into non-sum parents (sum dropped), get() resolution to terminal item, get_raw() preserving the variant verbatim, multi-hop chain (RefWithSum → Ref → Item), NonCounted-wrapped variant in CountSumTree, batch insert with sum propagation, batch refresh updating both link and sum, cross-type refresh rejection, predicates through round-trip, NotSummed rejection, multiple-link accumulation. - Test totals (no regressions): grovedb-element 90 (was 81), grovedb-merk 429 (unchanged), grovedb lib 1559 (was 1544). Deferred (documented as open risks): - grovedbg-types wire schema: the variant renders as a plain Reference in the debugger UI; the sum is dropped from the wire format with a TODO. - Cost-estimate regressions in downstream fee tables (the new variant + batch op are ~8 bytes heavier than their Reference counterparts). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: cover query / refresh-untrusted / cost-estimate / proof / aggregate-sum paths for ReferenceWithSumItem Addresses Codecov failure on PR #667 (patch coverage 69.76% < 90% target). The added arms in query.rs, batch refresh, cost estimation, aggregate-sum- query, and proof generation/verify all lacked test coverage. Adds: batch/estimated_costs/average_case_costs.rs: - test_refresh_reference_with_sum_item_average_case_cost: covers the new GroveOp::RefreshReferenceWithSumItem arm in average_case_cost. batch/estimated_costs/worst_case_costs.rs: - test_refresh_reference_with_sum_item_worst_case_cost: same for worst-case. element/aggregate_sum_query/tests.rs: - test_reference_with_sum_item_chain_followed_to_sum_item: aggregate-sum query follows the new variant to a SumItem and returns the target's sum (the carried sum is parent-aggregation-only). - test_reference_with_sum_item_chain_through_intermediate_reference: exercises the chain-continuation arm with a RefWithSum -> Ref -> SumItem chain. tests/reference_with_sum_item_tests.rs: - query_item_value_follows_reference_with_sum_item: covers operations/get/ query.rs query_item_value reference arm. - query_item_value_or_sum_follows_reference_with_sum_item: same for query_item_value_or_sum. - query_sums_follows_reference_with_sum_item_to_sum_item: same for query_sums. - query_encoded_many_follows_reference_with_sum_item: covers the multi- path query arm near query.rs:98. - batch_refresh_reference_with_sum_item_untrusted: covers the trust_refresh_reference=false branch which reads the on-disk element and verifies it is also a ReferenceWithSumItem before rebuilding. - prove_and_verify_reference_with_sum_item: prove_query + verify round- trip exercises both reference proof arms. Test totals: grovedb 1575 (was 1565, +10), grovedb-element 85, grovedb-merk 432. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(batch): preserve NonCounted wrapper across RefreshReferenceWithSumItem; exempt new refs from tree-override guard Addresses CodeRabbit review feedback on PR #667: Finding 1 (Minor, batch/mod.rs:2026): The validate_insertion_does_not_override_tree guard exempted only Element::Reference(..) on the outer enum via matches!, rejecting ReferenceWithSumItem and any NonCounted-wrapped reference as "attempting to overwrite a tree". Switched to element.is_reference(), which looks through NonCounted and recognizes both reference variants. Finding 2 (Major, batch/mod.rs:2338): The trusted-refresh branch unconditionally rebuilt a bare Element::ReferenceWithSumItem(...), silently stripping any NonCounted wrapper that was on disk. This would change count_value_or_default from 0 to 1 and corrupt the parent's count aggregate. Fixed by adding an explicit non_counted: bool field to GroveOp::RefreshReferenceWithSumItem and the refresh_reference_with_sum_item_op constructor: - Trusted path (trust_refresh_reference = true): caller's non_counted declaration is taken at face value; wraps in NonCounted iff the flag is set. - Untrusted path (trust_refresh_reference = false): cross-checks the declared non_counted against the on-disk element and returns Error::InvalidInput on mismatch, preventing silent wrapper drop or injection. Added two tests: - batch_refresh_reference_with_sum_item_trusted_preserves_non_counted_wrapper: seeds NonCounted(RefWithSum) in a CountSumTree, refreshes trusted with non_counted=true, asserts the parent's CountAndSum aggregate stays (0, new_sum) - wrapper preserved. - batch_refresh_reference_with_sum_item_untrusted_rejects_wrapper_mismatch: seeds a bare RefWithSum, attempts untrusted refresh with non_counted=true, asserts the apply path errors out. Total: 1577 grovedb tests (was 1575, +2). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(batch): always thread refresh op's reference_path_type into process_reference Addresses the critical "outside diff" CodeRabbit finding on PR #667. When a batch contained both a RefreshReference / RefreshReferenceWithSumItem op AND another reference (in the same batch) that pointed at the refreshed key, the dependent reference's value hash was computed against the stale on-disk target. The bug: let reference_info = if *trust_refresh_reference { Some(reference_path_type) } else { None // <-- falls back to on-disk lookup (pre-batch state) }; self.process_reference(.., reference_info, ..); When trust=false, process_reference received None, treated the reference as if it weren't being refreshed in the batch, and read the stale on-disk value hash. The apply path later wrote the refreshed element to disk, but other dependent refs in the same batch had already been committed with the wrong hash. Fix: always pass Some(reference_path_type). The op payload IS the authoritative new path during batch processing; the trust flag is orthogonal and only controls cross-validation against disk at apply time in execute_ops_on_path, not path resolution for dependent refs. This also fixes the same pre-existing bug in plain RefreshReference, since both ops share the merged match arm in process_reference's in-batch branch. Added regression test batch_dependent_reference_resolves_through_refreshed_path: builds a batch with a RefreshReferenceWithSumItem (trust=false, path: old to new) and a dependent Reference that points at the refreshed key. Re-resolving the dependent ref after the batch must return the NEW target's payload, proving the in-batch hash computation followed the refreshed path. 1578 grovedb tests (was 1577, +1). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(refresh-with-sum-item): cover op tag, debug format, wrapper-preserve, missing key Boosts patch coverage on PR #667 (codecov was 86.66% < 90% target). grovedb/src/operations/get/query.rs: Restructured the first match arm to keep `=> match reference_path { ... }` on the same line as the `| Element::ReferenceWithSumItem(...)` pattern extension. The previous form wrapped the inner match in an extra block, re-indenting the entire match body and causing codecov to flag all pre-existing inner branches as "patch" lines. Test additions in grovedb/src/tests/reference_with_sum_item_tests.rs: - refresh_reference_with_sum_item_op_tag_pin: compares the new op against Delete and InsertOrReplace via Ord::cmp, exercising the to_u8 arm at line 443 (RefreshReferenceWithSumItem to 17). This is the wire-format pin for batch op serialization. - refresh_reference_with_sum_item_debug_format: asserts the fmt::Debug arm produces a string with the op name, max_hop, sum, and trust flag. - batch_refresh_reference_with_sum_item_trusted_with_nc_wraps: trusted refresh + non_counted=true on a CountSumTree parent goes through Element::new_non_counted on the rebuilt inner. - batch_refresh_reference_with_sum_item_untrusted_matches_wrapper_succeeds: untrusted refresh against a NonCounted(RefWithSum) with non_counted=true succeeds (the cross-check passes and the wrapper is rebuilt). - batch_refresh_reference_with_sum_item_untrusted_missing_key_errors: refresh of a non-existing key with trust=false exercises the "trying to refresh a non existing reference" error arm. 1583 grovedb tests (was 1578, +5). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(batch): enforce parent-tree invariant on refresh-with-sum-item; cost arms honor non_counted P2 fix - trusted refresh could persist NonCounted in non-count-bearing trees. The direct insert/replace/patch path at grovedb/src/batch/mod.rs lines 2016-2037 rejects NonCounted-wrapped elements in non-count-bearing parents. The RefreshReferenceWithSumItem apply branch added in this PR constructs the element internally from the op's non_counted flag and bypassed that guard: with trust_refresh_reference = true, a caller could refresh a bare ReferenceWithSumItem in a NormalTree into NonCounted(ReferenceWithSumItem) - violating the invariant that NonCounted-wrapped elements only live in count-bearing trees. Fix: replicate the per-merk wrapper invariant in the refresh apply branch, after rebuilding element and before get_feature_type: if element.is_non_counted() && !in_tree_type.is_count_bearing() { return Err(Error::InvalidBatchOperation( "RefreshReferenceWithSumItem with non_counted=true requires \ a count-bearing parent", )).wrap_with_cost(cost); } Added regression test batch_refresh_reference_with_sum_item_trusted_with_nc_rejected_in_normal_tree: seeds a bare RefWithSum in a NormalTree, attempts a trusted refresh with non_counted=true, asserts the apply errors out AND the on-disk shape is unmodified. P3 fix - cost estimators ignored non_counted. The RefreshReferenceWithSumItem arms in average_case_costs.rs:130 and worst_case_costs.rs:123 destructured the op with `..` and always constructed a bare Element::ReferenceWithSumItem for cost computation, even when execution writes NonCounted(...). That undercounted the serialized value by the wrapper byte. Fix: build the element shape that the apply path will actually write: let inner = Element::ReferenceWithSumItem(...); let element = if *non_counted { Element::NonCounted(Box::new(inner)) } else { inner }; Added two tests that compute the cost for non_counted=true and bare variants and assert the non_counted estimate is >= the bare estimate (it must include the wrapper byte). 1586 grovedb tests (was 1583, +3). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(batch): remove buggy process_reference fast path that committed stale/wrong hashes P1 was REAL. Confirmed with a probe test that builds a batch with RefreshReferenceWithSumItem(link) plus an InsertOrReplace of a dependent reference dep to link. With the pre-fix code, verify_grovedb reported a hash mismatch on [test_leaf, dep] (the load-bearing assertion from CodeRabbit's local probe). The probe is now turned into a regression test. Root cause: process_reference at grovedb/src/batch/mod.rs:1332 had a `recursions_allowed == 1` fast path that returned merk.get_value_hash(target_key). Two ways that was wrong: 1. For an in-batch refreshed target, the on-disk merk value_hash is stale - the apply path hasn't written the refreshed element yet when the dependent ref's hash is being computed in the same batch. 2. For ANY Reference target (even unrefreshed), the merk value_hash is combine_hash(H(serialize(ref)), referenced_value) - NOT the simple hash of the terminal item, which is what insert_reference bakes in via Op::PutCombinedReference. So a dep to link to target chain committed via batch with max_hop=1 stored a different hash than what verify_grovedb (which follows the chain to terminal with MAX_REFERENCE_HOPS budget) recomputes. Fix: remove the fast path entirely. The dispatch is now: - intermediate_reference_info=Some (target in batch as refresh): hop through the op's new path, decrementing recursions_allowed. - intermediate_reference_info=None (target not in batch): always go through process_reference_with_hop_count_greater_than_one, which reads the actual on-disk element. Item terminals return H(serialize(item)) (consistent with direct insert + verify); a Reference target recurses with recursions_allowed - 1 and produces ReferenceLimit when the user-declared max_hop is exhausted. Behavior changes: - A dep to link to item chain inserted via batch now stores the same combined value_hash as the direct insert path computes - so verify_grovedb reports clean afterwards. - max_hop=Some(1) pointing at another Reference cleanly fails with ReferenceLimit at batch time, replacing the previous silent-stale behavior. The existing batch::tests::test_references explicitly covers this rejection (line 5997) and continues to pass. Tests added in grovedb/src/tests/reference_with_sum_item_tests.rs: - batch_dependent_ref_resolves_through_refreshed_path_via_chain: dep to link to target with link being refreshed in same batch. Post-batch verify_grovedb must be clean - the regression check for the P1 finding. - batch_one_hop_dependent_ref_into_ref_chain_rejected: dep with max_hop=Some(1) pointing at a Reference is rejected with ReferenceLimit. Documents that the strict enforcement is preserved. 1588 grovedb tests (was 1586, +2). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: pin exact GroveOp::to_u8 sort tag for RefreshReferenceWithSumItem Addresses CodeRabbit nitpick on PR #667 (grovedb/src/tests/reference_with_sum_item_tests.rs:621-653): the prior test only checked relative `Ord::cmp` ordering, so the new op's sort tag could silently drift from 17 to any larger value without failing the test. Bump `GroveOp::to_u8` from private to `pub(crate)` so the test can pin the exact value, and assert `refresh.to_u8() == 17` directly. Existing relative-ordering checks are retained as a sanity belt. 1588 grovedb tests, unchanged count. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(cost): InsertTreeWithRootHash / InsertNonMerkTree honor wrapper byte The average-case and worst-case cost estimators for the batch ops that insert merk trees (InsertTreeWithRootHash) and non-merk trees (InsertNonMerkTree) were under-counting the on-disk payload by 1 byte when the op's element is wrapped in NonCounted (or, for the merk-tree case, NotSummed/NotCountedOrSummed). The wrapper prepends a 1-byte discriminant ahead of the inner element's bincode payload — exactly the same accounting fix already applied for InsertReference and the RefreshReferenceWithSumItem cost arms. Mechanics: - average_case_merk_insert_tree / worst_case_merk_insert_tree gain a wrapper_overhead: u32 parameter that's folded into value_len next to flags_len. - A new pub(in crate::batch) helper wrapper_overhead_for(non_counted, not_summed) -> u32 in batch/estimated_costs/mod.rs returns 1 when either wrapper bit is set (the two are mutually exclusive on any element, so the result is always 0 or 1). - The four batch cost arms (avg / worst × InsertTreeWithRootHash / InsertNonMerkTree) destructure their wrapper bits and feed the helper. InsertNonMerkTree only carries non_counted, so it bypasses the helper and uses the literal. - Test callsites that called the two helpers with 6 args are updated to pass 0 for the new wrapper_overhead slot. Why this matters: the cost estimators are used to pre-budget storage fees before applying batches. A 1-byte under-count on every wrapped tree insert is small individually but compounds at batch size and silently lowers reservation requirements relative to actual storage consumption. Same shape as the bug already fixed for InsertReference in this PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(cost): InsertTreeWithRootHash also honors not_counted_or_summed wrapper byte The previous commit added wrapper-byte accounting for InsertTreeWithRootHash but only threaded the non_counted and not_summed flags. The op also carries a third wrapper bit, not_counted_or_summed (introduced in #666 as the combined sum+count opt-out), which was still being silently dropped. Mechanics: - wrapper_overhead_for now takes three flags (non_counted, not_summed, not_counted_or_summed). All three are mutually exclusive on any element (the wrapper-invariant validator enforces this), so the result is still 0 or 1, but the helper now matches the full set of wrappers that InsertTreeWithRootHash actually carries. - The two InsertTreeWithRootHash cost arms (avg + worst) destructure the third field and feed it to the helper. - InsertNonMerkTree arms are unchanged (non-Merk trees are never sum-bearing, so not_summed / not_counted_or_summed cannot apply). Coverage tests added (4 in-file tests): - test_insert_tree_with_root_hash_wrapper_bits_average_case_cost_direct - test_insert_tree_with_root_hash_wrapper_bits_worst_case_cost_direct Both construct InsertTreeWithRootHash with each wrapper bit set in turn and assert the bare and wrapped costs differ. Pins the +1 byte delta per wrapper bit. - test_insert_non_merk_tree_non_counted_average_case_cost_direct - test_insert_non_merk_tree_non_counted_worst_case_cost_direct Same for InsertNonMerkTree's single wrapper bit. These tests also exercise the previously-uncovered true-branch of wrapper_overhead_for, lifting patch coverage above the 90% threshold. 1592 grovedb tests pass (was 1588, +4). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(cost): tighten non_counted wrapper-byte assertions from >= to > Addresses CodeRabbit feedback on PR #667: > Line 1305 / 998 uses `>=`, so an undercount regression that produces > an identical cost would still pass. Assert a strict increase (or `+1` > minimum) to make this test load-bearing. The bug being pinned is "estimator silently ignores the non_counted flag and produces the same cost as bare". Equality is exactly the failure mode — non-strict `>=` would let that regression slip past green CI. Switching to strict `>` makes the assertions actually load-bearing. Two assertions tightened (matching the test pattern I already used in the InsertTreeWithRootHash / InsertNonMerkTree direct cost tests): - test_refresh_reference_with_sum_item_non_counted_average_case_cost - test_refresh_reference_with_sum_item_non_counted_worst_case_cost 1592 grovedb tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * revert(batch): restore process_reference fast path under well-formed-user contract Reverts the fast-path removal from bc421eb. The user contract is now explicit: when `max_reference_hop` is set to 1, the caller is asserting the target is an `Item` (or `SumItem` / `ItemWithSumItem`) terminal. Ill-formed callers (`max_hop = 1` pointing at another `Reference`) are out of scope — their behavior is undefined, not a system-level invariant to enforce on every well-formed hop=1 ref. What the fast path does and why it's sound under the contract: - `recursions_allowed == 1` means the user's budget allows exactly one more hop. - Under the contract that hop lands on an `Item` terminal. The merk- stored `value_hash` of an `Item` IS `H(serialize(item))` — the simple hash that `insert_reference` bakes into the dependent ref via `Op::PutCombinedReference`. So `merk.get_value_hash(target_key)` returns the exact value we'd otherwise derive via a full element decode. - Skipping the decode saves the cost of deserializing the terminal element on every leaf-of-chain dereference. Ill-formed input (covered by the deleted `batch_one_hop_dependent_ref_ into_ref_chain_rejected` test): if a user violates the contract by setting `max_hop = 1` and pointing at another `Reference`, the fast path returns the target's merk-combined hash as if it were a terminal simple hash. The dependent ref's stored hash then disagrees with what `verify_grovedb` recomputes (which walks the chain with the global `MAX_REFERENCE_HOPS` budget). The user broke their own budget; we don't pin behavior for that case. What stays from the earlier P1 work: - `9384dc09`'s fix is independent and correct: the `RefreshReference[WithSumItem]` arm in `follow_reference_get_value_hash` unconditionally threads the op's new path to `process_reference`. The `trust_refresh_reference` flag is orthogonal — it gates apply-time cross-checking, not path resolution for dependent refs. - `batch_dependent_ref_resolves_through_refreshed_path_via_chain` stays: dep has no `max_hop` set (budget = 10), so it never reaches the `recursions_allowed == 1` branch. The test's true coverage is the `9384dc09` path-threading fix, and the doc comment is updated to say so accurately. Test deleted: - `batch_one_hop_dependent_ref_into_ref_chain_rejected`: pinned the out-of-scope ill-formed case to `ReferenceLimit`. Under the new contract that case has no specified behavior, so the test is gone. 1591 grovedb tests pass (was 1592, -1 for the deleted out-of-scope test). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(batch): RefreshReferenceWithSumItem trust=false now refreshes sum only Reworks the `trust_refresh_reference` semantics on `RefreshReferenceWithSumItem` so the two trust modes are clean and non-overlapping: * `trust = true`: apply writes the full op payload (path, max_hop, sum_value, flags, non_counted wrapper). No on-disk read. Use this to repoint and/or adjust the carried sum atomically. * `trust = false`: apply reads disk, cross-checks variant + `non_counted` wrapper, then writes back with the on-disk path / max-hop / flags / wrapper — only `sum_value` is taken from the op. Op fields `reference_path_type`, `max_reference_hop`, `flags`, and the wrapper bit are intentionally ignored in this mode. Previously the untrusted mode validated the on-disk shape but then silently substituted the op's payload anyway — a shallow check that let a `trust=false` caller change path AND sum. That was hard to justify: "untrusted" effectively meant "validate one axis, clobber the rest." The new contract is that `trust=false` strictly means "I don't know / don't want to assert the path; just refresh the weight." Knock-on changes: - `follow_reference_get_value_hash` `RefreshReference` / `RefreshReferenceWithSumItem` arm: gate the path threaded into `process_reference` on `trust_refresh_reference`. `Some(op_path)` when trusted (apply will write op's path), `None` when untrusted (apply will keep on-disk's path). This keeps dependent-ref hash computation consistent with whichever path apply actually writes — and is now symmetric for both refresh ops, because both share the rule "trust=true uses op's path, trust=false keeps on-disk". - `GroveOp::RefreshReferenceWithSumItem` doc comment rewritten to document the two modes precisely (which op fields apply in each). - Public constructor `refresh_reference_with_sum_item_op` doc rewritten to match. Tests: - `batch_refresh_reference_with_sum_item_untrusted`: rewritten to assert the new contract — pass a bogus `ref_b`, observe the on-disk path stayed `ref_a`, only `sum_value` updated to 42. - `batch_dependent_reference_resolves_through_refreshed_path`: switched to `trust=true` (the only mode that rewrites the path). Added a `verify_grovedb` clean-state check. - New `batch_untrusted_refresh_keeps_on_disk_path_only_sum_updates`: positive coverage for the new untrusted contract — observe the path stays unchanged and the sum updates. 1592 grovedb tests pass (was 1591, +1 for the new untrusted test). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(batch): document trust=true cross-type silent-coercion contract for refresh ops `GroveOp::RefreshReference` and `GroveOp::RefreshReferenceWithSumItem` both follow the same trust-mode contract, but the docs were vague / in places outright wrong about what happens on cross-type mismatch. Specifically, the existing `RefreshReferenceWithSumItem` doc claimed "Cross-type refresh ... is rejected at apply time", which is only true under `trust=false` — under `trust=true` both ops silently coerce the on-disk element to the variant the op declares. This is intentional. The trust=true contract is "I'm asserting the on-disk shape; write my payload verbatim". A caller using trust=true against a mismatching on-disk variant gets: - `RefreshReference` (trust=true) on a `ReferenceWithSumItem`: silently overwrites with plain `Reference`, dropping the sum. Parent sum aggregate becomes inconsistent. - `RefreshReferenceWithSumItem` (trust=true) on a plain `Reference`: silently writes a `ReferenceWithSumItem` carrying the op's `sum_value`. Parent sum aggregate jumps by `+sum_value`. These are the caller's responsibility, the same way `max_hop = 1` pointing at a `Reference` is the caller's responsibility (the fast-path-restored-under-well-formed-contract decision in 4a1a73c). Changes: - `GroveOp::RefreshReference` doc rewritten to document the two trust modes precisely (which fields apply in each; what happens on cross-type mismatch). - `GroveOp::RefreshReferenceWithSumItem` doc rewritten to drop the incorrect "is rejected at apply time" claim and explicitly call out the trust=true silent-coercion behavior. - Public constructors `refresh_reference_op` and `refresh_reference_with_sum_item_op`: doc comments rewritten to match the variants', so callers reading the constructor see the contract at the API surface too. Tests (contract pins — will fail if a future contributor "fixes" the silent coercion as a bug, forcing a contract review): - `batch_refresh_reference_trusted_silently_coerces_ref_with_sum_item`: seed a `ReferenceWithSumItem(sum=10)` in a SumTree, refresh with `RefreshReference` trust=true, assert on-disk is now plain `Reference`. - `batch_refresh_reference_with_sum_item_trusted_silently_coerces_plain_reference`: symmetric — seed a plain `Reference` in a SumTree, refresh with `RefreshReferenceWithSumItem` trust=true sum=77, assert on-disk is now `ReferenceWithSumItem(sum=77)`. 1594 grovedb tests pass (was 1592, +2 for the two contract pins). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(batch): scope merk binding to fast-path only in process_reference Drops the awkward `let _ = merk;` no-op from the slow path of `process_reference`. The previous shape was: let merk = ... self.merks.entry(..) ...; // borrows self.merks if recursions_allowed == 1 { // uses merk return ...; } if let Some(_) = ... { self.follow_reference_get_value_hash(..) // &mut self } else { let _ = merk; // explicit drop of the &mut self.merks borrow self.process_reference_with_hop_count_greater_than_one(..) } The `let _ = merk;` released the `&mut self.merks` borrow so the next call could take `&mut self`. NLL usually handles this, but the binding was also confusing — the slow path never used `merk` and the helper internally opens (or reuses the cached) merk via the same `self.merks.entry(..)` path. Cleaner: open the merk only inside the fast-path branch where we actually use it. The slow path no longer carries a stale binding; the helpers open their own merk handle, which is a HashMap entry lookup against the same `self.merks` cache — no extra disk work. No behavior change. 1594 grovedb tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(grovedbg-types,debugger): dedicated wire variant for ReferenceWithSumItem Resolves the TODO in `grovedb/src/debugger.rs::element_to_grovedbg` that was rendering `Element::ReferenceWithSumItem` as a plain `Reference` and dropping the carried sum from the debugger UI. grovedbg-types changes (additive): - New `Element::ReferenceWithSumItem { reference: Reference, sum_item_value: i64 }` variant on the wire `Element` enum. Reuses the existing wire `Reference` enum (which already encodes `element_flags` inside every path discriminant), and tacks on the independent `sum_item_value` at the outer level — same shape as the existing `Element::ItemWithSumItem` variant. - `serde_json` added as a dev-dependency for the round-trip tests. debugger.rs changes: - Extracted `reference_path_to_grovedbg(reference_path, element_flags) -> grovedbg_types::Reference` helper that handles the seven-way `ReferencePathType` → wire `Reference` discriminant match. Removes the (previously inlined) 70+ lines of per-discriminant boilerplate from `element_to_grovedbg`. - `element_to_grovedbg`'s `Reference` arm collapses to one delegation call. New `ReferenceWithSumItem` arm uses the same helper to encode the path and wraps it in the new wire variant with the carried `sum_item_value`. Tests: - grovedbg-types: two JSON round-trip pins for the new wire variant — absolute-path-with-flags and sibling-no-flags. These lock in the wire schema so a future renumbering or rename trips the test. - grovedb (under the `grovedbg` feature, alongside the existing `element_to_grovedbg_converts_item_with_sum_item` test): two conversion tests — one absolute-path RefWithSumItem and one SiblingReference RefWithSumItem — exercise both the helper and the new wire variant end-to-end. Wire-schema impact: this is an additive change. Existing JSON payloads continue to round-trip unchanged. The new variant only appears on the wire when the producer is upgraded; older consumers will see an unknown-variant decode error if they encounter one, which is the standard serde behavior for additive enum changes. 1597 grovedb tests pass (with the `grovedbg` feature); 2 new grovedbg-types tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(batch): unify RefreshReference + RefreshReferenceWithSumItem into one variant Previously the two refresh ops were separate `GroveOp` variants with mostly-duplicate fields, differing only in: 1. `sum_value: SumValue` (only on the sum-item variant) 2. `non_counted: bool` (only on the sum-item variant; plain `RefreshReference` handled the wrapper transparently) This split forced parallel arms across the entire batch pipeline (apply, dispatch, cost-estimate, ordering, transient-op check, batch-only rejection, Display, propagation) and made the call sites hard to keep in sync. It also made the wrapper-handling story asymmetric: trusted `RefreshReference` silently dropped the `NonCounted` wrapper, while trusted `RefreshReferenceWithSumItem` preserved it. Unification: single `GroveOp::RefreshReference` with `sum_value: Option<SumValue>` and an explicit `non_counted: bool`. GroveOp::RefreshReference { reference_path_type: ReferencePathType, max_reference_hop: MaxReferenceHop, sum_value: Option<SumValue>, // None=plain, Some=RefWithSum flags: Option<ElementFlags>, non_counted: bool, trust_refresh_reference: bool, } Semantics, by `sum_value`: * `None` → writes `Element::Reference(..)` * `Some(v)` → writes `Element::ReferenceWithSumItem(.., v, ..)` Trust modes (unchanged from the prior sum-item op, now apply to both shapes): * `trust=true`: writes the op's payload verbatim; if the on-disk variant or wrapper disagrees, it's silently coerced. Caller's responsibility (same caller-asserted-shape contract). * `trust=false`: reads on-disk, cross-checks variant + wrapper (`sum_value=None` ↔ `Reference`, `sum_value=Some(..)` ↔ `ReferenceWithSumItem`), and writes back with on-disk path / max-hop / flags / wrapper. For sum-item refreshes the op's `sum_value` overrides on-disk's sum; plain refreshes write the on-disk element back verbatim. Mismatches are rejected. Sites collapsed (~9 dual arms became single): - `to_u8`: dropped the `RefreshReferenceWithSumItem => 17` arm. Tag 17 is now a "do not reuse" hole — comment in place. - `fmt::Debug` op-shape rendering: one arm; label switches between "Refresh Reference" and "Refresh Reference With Sum Item" based on `sum_value.is_some()`. - `follow_reference_get_value_hash` dispatch arm: collapsed to one pattern; threading behavior on `trust_refresh_reference` is unchanged (Some(op_path) when trusted, None when untrusted). - Apply path: one arm constructs the element by matching on `sum_value` (None → plain Reference, Some → RefWithSumItem), applies the `NonCounted` wrapper if declared, and does the cross-check + rebuild for `trust=false`. - Cost estimators (avg + worst): one arm builds the same element shape the apply path will write, so the wrapper byte is counted. - `batch_structure.rs` ordering classification: collapsed. - Insert-under-refreshed-reference rejection (line ~3269): collapsed. - Batch-only NotSupported rejection (line ~3721): collapsed and message updated. Public API: - `refresh_reference_op(..)`: unchanged signature; now builds the unified op with `sum_value=None`, `non_counted=false`. - `refresh_reference_with_sum_item_op(..)`: unchanged signature; now builds the unified op with `sum_value=Some(..)` and `non_counted` from the caller. Wire-format note: `GroveOp` is in-memory only (used for batch processing within a single apply call), so collapsing the variants is not a persisted-format break. `to_u8` ordering values for all other ops are unchanged. Tests: - `batch_unit_tests::test_grove_op_ord_all_variants`: updated to match the new shape (16 variants — RefreshReferenceWithSumItem was already missing from this list, so the "all 16" claim now matches the test's content). - `refresh_reference_op_tag_pin` (renamed from `refresh_reference_with_sum_item_op_tag_pin`): pins both constructors to op-tag 5 (the unified RefreshReference tag); asserts they share the same variant. - `refresh_reference_with_sum_item_debug_format`: updated assertion to expect "sum Some(42)" (Option-rendered) instead of "sum 42" (raw int). - `refresh_reference_constructors_share_unified_variant` (new): structural regression test pinning that both public constructors produce `GroveOp::RefreshReference` distinguished only by `sum_value`. 1595 grovedb tests pass (was 1594, +1 for the structural pin). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(batch): encode trust mode into RefreshReferenceMode (5 variants, no invalid pairs) Switches `GroveOp::RefreshReference` from a 3-variant mode + separate `trust_refresh_reference: bool` to a 5-variant enum where trust is encoded directly in the variant name. The invalid combination (SumItemReferenceNoValueUpdate + trust=true) is now unrepresentable by construction instead of being a runtime error. ```rust pub enum RefreshReferenceMode { PlainReferenceTrusted, PlainReferenceUntrusted, SumItemReferenceTrusted(SumValue), SumItemReferenceUntrustedValueUpdate(SumValue), SumItemReferenceUntrustedNoValueUpdate, } ``` There is no `SumItemReferenceTrustedNoValueUpdate` variant — "refresh a RefWithSumItem without changing its carried sum" only makes sense in untrusted mode (under trusted the apply path has no sum to write without reading disk). The previous design exposed that combination at compile time and rejected it at runtime; this design makes it impossible at compile time, which is the type-safe option C from the discussion. GroveOp::RefreshReference field changes: - `sum_value: Option<SumValue>` + `trust_refresh_reference: bool` → replaced by single `mode: RefreshReferenceMode`. - Other fields unchanged: `reference_path_type`, `max_reference_hop`, `flags`, `non_counted`. Knock-on changes: - `RefreshReferenceMode::is_trusted()` helper. Used by `follow_reference_get_value_hash` (replaces the prior `*trust_refresh_reference` check) to decide whether dependent refs should resolve against the op's path or the on-disk path. - Apply path collapses to a 5-way `match mode`. The runtime error path that previously rejected (NoValueUpdate, trust=true) is gone — that combination doesn't exist anymore. - Cost estimators (avg + worst): the sum-item match arms collapse `SumItemReferenceTrusted` and `SumItemReferenceUntrustedValueUpdate` into one pattern (both carry a sum value); the `NoValueUpdate` variant uses 0 as the cost-only stand-in sum. - Display: dispatches on the 5 mode variants; drops the `trust_reference` suffix (trust is in the mode name now). Public constructors (signatures unchanged for the existing two — backward compatible for callers): - `refresh_reference_op(.., trust_refresh_reference)`: builds `PlainReferenceTrusted` or `PlainReferenceUntrusted` based on the bool. - `refresh_reference_with_sum_item_op(.., sum_value, .., trust_refresh_reference)`: builds `SumItemReferenceTrusted(v)` or `SumItemReferenceUntrustedValueUpdate(v)` based on the bool. - `refresh_reference_with_sum_item_keep_sum_op(..)`: builds `SumItemReferenceUntrustedNoValueUpdate`. No trust parameter — untrusted is the only valid variant. Tests: - `refresh_reference_constructors_share_unified_variant`: extended to cover all 5 mode variants (the four trusted-vs-untrusted cells plus the untrusted-no-value-update). Pins the `is_trusted()` helper too. - `refresh_reference_with_sum_item_debug_format`: updated to expect the new "mode SumItemReferenceTrusted(42)" rendering and drops the now-removed `trust_reference` suffix. - `batch_refresh_keep_sum_trusted_rejected` (removed): the combination it was pinning is no longer constructible, so the apply-path runtime check is gone and the test is moot. 1596 grovedb tests pass; 1599 with `--features grovedbg`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(batch): extract RefreshReferenceMode into its own module Moves the `RefreshReferenceMode` enum + its `impl` (with the `is_trusted()` helper) out of the now-large `batch/mod.rs` into a dedicated `batch/refresh_reference_mode.rs` module. The type is re-exported from `batch` so existing call sites (e.g. `crate::batch::RefreshReferenceMode`) continue to work without changes. No behavior change. The intra-doc links to `Element::Reference`, `Element::ReferenceWithSumItem`, and `GroveOp::RefreshReference` are spelled with explicit `crate::` / `super::` paths since the type now lives in a sibling submodule. 1596 grovedb tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(batch): drop the do-not-reuse-17 comment from GroveOp::to_u8 GroveOp is in-memory only (used for batch processing within a single apply call) — there's no persisted wire format to reserve a tag for. Future ops are free to use 17. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(batch): add non_counted parameter to refresh_reference_op Restores symmetry with the other two refresh constructors. Previously `refresh_reference_op` hard-coded `non_counted: false`, while `refresh_reference_with_sum_item_op` and `refresh_reference_with_sum_item_keep_sum_op` both accepted the wrapper bit. Callers refreshing a plain `Reference` couldn't preserve or set a `NonCounted` wrapper through the public API. New signature: ```rust pub fn refresh_reference_op( path: Vec<Vec<u8>>, key: Vec<u8>, reference_path_type: ReferencePathType, max_reference_hop: MaxReferenceHop, flags: Option<ElementFlags>, non_counted: bool, // <-- new trust_refresh_reference: bool, ) -> Self ``` Behavior: - Under the trusted variant (`PlainReferenceTrusted`): written at face value into the rebuilt element. - Under the untrusted variant (`PlainReferenceUntrusted`): cross-checked against the on-disk wrapper; mismatch is rejected (a silent wrapper drop would corrupt the parent's count aggregate). Same contract as the sum-item constructors. This is a breaking change to the function signature. Updated all 17 in-tree callsites to pass `/* non_counted = */ false` explicitly — preserves the prior behavior at every call. The structural pin `refresh_reference_constructors_share_unified_variant` is extended to (a) call `refresh_reference_op` with `non_counted = true` and (b) assert that `non_counted` is threaded through correctly by all three constructors. 1596 grovedb tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
QuantumExplorer
added a commit
that referenced
this pull request
May 17, 2026
…to 19 Pulls in PR #667 (`Element::ReferenceWithSumItem` + `GroveOp::RefreshReferenceWithSumItem`) and uses the merge as an opportunity to re-position `Element::ProvableSumTree` from slot 17 to slot 19 (the end of the Element enum). This restores develop's on-disk encoding for every variant that landed before this PR. ## The skew this fixes Our earlier merge of PR #666 slid `NotCountedOrSummed` from develop's slot 17 to slot 18 so it wouldn't collide with our `ProvableSumTree` at slot 17. PR #667 then added `ReferenceWithSumItem` at slot 18 in develop. If we'd slid that to slot 19, every PR that lands on develop after `ProvableSumTree` would inherit a 2-slot rotation `[NotCountedOrSummed, ReferenceWithSumItem]` → `[ProvableSumTree, NotCountedOrSummed, ReferenceWithSumItem]`, breaking the wire format of every variant that landed before us. Re-slotting `ProvableSumTree` to the end keeps develop's encoding verbatim and limits the divergence to a single new appended variant. ## Final layout | Slot | Variant | Notes | |------|-----------------|----------------------------------------------| | 15 | NonCounted | wrapper byte | | 16 | NotSummed | wrapper byte | | 17 | NotCountedOrSummed | wrapper byte (matches develop) | | 18 | ReferenceWithSumItem | base type (matches develop) | | 19 | ProvableSumTree | this branch's addition, appended at the end | ## Discriminant changes - `ElementType::ProvableSumTree`: 17 → **19** (matches new Element position). - `NonCountedProvableSumTree`: 145 (`0x80|17`) → **147 (`0x80|19`)** (NonCounted twin formula `base | 0x80`). - `NotSummedProvableSumTree`: stays at **177** (`0xB1`) — already hand-assigned because base 17/19 both overflow the formula's low nibble. - `NotCountedOrSummedProvableSumTree`: stays at **193** (`0xC1`) — same hand-assigned-slot rationale. ## Conflict resolution - `Element` enum (mod.rs): kept develop's NotCountedOrSummed=17, ReferenceWithSumItem=18; moved ProvableSumTree to 19 with an updated doc comment explaining the positioning. - `ElementType` enum: restored develop's discriminant assignments for the wrapper-byte comment block (17 = NCOS wrapper) and updated the ProvableSumTree base + NonCounted twin to 19/147. - `is_non_counted`: adopted develop's range check (`0x80 <= disc < 0xB0`) — required to cover synthetic twins for high-numbered base discriminants like 147 (= `0x80|19`). - `from_serialized_value`: NonCounted-inner allowlist now includes `18` (ReferenceWithSumItem) AND `19` (ProvableSumTree). - NotSummed / NotCountedOrSummed inner-byte matches: `17 => *ProvableSumTree` → `19 => *ProvableSumTree`. - `element/helpers.rs`: collapsed the two-sided `match` arms in `sum_value_or_default`, `count_sum_value_or_default`, and `big_sum_value_or_default` so both `ProvableSumTree` and `ReferenceWithSumItem` route through the same `(_, sum_value, _)` pattern. - Tests: extended the discriminant table to 17 variants (added byte 19 for ProvableSumTree); merged the ProvableSumTree and ReferenceWithSumItem constructor-helper test blocks so both sets of tests live in the same module. - Updated all stale `base 17` / `145` / `0x80|17` references in doc comments to match the new layout. ## Verification - `cargo build --workspace`: clean. - `cargo clippy --workspace --all-features`: clean. - `cargo test --workspace`: 3193 / 0 fail (3138 before, +55 from merging in #667's new tests, all green after the discriminant fix-up). 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.
Summary
Adds a third element wrapper that suppresses both count and sum contributions when inserted into a count-and-sum-bearing parent. The existing wrappers each cover one axis:
NonCountedsuppresses count,NotSummedsuppresses sum.SumTree,BigSumTree,CountSumTree,ProvableCountSumTree.Same allow-list as
NotSummed— forSumTree/BigSumTreethe wrapper suppresses the implicit count-of-one a subtree would contribute to its parent count tree.CountSumTreeorProvableCountSumTree— the only tree types where suppressing both axes is meaningful. Any other parent rejects the wrapper at insert withInvalidInputError(per-merk) orInvalidBatchOperation(batch).Aggregation suppression flows through the standard helpers:
count_value_or_defaultreturns 0sum_value_or_default/big_sum_value_or_defaultreturn 0count_sum_value_or_defaultreturns(0, 0)Discriminant layout
0x800xb00xc0disc & 0xf00x800xb00xc0The deserialize pre-check rejects every ordered pair of wrapper bytes to bound bincode recursion — no path can construct a nested wrapper from on-disk bytes.
Wiring
grovedb-element: newElement::NotCountedOrSummedvariant,ElementTypetwins, constructor (new_not_counted_or_summed,into_not_counted_or_summed), helpers (is_not_counted_or_summed,is_wrapped), display, type-twin mapping, serde shadow, and serialize/deserialize guards.merk: newTreeType::is_count_and_sum_bearing()predicate; insert guards oninsert/insert_reference/insert_subtree;ElementReconstructExtensions,tree_typeextensions, cost paths, and v0/v1getpaths all look through the new wrapper.grovedb:GroveOp::InsertTreeWithRootHashgains anot_counted_or_summed: boolfield carried through propagation and re-applied at execution viainto_not_counted_or_summed(). Batch validation guard, query/proof/get match arms, debugger and estimated-cost paths all see the new wrapper.Tests
grovedb-element: discriminant pinning, helper predicates, serde round-trip and rejection cases.merk:TreeType::is_count_and_sum_bearingmatrix; per-merk insert acceptance inCountSumTree, rejection inNormalTree/SumTree/CountTree; aggregate-suppression assertion.grovedb: end-to-end constructor invariants, batch insert rejection in non-count-sum parents, direct + batch insert inCountSumTreeandProvableCountSumTreewith both axes suppressed, batch propagation preserves the wrapper on disk, andcheck_subtree_existstraverses through the wrapper.421 merk lib tests pass, 83 grovedb-element tests pass, 1552 grovedb lib tests pass;
cargo clippyclean on the touched crates.Relation to prior wrappers
This follows the pattern established by #604 (
NonCounted) and #659 (NotSummed). The discriminant range continues the contiguous wrapper-byte sequence (15 → 16 → 17) and the synthetic-twin upper-nibble sequence (0x80→0xb0→0xc0).🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
NotCountedOrSummedelement wrapper to exclude subtrees from both count and sum aggregation in compatible tree types.Tests