fix: make the V4 batch gates zero-cost by reusing already-loaded old elements - #790
Conversation
…elements The GROVE_V4 gates overwrite_indexed_cleanup_inspection and delete_tree_cleanup_type_source each bought their correctness with a dedicated stored-element read per op: every overwrite-capable op and every DeleteTree in a batch paid +1 seek and the loaded element bytes at V4, even when no indexed tree was anywhere near the batch (surfaced by Dash Platform identity-op fee tests at protocol v14 as +2000 seeks + 20 bytes per op unit). Derive the old element from data the apply already loads instead: - merk: apply_unchecked_with_old_value_observer threads an infallible (key, old_value, disposition) observer through the batch walk. The walker fetches an existing node before rewriting or deleting it, so surfacing its stored value is free — no storage read, no tracked cost. - overwrite gate: execute_ops_on_path registers overwrite-capable keys and classifies the displaced element in the observer. Bare Reference overwrites are now included (there is no longer a cost argument for leaving a reference over an indexed tree unswept), which closes #776 and un-ignores its regression test. Non-empty indexed replacements are still refused — normally by the earlier ungated empty-at-batch-insertion guard, with the classifier's NotSupported kept as defense in depth. - delete-tree gate: cleanup-namespace classification moves after apply_body, driven by the ACTUAL stored types captured by the observer (declared vs stored mismatches involving an indexed tree are rejected there). The pre-apply Error/Skip emptiness checks read the stored element once up front — the same single read V1..V3 pay directly or inside the child-merk open — and hand it to a new open-with-parent-element helper so nothing is read twice. The V1..V3 pre-scan path is byte-for-byte the released behaviour, now shared by both apply_batch entry points via scan_delete_tree_ops. - estimated costs: revert the batch estimator modeling of the gated reads (#789) in both the average- and worst-case models — with the applied reads gone the estimators were overshooting by exactly one read per gated op. A new cost-parity test pins a batch of plain overwrites plus a plain DeleteTree to an identical CostResult under GROVE_V3 and GROVE_V4, and the V4 refresh-reference cost test now asserts the same constants as its _v3_keeps_live_costs companion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughBatch application now captures old Merk values for indexed overwrite cleanup and ChangesBatch cleanup and cost behavior
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant apply_batch_structure
participant Merk
participant old_value_observer
participant BatchApplyCaptures
apply_batch_structure->>Merk: apply_unchecked_with_old_value_observer
Merk->>old_value_observer: report stored key, bytes, and disposition
old_value_observer->>BatchApplyCaptures: record cleanup or deleted tree type
apply_batch_structure->>BatchApplyCaptures: classify cleanup before commit
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 #790 +/- ##
===========================================
+ Coverage 92.13% 92.17% +0.03%
===========================================
Files 257 257
Lines 78297 78079 -218
===========================================
- Hits 72137 71967 -170
+ Misses 6160 6112 -48
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
grovedb/src/tests/indexed_tree_security_regression_tests.rs (1)
327-357: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd proof verification for the reference overwrite.
assert_verify_passeschecks local database invariants only. It does not generate or verify an authenticated query proof. After recreatingcidx, generate and verify a proof that the count secondary is empty.As per coding guidelines, “Every state-modifying operation must have proof-verification coverage.”
🤖 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/indexed_tree_security_regression_tests.rs` around lines 327 - 357, Add authenticated proof generation and verification after recreating the PCIT in the indexed-tree regression test, covering the empty count secondary at cidx. Use the existing proof-verification helper or established query-proof APIs visible in the test suite, and retain the local assert_verify_passes check alongside the new proof assertion.Source: Coding guidelines
grovedb/src/tests/provable_count_indexed_tree_tests.rs (1)
475-508: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDescribe the batch-insertion guard as the rejecting check.
The comment says that
classify_cidx_overwriterejects this operation. The assertion confirms that the earlier empty-at-batch-insertion guard returnsError::InvalidBatchOperation. State that the classifier remains defense in depth.Proposed correction
- // indexed → non-empty indexed must be rejected by - // `classify_cidx_overwrite` (storage-pointer ambiguity: the new - // root keys would refer to on-disk data that post-apply - // cleanup of the OLD cidx also clears). + // The batch-insertion emptiness guard rejects indexed → non-empty + // indexed replacements before `classify_cidx_overwrite` runs. + // The classifier retains the same check as defense in depth.🤖 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/provable_count_indexed_tree_tests.rs` around lines 475 - 508, Update the test comments around the non-empty indexed-tree insertion to identify the empty-at-batch-insertion guard as the rejecting check. State that the operation is refused with Error::InvalidBatchOperation before classify_cidx_overwrite runs, while the classifier’s NotSupported rejection remains defense in depth.
🧹 Nitpick comments (3)
grovedb/src/batch/mod.rs (2)
2495-2510: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider storing only the emptiness classification instead of cloning the element.
pending_overwrite_inspectionsholds a fullElementclone for every overwrite-capable op on V4+.classify_cidx_overwriteconsumes the new element only throughreplacement_indexed_emptiness, which returnsOption<bool>. For largeItempayloads this clone allocates the whole value on the batch hot path.Compute the classification at registration time and store
Option<bool>in the map. That removes the clone and keeps the observer logic unchanged.This requires exposing
replacement_indexed_emptiness(or a small wrapper) fromindexed_tree::overwriteand changingclassify_cidx_overwriteto accept the precomputed value.🤖 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 2495 - 2510, Update the V4+ overwrite inspection flow around pending_overwrite_inspections to compute replacement_indexed_emptiness when registering each operation and store only its Option<bool> classification instead of cloning the full Element. Expose replacement_indexed_emptiness or a focused wrapper from indexed_tree::overwrite, then change classify_cidx_overwrite to accept the precomputed classification while preserving the existing observer behavior.
5110-5164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated
batch_deleted_keyscomputation into a helper.This V4 branch duplicates the
batch_deleted_keyscollection and theBTreeSet<&[u8]>conversion from the V1..V3 branch at lines 5259-5283. The two copies must stay behaviorally identical, including the rule that excludesSubelementsDeletionBehavior::Skipops. A divergence between them would change emptiness-check results on one version only.Extract a private function that takes
opsandchild_pathand returnsVec<Vec<u8>>, then call it from both branches.🤖 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 5110 - 5164, Extract the duplicated batch_deleted_keys filtering logic into a private helper accepting ops and child_path and returning Vec<Vec<u8>>. Preserve the existing handling of GroveOp::Delete, non-Skip GroveOp::DeleteTree, and exclusion of DeleteTree with SubelementsDeletionBehavior::Skip. Replace the local computations in both the V4 branch and the V1–V3 branch with the helper, retaining each branch’s existing BTreeSet conversion and emptiness checks.merk/src/tree/ops.rs (1)
1358-1377: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a direct unit test for the observer contract.
The updated tests only pass no-op observers. No test in this file asserts that the observer fires exactly once per existing key, receives the pre-op stored value, and reports
Deletedfor delete-style ops andReplacedfor put-style ops. That contract is what the GroveDB gates depend on.Add a test that collects
(key, old_value, disposition)into aVecand applies a batch with one put over an existing key, one put on a new key, and one delete of an existing key. Assert that the new key produces no callback.As per coding guidelines: "When adding functionality, check GroveDB version compatibility, implement cost calculation, support proof generation and batch operations, and add comprehensive edge-case tests."
Also applies to: 1493-1521
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@merk/src/tree/ops.rs` around lines 1358 - 1377, Add a direct unit test near the existing Walker::apply_to tests that records observer callbacks as (key, old_value, disposition), then applies a batch containing a put over an existing key, a put for a new key, and a delete of an existing key. Assert exactly one callback per existing key, with pre-operation values and Replaced for the put and Deleted for the delete, and assert the new-key put produces no callback.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@grovedb/src/batch/mod.rs`:
- Line 3855: Update the documentation above the function returning
`(Option<OpsByLevelPath>, BatchApplyCaptures)` to replace the stale
`leftover_ops` and `cidx_overwrite_cleanup_paths` description with both fields
carried by `BatchApplyCaptures`, including `deleted_tree_actual_types`.
In `@grovedb/src/tests/batch_indexed_tree_tests.rs`:
- Around line 569-576: Tighten the deterministic batch-overwrite assertions in
grovedb/src/tests/batch_indexed_tree_tests.rs at lines 569-576 and 1015-1017:
for the non-empty indexed insertion guard, require only
Error::InvalidBatchOperation instead of accepting other errors; for the
indexed-to-Item safe overwrite case, require the inner apply_batch result to
succeed.
---
Outside diff comments:
In `@grovedb/src/tests/indexed_tree_security_regression_tests.rs`:
- Around line 327-357: Add authenticated proof generation and verification after
recreating the PCIT in the indexed-tree regression test, covering the empty
count secondary at cidx. Use the existing proof-verification helper or
established query-proof APIs visible in the test suite, and retain the local
assert_verify_passes check alongside the new proof assertion.
In `@grovedb/src/tests/provable_count_indexed_tree_tests.rs`:
- Around line 475-508: Update the test comments around the non-empty
indexed-tree insertion to identify the empty-at-batch-insertion guard as the
rejecting check. State that the operation is refused with
Error::InvalidBatchOperation before classify_cidx_overwrite runs, while the
classifier’s NotSupported rejection remains defense in depth.
---
Nitpick comments:
In `@grovedb/src/batch/mod.rs`:
- Around line 2495-2510: Update the V4+ overwrite inspection flow around
pending_overwrite_inspections to compute replacement_indexed_emptiness when
registering each operation and store only its Option<bool> classification
instead of cloning the full Element. Expose replacement_indexed_emptiness or a
focused wrapper from indexed_tree::overwrite, then change
classify_cidx_overwrite to accept the precomputed classification while
preserving the existing observer behavior.
- Around line 5110-5164: Extract the duplicated batch_deleted_keys filtering
logic into a private helper accepting ops and child_path and returning
Vec<Vec<u8>>. Preserve the existing handling of GroveOp::Delete, non-Skip
GroveOp::DeleteTree, and exclusion of DeleteTree with
SubelementsDeletionBehavior::Skip. Replace the local computations in both the V4
branch and the V1–V3 branch with the helper, retaining each branch’s existing
BTreeSet conversion and emptiness checks.
In `@merk/src/tree/ops.rs`:
- Around line 1358-1377: Add a direct unit test near the existing
Walker::apply_to tests that records observer callbacks as (key, old_value,
disposition), then applies a batch containing a put over an existing key, a put
for a new key, and a delete of an existing key. Assert exactly one callback per
existing key, with pre-operation values and Replaced for the put and Deleted for
the delete, and assert the new-key put produces no callback.
🪄 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 Plus
Run ID: 7c8cef31-079a-4375-b0a9-90a315bf8f4f
📒 Files selected for processing (19)
grovedb-version/src/version/grovedb_versions.rsgrovedb-version/src/version/v4.rsgrovedb/src/batch/estimated_costs/average_case_costs.rsgrovedb/src/batch/estimated_costs/worst_case_costs.rsgrovedb/src/batch/indexed_tree/delete_tree.rsgrovedb/src/batch/indexed_tree/mod.rsgrovedb/src/batch/indexed_tree/overwrite.rsgrovedb/src/batch/mod.rsgrovedb/src/batch/single_insert_cost_tests.rsgrovedb/src/estimated_costs/mod.rsgrovedb/src/tests/batch_indexed_overwrite_tests.rsgrovedb/src/tests/batch_indexed_tree_tests.rsgrovedb/src/tests/indexed_tree_security_regression_tests.rsgrovedb/src/tests/provable_count_indexed_tree_tests.rsmerk/src/lib.rsmerk/src/merk/apply.rsmerk/src/test_utils/mod.rsmerk/src/tree/mod.rsmerk/src/tree/ops.rs
…added Raises patch coverage over the 90% bar (codecov/patch was at 87.76%): - grovedb: four DeleteTree pre-scan tests — missing-key emptiness read, same-batch child-delete carve-out (with a plain Delete elsewhere and a Skip-policy sibling), the V1..V3 declared-type-at-face-value shapes on GROVE_V3, and a two-mismatch batch exercising the observer's short-circuit after the first gate rejection. - merk: consolidate the six duplicated inert-callback bundles in the ops test module into one apply_to_plain helper (whose no-op observer now executes via the update tests), and add a build-with-mid-delete test covering build's delete arm. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ite asserts - Document apply_batch_structure's BatchApplyCaptures return (both fields) instead of the stale two-tuple description. - Pin the non-empty indexed batch-insertion refusal to the exact InvalidBatchOperation message instead of a three-variant alternation. - Require the indexed-to-Item safe-subset overwrite to succeed instead of tolerating either outcome. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
grovedb/src/batch/mod.rs (3)
2488-2510: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRegister
GroveOp::RefreshReferencein the overwrite inspection map.The map at Lines 2488-2510 covers
InsertOrReplace,Replace, andPatch. TheGroveOp::RefreshReferencearm also writes a new value. Its trusted modes allow a different on-disk variant to be coerced. A trusted refresh can therefore replace an indexed-tree element without a pending inspection.The observer will not schedule cleanup for the old primary subtree or secondary namespaces. Register the refresh key after constructing
element, or reject trusted refreshes against indexed stored elements. Add a regression test.🤖 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 2488 - 2510, Extend the overwrite inspection registration to include the GroveOp::RefreshReference path after its replacement element is constructed, ensuring trusted refreshes that replace indexed-tree elements are added to pending_overwrite_inspections. Preserve the existing behavior for other operations and add a regression test covering cleanup of the old primary subtree and secondary namespaces.
6012-6033: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftReject or pre-scan callback-added
DeleteTreeoperations.
delete_tree_behaviorscomes from the initialscan_delete_tree_opscall. Lines 6012-6033 classify captures from both phases with this one map. ADeleteTreereturned byadd_on_operationsis applied during continuation without the initial emptiness andSkipchecks, and its captured path has no behavior entry.The parent key can be deleted while Merk subtree, non-Merk, or indexed secondary cleanup paths remain empty. Reject
DeleteTreein callback output, or pre-scan callback operations and merge their behavior state before classification.🤖 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 6012 - 6033, Ensure callback-generated operations cannot introduce an unvalidated DeleteTree: either reject DeleteTree operations returned by add_on_operations, or pre-scan those operations and merge their behavior entries into delete_tree_behaviors before classify_captured_delete_trees. Preserve the existing emptiness and Skip validation so every captured delete path has corresponding behavior state before cleanup classification.
5051-5167: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winAdd a V4 DeleteTree estimator parity check.
estimated_case_operations_for_batchbypassesscan_delete_tree_opsand callsapply_batch_structuredirectly, so V4ErrorandSkipDeleteTreepre-apply reads/opens are not counted. Add an end-to-end estimator cost test for these operations, or document that the estimator covers only the post-scan apply phase.🤖 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 5051 - 5167, Add an end-to-end cost test comparing the estimator from estimated_case_operations_for_batch with actual V4 Error and Skip DeleteTree execution, ensuring pre-apply element reads and child Merk opens performed by scan_delete_tree_ops are included; alternatively, explicitly document that the estimator intentionally covers only apply_batch_structure’s post-scan phase.
🤖 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.
Outside diff comments:
In `@grovedb/src/batch/mod.rs`:
- Around line 2488-2510: Extend the overwrite inspection registration to include
the GroveOp::RefreshReference path after its replacement element is constructed,
ensuring trusted refreshes that replace indexed-tree elements are added to
pending_overwrite_inspections. Preserve the existing behavior for other
operations and add a regression test covering cleanup of the old primary subtree
and secondary namespaces.
- Around line 6012-6033: Ensure callback-generated operations cannot introduce
an unvalidated DeleteTree: either reject DeleteTree operations returned by
add_on_operations, or pre-scan those operations and merge their behavior entries
into delete_tree_behaviors before classify_captured_delete_trees. Preserve the
existing emptiness and Skip validation so every captured delete path has
corresponding behavior state before cleanup classification.
- Around line 5051-5167: Add an end-to-end cost test comparing the estimator
from estimated_case_operations_for_batch with actual V4 Error and Skip
DeleteTree execution, ensuring pre-apply element reads and child Merk opens
performed by scan_delete_tree_ops are included; alternatively, explicitly
document that the estimator intentionally covers only apply_batch_structure’s
post-scan phase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3d1befaf-ceae-4719-9105-c51a1c80e2c2
📒 Files selected for processing (4)
grovedb/src/batch/mod.rsgrovedb/src/tests/batch_delete_tree_tests.rsgrovedb/src/tests/batch_indexed_tree_tests.rsmerk/src/tree/ops.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- grovedb/src/tests/batch_indexed_tree_tests.rs
- merk/src/tree/ops.rs
Bumps grovedb to develop b5bd7ef, which makes the v4 cleanup gates derive their inspection from data the merk apply already loads (dashpay/grovedb#790, on top of the estimator parity in #789). The gates keep their semantics — indexed-tree overwrite/delete-tree cleanup — at zero marginal cost, so every fee constant returns to its released value and the protocol v13/v14 test pairs now assert identical fees across the boundary. The ranked-trees book chapter's cost section is rewritten accordingly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Issue being fixed or feature implemented
The two GROVE_V4 batch gates —
apply_batch.overwrite_indexed_cleanup_inspectionandapply_batch.delete_tree_cleanup_type_source— each bought their correctness with a dedicated stored-element read per op. Every overwrite-capable op (InsertOrReplace/Replace/Patch) and everyDeleteTreein a batch paid +1 seek plus the loaded element bytes at V4, even when no indexed tree was anywhere near the batch. Dash Platform's identity/balance ops all pay this at protocol v14; its fee tests measured +2000·seeks + 20·bytes per gated op vs PV13.What was done?
Keep the V4 correctness (a batch overwrite of a PCIT/PSIT/PCPSIT schedules its per-axis secondary storage for cleanup or refuses the ambiguous case;
DeleteTreeselects cleanup namespaces from the ACTUAL stored type), but derive the old element from data the apply already loads:apply_unchecked_with_old_value_observerthreads an infallible(key, old_value, Replaced|Deleted)observer through the batch walk. The walker fetches an existing node before rewriting or deleting it, so surfacing its stored value is free — no storage read, no tracked cost.apply_uncheckedkeeps its signature and delegates with a no-op observer.execute_ops_on_pathregisters overwrite-capable keys and classifies the displaced element inside the observer. Fresh inserts never fire it — exactly the case that used to pay a wasted read. BareReferenceoverwrites are now included (with the read gone there is no cost argument for leaving a reference over an indexed tree unswept), closing Bare Reference overwrite of an indexed tree bypasses secondary-index cleanup (needs GROVE_V4 to gate) #776; its regression test is un-ignored.apply_body, driven by the actual stored types the observer captures (declared/stored mismatches involving an indexed tree are rejected there). The pre-apply Error/Skip emptiness checks read the stored element once up front — the same single read V1..V3 pay directly or inside the child-merk open — and hand it to a newopen_batch_transactional_merk_with_parent_elementhelper so nothing is read twice. The V1..V3 pre-scan path is the released behaviour byte-for-byte, now shared by bothapply_batch*entry points viascan_delete_tree_ops.Behaviour note (V4 only, nothing shipped): overwriting an indexed tree with a NON-EMPTY indexed tree now surfaces as the earlier ungated empty-at-batch-insertion
InvalidBatchOperationinstead of the classifier'sNotSupported(the classifier now runs during the apply, behind that guard, and stays as defense in depth). Three tests updated accordingly.How Has This Been Tested?
test_batch_plain_overwrites_and_tree_delete_cost_parity_v3_v4pins a batch of plain overwrites plus a plainDeleteTreeto an identicalCostResultunderGROVE_V3andGROVE_V4._v3_keeps_live_costscompanion.cargo test --workspace --all-features,cargo clippy --workspace --all-features -- -D warnings, andcargo fmt --allare clean.Breaking Changes
None released — GROVE_V4 has not shipped, so amending its behaviour in place is safe; V1..V3 paths are byte-identical. Merk gains one additive public API (
apply_unchecked_with_old_value_observer, plus theOldValueDispositionenum).Downstream: once Dash Platform re-pins, the fee-constant updates carried by dashpay/platform#4266 revert to the released values (the PV13/PV14 test pairs should then assert equal fees), and the separate task about modeling the inspection read in batch estimated costs becomes moot.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
Performance
Validation
Reliability