feat(merk): extend EstimatedLayerSizes/EstimatedSumTrees for sum-bearing leaves and Provable* trees - #674
Merged
Conversation
…ing 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>
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughAdds provable-weight fields and version-dispatched average-case Merk cost logic: new types/helpers, v0/v1/v2 sizing and propagation implementations (v2 folds provable weights), version metadata and a macro, plus updated tests to supply new provable weight fields. ChangesProvable weights cost estimation
Sequence Diagram(s)No sequence diagram generated in the visible section. Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #674 +/- ##
===========================================
+ Coverage 91.39% 91.41% +0.01%
===========================================
Files 224 235 +11
Lines 65441 66867 +1426
===========================================
+ Hits 59811 61127 +1316
- Misses 5630 5740 +110
🚀 New features to boost your workflow:
|
…nd 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>
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
merk/src/estimated_costs/average_case_costs.rs (1)
800-803:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe mixed propagation total is divided by
nodes_updatedwhen it should be scaled by it.
total_updates_costis already the weighted per-node sum. To get total bytes for the propagation, this needs to be converted withnodes_updated * total_updates_cost / total_weight(or an equivalent checked form). Dividing bynodes_updated * total_weightcollapses multi-level mixes to a fraction of a single-node update, and the new*_with_sum_item_sizebranches inherit the same undercount.Also applies to: 891-897, 981-983, 1082-1088, 1205-1207, 1296-1302, 1386-1388, 1487-1493
🤖 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/estimated_costs/average_case_costs.rs` around lines 800 - 803, The mixed-propagation calculation incorrectly divides by nodes_updated (and by nodes_updated * total_weight) instead of scaling the weighted per-node sum to a total; fix the branches that compute propagation bytes (including the new *_with_sum_item_size branches) to compute total propagation as nodes_updated * total_updates_cost / total_weight using checked arithmetic (e.g., compute weighted_nodes_updated = (nodes_updated as u64).checked_mul(total_updates_cost as u64)? then divide by total_weight via checked_div or equivalent), replacing any instances where total_updates_cost is divided by nodes_updated or by nodes_updated * total_weight; update the same pattern at the other listed sites (around the weighted_nodes_updated use and at lines noted in the comment) to ensure multi-level mixes are scaled up, not down.
🤖 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-version/src/lib.rs`:
- Around line 85-94: The macro check_grovedb_v0_v1_or_v2! currently emits a bare
GroveVersionError::UnknownVersionMismatch which requires callers to have
GroveVersionError in scope; update the macro to reference the error via the
crate path (use $crate::error::GroveVersionError::UnknownVersionMismatch) so the
expansion is hygienic and independent of the caller's imports, and adjust any
other GroveVersionError occurrences in that macro to
$crate::error::GroveVersionError accordingly.
In `@merk/src/estimated_costs/average_case_costs.rs`:
- Around line 460-476: combined_size is computed by summing raw sizes rather
than size*weight, which undercounts items with weight>1; update the averaging to
multiply each size by its corresponding weight before adding. Specifically, in
the code that computes combined_size (currently using item_size, ref_size,
subtree_size, item_sum_size, ref_sum_size) replace the plain checked_add chain
with checked_mul of each size by its matching weight
(item_size.checked_mul(item_weight), ref_size.checked_mul(ref_weight),
subtree_size.checked_mul(subtree_weight),
item_sum_size.checked_mul(item_sum_weight),
ref_sum_size.checked_mul(ref_sum_weight)) and then checked_add them together,
propagating the same Error::Overflow on any checked_* failure, and finally
divide that weighted numerator by combined_weight as before.
- Around line 118-123: The v0 branch currently checks total_weight_legacy but
then divides by (*sum_trees_weight as u32 + *non_sum_trees_weight as u32), which
can still be zero; update the guard inside the version == 0 branch to explicitly
check the actual divisor (sum_trees_weight + non_sum_trees_weight) and return
Err(Error::DivideByZero(...)) when that sum is zero before performing the
division; locate the check and division around the variables version,
total_weight_legacy, sum_trees_weight, and non_sum_trees_weight and replace the
guard to validate the actual divisor used.
---
Outside diff comments:
In `@merk/src/estimated_costs/average_case_costs.rs`:
- Around line 800-803: The mixed-propagation calculation incorrectly divides by
nodes_updated (and by nodes_updated * total_weight) instead of scaling the
weighted per-node sum to a total; fix the branches that compute propagation
bytes (including the new *_with_sum_item_size branches) to compute total
propagation as nodes_updated * total_updates_cost / total_weight using checked
arithmetic (e.g., compute weighted_nodes_updated = (nodes_updated as
u64).checked_mul(total_updates_cost as u64)? then divide by total_weight via
checked_div or equivalent), replacing any instances where total_updates_cost is
divided by nodes_updated or by nodes_updated * total_weight; update the same
pattern at the other listed sites (around the weighted_nodes_updated use and at
lines noted in the comment) to ensure multi-level mixes are scaled up, not down.
🪄 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: 9e112fdc-e123-441f-a828-09e3ba855455
📒 Files selected for processing (5)
grovedb-version/src/lib.rsgrovedb-version/src/version/v3.rsgrovedb/src/batch/estimated_costs/average_case_costs.rsgrovedb/src/tests/misc_coverage_tests.rsmerk/src/estimated_costs/average_case_costs.rs
…atedSumTrees 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>
…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>
…les (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>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
merk/src/estimated_costs/average_case_costs/value_with_feature_and_flags_size/v0.rs (1)
116-118: 💤 Low valueMinor: Error message is misleading for
checked_div.
checked_divreturnsNoneon divide-by-zero, not overflow. While this path is unreachable due to thenonzero_kinds == 0guard at line 79, the error message could be more accurate for defensive coding.🔧 Suggested improvement
combined_size .checked_div(combined_weight) - .ok_or(Error::Overflow("overflow for value size")) + .ok_or(Error::Overflow("value size divide by zero"))🤖 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/estimated_costs/average_case_costs/value_with_feature_and_flags_size/v0.rs` around lines 116 - 118, The error message for the checked_div call on combined_size / combined_weight is misleading (checked_div returns None on divide-by-zero); update the error to accurately reflect a division-by-zero failure instead of overflow—replace the Error::Overflow("overflow for value size") used with combined_size.checked_div(combined_weight) with a DivisionByZero-style error or a message like "division by zero for value size" (preserve the existing nonzero_kinds guard logic that makes this path unreachable but keep defensive, clear wording).
🤖 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 `@merk/src/estimated_costs/average_case_costs/mod.rs`:
- Around line 199-213: The two calls that propagate errors
directly—estimated_sum_trees.estimated_size(grove_version) inside
EstimatedLayerSizes::AllSubtrees and est.estimated_size(grove_version) inside
EstimatedLayerSizes::Mix—should be wrapped with a map_err to add call-site
context (use Error::CorruptedData(format!("...: {}", e))) so failures include a
descriptive message; update both sites to replace the `?`-propagated call with
`.map_err(|e| Error::CorruptedData(format!("failed to estimate subtree size at
layer X: {}", e)))?` (adjust the literal context to mention subtree
sizing/level) to follow the repository error-wrapping guideline.
---
Nitpick comments:
In
`@merk/src/estimated_costs/average_case_costs/value_with_feature_and_flags_size/v0.rs`:
- Around line 116-118: The error message for the checked_div call on
combined_size / combined_weight is misleading (checked_div returns None on
divide-by-zero); update the error to accurately reflect a division-by-zero
failure instead of overflow—replace the Error::Overflow("overflow for value
size") used with combined_size.checked_div(combined_weight) with a
DivisionByZero-style error or a message like "division by zero for value size"
(preserve the existing nonzero_kinds guard logic that makes this path
unreachable but keep defensive, clear wording).
🪄 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: 27f99aa0-baa5-40f8-b332-eca0beb1347d
📒 Files selected for processing (18)
grovedb-version/src/lib.rsgrovedb-version/src/version/merk_versions.rsgrovedb-version/src/version/v1.rsgrovedb-version/src/version/v2.rsgrovedb-version/src/version/v3.rsmerk/src/estimated_costs/average_case_costs.rsmerk/src/estimated_costs/average_case_costs/add_average_case_merk_propagate/mod.rsmerk/src/estimated_costs/average_case_costs/add_average_case_merk_propagate/v0.rsmerk/src/estimated_costs/average_case_costs/add_average_case_merk_propagate/v1.rsmerk/src/estimated_costs/average_case_costs/add_average_case_merk_propagate/v2.rsmerk/src/estimated_costs/average_case_costs/estimated_sum_trees_size/mod.rsmerk/src/estimated_costs/average_case_costs/estimated_sum_trees_size/v0.rsmerk/src/estimated_costs/average_case_costs/estimated_sum_trees_size/v1.rsmerk/src/estimated_costs/average_case_costs/estimated_sum_trees_size/v2.rsmerk/src/estimated_costs/average_case_costs/mod.rsmerk/src/estimated_costs/average_case_costs/value_with_feature_and_flags_size/mod.rsmerk/src/estimated_costs/average_case_costs/value_with_feature_and_flags_size/v0.rsmerk/src/estimated_costs/average_case_costs/value_with_feature_and_flags_size/v1.rs
💤 Files with no reviewable changes (1)
- merk/src/estimated_costs/average_case_costs.rs
✅ Files skipped from review due to trivial changes (1)
- grovedb-version/src/version/v1.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- grovedb-version/src/lib.rs
Comment on lines
+199
to
+213
| EstimatedLayerSizes::AllSubtrees(_, estimated_sum_trees, flags_size) => { | ||
| // 1 for enum type | ||
| // 1 for empty | ||
| // 1 for flags size | ||
| Ok(estimated_sum_trees.estimated_size(grove_version)? | ||
| + flags_size.unwrap_or_default() | ||
| + 3) | ||
| } | ||
| EstimatedLayerSizes::Mix { subtrees_size, .. } => match subtrees_size { | ||
| None => Err(Error::WrongEstimatedCostsElementTypeForLevel( | ||
| "this layer is a mix but doesn't have subtrees", | ||
| )), | ||
| Some((_, est, fs, _)) => { | ||
| Ok(est.estimated_size(grove_version)? + fs.unwrap_or_default() + 3) | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Wrap estimated_size errors with call-site context in subtree sizing.
Both estimated_size(grove_version)? paths bubble errors without context, making diagnosis harder in dispatch failures. Please wrap them with contextual map_err(...) per repository guideline.
Proposed patch
EstimatedLayerSizes::AllSubtrees(_, estimated_sum_trees, flags_size) => {
// 1 for enum type
// 1 for empty
// 1 for flags size
- Ok(estimated_sum_trees.estimated_size(grove_version)?
+ Ok(estimated_sum_trees
+ .estimated_size(grove_version)
+ .map_err(|e| {
+ Error::CorruptedData(format!(
+ "subtree_with_feature_and_flags_size (all_subtrees): {}",
+ e
+ ))
+ })?
+ flags_size.unwrap_or_default()
+ 3)
}
EstimatedLayerSizes::Mix { subtrees_size, .. } => match subtrees_size {
None => Err(Error::WrongEstimatedCostsElementTypeForLevel(
"this layer is a mix but doesn't have subtrees",
)),
Some((_, est, fs, _)) => {
- Ok(est.estimated_size(grove_version)? + fs.unwrap_or_default() + 3)
+ Ok(est
+ .estimated_size(grove_version)
+ .map_err(|e| {
+ Error::CorruptedData(format!(
+ "subtree_with_feature_and_flags_size (mix): {}",
+ e
+ ))
+ })?
+ + fs.unwrap_or_default()
+ + 3)
}
},As per coding guidelines, "Wrap errors with context using .map_err(|e| Error::CorruptedData(format!("context: {}", e))) pattern in Rust source files".
📝 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
| EstimatedLayerSizes::AllSubtrees(_, estimated_sum_trees, flags_size) => { | |
| // 1 for enum type | |
| // 1 for empty | |
| // 1 for flags size | |
| Ok(estimated_sum_trees.estimated_size(grove_version)? | |
| + flags_size.unwrap_or_default() | |
| + 3) | |
| } | |
| EstimatedLayerSizes::Mix { subtrees_size, .. } => match subtrees_size { | |
| None => Err(Error::WrongEstimatedCostsElementTypeForLevel( | |
| "this layer is a mix but doesn't have subtrees", | |
| )), | |
| Some((_, est, fs, _)) => { | |
| Ok(est.estimated_size(grove_version)? + fs.unwrap_or_default() + 3) | |
| } | |
| EstimatedLayerSizes::AllSubtrees(_, estimated_sum_trees, flags_size) => { | |
| // 1 for enum type | |
| // 1 for empty | |
| // 1 for flags size | |
| Ok(estimated_sum_trees | |
| .estimated_size(grove_version) | |
| .map_err(|e| { | |
| Error::CorruptedData(format!( | |
| "subtree_with_feature_and_flags_size (all_subtrees): {}", | |
| e | |
| )) | |
| })? | |
| flags_size.unwrap_or_default() | |
| 3) | |
| } | |
| EstimatedLayerSizes::Mix { subtrees_size, .. } => match subtrees_size { | |
| None => Err(Error::WrongEstimatedCostsElementTypeForLevel( | |
| "this layer is a mix but doesn't have subtrees", | |
| )), | |
| Some((_, est, fs, _)) => { | |
| Ok(est | |
| .estimated_size(grove_version) | |
| .map_err(|e| { | |
| Error::CorruptedData(format!( | |
| "subtree_with_feature_and_flags_size (mix): {}", | |
| e | |
| )) | |
| })? | |
| fs.unwrap_or_default() | |
| 3) | |
| } | |
| } |
🤖 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/estimated_costs/average_case_costs/mod.rs` around lines 199 - 213,
The two calls that propagate errors
directly—estimated_sum_trees.estimated_size(grove_version) inside
EstimatedLayerSizes::AllSubtrees and est.estimated_size(grove_version) inside
EstimatedLayerSizes::Mix—should be wrapped with a map_err to add call-site
context (use Error::CorruptedData(format!("...: {}", e))) so failures include a
descriptive message; update both sites to replace the `?`-propagated call with
`.map_err(|e| Error::CorruptedData(format!("failed to estimate subtree size at
layer X: {}", e)))?` (adjust the literal context to mention subtree
sizing/level) to follow the repository error-wrapping guideline.
Member
Author
There was a problem hiding this comment.
This is Claude. Acknowledged but skipping: estimated_size(grove_version)? is propagated bare from ~16 call sites across this directory (value_with_feature_and_flags_size_v0/_v1 and the three add_average_case_merk_propagate_v0/_v1/_v2 Mix arms — see rg 'estimated_size\(grove_version\)\?'). Wrapping just these two would leave the other ~14 inconsistent, and the code in subtree_with_feature_and_flags_size is pre-existing (this PR only restructured files around it). Better to handle uniformly in a separate refactor PR — happy to file an issue if useful.
Contributor
There was a problem hiding this comment.
@QuantumExplorer, that's a fair and well-reasoned call — patching just two of ~16 bare-propagation sites would leave the rest inconsistent and make the codebase harder to reason about. Deferring to a uniform refactor PR makes sense.
Would you like me to open a GitHub issue to track the follow-up refactor ("Wrap all estimated_size(grove_version)? call sites with map_err context across average_case_costs/")?
🧠 Learnings used
Learnt from: QuantumExplorer
Repo: dashpay/grovedb PR: 669
File: merk/src/proofs/query/count_offset/tests.rs:54-64
Timestamp: 2026-05-17T11:22:44.675Z
Learning: In Rust code reviews, do not flag `impl Trait` used in *argument-position* as an error when it appears inside a concrete generic type in a function parameter type, e.g. `fn foo(x: &Merk<impl StorageContext<'static>>)` / `Foo<impl Bar>`. This is valid argument-position `impl Trait` (APIT) that desugars to an anonymous generic parameter and compiles on stable Rust.
Only treat `impl Trait` as unsupported when it’s nested/used in an invalid `impl Trait`-within-`impl Trait` position (e.g. forms like `impl Fn(impl Trait)`).
Learnt from: QuantumExplorer
Repo: dashpay/grovedb PR: 670
File: grovedb/src/tests/provable_count_provable_sum_tree_tests.rs:80-86
Timestamp: 2026-05-17T14:06:21.953Z
Learning: When reviewing Rust `matches!(value, pattern)` usages, do not warn that `value` is being moved if the pattern uses only non-binding `_` discards (e.g., `SomeEnum::Variant(_, _, _, _)` or other `_`-only wildcards). In this case, `_` is a non-binding discard and will not move or borrow `value`, so `value` remains usable after the `matches!` call. A move can occur only if the pattern binds fields (e.g., `Some(x)`), especially for non-`Copy` fields. Therefore, `matches!(non_copy_value, AllWildcardPattern)` is safe and should not be flagged as a potential move.
Member
Author
There was a problem hiding this comment.
This is Claude. Thanks — leaving the tracking-issue decision to @QuantumExplorer rather than spawning one unprompted. The thread is enough breadcrumb if/when someone picks this up.
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>
…vel #![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>
…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>
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
Extends merk's average-case cost estimation to fully account for v12-only sum-bearing element variants and the
Provable*tree family. Building block for dash-platform PR #3661, which can switch itsEstimatedLayerInformationplumbing onto these new variants once this lands and the pin is bumped.Today, callers approximating layers built on
Element::ItemWithSumItemorElement::ReferenceWithSumItemcan only reachEstimatedLayerSizes::AllItems(...)/AllReference(...), undercharging by ~10 bytes per insert (ani64sum_value's worst-case varint). For property-name trees declaredrange_summable/range_summable + range_countable, theProvable*tree types collapse intonon_sum_trees_weightand silently report zero per-node aggregate cost.What changed
EstimatedLayerSizes:AllItemsWithSumItem(...)/AllReferencesWithSumItem(...)variants, sized as plain-item/reference ++10for the i64 sum_value varint — matchingElement::required_{item,reference}_with_sum_item_spacefrom feat(element): add required_{item,reference}_with_sum_item_space helpers #673.Mixextended withitems_with_sum_item_sizeandreferences_with_sum_item_sizefor weighted layers.EstimatedSumTrees:SomeSumTreesgains four weights:provable_sum_trees_weight,provable_count_trees_weight,provable_count_sum_trees_weight,provable_count_provable_sum_trees_weight.AllProvable*homogeneous shortcut variants mirroringAllSumTrees/AllCountTreesetc.Versioning:
MerkAverageCaseCostsVersions::sum_tree_estimated_sizebumped1 → 2in v3 only. v0 and v1 formulas are byte-stable for already-shipped grove versions; the newprovable_*weights are silently ignored on those paths.check_grovedb_v0_v1_or_v2!macro added to support the three-version dispatch.All four propagate match arms (
v0+v1, each forreplaced_bytes+storage_loaded_bytes) wired through the new variants and Mix fields.The new
EstimatedLayerSizesvariants are not version-gated themselves — they're discriminants new at the type level, so existing callers compile unchanged. dash-platform's v11 sites stay onAllItems(...)(consensus-locked) and v12+ sites can opt in toAllItemsWithSumItem(...).Tests
14 new unit tests in
average_case_costs::tests:+10layer-size formula forAllItemsWithSumItem/AllReferencesWithSumItem(assertion:plain + 10).add_average_case_merk_propagatecost strictly exceeds the plain variant for the same key/value/flags, on bothreplaced_bytesandstorage_loaded_bytes.Mixwith only the new sum-item fields populated.provable_*weights (regression — grove v1 path).provable_*weights (regression — grove v2 production path).SomeSumTreesequals the matchingAllProvable*shortcut.All existing tests pass:
grovedb-merk645 ✓,grovedb1834 ✓.Test plan
cargo build— full workspacecargo test -p grovedb-merk— 645 passcargo test -p grovedb— 1834 passRelated
required_{item,reference}_with_sum_item_spacehelpers).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests