fix(drive)!: make shared-prefix aggregate indexes insertable at protocol v14 - #4265
Conversation
📝 WalkthroughWalkthroughProtocol v14 adds v2 contract-operation index walkers. The walkers demote continuation tree types, insert zero-contribution wrappers, remove nested index references, and validate shared-prefix aggregate layouts through expanded E2E coverage. ChangesShared-prefix aggregate index support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DocumentOperation
participant V2IndexWalker
participant GroveDB
DocumentOperation->>V2IndexWalker: insert or remove contract-operation indexes
V2IndexWalker->>V2IndexWalker: derive continuation-aware tree types
V2IndexWalker->>GroveDB: apply index tree operations and wrappers
GroveDB-->>V2IndexWalker: return operation result
V2IndexWalker-->>DocumentOperation: complete recursive index processing
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 |
|
ℹ️ Review skipped (commit 7ca8f9c) |
180c895 to
573dbda
Compare
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Carried-forward prior findings: the key-changing update path still bypasses the v14 shared-prefix layout and remains blocking; the helper-visibility, stateless-estimation coverage, and protocol-upgrade coverage suggestions also remain valid. The prior mixed-version estimation blocker is outdated because pinned GroveDB assigns identical feature and operation costs to all three count-sum tree variants. Genuinely new findings: none.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 3 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v4.rs`:
- [BLOCKING] packages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v4.rs:86-93: Apply the v14 index layout to key-changing document updates
Protocol v14 activates the corrected v2 insert and delete walkers, but `update_document_for_contract_operations` remains on v0. A key-changing update can independently materialize a previously unseen index branch through that implementation, which still derives the pre-v14 value-tree type and inserts continuation property-name trees with the unwrapped `batch_insert_empty_tree_if_not_exists` helper. For example, with a count-and-sum `[resourceId]` index alongside plain `[resourceId, rating]`, moving one document to a new `resourceId` creates an unwrapped structural `rating` child under the new `CountSumTree`; that child contributes to the count, so the authenticated aggregate can report two entries for one document. Other combinations can leak continuation sums or retain a provable parent that is incompatible with the v14 wrapper rules. Add a v14-dispatched update implementation that shares the continuation-demotion and zero-contribution wrapper logic with the v2 walkers, while preserving v0 for earlier protocols, and cover a shared-prefix key-changing update end to end.
In `packages/rs-drive/src/util/grove_operations/batch_insert_empty_tree_if_not_exists/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/util/grove_operations/batch_insert_empty_tree_if_not_exists/mod.rs:152-154: Keep the v14-only insertion helper crate-private
This helper directly invokes the v0 body with `ContributingZeroToParent` and performs no version dispatch of its own. Its documented safety invariant depends on calls coming exclusively from the protocol-gated v14 v2 walkers, but `pub fn` exposes it to downstream crates that can bypass that gate. The only current caller is inside `rs-drive`, so crate-private visibility enforces the intended ownership boundary without restricting legitimate use.
In `packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/shared_prefix_aggregation_e2e_tests.rs`:
- [SUGGESTION] packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/shared_prefix_aggregation_e2e_tests.rs:334-379: Exercise the stateless insert and delete estimation branches
The matrix helper always inserts with `apply: true`, and both deletion calls also apply their operations. Consequently, the newly changed v2 stateless-estimation branches remain unexercised, including their post-demotion `tree_type`, `EstimatedSumTrees`, and stateless wrapper-operation construction. Add representative `apply: false` cases for demoted and non-demoted layouts, assert that fee estimation succeeds, and verify that each dry run leaves state unchanged.
- [SUGGESTION] packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/shared_prefix_aggregation_e2e_tests.rs:503-521: Pin protocol v14 and cover values created before activation
The activation matrix uses `PlatformVersion::latest()` rather than protocol 14, so once a later protocol is introduced it will no longer verify the exact v14-to-Drive-v9-to-v2-walker dispatch chain. The v13 suite also uses separate fresh databases and therefore does not test the mixed layout explicitly supported by this change: a provable count-sum value tree created at v13 remains in state while newly seen values use `CountSumTree` at v14. Pin the activation matrix with `PlatformVersion::get(14)` and add a single-database scenario that creates a `ProvableCountSumTree` or `ProvableCountProvableSumTree` value at v13, then inserts into and deletes from that shared prefix at v14.
| pub fn batch_insert_empty_tree_contributing_zero_to_aggregating_parent_if_not_exists< | ||
| const N: usize, | ||
| >( |
There was a problem hiding this comment.
🟡 Suggestion: Keep the v14-only insertion helper crate-private
This helper directly invokes the v0 body with ContributingZeroToParent and performs no version dispatch of its own. Its documented safety invariant depends on calls coming exclusively from the protocol-gated v14 v2 walkers, but pub fn exposes it to downstream crates that can bypass that gate. The only current caller is inside rs-drive, so crate-private visibility enforces the intended ownership boundary without restricting legitimate use.
| pub fn batch_insert_empty_tree_contributing_zero_to_aggregating_parent_if_not_exists< | |
| const N: usize, | |
| >( | |
| pub(crate) fn batch_insert_empty_tree_contributing_zero_to_aggregating_parent_if_not_exists< | |
| const N: usize, | |
| >( |
source: ['codex']
| @@ -242,124 +360,257 @@ fn insert_review_document_for_case(case: &SharedPrefixCase) -> SharedPrefixOutco | |||
| .add_document_for_contract( | |||
| DocumentAndContractInfo { | |||
| owned_document_info: OwnedDocumentInfo { | |||
| // Use the document's own random owner — the delete | |||
| // path re-reads `$ownerId` from the stored document, | |||
| // so an override here would strand the owner-index | |||
| // entries. | |||
| document_info: DocumentRefInfo((&document, None)), | |||
| owner_id: Some(generate_random_identifier_struct().into()), | |||
| owner_id: None, | |||
| }, | |||
| contract: &contract, | |||
| contract, | |||
| document_type, | |||
| }, | |||
| false, | |||
| BlockInfo::default(), | |||
| true, | |||
| None, | |||
| pv, | |||
| platform_version, | |||
| None, | |||
| ) | |||
There was a problem hiding this comment.
🟡 Suggestion: Exercise the stateless insert and delete estimation branches
The matrix helper always inserts with apply: true, and both deletion calls also apply their operations. Consequently, the newly changed v2 stateless-estimation branches remain unexercised, including their post-demotion tree_type, EstimatedSumTrees, and stateless wrapper-operation construction. Add representative apply: false cases for demoted and non-demoted layouts, assert that fee estimation succeeds, and verify that each dry run leaves state unchanged.
source: ['codex']
| #[test] | ||
| #[ignore = "tracks #3960; unsupported shared-prefix aggregate layouts are accepted but fail insertion today"] | ||
| fn shared_prefix_aggregate_index_combinations_reject_or_insert() { | ||
| let cases = [ | ||
| SharedPrefixCase { | ||
| name: "count_parent_plain_child", | ||
| prefix_flags: IndexFlags::COUNT, | ||
| child_flags: IndexFlags::PLAIN, | ||
| must_insert: true, | ||
| }, | ||
| SharedPrefixCase { | ||
| name: "count_parent_range_count_child", | ||
| prefix_flags: IndexFlags::COUNT, | ||
| child_flags: IndexFlags::RANGE_COUNT, | ||
| must_insert: true, | ||
| }, | ||
| SharedPrefixCase { | ||
| name: "count_parent_range_sum_child", | ||
| prefix_flags: IndexFlags::COUNT, | ||
| child_flags: IndexFlags::RANGE_SUM, | ||
| must_insert: false, | ||
| }, | ||
| SharedPrefixCase { | ||
| name: "count_parent_range_count_sum_child", | ||
| prefix_flags: IndexFlags::COUNT, | ||
| child_flags: IndexFlags::RANGE_COUNT_SUM, | ||
| must_insert: false, | ||
| }, | ||
| SharedPrefixCase { | ||
| name: "sum_parent_plain_child", | ||
| prefix_flags: IndexFlags::SUM, | ||
| child_flags: IndexFlags::PLAIN, | ||
| must_insert: false, | ||
| }, | ||
| SharedPrefixCase { | ||
| name: "sum_parent_range_count_child", | ||
| prefix_flags: IndexFlags::SUM, | ||
| child_flags: IndexFlags::RANGE_COUNT, | ||
| must_insert: false, | ||
| }, | ||
| SharedPrefixCase { | ||
| name: "sum_parent_range_sum_child", | ||
| prefix_flags: IndexFlags::SUM, | ||
| child_flags: IndexFlags::RANGE_SUM, | ||
| must_insert: true, | ||
| }, | ||
| SharedPrefixCase { | ||
| name: "sum_parent_range_count_sum_child", | ||
| prefix_flags: IndexFlags::SUM, | ||
| child_flags: IndexFlags::RANGE_COUNT_SUM, | ||
| must_insert: true, | ||
| }, | ||
| SharedPrefixCase { | ||
| name: "count_sum_parent_plain_child", | ||
| prefix_flags: IndexFlags::COUNT_SUM, | ||
| child_flags: IndexFlags::PLAIN, | ||
| must_insert: false, | ||
| }, | ||
| SharedPrefixCase { | ||
| name: "count_sum_parent_range_count_child", | ||
| prefix_flags: IndexFlags::COUNT_SUM, | ||
| child_flags: IndexFlags::RANGE_COUNT, | ||
| must_insert: false, | ||
| }, | ||
| SharedPrefixCase { | ||
| name: "count_sum_parent_range_sum_child", | ||
| prefix_flags: IndexFlags::COUNT_SUM, | ||
| child_flags: IndexFlags::RANGE_SUM, | ||
| must_insert: true, | ||
| }, | ||
| SharedPrefixCase { | ||
| name: "count_sum_parent_range_count_sum_child", | ||
| prefix_flags: IndexFlags::COUNT_SUM, | ||
| child_flags: IndexFlags::RANGE_COUNT_SUM, | ||
| must_insert: true, | ||
| }, | ||
| ]; | ||
| fn shared_prefix_aggregate_index_combinations_insert_and_delete_at_latest() { | ||
| let platform_version = PlatformVersion::latest(); | ||
| assert!( | ||
| platform_version.protocol_version >= 14, | ||
| "this suite exercises the v2 index walkers" | ||
| ); | ||
|
|
||
| for case in all_cases() { | ||
| let drive = setup_drive_with_initial_state_structure(None); | ||
| let contract = build_review_contract(case.prefix_flags, case.child_flags) | ||
| .unwrap_or_else(|error| panic!("{}: contract must build: {error}", case.name)); | ||
| apply_contract(&drive, &contract, platform_version) | ||
| .unwrap_or_else(|error| panic!("{}: contract must apply: {error}", case.name)); | ||
|
|
||
| let first_document = insert_review_document(&drive, &contract, 1, 5, platform_version) | ||
| .unwrap_or_else(|error| panic!("{}: first insert must succeed: {error}", case.name)); | ||
| let second_document = insert_review_document(&drive, &contract, 2, 3, platform_version) | ||
| .unwrap_or_else(|error| panic!("{}: second insert must succeed: {error}", case.name)); |
There was a problem hiding this comment.
🟡 Suggestion: Pin protocol v14 and cover values created before activation
The activation matrix uses PlatformVersion::latest() rather than protocol 14, so once a later protocol is introduced it will no longer verify the exact v14-to-Drive-v9-to-v2-walker dispatch chain. The v13 suite also uses separate fresh databases and therefore does not test the mixed layout explicitly supported by this change: a provable count-sum value tree created at v13 remains in state while newly seen values use CountSumTree at v14. Pin the activation matrix with PlatformVersion::get(14) and add a single-database scenario that creates a ProvableCountSumTree or ProvableCountProvableSumTree value at v13, then inserts into and deletes from that shared prefix at v14.
source: ['codex']
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4265 +/- ##
============================================
- Coverage 87.56% 87.52% -0.04%
============================================
Files 2672 2678 +6
Lines 339758 341047 +1289
============================================
+ Hits 297508 298516 +1008
- Misses 42250 42531 +281
🚀 New features to boost your workflow:
|
…col v14 A data contract could declare an aggregating (countable/summable) index terminating at a property that also prefixes a compound index (e.g. summable [a] next to [a, b]). The contract registered fine, but for most flag combinations every document insert was rejected: the index walker had no legal way to hang the compound continuation tree under the aggregating per-value tree. grovedb's NotSummed/NotCountedOrSummed wrappers require sum-bearing inners, the NonCounted helper rejected sum-bearing ones, and provable count-bearing value trees reject count-suppressed children entirely. Three such bricked contracts exist on testnet today; mainnet has none. Fixed at new protocol version 14 via v2 index walkers (insert/delete x top-level/recursive): - tree types derive through a shared helper that demotes provable count-bearing value trees to CountSumTree when compound continuations exist (index_level_tree_types.rs), keeping insert and delete estimation in lockstep; - continuation inserts route through a completed zero-contribution wrapper matrix: NonCounted for non-sum children of count-sum parents, unwrapped inserts for non-sum children of sum-only parents (they contribute 0 to a sum naturally), and an extended NonCounted inner set for count-only parents. Every value tree keeps the invariant count/sum == the [0] ref-bucket contribution. Shapes insertable at v13 produce bit-identical operations, except provable value trees whose continuations were all sum-bearing (insertable pre-v14 only because grovedb's wrapper-vs-provable batch guard fires solely when the parent merk pre-exists): values first seen at v14 get CountSumTree value trees, which readers treat identically. The pre-v14 walkers stay byte-identical. A frozen-at-v13 test pins their empirical accept/reject matrix, and a 32-combination suite proves insert, aggregate correctness, delete, and full tree cleanup across the whole matrix at v14. Fixes #3960 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
573dbda to
39d644c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/shared_prefix_aggregation_e2e_tests.rs (3)
518-521: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the dependency on distinct random owners.
Both documents use the fixed
resourceIdvalue"resource-1". TheownerAndResourceindex is unique on($ownerId, resourceId). The two inserts therefore succeed only because seeds1and2produce different random owner IDs. If a future change reuses a seed or fixes the owner, the failure appears as a unique-index violation, which is unrelated to the aggregate behavior under test. Add a short comment stating that the seeds must yield distinct owners.🤖 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 `@packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/shared_prefix_aggregation_e2e_tests.rs` around lines 518 - 521, The test setup around insert_review_document must document that seeds 1 and 2 are intentionally different because they generate distinct random owners for the shared resourceId; add a short comment before the two insert calls stating that the seeds must yield distinct owners to satisfy the unique ownerAndResource index.
511-578: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollect failures across the matrix instead of stopping at the first case.
The v14 loop panics inside the iteration, so one failing combination hides the remaining cases. The v13 test already uses the better pattern: it collects
mismatchesand asserts once at the end. Apply the same pattern here, so a single run reports every broken combination in the 32-case matrix.🤖 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 `@packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/shared_prefix_aggregation_e2e_tests.rs` around lines 511 - 578, The v14 matrix loop currently panics on the first failing case, preventing the remaining combinations from running. Update the test around the all_cases() iteration to collect each case’s failures in a mismatches collection, continue executing every case, and perform one final assertion after the loop that reports all accumulated failures, matching the existing v13 pattern.
107-112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
RANGE_COUNT_SUMto reflect both range flags.
RANGE_COUNT_SUMsetsrange_countableandrange_summable. The sibling constantsCOUNT_SUM_RANGE_COUNTandCOUNT_SUM_RANGE_SUMname each range axis explicitly, so the current name reads as "range-countable plus plain summable". The constant appears in generated case names at lines 181 and 187, so the name shows up in every failure message for those combinations.RANGE_COUNT_RANGE_SUMis unambiguous.🤖 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 `@packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/shared_prefix_aggregation_e2e_tests.rs` around lines 107 - 112, Rename the constant RANGE_COUNT_SUM to RANGE_COUNT_RANGE_SUM and update all references, including the generated cases around the affected tests, so failure messages use the unambiguous name while preserving its existing flag values.packages/rs-platform-version/src/version/drive_versions/v9.rs (1)
35-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRescope the inline
changed:markers.Lines 35, 57, 61 and 91 carry bare
changed:markers copied from earlier version files. Inv9.rsonly line 59 changed relative to V8. A reader can misread these markers as V9 deltas. Lines 74 and 135 already carry the correctchanged in v8/changed in v7form. Apply the same form to the remaining markers.♻️ Proposed marker rescope
- create_initial_state_structure: 3, // changed: adds shielded pool trees (commitment tree, nullifiers, anchors) + create_initial_state_structure: 3, // changed in v7: adds shielded pool trees (commitment tree, nullifiers, anchors)Also applies to: 57-57, 61-61, 74-74, 91-91, 135-135
🤖 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 `@packages/rs-platform-version/src/version/drive_versions/v9.rs` at line 35, Update the inline comments on the version entries in v9.rs that use bare “changed:” markers, including create_initial_state_structure and the entries at the referenced locations, to identify the version where each change actually occurred. Match the existing “changed in v8” and “changed in v7” wording used by the correctly scoped markers, while preserving the lone V9 change marker on line 59.packages/rs-drive/src/util/grove_operations/batch_insert_empty_tree_if_not_exists/mod.rs (1)
166-176: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider keeping the grove feature-version guard for consistency.
The two sibling helpers in this file match on
drive_version.grove_methods.batch.batch_insert_empty_tree_if_not_existsand returnUnknownVersionMismatchfor unknown values. This helper callsbatch_insert_empty_tree_if_not_exists_v0directly. If that grove feature version later gains a v1 with different batch-dedup semantics, the two aggregating helpers will diverge silently: one dispatches, one stays pinned to v0.The doc comment explains that the platform gate lives in the callers. That argument covers the platform version, not the grove method version.
♻️ Proposed guard
- self.batch_insert_empty_tree_if_not_exists_v0( - path_key_info, - tree_type, - EmptyTreeInsertMode::ContributingZeroToParent(aggregating_parent_tree_type), - storage_flags, - apply_type, - transaction, - check_existing_operations, - drive_operations, - drive_version, - ) + match drive_version + .grove_methods + .batch + .batch_insert_empty_tree_if_not_exists + { + 0 => self.batch_insert_empty_tree_if_not_exists_v0( + path_key_info, + tree_type, + EmptyTreeInsertMode::ContributingZeroToParent(aggregating_parent_tree_type), + storage_flags, + apply_type, + transaction, + check_existing_operations, + drive_operations, + drive_version, + ), + version => Err(Error::Drive(DriveError::UnknownVersionMismatch { + method: + "batch_insert_empty_tree_contributing_zero_to_aggregating_parent_if_not_exists" + .to_string(), + known_versions: vec![0], + received: version, + })), + }🤖 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 `@packages/rs-drive/src/util/grove_operations/batch_insert_empty_tree_if_not_exists/mod.rs` around lines 166 - 176, Update the helper containing the direct batch_insert_empty_tree_if_not_exists_v0 call to dispatch on drive_version.grove_methods.batch.batch_insert_empty_tree_if_not_exists, matching the two sibling aggregating helpers. Preserve v0 behavior for the supported version and return UnknownVersionMismatch for unknown grove method versions, rather than bypassing the feature-version guard.packages/rs-drive/src/drive/document/index_level_tree_types.rs (1)
113-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDemotion set matches the accepted-parent set.
The post-demotion
value_tree_typerange is{NormalTree, CountTree, SumTree, CountSumTree}.for_known_path_key_empty_tree_contributing_zero_to_parentaccepts exactlyCountTree,CountSumTree,SumTree,BigSumTree, andProvableSumTree, andNormalTreeparents never reach it. The table never yieldsProvableCountTreeorProvableSumTreefor a value tree, so the rejected provable arms stay unreachable from the v2 walkers.Consider adding a unit test that asserts this invariant over all flag combinations. The invariant is the load-bearing link between this file and the dispatcher, and a future table edit could break it silently.
🤖 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 `@packages/rs-drive/src/drive/document/index_level_tree_types.rs` around lines 113 - 123, Add a unit test covering all value-tree flag combinations around the demotion logic in the relevant index-level tree type code, asserting that every post-demotion value_tree_type is accepted by for_known_path_key_empty_tree_contributing_zero_to_parent and that unreachable provable variants are not produced. Keep the test focused on preserving this cross-module invariant without changing the existing demotion behavior.packages/rs-drive/src/fees/op.rs (2)
830-853: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: reuse the existing tree-type-to-element mapping.
This match repeats the nine-variant mapping already present in
LowLevelDriveOperationTreeTypeConverter::empty_tree_operation_for_known_path_keyat Lines 1228-1254. Three copies of this table now exist in the file. A future grovedb variant addition must be applied to each copy.Consider extracting a single
fn empty_element_for_tree_type(tree_type: TreeType, element_flags: Option<ElementFlags>) -> Result<Element, Error>helper and calling it from all sites.🤖 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 `@packages/rs-drive/src/fees/op.rs` around lines 830 - 853, Extract the repeated TreeType-to-empty-Element mapping into a shared helper such as empty_element_for_tree_type, returning Result<Element, Error> and preserving the existing unsupported-tree error. Replace the match in the non-counted-wrapping path and the mappings in LowLevelDriveOperationTreeTypeConverter::empty_tree_operation_for_known_path_key and the other duplicate site with this helper.
732-805: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit tests for the completed parent×inner dispatch matrix.
This dispatcher decides consensus-relevant element shapes for every v14 continuation insert. The reachable matrix is small and fully enumerable: five accepted parent types × nine inner types, plus the four rejected parent types. Table-driven tests would pin the exact wrapper choice for each cell and would catch a future regression in the demotion helper that routes a provable parent here.
🤖 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 `@packages/rs-drive/src/fees/op.rs` around lines 732 - 805, Add table-driven unit tests for for_known_path_key_empty_tree_contributing_zero_to_parent covering all five supported aggregating parent types across all nine inner TreeType values, asserting the exact returned operation wrapper for every matrix cell. Add separate cases for the four rejected provable parent types and assert the expected NotSupported error, including the demotion-helper route into this dispatcher.packages/rs-drive/src/drive/document/delete/remove_indices_for_top_index_level_for_contract_operations/v2/mod.rs (1)
40-41: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider
#[inline]instead of#[inline(always)].The function body is large and contains a loop over all sub-levels.
#[inline(always)]forces expansion at every call site and increases code size without a measured benefit. If the v0 and v1 siblings carry the same attribute, keep it for consistency; otherwise drop it.🤖 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 `@packages/rs-drive/src/drive/document/delete/remove_indices_for_top_index_level_for_contract_operations/v2/mod.rs` around lines 40 - 41, Replace #[inline(always)] on remove_indices_for_top_index_level_for_contract_operations_v2 with #[inline], unless the corresponding v0 and v1 sibling functions use #[inline(always)], in which case retain the attribute for consistency.packages/rs-drive/src/util/grove_operations/batch_insert_empty_tree_if_not_exists/v0/mod.rs (1)
321-604: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the test module to cover the two wrapping modes.
All seven tests pass
EmptyTreeInsertMode::NotWrapped. NeitherNonAggregatedForParentnorContributingZeroToParenthas coverage here, andContributingZeroToParentis the new consensus-affecting path. Add cases that assert the producedLowLevelDriveOperationcarries the expected wrapper element for at least:CountTreeparent withNormalTreeinner,CountSumTreeparent withNormalTreeinner,CountSumTreeparent withSumTreeinner, andSumTreeparent withNormalTreeinner (unwrapped).🤖 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 `@packages/rs-drive/src/util/grove_operations/batch_insert_empty_tree_if_not_exists/v0/mod.rs` around lines 321 - 604, Extend the tests around batch_insert_empty_tree_if_not_exists_v0 to cover both NonAggregatedForParent and ContributingZeroToParent, asserting the generated LowLevelDriveOperation wrapper element for CountTree/NormalTree, CountSumTree/NormalTree, CountSumTree/SumTree, and SumTree/NormalTree (unwrapped). Reuse the existing setup patterns and verify the operation’s element type and wrapper value, including the zero-contribution behavior.
🤖 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
`@packages/rs-drive/src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/v2/mod.rs`:
- Around line 159-163: Update the overflow message in the insert walker’s
document field size check to remove the trailing “on delete” wording, leaving
the message specific to an oversized document field during insert indexing.
In `@packages/rs-drive/src/fees/op.rs`:
- Around line 823-858: Update for_known_path_key_empty_non_counted_any_tree to
reject sum-bearing TreeType variants before constructing the inner Element,
matching Element::new_non_counted support. Remove SumTree, BigSumTree,
CountSumTree, ProvableSumTree, ProvableCountSumTree, and
ProvableCountProvableSumTree from its accepted cases, while retaining only
supported non-sum variants and directing sum-bearing continuations to the
dedicated NotSummed or NotCountedOrSummed paths.
In
`@packages/rs-drive/src/util/grove_operations/batch_insert_empty_tree_if_not_exists/v0/mod.rs`:
- Around line 45-67: The build_op closure’s inner match on insert_mode is
incorrectly indented and will fail rustfmt checks. Reformat the match block and
its arms using standard rustfmt defaults, without changing the
operation-selection behavior.
---
Nitpick comments:
In
`@packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/shared_prefix_aggregation_e2e_tests.rs`:
- Around line 518-521: The test setup around insert_review_document must
document that seeds 1 and 2 are intentionally different because they generate
distinct random owners for the shared resourceId; add a short comment before the
two insert calls stating that the seeds must yield distinct owners to satisfy
the unique ownerAndResource index.
- Around line 511-578: The v14 matrix loop currently panics on the first failing
case, preventing the remaining combinations from running. Update the test around
the all_cases() iteration to collect each case’s failures in a mismatches
collection, continue executing every case, and perform one final assertion after
the loop that reports all accumulated failures, matching the existing v13
pattern.
- Around line 107-112: Rename the constant RANGE_COUNT_SUM to
RANGE_COUNT_RANGE_SUM and update all references, including the generated cases
around the affected tests, so failure messages use the unambiguous name while
preserving its existing flag values.
In
`@packages/rs-drive/src/drive/document/delete/remove_indices_for_top_index_level_for_contract_operations/v2/mod.rs`:
- Around line 40-41: Replace #[inline(always)] on
remove_indices_for_top_index_level_for_contract_operations_v2 with #[inline],
unless the corresponding v0 and v1 sibling functions use #[inline(always)], in
which case retain the attribute for consistency.
In `@packages/rs-drive/src/drive/document/index_level_tree_types.rs`:
- Around line 113-123: Add a unit test covering all value-tree flag combinations
around the demotion logic in the relevant index-level tree type code, asserting
that every post-demotion value_tree_type is accepted by
for_known_path_key_empty_tree_contributing_zero_to_parent and that unreachable
provable variants are not produced. Keep the test focused on preserving this
cross-module invariant without changing the existing demotion behavior.
In `@packages/rs-drive/src/fees/op.rs`:
- Around line 830-853: Extract the repeated TreeType-to-empty-Element mapping
into a shared helper such as empty_element_for_tree_type, returning
Result<Element, Error> and preserving the existing unsupported-tree error.
Replace the match in the non-counted-wrapping path and the mappings in
LowLevelDriveOperationTreeTypeConverter::empty_tree_operation_for_known_path_key
and the other duplicate site with this helper.
- Around line 732-805: Add table-driven unit tests for
for_known_path_key_empty_tree_contributing_zero_to_parent covering all five
supported aggregating parent types across all nine inner TreeType values,
asserting the exact returned operation wrapper for every matrix cell. Add
separate cases for the four rejected provable parent types and assert the
expected NotSupported error, including the demotion-helper route into this
dispatcher.
In
`@packages/rs-drive/src/util/grove_operations/batch_insert_empty_tree_if_not_exists/mod.rs`:
- Around line 166-176: Update the helper containing the direct
batch_insert_empty_tree_if_not_exists_v0 call to dispatch on
drive_version.grove_methods.batch.batch_insert_empty_tree_if_not_exists,
matching the two sibling aggregating helpers. Preserve v0 behavior for the
supported version and return UnknownVersionMismatch for unknown grove method
versions, rather than bypassing the feature-version guard.
In
`@packages/rs-drive/src/util/grove_operations/batch_insert_empty_tree_if_not_exists/v0/mod.rs`:
- Around line 321-604: Extend the tests around
batch_insert_empty_tree_if_not_exists_v0 to cover both NonAggregatedForParent
and ContributingZeroToParent, asserting the generated LowLevelDriveOperation
wrapper element for CountTree/NormalTree, CountSumTree/NormalTree,
CountSumTree/SumTree, and SumTree/NormalTree (unwrapped). Reuse the existing
setup patterns and verify the operation’s element type and wrapper value,
including the zero-contribution behavior.
In `@packages/rs-platform-version/src/version/drive_versions/v9.rs`:
- Line 35: Update the inline comments on the version entries in v9.rs that use
bare “changed:” markers, including create_initial_state_structure and the
entries at the referenced locations, to identify the version where each change
actually occurred. Match the existing “changed in v8” and “changed in v7”
wording used by the correctly scoped markers, while preserving the lone V9
change marker on line 59.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2da83e8d-b2d1-455d-b61a-7fd92aadac6a
📒 Files selected for processing (19)
packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/shared_prefix_aggregation_e2e_tests.rspackages/rs-drive/src/drive/document/delete/remove_indices_for_index_level_for_contract_operations/mod.rspackages/rs-drive/src/drive/document/delete/remove_indices_for_index_level_for_contract_operations/v2/mod.rspackages/rs-drive/src/drive/document/delete/remove_indices_for_top_index_level_for_contract_operations/mod.rspackages/rs-drive/src/drive/document/delete/remove_indices_for_top_index_level_for_contract_operations/v2/mod.rspackages/rs-drive/src/drive/document/index_level_tree_types.rspackages/rs-drive/src/drive/document/insert/add_indices_for_index_level_for_contract_operations/mod.rspackages/rs-drive/src/drive/document/insert/add_indices_for_index_level_for_contract_operations/v2/mod.rspackages/rs-drive/src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/mod.rspackages/rs-drive/src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/v2/mod.rspackages/rs-drive/src/drive/document/mod.rspackages/rs-drive/src/fees/op.rspackages/rs-drive/src/util/grove_operations/batch_insert_empty_tree_if_not_exists/mod.rspackages/rs-drive/src/util/grove_operations/batch_insert_empty_tree_if_not_exists/v0/mod.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/mod.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v4.rspackages/rs-platform-version/src/version/drive_versions/mod.rspackages/rs-platform-version/src/version/drive_versions/v9.rspackages/rs-platform-version/src/version/v14.rs
| pub fn for_known_path_key_empty_non_counted_any_tree( | ||
| path: Vec<Vec<u8>>, | ||
| key: Vec<u8>, | ||
| tree_type: TreeType, | ||
| storage_flags: Option<&StorageFlags>, | ||
| ) -> Result<Self, Error> { | ||
| let element_flags = storage_flags.map(|s| s.to_element_flags()); | ||
| let inner = match tree_type { | ||
| TreeType::NormalTree => Element::empty_tree_with_flags(element_flags), | ||
| TreeType::SumTree => Element::empty_sum_tree_with_flags(element_flags), | ||
| TreeType::BigSumTree => Element::empty_big_sum_tree_with_flags(element_flags), | ||
| TreeType::CountTree => Element::empty_count_tree_with_flags(element_flags), | ||
| TreeType::CountSumTree => Element::empty_count_sum_tree_with_flags(element_flags), | ||
| TreeType::ProvableCountTree => { | ||
| Element::empty_provable_count_tree_with_flags(element_flags) | ||
| } | ||
| TreeType::ProvableCountSumTree => { | ||
| Element::empty_provable_count_sum_tree_with_flags(element_flags) | ||
| } | ||
| TreeType::ProvableSumTree => Element::empty_provable_sum_tree_with_flags(element_flags), | ||
| TreeType::ProvableCountProvableSumTree => { | ||
| Element::empty_provable_count_provable_sum_tree_with_flags(element_flags) | ||
| } | ||
| _ => { | ||
| return Err(Error::Drive(DriveError::NotSupported( | ||
| "NonCounted-wrapping is only supported for the nine standard merk tree \ | ||
| variants; special trees (commitment / MMR / bulk-append / dense) are \ | ||
| never index continuation trees.", | ||
| ))); | ||
| } | ||
| }; | ||
| let tree = Element::new_non_counted(inner)?; | ||
| Ok(LowLevelDriveOperation::insert_for_known_path_key_element( | ||
| path, key, tree, | ||
| )) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the pinned grovedb source and inspect new_non_counted's accepted inner set.
rg -n -C3 'grovedb' packages/rs-drive/Cargo.toml
rg -n -C15 'fn new_non_counted' --type=rust ~/.cargo/registry/src 2>/dev/null | head -80
rg -n -C10 'accepts_non_counted_children' --type=rust ~/.cargo/registry/src 2>/dev/null | head -60Repository: dashpay/platform
Length of output: 2283
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files named op.rs or element-related =="
fd -a 'op\.rs$|element|tree' . | sed 's#^\./##' | head -80
echo "== inspect packages/rs-drive/src/fees/op.rs relevant sections =="
wc -l packages/rs-drive/src/fees/op.rs
sed -n '460,540p' packages/rs-drive/src/fees/op.rs
sed -n '780,870p' packages/rs-drive/src/fees/op.rs
echo "== all new_non_counted definitions/usages in repo =="
rg -n -C5 'new_non_counted|empty_non_counted|non_counted_wrapper|NonCounted' --type=rust packages/rs-drive/src packages/rs-drive/test 2>/dev/null | head -200Repository: dashpay/platform
Length of output: 43039
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== resolve grovedb v5.0.1 tag to commit =="
gh api repos/dashpay/grovedb/git/refs/tags/v5.0.1 --jq '{ref:.ref, object_type:.object.type, object_sha:.object.sha}'
TAG_OBJ_SHA="$(gh api repos/dashpay/grovedb/git/refs/tags/v5.0.1 --jq '.object.sha')"
echo "tag_type=$TAG_TYPE" "object_sha=$TAG_OBJ_SHA"
gh api "repos/dashpay/grovedb/git/tags/${TAG_OBJ_SHA}" --jq '{type:.type, object_type:.object.type, object_sha:.object.sha}' || true
echo "== fetch relevant grovedb v5.0.1 files without installing deps =="
git clone --depth 1 --filter=blob:none --sparse https://github.com/dashpay/grovedb.git /tmp/grovedb-v5-work 2>/tmp/grovedb-clone.log
cd /tmp/grovedb-v5-work
git fetch origin tag v5.0.1 --no-tags --depth 1 2>/tmp/grovedb-fetch.log || true
git fetch origin --tags 2>/tmp/grovedb-fetch2.log || true
git fetch --depth=1 origin v5.0.1 2>/tmp/grovedb-fetch3.log || true
git checkout FETCH_HEAD 2>/tmp/grovedb-checkout.log || true
git status --short
echo "== searches for new_non_counted across all ref versions/branches if available =="
rg -n -C12 'fn new_non_counted|accepts_non_counted_children|NonCounted|ProvableCount' --type=rust . | head -260
echo "== focused file matches containing non_counted =="
rg -n -C10 'non_counted|NonCounted' --type=rust grovedb element src packages crates . | head -300Repository: dashpay/platform
Length of output: 344
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== grovedb tag commit =="
TAG_COMMIT="6bb2ebff62a842a3c416126bbcfcf7774dfe02"
gh api repos/dashpay/grovedb/compare/v5.0.1...${TAG_COMMIT} --jq '{base_sha: .base_commit.sha, compare_sha: .merge_base_commit.sha, ahead_by:.ahead_by}'
TMP="$(mktemp -d)"
cd "$TMP"
git clone --depth 1 --filter=blob:none --sparse --branch "${TAG_COMMIT}" \
https://github.com/dashpay/grovedb.git grovedb-v5 2>/tmp/grovedb-clone.log
cd grovedb-v5
git sparse-checkout set --no-cone crates packages grovedb cargo.toml Cargo.toml 2>/tmp/grovedb-sparse.log
echo "== new_non_counted definitions =="
rg -n -C16 'fn new_non_counted|pub fn new_non_counted|new_non_counted' --type=rust . | head -280
echo "== accepts_non_counted_children definitions =="
rg -n -C14 'fn accepts_non_counted_children|pub fn accepts_non_counted_children|accepts_non_counted_children' --type=rust . | head -200
echo "== exact file excerpts from likely element crates =="
files="$(rg -l 'fn new_non_counted|pub fn new_non_counted' --type=rust . | sort -u)"
printf '%s\n' "$files"
for f in $files; do
echo "--- $f ---"
wc -l "$f"
rg -n -C20 'fn new_non_counted|pub fn new_non_counted|accepts_non_counted_children' "$f"
done
echo "== call sites of accepts_non_counted_children or NonCounted insert guards =="
rg -n -C10 'accepts_non_counted_children|NonCounted|not_supported_non_counted|ProvableCount' --type=rust . | head -300Repository: dashpay/platform
Length of output: 309
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== git config required =="
git config user.name "CodeRabbit"
git config user.email "coderabbit@example.com"
TMP="$(mktemp -d)"
cd "$TMP"
git clone --depth 1 --filter=blob:none --sparse --branch "6bb2ebff62a842a3c416126bbcfcf7774dfe02" \
https://github.com/dashpay/grovedb.git grovedb-v5 2>/tmp/grovedb-clone.log || {
cat /tmp/grovedb-clone.log
exit 1
}
cd grovedb-v5
git sparse-checkout list 2>/tmp/grovedb-spm.log || true
git sparse-checkout set --no-cone crates packages grovedb cargo.toml Cargo.toml 2>/tmp/grovedb-sparse.log || true
echo "== new_non_counted definitions =="
rg -n -C16 'fn new_non_counted|pub fn new_non_counted|new_non_counted' --type=rust . | head -280
echo "== accepts_non_counted_children definitions =="
rg -n -C14 'fn accepts_non_counted_children|pub fn accepts_non_counted_children|accepts_non_counted_children' --type=rust . | head -200
echo "== exact file excerpts from files containing new_non_counted =="
files="$(rg -l 'fn new_non_counted|pub fn new_non_counted' --type-rust . | sort -u || true)"
printf '%s\n' "$files"
for f in $files; do
echo "--- $f ---"
wc -l "$f"
rg -n -C24 'fn new_non_counted|pub fn new_non_counted|accepts_non_counted_children' "$f"
done
echo "== NonCounted insert/error guards =="
rg -n -C10 'accepts_non_counted_children|NonCounted|not_supported_non_counted|ProvableCount|CountSumTree|SumTree|BigSumTree|ProvableSumTree|ProvableCountSumTree|ProvableCountProvableSumTree' --type=rust . | head -400Repository: dashpay/platform
Length of output: 298
🌐 Web query:
grovedb v5.0.1 new_non_counted accepts_non_counted_children Element::NonCounted
💡 Result:
In GroveDB, Element::NonCounted is a wrapper variant introduced to allow specific elements to opt-out of count propagation [1]. This ensures that when an element is inserted into a parent tree (such as a CountTree or ProvableCountTree), it does not contribute to the parent's aggregate count [2][3]. Key details regarding Element::NonCounted: Purpose: It is used to prevent "count pollution" in hierarchical tree structures [2][3]. For example, when an index structure involves sibling continuation trees (such as sub-properties under a value tree), these continuations are wrapped in Element::NonCounted so that the parent tree's count correctly reflects only the desired terminal document counts rather than the number of sub-trees or sibling elements [2][3]. Behavior: Elements wrapped in Element::NonCounted store data as normal but effectively contribute 0 to the count aggregate of their parent tree [2][3]. Usage: It is designed to be used inside tree variants that support aggregation, such as CountTree, ProvableCountTree, CountSumTree, and ProvableCountSumTree [3]. While the specific new_non_counted API might vary by version, this wrapper is a fundamental part of the aggregation and query system introduced in recent GroveDB versions (e.g., starting around v5.0.0) [1]. Related variants: GroveDB also provides similar opt-out wrappers for other aggregation axes, such as Element::NotSummed (to opt-out of sum propagation) and Element::NotCountedOrSummed (to opt-out of both) [1][4].
Citations:
- 1: https://github.com/dashpay/grovedb/releases/tag/v5.0.0
- 2: https://dashpay.github.io/platform/drive/count-index-examples.html
- 3: https://dashpay.github.io/platform/drive/indexes.html
- 4: feat: add Element::ProvableCountProvableSumTree + dual-axis crossover proofs grovedb#670
🌐 Web query:
site:github.com/dashpay/grovedb "accepts_non_counted_children" "v5.0.1"
💡 Result:
The term accepts_non_counted_children is not a standard standalone function, library, or public API constant in GroveDB v5.0.1. Instead, it relates to the internal data structure handling and element wrappers within the GroveDB ecosystem (a hierarchical authenticated data structure database used by Dash Platform) [1]. In GroveDB v5.0.0 and later, developers introduced specific element variants to manage how data is counted and summed within the Merkle tree hierarchy [2]. Specifically, the library provides the following wrappers to opt-out of count or sum propagation: 1. Element::NonCounted: Used to opt-out of count propagation [2]. 2. Element::NotSummed: Used to opt-out of sum propagation [2]. 3. Element::NotCountedOrSummed: A combined wrapper for opting out of both count and sum propagation [2]. These variants are used during batch operations and tree insertions, such as when constructing a GroveOp::InsertTreeWithRootHash, where fields like non_counted, not_summed, and not_counted_or_summed are explicitly defined to control aggregation behavior [3]. If you encountered accepts_non_counted_children, it is likely a reference to a field, parameter, or internal logic within the codebase responsible for handling these aggregation flags during tree insertion or proof verification [3][4]. For specific implementation details, you should consult the source code in the grovedb/src/batch/mod.rs or related query validation modules in the official repository [3][4].
Citations:
- 1: https://github.com/dashpay/grovedb/
- 2: https://github.com/dashpay/grovedb/releases/tag/v5.0.0
- 3: feat: add Element::ProvableCountProvableSumTree + dual-axis crossover proofs grovedb#670
- 4: feat(grovedb,query): allow AggregateCountOnRange as carrier subquery grovedb#663
🌐 Web query:
github dashpay grovedba v5.0.1 crates element error AcceptanceError NotSupported InnerProvableCount ProvableCount SumTree NonCounted
💡 Result:
The error message "AcceptanceError NotSupported" encountered in GroveDB (associated with the dashpay/grovedb repository) relates to the validation logic for Element types and aggregate data, particularly following the introduction of new variants in v5.0.0 and related updates [1]. In GroveDB, Element::NonCounted and Element::NotSummed were introduced to allow specific items to opt-out of count or sum propagation [1]. The error typically arises when an operation or query encounters a combination of data types or structures that are not supported by the current validation or execution path. Specifically: 1. Aggregate Data Consistency: The library enforces consistency between the tree's configured type (e.g., ProvableCountTree or ProvableSumTree) and the Element variants stored within it [2][1]. Attempts to insert unsupported variants (such as NonCounted or NotCountedOrSummed elements) into provable count parents may trigger rejection errors [1]. 2. Internal Proof Verification: Errors involving InnerProvableCount, ProvableCount, and SumTree often stem from the merk proof verification layer or GroveDB's aggregate query engine [2]. When an unexpected aggregate variant is encountered—such as a ProvableSumTree variant in a context where only ProvableCount was expected—the execution path may fail with an error [2]. 3. Structural Compatibility: In version 5.x, updates were made to handle dual-axis crossover proofs and aggregate queries (e.g., AggregateCountOnRange) [2][1]. Mismatches between the requested query type and the underlying tree structure can lead to "not supported" or validation errors, often originating from validate_leaf_aggregate_count_on_range or similar validation methods in grovedb/src/query/mod.rs [3]. If you are encountering this error, it is often due to performing an operation that is incompatible with the tree's configuration or a mismatch between the expected and actual aggregate data variants [2][1]. It is recommended to check if the Element type being used matches the TreeType of the parent tree and to ensure the query shape is compatible with the latest GroveDB aggregate API [2][1][3].
Citations:
- 1: https://github.com/dashpay/grovedb/releases/tag/v5.0.0
- 2: feat: add Element::ProvableCountProvableSumTree + dual-axis crossover proofs grovedb#670
- 3: feat(grovedb,query): allow AggregateCountOnRange as carrier subquery grovedb#663
🌐 Web query:
grovedb 5.0.1 src crates element element.rs accepts_non_counted_children
💡 Result:
In GroveDB, accepts_non_counted_children is a method or property associated with the Element enum (specifically within the grovedb-element crate) that determines whether a given element type allows for non-counted children [1][2]. As of GroveDB v5.0.0 and subsequent updates, the element system was expanded to include various wrapper variants to manage count and sum propagation [3]. The Element enum includes variants that opt out of specific counting or summing behaviors, such as Element::NonCounted, Element::NotSummed, and Element::NotCountedOrSummed [3]. The method accepts_non_counted_children is used to check if an Element variant is compatible with or explicitly permits structures that do not participate in count tracking [1][3]. This is part of the broader logic in the grovedb-element crate (located at grovedb-element/src/element/mod.rs or related files) which handles the serialization, validation, and type-checking of elements within the hierarchical Merkle tree structure [1][4][2]. For exact implementation details for version 5.0.1, you should refer to the source code within the grovedb repository at grovedb-element/src/element/mod.rs, where the Element enum and its associated helper methods are defined [1][4][2].
Citations:
- 1: feat: add Element::ProvableCountProvableSumTree + dual-axis crossover proofs grovedb#670
- 2: https://github.com/dashpay/grovedb
- 3: https://github.com/dashpay/grovedb/releases/tag/v5.0.0
- 4: https://docs.rs/grovedb/latest/grovedb/
Drop the unsupported sum-bearing NonCounted cases from for_known_path_key_empty_non_counted_any_tree.
new_non_counted rejects sum-bearing inner variants in grovedb v5.0.1: SumTree, BigSumTree, CountSumTree, ProvableSumTree, ProvableCountSumTree, and ProvableCountProvableSumTree. The helper currently passes those variants through to Element::new_non_counted, so calls with sum-bearing continuations return Error::Drive(DriveError::NotSupported(...)) instead of succeeding. Keep sum-bearing continuations on the dedicated NotSummed / NotCountedOrSummed paths.
🤖 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 `@packages/rs-drive/src/fees/op.rs` around lines 823 - 858, Update
for_known_path_key_empty_non_counted_any_tree to reject sum-bearing TreeType
variants before constructing the inner Element, matching
Element::new_non_counted support. Remove SumTree, BigSumTree, CountSumTree,
ProvableSumTree, ProvableCountSumTree, and ProvableCountProvableSumTree from its
accepted cases, while retaining only supported non-sum variants and directing
sum-bearing continuations to the dedicated NotSummed or NotCountedOrSummed
paths.
| match insert_mode { | ||
| EmptyTreeInsertMode::NotWrapped => { | ||
| tree_type.empty_tree_operation_for_known_path_key(path, key, storage_flags) | ||
| } | ||
| EmptyTreeInsertMode::NonAggregatedForParent(parent_tt) => { | ||
| LowLevelDriveOperation::wrap_in_non_aggregated_for_parent_tree_type( | ||
| path, | ||
| key, | ||
| parent_tt, | ||
| tree_type, | ||
| storage_flags, | ||
| ) | ||
| } | ||
| EmptyTreeInsertMode::ContributingZeroToParent(parent_tt) => { | ||
| LowLevelDriveOperation::for_known_path_key_empty_tree_contributing_zero_to_parent( | ||
| path, | ||
| key, | ||
| parent_tt, | ||
| tree_type, | ||
| storage_flags, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Run rustfmt on the build_op closure.
The inner match insert_mode block and its arms are indented one level deeper than the closure body requires. rustfmt will reformat this region, so a cargo fmt --check gate will fail.
As per coding guidelines: "Use rustfmt defaults, keep Rust code clippy-clean".
🤖 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
`@packages/rs-drive/src/util/grove_operations/batch_insert_empty_tree_if_not_exists/v0/mod.rs`
around lines 45 - 67, The build_op closure’s inner match on insert_mode is
incorrectly indented and will fail rustfmt checks. Reformat the match block and
its arms using standard rustfmt defaults, without changing the
operation-selection behavior.
Source: Coding guidelines
…d-prefix layout The internal update walker materializes index branches itself when an indexed key changes, and its v0 derives pre-demotion value-tree types and inserts continuation property-name trees unwrapped — diverging from the v2 insert walkers' shapes at v14 and letting a continuation's own aggregates leak into the per-value count/sum. Add a v1 (dispatched at protocol v14 via update_document_for_contract_operations: 1) that derives tree types through the shared continuation-demotion helper and routes continuation inserts through the zero-contribution batch helper. The estimation branch already delegated to the insert walkers and needs no change. Review follow-ups in the same pass: - the zero-contribution batch helper is now crate-private (its safety invariant assumes protocol-gated callers) and dispatches the grove feature version like its siblings - the v14 e2e matrix now runs insert -> key-changing update (branch materialized by the update walker) -> delete with exact aggregate and wrapper assertions per step, collects all case failures, pins protocol 14 explicitly instead of latest(), and gains suites for the stateless-estimation branches (apply: false writes nothing) and for v13-created provable value trees coexisting with v14-demoted siblings in one database through inserts and full-cleanup deletes - new unit tests pin the zero-contribution dispatcher's full parent x inner matrix, the derivation/dispatcher cross-module invariant over every flag combination, and the batch helper's wrapping modes - RANGE_COUNT_RANGE_SUM naming, distinct-seed comment, insert-walker overflow message wording, and v9.rs changed-marker scoping cleanups Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Review responses — addressed in 7ca8f9c: thepastaclaw blocker (key-changing updates bypass the v14 layout): confirmed and fixed. The internal update walker does materialize index branches itself, with pre-demotion tree types and — worse — unwrapped continuation property-name trees. Added thepastaclaw suggestions: all applied. The zero-contribution helper is now CodeRabbit actionable comments:
CodeRabbit nitpicks: applied the failure-collection pattern in the v14 suite, the distinct-seeds comment, the Suites after the changes: rs-drive lib 3258, dpp 3807, drive-abci 2629 — all green. |
|
Reviewed |
Issue being fixed or feature implemented
A data contract can declare an aggregating (
countable/summable) index that terminates at a property which is also the prefix of a compound index — e.g. a summable[a]next to[a, b]. Index levels merge into one trie keyed by property name, so the compound continuation tree forbhangs inside the aggregating per-value trees of[a]. The contract registers successfully (contract validation performs no cross-index shape analysis), but for most flag combinations every document insert is then rejected: grovedb'sNotSummed/NotCountedOrSummedwrappers only accept sum-bearing inners, theNonCountedhelper only accepted count-ish inners, and provable count-bearing value trees reject count-suppressed children entirely.Example: count + sum aggregate next to a compound index
A star-rating app wants "how many reviews does this resource have, and what do they sum to" as a one-lookup aggregate, plus the usual compound index for listing reviews of a resource by rating:
{ "review": { "type": "object", "properties": { "resourceId": { "type": "string", "maxLength": 63, "position": 0 }, "rating": { "type": "integer", "minimum": 1, "maximum": 5, "position": 1 } }, "required": ["resourceId", "rating"], "additionalProperties": false, "indices": [ { "name": "resourceRatingAggregate", "properties": [{ "resourceId": "asc" }], "countable": "countable", "summable": "rating" }, { "name": "resourceRatingDistribution", "properties": [{ "resourceId": "asc" }, { "rating": "asc" }] } ] } }Today this contract registers fine, and then every
reviewinsert fails with:because both indexes merge into one trie: the
ratingcontinuation tree ofresourceRatingDistributionmust live inside the per-resourceCountSumTreethatresourceRatingAggregatecreates, and Drive had no legal way to make it contribute zero to that tree's count. (Three registered testnet contracts are bricked in exactly this state.)At v14, inserting two reviews for
resource-1(ratings 5 and 3) produces:The
resource-1element answers count/sum point lookups directly, and[resourceId, rating]queries walk theratingsubtree as usual. Deleting a review subtracts exactly its contribution (delete doc2 →count: 1, sum: 5), and deleting the last one cleans the whole branch away.Example: provable range aggregates next to a compound index
The
range*flags buy per-node commitments in the property-name tree, so range aggregates verify in O(log n). A sales ledger wants "provable count + total of sales over any day range", plus a compound index for drilling into a day by region:{ "sale": { "type": "object", "properties": { "day": { "type": "integer", "minimum": 0, "position": 0 }, "region": { "type": "string", "maxLength": 32, "position": 1 }, "amount": { "type": "integer", "minimum": 0, "maximum": 1000000, "position": 2 } }, "required": ["day", "region", "amount"], "additionalProperties": false, "indices": [ { "name": "dailySales", "properties": [{ "day": "asc" }], "countable": "countable", "summable": "amount", "rangeCountable": true, "rangeSummable": true }, { "name": "dailySalesByRegion", "properties": [{ "day": "asc" }, { "region": "asc" }] } ] } }Today this registers and then rejects every insert, same as the first example. At v14, after three sales — day 100: (US, 500), (EU, 200); day 101: (EU, 250) — the layout is:
The provable machinery lives one level up from the demotion and is untouched: each per-day
CountSumTreeelement feeds its (count, sum) into thedaytree's per-node commitments exactly as a provable child would, soAggregateCountOnRange/AggregateSumOnRangeover day ranges keep their O(log n) proofs. What the demotion changes is only the inside of each per-day tree — per-value point lookups read (count, sum) off the element either way, and nothing range-aggregates over a value tree's own children. Without the compound sibling there is no demotion: the per-day value trees stayProvableCountProvableSumTree, bit-identical to v13.The empirically pinned pre-v14 matrix: count-only value trees accept only non-sum continuations, sum-bearing value trees accept only sum-bearing continuations, and everything else errors. (Provable count-bearing value trees accept sum-bearing continuations only through an unenforced path: grovedb's wrapper-vs-provable batch guard fires solely when the parent merk pre-exists, and the walker always creates parent and wrapped child in one batch.)
A full scan of registered contracts via the platform-explorer API found three bricked contracts on testnet (all matching the issue's review schema — registered, but every insert fails), none on mainnet, and zero contracts on either network using range-flagged prefixes with compound siblings.
Fixes #3960
What was done?
Stacked on #4267, which introduces the protocol version 14 gate. This PR consumes it:
PLATFORM_V14.drivemoves toDRIVE_VERSION_V9→DRIVE_DOCUMENT_METHOD_VERSIONS_V4; all pre-v14 walker code stays byte-identical.index_level_tree_types.rs, so the insert walkers and the delete walkers' estimation layer info cannot drift. The helper demotes provable count-bearing value trees (ProvableCountSumTree/ProvableCountProvableSumTree) toCountSumTreewhen compound continuations exist, since grovedb rejects count-suppressed children under provable count parents by design and a plain continuation has no legal wrapper there at all.LowLevelDriveOperation::for_known_path_key_empty_tree_contributing_zero_to_parent): non-sum continuations underCountSumTreeparents getElement::NonCounted; non-sum continuations under sum-only parents are inserted unwrapped (a non-sum child contributes 0 to a sum naturally); count-only parents accept any inner via an extendedNonCountedbuilder. The v0 dispatcher and helpers are untouched.Every value tree keeps the invariant that its count/sum equals exactly the
[0]ref-bucket contribution, never the structural overhead of sibling continuations. Shapes insertable at v13 produce bit-identical operations; the one intentional difference is that provable value trees whose continuations were all sum-bearing getCountSumTreevalue trees for values first seen at v14+ (readers are indifferent — both variants serialize their aggregates into the element and contribute identically to the property-name tree's per-node commitments). No such shape exists on any network today.Reviewer note — a deliberate design choice to weigh in on: instead of demoting, we could keep PCPST value trees and upgrade plain continuations to a sum-bearing inner, but with grovedb v5.0.1 that permanently depends on the unenforced in-batch guard path; making it official would need a grovedb-side policy change (allowing
NonCounted/NotCountedOrSummedunder provable count parents, mirroring howNotSummedis already allowed under provable sum parents). No reader of per-node commitments inside value trees was found — range aggregates walk the property-name tree above, point lookups prove the element — so the demotion loses nothing observable today. Happy to switch if the ranked-aggregates roadmap wants provable value trees preserved.How Has This Been Tested?
shared_prefix_aggregation_e2e_tests.rsrewritten (previously#[ignore]d): a 32-combination matrix (8 aggregating prefix flag-sets × 4 child flag-sets) proving at v14: contract applies, two documents insert, the value tree carries exactly (count = 2, sum = ratings) with the exact expected element type, the continuation tree lands with the exact expected wrapper, deleting one document subtracts exactly its contribution, and deleting the last document cleans the trees away entirely...._frozen_at_v13suite pins the empirical pre-v14 accept/reject matrix per combination, so the consensus-locked v1 walkers cannot drift.cargo check --workspace --all-targets, all-features clippy, and the--no-default-features --features verifybuild of rs-drive.Breaking Changes
Consensus-breaking by design: first consumer of the v14 gate from #4267. At activation, shared-prefix aggregate index shapes become insertable (previously every such insert failed), and provable count-bearing value trees with compound continuations are created as
CountSumTreefor newly seen values. Pre-v14 behavior is bit-identical and pinned by the frozen-at-v13 test.Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests