feat(element): add required_{item,reference}_with_sum_item_space helpers - #673
Merged
Conversation
Adds two worst-case storage-sizing helpers paralleling Element::required_item_space for the sum-bearing variants introduced in #670 / #667: - Element::required_item_with_sum_item_space - Element::required_reference_with_sum_item_space Both reserve 10 bytes for the i64 sum_value as an upper bound (bincode 2.x maxes at 9 bytes for a zigzag-encoded u64; 10 is a safety margin) so dry-run / stateless-cost callers in dash-platform never undercharge on summable-index writes. Wires required_item_with_sum_item_space and required_reference_with_sum_item_space FeatureVersion fields through GroveDBElementMethodVersions (initialized to 0 in v1, v2, v3). Tests cover the manual-formula contract and exhaustively sweep boundary sum values (0, +-1, +-250, i64::MAX, i64::MIN), payload sizes, max_hop, and flag variants to assert helper >= serialize().len() for every combination. 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 (6)
✨ 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #673 +/- ##
========================================
Coverage 91.38% 91.39%
========================================
Files 224 224
Lines 65404 65441 +37
========================================
+ Hits 59771 59811 +40
+ Misses 5633 5630 -3
🚀 New features to boost your workflow:
|
Adds required_with_sum_item_space_helpers_reject_unknown_version to exercise the check_grovedb_v0! mismatch arm on both new helpers, lifting diff coverage above the 90% codecov gate (the macro's error branch was the only uncovered region in the previous diff). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
QuantumExplorer
added a commit
that referenced
this pull request
May 19, 2026
…ing leaves and Provable* trees (#674) * feat(merk): extend EstimatedLayerSizes/EstimatedSumTrees for sum-bearing leaves and Provable* trees Adds layer-level cost estimation support for the v12-only sum-bearing element variants and the Provable* tree family. Without these, dash-platform's EstimatedLayerInformation plumbing can only reach approximations that under-count by ~10 bytes per ItemWithSumItem insert (an i64 sum_value's worst-case varint), and silently zeros out the cost of every property-name tree built on ProvableSumTree / ProvableCountTree / ProvableCountSumTree / ProvableCountProvableSumTree. EstimatedLayerSizes additions: - AllItemsWithSumItem / AllReferencesWithSumItem variants matching the +10 sum-value worst-case constant used by Element::required_{item,reference}_with_sum_item_space (#673). - Mix extended with items_with_sum_item_size / references_with_sum_item_size fields so a mixed layer can weight sum-bearing leaves alongside plain ones. EstimatedSumTrees additions: - SomeSumTrees gains four new weights: provable_sum_trees_weight, provable_count_trees_weight, provable_count_sum_trees_weight, provable_count_provable_sum_trees_weight. - Four AllProvable* homogeneous shortcut variants mirroring the existing AllSumTrees / AllCountTrees etc. Versioning: - estimated_size bumped 1 -> 2 in v3 only. v0/v1 formulas remain byte-stable for already-shipped grove versions (regression tests pin this); the new provable_* weights are silently ignored by v0/v1 and only folded into the weighted average by v2. All four propagate match arms (v0 + v1, replaced_bytes + storage_loaded_bytes) wired through; new layer variants add +10 on top of the +3/+5 plain-item/reference base. Test fixtures in grovedb/src/batch and grovedb/src/tests/misc_coverage_tests updated for the new SomeSumTrees fields (defaulted to 0). 14 new tests cover: the +10 layer-size formula for items/references, propagate cost strictly exceeding plain variants, Mix with only sum-item fields, v0/v1 output stability under new weights, v2 actually using them, homogeneous-vs-shortcut equivalence, and the v2 divide-by-zero guard. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(merk): add coverage for v0 propagate with sum-bearing variants and version-gate error Closes the coverage gaps codecov/patch flagged on PR #674: - v0 propagate (grove v1 dispatch) with `AllItemsWithSumItem` and `AllReferencesWithSumItem` — the +10 sum-value adjustment must flow through identically on the v0 path even though dash-platform's v11 sites stay on plain `AllItems`. - v0 propagate with `Mix` containing both `items_with_sum_item_size` and `references_with_sum_item_size` populated — exercises both the replaced_bytes and storage_loaded_bytes Mix arms in v0. - v1 propagate with `Mix` containing only `references_with_sum_item_size` populated — pre-existing test covered the items branch but not refs. - `EstimatedSumTrees::estimated_size` version-gate error path — exercises `check_grovedb_v0_v1_or_v2!` for an unknown version. All 57 estimated_costs tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(merk,grovedb-version): macro hygiene + v0 divisor guard for EstimatedSumTrees Addresses CodeRabbit review on PR #674: 1. `check_grovedb_v0_v1_or_v2!` macro: qualify `GroveVersionError` as `$crate::error::GroveVersionError` so the expansion is hygienic and doesn't require callers to import the type. Matches the convention in the `*_with_cost` variants already in the file. 2. `EstimatedSumTrees::estimated_size` v0 path: guard the actual v0 divisor `(sum_trees_weight + non_sum_trees_weight)` rather than the legacy total. A layer with only `big_sum_trees_weight` (or any count weight) populated produced a nonzero legacy total but a zero v0 denominator — silently panicking on `/ 0` instead of surfacing `DivideByZero`. The original guard had the same latent bug (this PR's branch reorg made it observable); fix in v0 only, v1/v2 denominators remain `total_weight_legacy` / `total_weight` as before. Regression test pins the new v0 guard. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(merk): version-gate the Mix-arm cost formulas; grove v3 gets the fixes Closes the second half of CodeRabbit's review on PR #674. Both bugs are pre-existing, but now version-gated so grove v1/v2 preserve their shipped (buggy) outputs and only grove v3+ picks up the corrections. ### Bug 1: `EstimatedLayerSizes::value_with_feature_and_flags_size` The Mix arm averaged kinds as: Σ size_i / Σ weight_i (v0, legacy) instead of the proper weighted average: Σ (size_i · weight_i) / Σ weight_i (v1, fixed) On a 3:1:2 item/ref/subtree mix that's `(37+30+7)/6 = 12` (legacy) vs the correct `(3·37 + 1·30 + 2·7)/6 = 25`. The function is now dispatched via a new `MerkAverageCaseCostsVersions::value_with_feature_and_flags_size` field (`v1.rs/v2.rs → 0`, `v3.rs → 1`). Non-Mix arms are unchanged. ### Bug 2: `add_average_case_merk_propagate` Mix arms v1 (used by grove v2) computes: total_replaced_bytes = Σ(weight_i · cost_i) / (nodes_updated · total_weight) This is the per-node weighted average divided by `nodes_updated`. For an items-only Mix layer it returns `cost / nodes_updated²` — a Mix that's semantically equivalent to `AllItems` underestimates by a factor of `nodes_updated²`. v2 (new, used by grove v3) computes: total_replaced_bytes = nodes_updated · Σ(weight_i · cost_i) / total_weight which matches what the homogeneous `AllItems`/`AllSubtrees` arms already do. Bumped `add_average_case_merk_propagate: 1 → 2` in v3 only. Pinning test asserts v2 Mix-items-only equals AllItems exactly. ### Version table (grovedb-version) - v1 grove (consensus-locked v11 production): unchanged — v0 propagate, v0 value_with... - v2 grove (consensus-locked v12 production): unchanged — v1 propagate, v0 value_with... - v3 grove (active dev version): v2 propagate, v1 value_with... (new formulas) ### Tests 8 new tests: - `test_value_size_mix_weighted_combination_v1` — assert `25`, the correct weighted average - `test_value_size_mix_weighted_combination_v0_legacy` — assert `12`, pinning the v0 path - `test_value_with_feature_and_flags_size_unknown_version_error` - `test_propagate_v2_mix_items_only_matches_all_items` — v2 invariant - `test_propagate_v2_mix_exceeds_v1_mix` — v2 strictly larger than v1 on same Mix - `test_propagate_v1_v2_agree_on_non_mix_variants` — only the Mix arm changed - `test_propagate_unknown_version_error_lists_all_three` — error path for new dispatch row - (renamed the original test_value_size_mix_weighted_combination → _v1) All 657 merk tests pass; all 1834 grovedb tests pass (the existing batch fee-cost tests don't use Mix, so no fee drift in production scenarios). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(merk): split versioned avg-case cost fns into per-version files (platform-style) Restructures `merk/src/estimated_costs/average_case_costs.rs` from a single file with inline version branches / co-located v* functions into the dash-platform-style layout: one directory per versioned function with a `mod.rs` dispatcher and per-version `v0.rs`/`v1.rs`/ `v2.rs` files. ``` merk/src/estimated_costs/average_case_costs/ ├── mod.rs # types, non-versioned methods, helpers, tests ├── estimated_sum_trees_size/ # EstimatedSumTrees::estimated_size │ ├── mod.rs # version dispatcher │ ├── v0.rs # grove v1 formula │ ├── v1.rs # grove v2 formula │ └── v2.rs # grove v3 formula (adds Provable* weights) ├── value_with_feature_and_flags_size/ # EstimatedLayerSizes::value_with_... │ ├── mod.rs │ ├── v0.rs # legacy unweighted Mix average │ └── v1.rs # proper weighted Mix average └── add_average_case_merk_propagate/ # free fn dispatcher ├── mod.rs # also exports average_case_merk_propagate wrapper ├── v0.rs # grove v1 ├── v1.rs # grove v2 └── v2.rs # grove v3 (Mix divisor fix) ``` `EstimatedSumTrees::estimated_size` was the one CodeRabbit-style target: previously it had an inline `if version == 0 { ... } else if version == 1 { ... }` chain after a `check_grovedb_v0_v1_or_v2!` macro. It now follows the same dispatcher-via-match pattern as the other two functions (and the per-version logic is split across three files for readability). No behavior change — code was moved as-is. All 657 merk tests + 1834 grovedb tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(merk): use DivideByZero for the v0 Mix checked_div ok_or path Addresses CodeRabbit nitpick on PR #674: the `combined_size.checked_div(combined_weight).ok_or(Error::Overflow(...))` on `value_with_feature_and_flags_size_v0` returned `Error::Overflow` even though `checked_div` only fails on divide-by-zero. The path is unreachable (the `nonzero_kinds == 0` guard above rules out a zero divisor), but the wording was misleading. Now surfaces `Error::DivideByZero("value size divisor was zero")` instead. No behavior change for any reachable input. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(merk): hoist per-item #[cfg(feature = "minimal")] to file-level #![cfg(...)] Inside the three per-function submodules (`add_average_case_merk_propagate/`, `estimated_sum_trees_size/`, `value_with_feature_and_flags_size/`), every item carried its own `#[cfg(feature = "minimal")]`. The parent `average_case_costs/mod.rs` already gates each submodule with `#[cfg(feature = "minimal")] mod foo;`, so when the feature is off the entire submodule isn't compiled — the per-item gates were redundant. Replaced with a single `#![cfg(feature = "minimal")]` inner attribute at the top of each file, removing ~50 redundant per-item gates across 11 files. Self-documents the file-level dependency without changing behavior. All 657 merk tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(merk): drop redundant feature gates from average_case_costs subtree `merk/src/estimated_costs/mod.rs` declares the submodule as: #[cfg(feature = "minimal")] pub mod average_case_costs; so when the feature is off the entire module tree (including all the new per-function subdirectories) is excluded by Cargo. Every per-item `#[cfg(feature = "minimal")]` inside `average_case_costs/mod.rs` and every file-level `#![cfg(feature = "minimal")]` inside the `add_average_case_merk_propagate/`, `estimated_sum_trees_size/`, and `value_with_feature_and_flags_size/` subdirs is therefore redundant. Removed 41 redundant cfg attributes across 12 files (1 file-level `#![cfg(...)]` per submodule file × 11, plus 30 per-item `#[cfg(feature = "minimal")]` in `average_case_costs/mod.rs`). No behavior change. All 657 merk tests still 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
grovedb-elementexposes a worst-case storage-sizing helper for plainElement::Item(Element::required_item_space), but PR #670 (ItemWithSumItem) and #667 (ReferenceWithSumItem) shipped their sum-bearing siblings without equivalent helpers.Drive callers in dash-platform need them for stateless-cost / dry-run fee estimation on summable-index inserts. Without them the estimate undercharges the bytes the
i64 sum_valueadds to the serialized element, and dry-run fees diverge from applied fees on everydocumentsSummable/summable/rangeSummablewrite. Three call sites inrs-drivecurrently carryTODO(sum-feature, cost):comments pointing at this gap.What was done?
Added two helpers on
impl Elementingrovedb-element/src/element/helpers.rs, parallelingrequired_item_space:Both add
+10on top of the plain-item formula for the worst-casei64sum_value. bincode 2.x varint maxes at 9 bytes for a zigzag-encodedu64;+10is a deliberate safety margin so callers using this for stateless-cost / dry-run fee estimation never undercharge. The contract is a strict upper bound — exact equality is not required.Wired two new
FeatureVersionfields throughgrovedb-version/src/version/grovedb_versions.rs::GroveDbVersionElement:…and initialized them to
0alongside the existingrequired_item_space: 0inv1.rs,v2.rs, andv3.rs.How Has This Been Tested?
Four new tests in
grovedb-element/tests/element_constructors_helpers.rs:required_item_with_sum_item_space_matches_manual_formula— mirrors the existingrequired_item_space_matches_manual_formulashape.required_item_with_sum_item_space_is_upper_bound— sweeps payload sizes (0,1,250,65_500bytes), flag variants, and boundary sum values (0,±1,±250,i64::MAX,i64::MIN,i32::MAX), assertingrequired >= serialize().len()for every combination.required_reference_with_sum_item_space_matches_manual_formula.required_reference_with_sum_item_space_is_upper_bound— sweeps path variants,max_hopvalues, flag variants, and boundary sums; uses bincode to compute the worst-case path payload size the way real callers do (e.g.add_document_to_primary_storage).Verified:
cargo test -p grovedb-element --test element_constructors_helpers— all 24 tests pass.cargo build— full workspace builds cleanly.Breaking Changes
None. Two new
pub fnhelpers, two new fields appended toGroveDBElementMethodVersions(initialized to0in every version table), and no changes to existing behavior.Checklist:
Follow-up
Dash-platform PR can now switch from
Element::required_item_space(...)to the new sum-aware helpers in threers-drivecall sites currently carryingTODO(sum-feature, cost)::packages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rs:591packages/rs-drive/src/drive/document/insert/add_reference_for_index_level_for_contract_operations/v0/mod.rs:236, :280Once both are wired through,
add_estimation_costs_for_contract_insertion/v1/mod.rs:154can set non-zerosum_trees_weight/count_sum_trees_weightweights for the estimation layers (currently hardcoded to 0).