feat(drive): document sum + average proof primitives, with SDK fan-out scaffolding and reproducible benchmarks#3661
Open
QuantumExplorer wants to merge 16 commits into
Open
Conversation
… contracts) Wire grovedb PR #670's sum-tree and dual-axis (count+sum) proof primitives through the full Drive → SDK stack and produce two design-first chapters in the book backed by reproducible benchmarks. ## What ### Sum-tree feature (tip-jar contract) - DPP schema accepts `documentsSummable: "<prop>"`, `summable: "<prop>"`, and `rangeSummable: true` (per-index + top-level). - Drive's index walker selects the right TreeType per axis: SumTree, ProvableSumTree, CountSumTree, ProvableCountSumTree, and the new ProvableCountProvableSumTree (PCPS) for combined per-node count+sum. - New `Element::ReferenceWithSumItem` + `ItemWithSumItem` at primary storage propagate per-doc sum contributions to ancestor sum trees. - Continuation wrappers (`NotSummed`, `NotCountedOrSummed`) keep parent aggregates honest under compound indexes. - `DriveDocumentSumQuery` family with point-lookup, range-aggregate, per-In carrier, and PCPS-carrier executors + verifiers, all calling the matching grovedb primitives (`verify_query`, `verify_aggregate_sum_query`, `verify_aggregate_sum_query_per_key`, `verify_aggregate_count_and_sum_query`, `verify_aggregate_count_and_sum_query_per_key`). ### Benches + book chapters - `packages/rs-drive/benches/document_sum_worst_case.rs` — 100k-row tip-jar fixture; Q1–Q9 cover total/point/range/carrier shapes. - `packages/rs-drive/benches/document_average_worst_case.rs` — 50k-row grades-contract fixture with realistic 3-axis score model (class baseline × spread, deterministic student skill, per-grade noise) and per-class enrollment popularity filter. Q1–Q7 cover point lookups, AggregateCountAndSumOnRange ranges, and PCPS carrier. - `book/src/drive/sum-index-examples.md` — 9 worked queries against the tip-jar contract with proof_size, median µs, proof_ast `<details>` blocks (with visualizer URLs), per-layer mermaid diagrams. - `book/src/drive/average-index-examples.md` — 7 worked queries against the grades contract, same format. Independently validated against a Python reimplementation of the bench's deterministic score + enrollment functions; every bench-reported (count, sum) matches byte-for-byte. ### Drive-side updates - Drive contract creation extended to handle range_summable + documents_summable promotion (top-level rangeSummable still has a known write-side gap surfaced in Q7 of sum chapter, Q1 of avg). - New `for_known_path_key_empty_tree_under_aggregating_parent` helper picks the right NotCounted/NotSummed/NotCountedOrSummed wrapper based on parent TreeType. - Bumped grovedb to PR #670 head `e69df59f` across all 6 Cargo.toml references — that PR ships the leaf and carrier primitives this feature consumes; no grovedb code modified, only the dependency pin advanced. ## Why Provides cryptographically-attested sum and average queries with single-proof commits (rather than two independent count + sum proofs the client has to splice). The PCPS variant is especially load-bearing for averages: one proof returns `(count, sum)` from the same set, so `avg = sum / count` is verifiable from a single root-hash. Carrier proofs collapse N round-trips into one for group-by-style queries. ## Testing - `cargo check -p drive` — clean (both `server` and `verify` features) - `cargo test -p drive --lib` — 3,154 / 3,154 pass (5 new tests) - `cargo bench -p drive --bench document_sum_worst_case -- --test` — runs end-to-end through Q1–Q9; reproducible numbers in the book. - `cargo bench -p drive --bench document_average_worst_case -- --test` — runs end-to-end through Q1–Q7; 31,620 grades across all enrolled triples; per-class enrollment matches documented popularity ±0.5pp. - Python validation script (`/tmp/avg_validate.py` referenced in chapter) independently reproduces every per-bucket (count, sum) for Q2–Q7 from the deterministic score + enrollment functions. - `mdbook build` clean. ## Known follow-ups (out of scope) 1. `distinct_sum_path_query` — full ~280-line port deferred; blocks `GroupByRange` + 4th criterion case on the sum bench. 2. `documentsCountable + documentsSummable` primary-key tree promotion — the doctype root doesn't promote to CountSumTree at insert time. Read path is correct (proof verifies, 614 B); only the stored (count, sum) is zero. Surfaces in Q1 of both sum + averages chapters with explicit known-bug callouts. 3. Top-level single-property `rangeSummable` promotion — same write path; compound indexes work, top-level singles don't. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Contributor
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-05-19T02:49:26.781Z |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughI can rebuild the hidden artifact correctly, but it requires assembling every provided rangeId (hundreds) into a valid stack which will take multiple steps. Do you want me to proceed and produce the full corrected hidden review stack now? ✨ Finishing Touches🧪 Generate unit tests (beta)
|
Collaborator
Review GateCommit:
|
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (46.97%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3661 +/- ##
============================================
- Coverage 88.06% 87.37% -0.69%
============================================
Files 2521 2551 +30
Lines 308995 317222 +8227
============================================
+ Hits 272122 277179 +5057
- Misses 36873 40043 +3170
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rs (1)
293-317:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftHandle the new sum-capable primary-key tree variants here.
primary_key_tree_type()can now returnSumTree,ProvableSumTree,CountSumTree,ProvableCountSumTree, andProvableCountProvableSumTree, but this match still only special-cases the two count-only variants. All of those sum-capable doctypes currently fall through tobatch_insert_empty_tree(), so the[0]primary-key tree is created as a plain tree and later sum-aware writes/proofs will run against the wrong element type.🤖 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/mod.rs` around lines 293 - 317, The match on primary_key_tree_type in insert_contract v0 only handles TreeType::ProvableCountTree and TreeType::CountTree; add explicit arms for the new sum-capable variants (TreeType::SumTree, ProvableSumTree, CountSumTree, ProvableCountSumTree, ProvableCountProvableSumTree) so they call the corresponding sum-aware batch insert helpers instead of falling through to batch_insert_empty_tree; update the match in the block that calls document_type.as_ref().primary_key_tree_type(platform_version)? to invoke the correct functions (e.g., batch_insert_empty_sum_tree, batch_insert_empty_provable_sum_tree, batch_insert_empty_count_sum_tree, batch_insert_empty_provable_count_sum_tree, batch_insert_empty_provable_count_provable_sum_tree—or the existing project equivalents) with the same arguments (type_path, key_info, storage_flags.as_ref(), &mut batch_operations, &platform_version.drive) so the primary-key tree is created with the correct sum-capable element type.packages/rs-drive/src/drive/document/insert/add_reference_for_index_level_for_contract_operations/v0/mod.rs (1)
238-245:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix stateless fee sizing for sum-bearing references.
Line 238 and Line 283 still use plain item-space sizing, and Line 304 computes target size without the sum-item payload. For
summableindexes this underestimates fees by the extrai64per reference.Suggested patch (concept)
- Element::required_item_space( + if sum_property_name.is_some() { + Element::required_item_with_sum_item_space( + *max_size, + STORAGE_FLAGS_SIZE, + &drive_version.grove_version, + )? + } else { + Element::required_item_space( *max_size, STORAGE_FLAGS_SIZE, &drive_version.grove_version, - )?, + )? + }, - document_reference_size(document_and_contract_info.document_type) + document_reference_size(document_and_contract_info.document_type) + + if sum_property_name.is_some() { 8 } else { 0 }Also applies to: 283-289, 304-308
🤖 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/insert/add_reference_for_index_level_for_contract_operations/v0/mod.rs` around lines 238 - 245, The size computation underestimates stateless fees for summable indexes: replace calls to Element::required_item_space (the occurrences using *max_size, STORAGE_FLAGS_SIZE, &drive_version.grove_version) with Element::required_item_with_sum_item_space when sum_property_name.is_some() (or equivalent summable check), and adjust the target_size calculation (the block that computes target_size for references) to add 8 bytes per reference (i64) when summable; apply this change for all three affected spots (the two Element::required_item_space calls and the target_size computation) so the extra sum-item payload is accounted for.book/src/drive/indexes.md (1)
408-409:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the correct sum-suppression wrapper name.
Element::NonCountedItemWithSumItemconflicts with the wrapper naming used elsewhere in this chapter (NotSummed/NotCountedOrSummed). Please align terminology to the actual element variants to prevent implementation mistakes.🤖 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 `@book/src/drive/indexes.md` around lines 408 - 409, The documentation uses the incorrect wrapper name Element::NonCountedItemWithSumItem; replace that variant with the actual variant names used elsewhere — e.g., Element::NotSummed or Element::NotCountedOrSummed — throughout the paragraph that mentions 'shape' so the terminology matches the chapter (update any surrounding examples and explanatory text that reference Element::NonCountedItemWithSumItem to use Element::NotSummed / Element::NotCountedOrSummed instead).
🧹 Nitpick comments (7)
packages/rs-drive/src/query/mod.rs (1)
35-41: ⚡ Quick winSplit
DriveDocumentSumQueryre-export from server-only types.
DriveDocumentSumQueryis currently server-gated (Line 37), while the module itself is exposed forserver|verify(Line 199). This makes the top-level API inconsistent for verify-only consumers.♻️ Proposed refactor
-#[cfg(feature = "server")] -pub use drive_document_sum_query::{ - DocumentSumRequest, DocumentSumResponse, DriveDocumentSumQuery, RangeSumOptions, SumEntry, - SumMode, -}; +#[cfg(any(feature = "server", feature = "verify"))] +pub use drive_document_sum_query::DriveDocumentSumQuery; + +#[cfg(feature = "server")] +pub use drive_document_sum_query::{ + DocumentSumRequest, DocumentSumResponse, RangeSumOptions, SumEntry, SumMode, +};🤖 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/query/mod.rs` around lines 35 - 41, The current re-export group gates DriveDocumentSumQuery with server-only types causing verify-only builds to lack DriveDocumentSumQuery; split the exports so DriveDocumentSumQuery is exported under the broader visibility used by the module (e.g. cfg(any(feature = "server", feature = "verify")) or unconditionally) while keeping DocumentSumRequest, DocumentSumResponse, RangeSumOptions, SumEntry, and SumMode behind cfg(feature = "server"); update the pub use block to have one line exporting DriveDocumentSumQuery with the correct cfg and a separate cfg(server)-guarded pub use for the remaining server-only types so verify-only consumers can access DriveDocumentSumQuery.packages/rs-drive/benches/document_sum_worst_case.rs (2)
170-228: ⚡ Quick winAvoid maintaining a second tip-jar schema here.
This bench now duplicates
packages/rs-drive/tests/supporting_files/contract/tip-jar/tip-jar-contract.json. The grades bench already treats the JSON file as the source of truth; keeping an inline copy here makes the fixture, docs, and benchmark easy to drift on the next schema change.♻️ Suggested direction
+use dpp::tests::json_document::json_document_to_contract; + fn tip_jar_contract() -> DataContract { - let factory = - DataContractFactory::new(PROTOCOL_VERSION_V12).expect("expected to create factory"); - let document_schema = platform_value!({ - ... - }); - let schemas = platform_value!({ DOCUMENT_TYPE_NAME: document_schema }); - - factory - .create_with_value_config(Identifier::from([42u8; 32]), 0, schemas, None, None) - .expect("expected to create sum bench data contract") - .data_contract_owned() + json_document_to_contract( + "tests/supporting_files/contract/tip-jar/tip-jar-contract.json", + false, + PlatformVersion::latest(), + ) + .expect("expected to parse tip-jar contract") }🤖 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/benches/document_sum_worst_case.rs` around lines 170 - 228, The tip_jar_contract() function duplicates the tip-jar JSON schema; instead load and parse the canonical JSON file used elsewhere rather than maintaining an inline schema literal. Update tip_jar_contract() to read the existing supporting file (the same file used by the grades bench), parse it into a platform value/DataContract and then call DataContractFactory::create_with_value_config(...) (or otherwise construct the DataContract) so the bench uses the single source-of-truth JSON instead of the inline platform_value! literal.
1178-1185: ⚡ Quick winSerialize the
sentAtprobe key the same way queries do.This probe hardcodes
to_be_bytes()forsentAt, while the rest of the bench builds index keys through document-type serialization. If the integer-key encoding changes, the diagnostic starts probing the wrong node shape even though the actual benchmark path queries still work.🤖 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/benches/document_sum_worst_case.rs` around lines 1178 - 1185, The probe currently builds mid_sent_at with (fixture.row_count / 2).to_be_bytes(), which hardcodes big-endian integer bytes and can diverge from how queries serialize the "sentAt" index key; update mid_sent_at so it is produced by the same document-type/index-key serialization used elsewhere in this benchmark (the same code path used to build query keys for the "sentAt" index) and return a Vec<u8> (so the entry in cases for "bySentAt" remains mid_sent_at.to_vec()/Vec<u8>); replace the direct to_be_bytes() usage with that serializer so mid_sent_at matches query key encoding.packages/rs-drive/src/fees/op.rs (1)
1006-1008: ⚡ Quick winAdd a regression test for the new
ProvableCountProvableSumTreeconverter arm.There is a test for
TreeType::ProvableSumTree, but not for the newly added PCPS arm. Add a parallel assertion to avoid silent regressions in empty-tree conversion.🤖 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 1006 - 1008, Add a regression test mirroring the existing ProvableSumTree empty-tree conversion check but for TreeType::ProvableCountProvableSumTree: call the conversion path that produces Element::empty_provable_count_provable_sum_tree_with_flags(element_flags) and assert its output equals the expected empty Element (same style/fixtures as the ProvableSumTree test). Locate the existing test for ProvableSumTree and add a parallel assertion referencing TreeType::ProvableCountProvableSumTree and Element::empty_provable_count_provable_sum_tree_with_flags to ensure the new arm is covered.packages/rs-drive/src/util/grove_operations/grove_insert_empty_tree/v0/mod.rs (1)
31-33: ⚡ Quick winAdd a test for PCPS empty-tree insertion path.
Please add a
test_grove_insert_empty_tree_provable_count_provable_sumcase to lock in this new tree-type branch.🤖 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/grove_insert_empty_tree/v0/mod.rs` around lines 31 - 33, Add a unit test named test_grove_insert_empty_tree_provable_count_provable_sum that exercises the new TreeType::ProvableCountProvableSumTree branch used during empty-tree insertion: mirror the existing empty-tree tests for other TreeType variants, call the same grove_insert_empty_tree path (the helper/test harness used for other empty-tree cases), and assert that the returned Element equals Element::empty_provable_count_provable_sum_tree() and that insertion succeeds without panics; place the test alongside the other grove empty-tree tests so it runs in the same test module.packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rs (1)
213-248: ⚡ Quick winAdd
validate_configtests fordocuments_summableandrange_summablemutations.These two new immutability checks are behavior changes, but the local
validate_configtest suite doesn’t currently assert them. Adding explicit old/new schema cases here will lock this contract down.🤖 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-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rs` around lines 213 - 248, The tests for validate_config are missing assertions that changing documents_summable or range_summable is rejected; add test cases in the validate_config test suite that construct an original DocumentType and a new_document_type with different documents_summable and another case with different range_summable, then call the validate_update logic (the same path exercised by validate_config) and assert it returns a SimpleConsensusValidationResult error containing DocumentTypeUpdateError for the respective mutation; reference the DocumentType methods documents_summable() and range_summable() and the validate_update/v0 entry point used by the existing tests to locate where to add these two negative test cases.packages/rs-dpp/src/data_contract/document_type/index_level/mod.rs (1)
315-417: ⚡ Quick winAdd dedicated update-immutability tests for the new sum axis.
find_first_summability_change()is now part ofvalidate_update, but this module’s tests don’t yet pinsummable/range_summabletransition cases (e.g.,None -> Some, property rename, andrange_summabletoggles). Adding parity tests with the existing countability cases would harden this path.🤖 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-dpp/src/data_contract/document_type/index_level/mod.rs` around lines 315 - 417, Tests for index update validation are missing coverage for the new sum axis: add unit tests that mirror the existing countability tests but exercise find_first_summability_change via validate_update, covering transitions like None -> Some summable, property-name changes (rename), and toggling range_summable; assert that validate_update returns DataContractInvalidIndexDefinitionUpdateError with the correct path for each case by constructing old and new IndexLevel trees and calling IndexLevel::validate_update (or the public wrapper used in tests) to ensure the summability immutability is enforced.
🤖 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 `@book/src/drive/document-sum-trees.md`:
- Line 3: The opening sentence incorrectly says the feature is exposed by “two
endpoints”; update it to match the rest of the chapter by stating it is exposed
via a single unified GetDocumentsSum endpoint (referencing GetDocumentsSum) and
scan nearby text in this section to ensure no other references still claim two
endpoints (also reconcile mentions with the Document Count Trees chapter if
linked) so the wording is consistent throughout.
In `@packages/rs-dpp/src/data_contract/document_type/accessors/v2/mod.rs`:
- Around line 12-17: Update the documentation on documents_summable to avoid
overstating complexity: change the sentence that claims enabling this yields
O(log n) GetDocumentsSum queries to explain that while per-key range sums use a
SumTree (or ProvableSumTree when Self::range_summable is true) and support O(log
n) range queries, the doctype-level total-sum fast path reads the root aggregate
in O(1); reference documents_summable, Self::range_summable,
SumTree/ProvableSumTree and GetDocumentsSum in the revised comment.
In `@packages/rs-dpp/src/data_contract/document_type/index/mod.rs`:
- Around line 761-772: The code currently accepts Value::Text("") and sets
summable = Some(""), which should be rejected; in the match over value_value
(the block assigning summable) update the Value::Text arm to check s.is_empty()
and return Err(DataContractError::ValueWrongType(...)) for empty strings,
keeping the existing error message (or a slightly amended one indicating empty
property names are invalid), so only non-empty strings become Some(s.clone())
and null remains None; reference the summable binding, value_value match, and
DataContractError::ValueWrongType.
In `@packages/rs-dpp/src/data_contract/document_type/v2/accessors.rs`:
- Around line 245-253: The setter set_range_summable currently early-returns
when range_summable is true but documents_summable is None, leaving the struct
potentially unchanged; change it to unconditionally normalize the field by
assigning self.range_summable = range_summable &&
self.documents_summable.is_some() so that the invariant (range_summable only
true when documents_summable is present) is always enforced after calling the
method.
In `@packages/rs-drive-abci/src/query/document_query/v1/mod.rs`:
- Around line 641-649: Replace the direct error return
Err(Error::Query(QueryError::NotYetImplemented(...))) with a typed validation
response: construct and return Ok(QueryValidationResult::new_with_error(/* same
message string */)) so the handler yields a QueryValidationResult rather than
bubbling Error::Query; keep the original NotYetImplemented message text and use
QueryValidationResult::new_with_error to match other v1 query gates.
In `@packages/rs-drive-proof-verifier/src/proof/document_split_sum.rs`:
- Around line 50-84: The method DocumentSplitSums::FromProof
(maybe_from_proof_with_metadata) currently always returns Error::NotImplemented;
replace the stub with the same verifier flow used by
DocumentSplitCounts::FromProof: match the response.result oneof for
SumResults::entries vs Proof(p); for Proof(p) dispatch based on the query mode
to DriveDocumentSumQuery::verify_distinct_sum_proof for per-key distinct sums or
to verify_carrier_aggregate_sum_proof for compound In+range branches; verify the
Tenderdash proof, map the verified entries into Vec<SplitSumEntry>, construct
and return Some(DocumentSplitSums) along with ResponseMetadata and the Proof;
follow the implementation patterns and error handling in document_sum.rs and
DocumentSplitCounts::FromProof to locate where to wire up
verify_distinct_sum_proof, verify_carrier_aggregate_sum_proof, and the resulting
mapping.
In `@packages/rs-drive-proof-verifier/src/proof/document_sum.rs`:
- Around line 28-68: Implement
DocumentSum::FromProof::maybe_from_proof_with_metadata to replace the
NotImplemented error: convert the incoming request into a DriveDocumentSumQuery
(mirror DocumentCount's request conversion), match on the response's result
oneof and (a) when SumResults::aggregate_sum wrap the i64 in
Some(DocumentSum(sum)), (b) when SumResults::entries return an error indicating
caller should use DocumentSplitSums, and (c) when SumResults::proof (Proof(p))
call the DriveDocumentSumQuery verify_proof helper (which uses grovedb's
verify_aggregate_sum_query) to verify the Grovedb proof, then call
verify_tenderdash_proof to validate the tenderdash signature, collect and return
the ResponseMetadata and Proof on success; follow DocumentCount::FromProof for
control flow and error handling and reference DriveDocumentSumQuery,
maybe_from_proof_with_metadata, verify_aggregate_sum_query, verify_proof,
verify_tenderdash_proof, and DocumentSplitSums when implementing.
In `@packages/rs-drive/benches/document_average_worst_case.rs`:
- Around line 313-323: populate_fixture currently discards the actual number of
documents inserted (many triples are dropped by is_enrolled) but the benchmark
still uses the original row_count for throughput and headers; modify
populate_fixture to return the actual inserted count (e.g., usize/usize) and
update callers in document_average_worst_case.rs (the construction around
populate_fixture and the Self::new(...) call) to capture that returned inserted
value and use it for Criterion throughput and proof-size/report headers instead
of the original row_count; ensure all other places noted (the other blocks
around lines 421-470 and 573-576) also receive and propagate the inserted count
into their benchmark/accounting code.
In
`@packages/rs-drive/src/drive/document/delete/remove_indices_for_index_level_for_contract_operations/mod.rs`:
- Around line 25-38: The docstring warns that the parameter
parent_value_tree_is_range_countable: bool cannot express sum-bearing parent
variants; change that parameter to a richer enum/type (e.g. TreeType or a
bitflags struct capturing CountTree vs SumTree/Provable variants) on the
function remove_indices_for_index_level_for_contract_operations (and its
versioned helper remove_indices_for_index_level_for_contract_operations_v0),
update every internal use and callers to pass the new TreeType, and adjust
fee/estimation logic that branches on parent_value_tree_is_range_countable to
inspect the new TreeType flags instead so sum-bearing layers are correctly
charged; ensure any serialization/trait bounds are implemented for the new type
where required.
In
`@packages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rs`:
- Around line 323-327: The sizing for summable documents is using
Element::required_item_space which omits the extra sum-item overhead for
ItemWithSumItem; update the three spots that call
Element::required_item_space(*max_size, STORAGE_FLAGS_SIZE,
&platform_version.drive.grove_version) to call the corresponding sum-item-aware
sizing helper (the Element API for sum-item sizing used in this crate) so the
returned required space includes the sum-item overhead for ItemWithSumItem, and
also adjust the target size computation for documents_summable to add the
sum-item overhead (use the same Element sum-item sizing helper or add the
sum-item constant) so the final target size includes sum-item costs; touch the
calls around Element::required_item_space, the target size calculation for
documents_summable, and any uses of max_size/STORAGE_FLAGS_SIZE with
ItemWithSumItem to ensure consistency.
In `@packages/rs-drive/src/drive/document/mod.rs`:
- Around line 177-185: The error path currently leaks memory via Box::leak when
constructing DriveError::CorruptedCodeExecution; fix by removing Box::leak —
either (A) replace the formatted message with a static &'static str literal (no
formatting) in this call to value.to_integer::<i64>().map_err(...), or
(preferred) refactor DriveError::CorruptedCodeExecution to own a String (or
Cow<'static, str>) and update its constructor/usages, then here build a normal
owned String with format!(...) and pass it into
DriveError::CorruptedCodeExecution (no Box::leak).
In `@packages/rs-drive/src/query/drive_document_sum_query/drive_dispatcher.rs`:
- Around line 124-127: The code currently casts effective_limit (derived from
request.limit or crate::config::DEFAULT_QUERY_LIMIT) from u32 to u16 via
limit_u16, which can silently truncate values and bypass drive_config max logic;
update the logic in drive_dispatcher.rs (symbols: effective_limit,
request.limit, DEFAULT_QUERY_LIMIT, limit_u16) to avoid lossy u32->u16 casts by
either using u32 throughout the proof/query path or explicitly
validating/clamping the u32 against the allowed maximum (and u16::MAX if a
16-bit value is truly required) and returning an error if out of range; ensure
the chosen approach preserves the documented default/max behavior from
drive_config and propagate the type change or validated value to where limit_u16
is consumed.
In `@packages/rs-drive/src/query/drive_document_sum_query/execute_range_sum.rs`:
- Around line 55-71: The code currently picks the first WhereClause with
WhereOperator::In and silently ignores additional In clauses; change the logic
in the execute_range_sum path to validate there is exactly one In clause before
proceeding: count/filter self.where_clauses for operator == WhereOperator::In,
return an Error::Query(QuerySyntaxError::InvalidWhereClauseComponents(...)) if
the count is zero or greater than one, then use that single in_clause to call
in_values().into_data_with_error()?? and build other_clauses by removing that
single In clause (not all In clauses).
In
`@packages/rs-drive/src/query/drive_document_sum_query/executors/per_in_value.rs`:
- Around line 37-52: The code currently grabs the first WhereClause with
WhereOperator::In and ignores any others; change the logic to explicitly enforce
exactly one In clause by counting/filtering where_clauses for wc.operator ==
WhereOperator::In, returning the same QuerySyntaxError if the count != 1, then
extract that single in_clause and call
in_clause.in_values().into_data_with_error()??; keep building other_clauses by
filtering out WhereOperator::In as before. Ensure you reference the existing
symbols WhereOperator::In, where_clauses, in_clause, in_values, and the
QuerySyntaxError message "execute_document_sum_per_in_value_no_proof requires
exactly one `in` clause".
In `@packages/rs-drive/src/query/drive_document_sum_query/index_picker.rs`:
- Around line 134-156: The two-range index matcher (involving outer_range_field,
index.properties, prefix_fields and the intermediate_props_ok logic) currently
only verifies that each intermediate index property is in prefix_fields but
doesn't ensure all Equal/In prefix_fields are represented; update the check so
that the set (or count) of intermediate index property names between first and
terminator exactly matches the prefix_fields set (no missing or extra prefix
fields) before returning Some(index), e.g., collect the intermediate property
names and compare to prefix_fields (or their lengths) rather than the current
one-way containment test.
In `@packages/rs-drive/src/query/drive_document_sum_query/path_query.rs`:
- Around line 238-247: The current code in aggregate_sum_path_query and the
other aggregate builder finds the first range where-clause via
where_clauses.iter().find(|wc| is_range_operator(wc.operator)), which can pick
the wrong clause when multiple range-like clauses exist; change the predicate to
bind to the index terminator property (e.g. find(|wc|
is_range_operator(wc.operator) && wc.property == self.index_terminator_field or
the actual terminator field name used in the struct) so the selected
range_clause is the one for the index terminator, then pass that clause to
self.range_clause_to_query_item; apply the same fix to the other occurrence
around the 316-326 block.
- Around line 454-467: The startsWith handling in WhereOperator::StartsWith
currently only increments the last byte and wrongly rejects prefixes like [0x12,
0xFF]; change the logic to perform proper byte-wise carry propagation across
right_key: clone the serialized left_key to right_key, iterate bytes from the
last index backward adding 1 and clearing trailing 0xFFs until a byte increments
without overflow, then truncate right_key to that byte+1 position and use
QueryItem::Range(left_key..right_key); only return the InvalidStartsWithClause
error if every byte was 0xFF (i.e., carry overflowed past the most significant
byte). Ensure this change is applied in the WhereOperator::StartsWith block that
uses serialize(...) and produces QueryItem::Range.
In `@packages/rs-sdk/Cargo.toml`:
- Line 21: The Cargo.toml dependency entry for grovedb-commitment-tree pins a
non-existent commit hash; update the rev value for the grovedb-commitment-tree
dependency so it references a valid commit (for example replace rev =
"e69df59f81371902df9107526e2a1f2ba286dd58" with the actual merge commit rev =
"e98bab5f" that contains PR `#670`), or alternatively point the dependency to a
published crate version instead of the git rev; adjust the
grovedb-commitment-tree line in Cargo.toml accordingly so cargo fetch succeeds.
---
Outside diff comments:
In `@book/src/drive/indexes.md`:
- Around line 408-409: The documentation uses the incorrect wrapper name
Element::NonCountedItemWithSumItem; replace that variant with the actual variant
names used elsewhere — e.g., Element::NotSummed or Element::NotCountedOrSummed —
throughout the paragraph that mentions 'shape' so the terminology matches the
chapter (update any surrounding examples and explanatory text that reference
Element::NonCountedItemWithSumItem to use Element::NotSummed /
Element::NotCountedOrSummed instead).
In `@packages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rs`:
- Around line 293-317: The match on primary_key_tree_type in insert_contract v0
only handles TreeType::ProvableCountTree and TreeType::CountTree; add explicit
arms for the new sum-capable variants (TreeType::SumTree, ProvableSumTree,
CountSumTree, ProvableCountSumTree, ProvableCountProvableSumTree) so they call
the corresponding sum-aware batch insert helpers instead of falling through to
batch_insert_empty_tree; update the match in the block that calls
document_type.as_ref().primary_key_tree_type(platform_version)? to invoke the
correct functions (e.g., batch_insert_empty_sum_tree,
batch_insert_empty_provable_sum_tree, batch_insert_empty_count_sum_tree,
batch_insert_empty_provable_count_sum_tree,
batch_insert_empty_provable_count_provable_sum_tree—or the existing project
equivalents) with the same arguments (type_path, key_info,
storage_flags.as_ref(), &mut batch_operations, &platform_version.drive) so the
primary-key tree is created with the correct sum-capable element type.
In
`@packages/rs-drive/src/drive/document/insert/add_reference_for_index_level_for_contract_operations/v0/mod.rs`:
- Around line 238-245: The size computation underestimates stateless fees for
summable indexes: replace calls to Element::required_item_space (the occurrences
using *max_size, STORAGE_FLAGS_SIZE, &drive_version.grove_version) with
Element::required_item_with_sum_item_space when sum_property_name.is_some() (or
equivalent summable check), and adjust the target_size calculation (the block
that computes target_size for references) to add 8 bytes per reference (i64)
when summable; apply this change for all three affected spots (the two
Element::required_item_space calls and the target_size computation) so the extra
sum-item payload is accounted for.
---
Nitpick comments:
In `@packages/rs-dpp/src/data_contract/document_type/index_level/mod.rs`:
- Around line 315-417: Tests for index update validation are missing coverage
for the new sum axis: add unit tests that mirror the existing countability tests
but exercise find_first_summability_change via validate_update, covering
transitions like None -> Some summable, property-name changes (rename), and
toggling range_summable; assert that validate_update returns
DataContractInvalidIndexDefinitionUpdateError with the correct path for each
case by constructing old and new IndexLevel trees and calling
IndexLevel::validate_update (or the public wrapper used in tests) to ensure the
summability immutability is enforced.
In
`@packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rs`:
- Around line 213-248: The tests for validate_config are missing assertions that
changing documents_summable or range_summable is rejected; add test cases in the
validate_config test suite that construct an original DocumentType and a
new_document_type with different documents_summable and another case with
different range_summable, then call the validate_update logic (the same path
exercised by validate_config) and assert it returns a
SimpleConsensusValidationResult error containing DocumentTypeUpdateError for the
respective mutation; reference the DocumentType methods documents_summable() and
range_summable() and the validate_update/v0 entry point used by the existing
tests to locate where to add these two negative test cases.
In `@packages/rs-drive/benches/document_sum_worst_case.rs`:
- Around line 170-228: The tip_jar_contract() function duplicates the tip-jar
JSON schema; instead load and parse the canonical JSON file used elsewhere
rather than maintaining an inline schema literal. Update tip_jar_contract() to
read the existing supporting file (the same file used by the grades bench),
parse it into a platform value/DataContract and then call
DataContractFactory::create_with_value_config(...) (or otherwise construct the
DataContract) so the bench uses the single source-of-truth JSON instead of the
inline platform_value! literal.
- Around line 1178-1185: The probe currently builds mid_sent_at with
(fixture.row_count / 2).to_be_bytes(), which hardcodes big-endian integer bytes
and can diverge from how queries serialize the "sentAt" index key; update
mid_sent_at so it is produced by the same document-type/index-key serialization
used elsewhere in this benchmark (the same code path used to build query keys
for the "sentAt" index) and return a Vec<u8> (so the entry in cases for
"bySentAt" remains mid_sent_at.to_vec()/Vec<u8>); replace the direct
to_be_bytes() usage with that serializer so mid_sent_at matches query key
encoding.
In `@packages/rs-drive/src/fees/op.rs`:
- Around line 1006-1008: Add a regression test mirroring the existing
ProvableSumTree empty-tree conversion check but for
TreeType::ProvableCountProvableSumTree: call the conversion path that produces
Element::empty_provable_count_provable_sum_tree_with_flags(element_flags) and
assert its output equals the expected empty Element (same style/fixtures as the
ProvableSumTree test). Locate the existing test for ProvableSumTree and add a
parallel assertion referencing TreeType::ProvableCountProvableSumTree and
Element::empty_provable_count_provable_sum_tree_with_flags to ensure the new arm
is covered.
In `@packages/rs-drive/src/query/mod.rs`:
- Around line 35-41: The current re-export group gates DriveDocumentSumQuery
with server-only types causing verify-only builds to lack DriveDocumentSumQuery;
split the exports so DriveDocumentSumQuery is exported under the broader
visibility used by the module (e.g. cfg(any(feature = "server", feature =
"verify")) or unconditionally) while keeping DocumentSumRequest,
DocumentSumResponse, RangeSumOptions, SumEntry, and SumMode behind cfg(feature =
"server"); update the pub use block to have one line exporting
DriveDocumentSumQuery with the correct cfg and a separate cfg(server)-guarded
pub use for the remaining server-only types so verify-only consumers can access
DriveDocumentSumQuery.
In
`@packages/rs-drive/src/util/grove_operations/grove_insert_empty_tree/v0/mod.rs`:
- Around line 31-33: Add a unit test named
test_grove_insert_empty_tree_provable_count_provable_sum that exercises the new
TreeType::ProvableCountProvableSumTree branch used during empty-tree insertion:
mirror the existing empty-tree tests for other TreeType variants, call the same
grove_insert_empty_tree path (the helper/test harness used for other empty-tree
cases), and assert that the returned Element equals
Element::empty_provable_count_provable_sum_tree() and that insertion succeeds
without panics; place the test alongside the other grove empty-tree tests so it
runs in the same test module.
🪄 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: b6b5ccd6-7543-4ee0-b481-5a7a984cbd10
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (95)
book/src/SUMMARY.mdbook/src/drive/average-index-examples.mdbook/src/drive/document-sum-trees.mdbook/src/drive/indexes.mdbook/src/drive/sum-index-examples.mdpackages/dapi-grpc/protos/platform/v0/platform.protopackages/rs-dpp/Cargo.tomlpackages/rs-dpp/schema/meta_schemas/document/v1/document-meta.jsonpackages/rs-dpp/src/data_contract/document_type/accessors/mod.rspackages/rs-dpp/src/data_contract/document_type/accessors/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/index/mod.rspackages/rs-dpp/src/data_contract/document_type/index/random_index.rspackages/rs-dpp/src/data_contract/document_type/index_level/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/accessors.rspackages/rs-dpp/src/data_contract/document_type/v2/mod.rspackages/rs-drive-abci/Cargo.tomlpackages/rs-drive-abci/src/query/document_query/v1/mod.rspackages/rs-drive-proof-verifier/src/lib.rspackages/rs-drive-proof-verifier/src/proof.rspackages/rs-drive-proof-verifier/src/proof/document_split_sum.rspackages/rs-drive-proof-verifier/src/proof/document_sum.rspackages/rs-drive/Cargo.tomlpackages/rs-drive/benches/document_average_worst_case.rspackages/rs-drive/benches/document_sum_worst_case.rspackages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.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/v0/mod.rspackages/rs-drive/src/drive/document/delete/remove_indices_for_top_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/delete/remove_reference_for_index_level_for_contract_operations/mod.rspackages/rs-drive/src/drive/document/delete/remove_reference_for_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.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/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_reference_for_index_level_for_contract_operations/mod.rspackages/rs-drive/src/drive/document/insert/add_reference_for_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/mod.rspackages/rs-drive/src/drive/document/primary_key_tree_type.rspackages/rs-drive/src/fees/op.rspackages/rs-drive/src/query/drive_document_count_query/tests.rspackages/rs-drive/src/query/drive_document_sum_query/drive_dispatcher.rspackages/rs-drive/src/query/drive_document_sum_query/execute_point_lookup.rspackages/rs-drive/src/query/drive_document_sum_query/execute_range_sum.rspackages/rs-drive/src/query/drive_document_sum_query/executors/mod.rspackages/rs-drive/src/query/drive_document_sum_query/executors/per_in_value.rspackages/rs-drive/src/query/drive_document_sum_query/executors/point_lookup_proof.rspackages/rs-drive/src/query/drive_document_sum_query/executors/range_aggregate_carrier_proof.rspackages/rs-drive/src/query/drive_document_sum_query/executors/range_distinct_proof.rspackages/rs-drive/src/query/drive_document_sum_query/executors/range_no_proof.rspackages/rs-drive/src/query/drive_document_sum_query/executors/range_proof.rspackages/rs-drive/src/query/drive_document_sum_query/executors/total.rspackages/rs-drive/src/query/drive_document_sum_query/grovedb_pr_670.rspackages/rs-drive/src/query/drive_document_sum_query/index_picker.rspackages/rs-drive/src/query/drive_document_sum_query/mod.rspackages/rs-drive/src/query/drive_document_sum_query/mode_detection.rspackages/rs-drive/src/query/drive_document_sum_query/path_query.rspackages/rs-drive/src/query/drive_document_sum_query/tests.rspackages/rs-drive/src/query/mod.rspackages/rs-drive/src/util/grove_operations/batch_insert_empty_provable_count_provable_sum_tree/mod.rspackages/rs-drive/src/util/grove_operations/batch_insert_empty_provable_count_provable_sum_tree/v0/mod.rspackages/rs-drive/src/util/grove_operations/batch_insert_empty_provable_count_sum_tree/mod.rspackages/rs-drive/src/util/grove_operations/batch_insert_empty_provable_count_sum_tree/v0/mod.rspackages/rs-drive/src/util/grove_operations/batch_insert_empty_provable_sum_tree/mod.rspackages/rs-drive/src/util/grove_operations/batch_insert_empty_provable_sum_tree/v0/mod.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-drive/src/util/grove_operations/grove_insert_empty_tree/v0/mod.rspackages/rs-drive/src/util/grove_operations/mod.rspackages/rs-drive/src/verify/document_sum/mod.rspackages/rs-drive/src/verify/document_sum/verify_aggregate_count_and_sum_proof/mod.rspackages/rs-drive/src/verify/document_sum/verify_aggregate_count_and_sum_proof/v0/mod.rspackages/rs-drive/src/verify/document_sum/verify_carrier_aggregate_count_and_sum_proof/mod.rspackages/rs-drive/src/verify/document_sum/verify_carrier_aggregate_count_and_sum_proof/v0/mod.rspackages/rs-drive/src/verify/document_sum/verify_carrier_aggregate_sum_proof/mod.rspackages/rs-drive/src/verify/document_sum/verify_carrier_aggregate_sum_proof/v0/mod.rspackages/rs-drive/src/verify/mod.rspackages/rs-drive/tests/supporting_files/contract/grades/grades-contract.jsonpackages/rs-drive/tests/supporting_files/contract/tip-jar/tip-jar-contract.jsonpackages/rs-platform-version/Cargo.tomlpackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v2.rspackages/rs-platform-version/src/version/drive_versions/drive_grove_method_versions/mod.rspackages/rs-platform-version/src/version/drive_versions/drive_grove_method_versions/v1.rspackages/rs-platform-version/src/version/drive_versions/drive_verify_method_versions/mod.rspackages/rs-platform-version/src/version/drive_versions/drive_verify_method_versions/v1.rspackages/rs-platform-wallet/Cargo.tomlpackages/rs-sdk-ffi/src/document/queries/mod.rspackages/rs-sdk-ffi/src/document/queries/sum.rspackages/rs-sdk/Cargo.tomlpackages/rs-sdk/src/platform/documents/document_split_sums.rspackages/rs-sdk/src/platform/documents/document_sum.rspackages/rs-sdk/src/platform/documents/mod.rspackages/wasm-sdk/src/queries/document.rs
CI's clippy step runs `-D warnings`, which surfaced several lints/errors not caught by local `cargo check`: - DPP `doc_lazy_continuation` on a `+ reference variant` doc line that parsed as a markdown list marker. Rephrased to `, and the reference variant`. - Drive `unreachable_patterns` on three exhaustive 4-tuple matches that had defensive `_ =>` arms. Removed the arms; consolidated the unused helper bindings. - Drive `unused_imports`: `dpp::document::Document` and `dpp::platform_value::Value` in the new sum module. - Drive `clippy::too_many_arguments` on `execute_document_sum_total_no_proof` (8 args). Added `#[allow]` matching the executor's sibling pattern. - drive-proof-verifier: removed `Error::NotImplemented` (variant doesn't exist); swapped to `Error::DriveError`. Added `#[allow(clippy::extra_unused_lifetimes)]` on `FromProof` impls. - drive-abci `dispatch_sum_v1`: fixed parameter types (`Vec<u8>` / `RequestV1Start` matching `dispatch_count_v1`) and return shape (`Ok(QueryValidationResult::new_with_error(not_yet_implemented(…)))`). - wasm-drive-verify: added missing `QueryItem::AggregateCountAndSumOnRange` arm (returns the same typed rejection as the other Aggregate variants). - rs-sdk: added MockResponse impls for `DocumentSum` and `DocumentSplitSums` mirroring the count-side pattern. - rs-sdk-ffi: fixed the sum.rs import path (`crate::DashSDKError` not `crate::types::DashSDKError`); changed `Unsupported` → `NotImplemented` (the existing variant); removed unused re-export in `mod.rs`. - rs-sdk-ffi path_elements.rs: added missing `Element::ProvableCountProvableSumTree` arms in both `format_element_data` and `format_element_type`. - wasm-sdk: swapped `WasmSdkError::NotImplemented(…)` for `WasmSdkError::generic(…)`. All 3,154 drive lib tests still pass. `cargo clippy --workspace --all-features --locked -- --no-deps -D warnings` is now clean across the entire workspace. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
PR #670 has merged to grovedb's develop branch. Point the dep at develop's tip (e98bab5f) instead of the PR-branch SHA. Updates the 6 Cargo.toml refs + 15+ in-code/book references to the SHA. All 3,154 drive lib tests pass. Workspace clippy with -D warnings remains clean. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
QuantumExplorer
changed the title
Acts on 14 review comments. Outright bug fixes for the ones that
were correctness issues; doc tweaks for the ones that were wording.
Critical fixes:
- `read_document_sum_contribution` no longer leaks memory via
`Box::leak` in the error path. The conversion-error detail (which
was the only reason for the leak) gets dropped — the static
"shouldn't be reachable, validator should have caught" message
carries the diagnostic intent.
- `insert_contract` primary-key tree creation now handles all
sum-capable `TreeType` variants (SumTree, ProvableSumTree,
CountSumTree, ProvableCountSumTree, ProvableCountProvableSumTree).
The previous catch-all `_ => batch_insert_empty_tree` would have
created a plain `NormalTree` at the doctype root for any sum-
bearing doctype, with all subsequent sum-aware writes / proofs
operating against the wrong element variant.
Correctness fixes:
- Dispatcher u32→u16 limit casts now clamp against
`drive_config.max_query_limit` and return a typed error on
overflow instead of silently truncating.
- `execute_range_sum` + `executors/per_in_value` enforce exactly
one `In` clause (count + filter instead of first-match), so
multi-`In` requests don't silently broaden to the first match.
- `index_picker` two-range matcher enforces full clause coverage
(intermediate index property count must equal Equal/In prefix
field count) so extra prefix fields can't silently pick a
non-covering index.
- `aggregate_sum_path_query` + `aggregate_count_and_sum_path_query`
bind the range clause to the index's *terminator* property so
requests with multiple range-like clauses pick the right one.
- `range_clause_to_query_item` startsWith handler does proper
byte-wise carry propagation instead of single-byte increment,
so prefixes like `[0x12, 0xFF]` correctly produce `[0x13]` as
the exclusive upper bound. Only fails when every byte is 0xFF.
Smaller correctness fixes:
- `try_from_schema` no longer shadows the doctype `name` with the
loop variable in the cross-index mismatch error message.
- `set_range_summable` clamps unconditionally so the invariant
(`range_summable ⇒ documents_summable.is_some()`) always holds
after the setter returns.
- `Index::summable` parser rejects empty strings (`Value::Text("")`
no longer becomes `Some("")`).
Bench:
- `populate_fixture` now returns the actual inserted-grade count
(after the `is_enrolled` filter); `AvgBenchFixture` carries
`inserted_count` alongside `row_count` and Criterion throughput
+ `[proof-size]` headers report the real cardinality instead of
the walked-triple count (which overstates by ~30% at default
settings).
Doc tweaks:
- `document-sum-trees.md` opener: "two endpoints" → "unified
`GetDocumentsSum` endpoint" matching the rest of the chapter.
- `accessors/v2/mod.rs` `documents_summable` doc clarifies that
the doctype-level total-sum fast path is O(1), not O(log n)
(only per-key range sums are O(log n)).
Skipped (tracked as out-of-scope SDK fan-out follow-ups, called
out explicitly in the PR description's "Known follow-ups"):
- DocumentSum/DocumentSplitSums FromProof byte-plumbing
- bool→TreeType refactor on the index-walker delete path
- Stateless-cost sizing for ItemWithSumItem / ReferenceWithSumItem
(cost-estimation path; real write path uses actual grovedb sizing)
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
CI ran the fmt check on a merge ref that predated upstream 798cd25 (chore: apply rustfmt) by ~90 seconds. The persistence.rs:2313 diff CI reported was a v3.1-dev file, not from this branch.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
packages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rs (1)
313-327:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix stateless size accounting for summable primary storage elements.
Line 322, Line 457, and Line 591 still use plain-item sizing, and Line 603 still omits sum-item overhead in
target. Whendocuments_summableis set, this underestimates stateless costs/fees.Suggested patch direction
- Element::required_item_space( + if primary_key_sum_property.is_some() { + Element::required_item_with_sum_item_space( + *max_size, + STORAGE_FLAGS_SIZE, + &platform_version.drive.grove_version, + )? + } else { + Element::required_item_space( + *max_size, + STORAGE_FLAGS_SIZE, + &platform_version.drive.grove_version, + )? + } - target: QueryTargetValue(document_type.estimated_size(platform_version)? as u32), + target: QueryTargetValue( + document_type.estimated_size(platform_version)? as u32 + + if primary_key_sum_property.is_some() { 8 } else { 0 }, + ),#!/bin/bash # Verify all stateless sizing/target callsites in this file. rg -n "required_item_space|required_item_with_sum_item_space|QueryTargetValue\\(" \ packages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rsExpected result: all summable cost-only paths use sum-item-aware sizing, and stateless
targetincludes sum overhead whenprimary_key_sum_property.is_some().Also applies to: 451-462, 585-596, 602-604
🤖 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/insert/add_document_to_primary_storage/v0/mod.rs` around lines 313 - 327, The stateless sizing calls in add_document_to_primary_storage v0 are using plain-item sizing (Element::required_item_space) and omitting the sum-item overhead when documents_summable/primary_key_sum_property is present; update all such callsites (e.g., where PathKeyUnknownElementSize is constructed for contract_documents_keeping_history_primary_key_path_for_unknown_document_id and other QueryTargetValue/target computations) to use the sum-aware helper (Element::required_item_with_sum_item_space or the sum-aware sizing function used elsewhere) and include the sum-item overhead in the target computation when primary_key_sum_property.is_some(); find and replace required_item_space -> required_item_with_sum_item_space (or call the sum-aware API) for the code paths that depend on documents_summable and ensure QueryTargetValue/target additions include the extra sum-item bytes.
🤖 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 `@book/src/drive/document-sum-trees.md`:
- Around line 69-77: The match currently treats range_summable as always mapping
to ProvableSumTree and omits combined count+sum variants; update the selection
logic in the snippet that uses the tuple (range_summable, documents_summable,
range_countable, documents_countable) to include branches for combined sum+count
trees (e.g., when both summable and countable flags are true select the
appropriate combined TreeType such as ProvableSumCountTree or SumCountTree, and
when only documents are count+sum choose the non-range combined variant),
keeping the priority order explicit, and ensure you reference the existing
TreeType enum variants (or add new combined variants) or alternatively label
this snippet as "sum-only projection" if you intend not to add combined types.
In `@packages/rs-drive/benches/document_sum_worst_case.rs`:
- Around line 120-121: The fixture reuse key currently ignores the
platform/grovedb version: when creating or checking fixtures, include the
effective version instead of (or in addition to) the existing marker so cached
fixtures are version-aware; specifically, obtain PlatformVersion::latest() (or a
pinned PlatformVersion) and pass its identifying string/value into
fixture_marker(...) used around Drive::open (and the other similar fixture
checks in this file), so fixture creation and reuse compare the platform/grovedb
version as part of the cache key.
In
`@packages/rs-drive/src/drive/document/insert/add_reference_for_index_level_for_contract_operations/v0/mod.rs`:
- Around line 142-148: The stateless cost paths currently call
Element::required_item_space(...) even when index_type.summable.is_some(), so
the extra i64 sum (8 bytes) for ReferenceWithSumItem is not counted; at each
TODO site replace Element::required_item_space(...) with
Element::required_item_with_sum_item_space(...) when
index_type.summable.is_some() (i.e., branch or conditional that mirrors the
summable check used when creating ReferenceWithSumItem), keeping the same
arguments otherwise so the fee estimate includes the extra 8-byte sum; update
the three TODO locations that mention required_item_space to use
required_item_with_sum_item_space under the summable condition.
In `@packages/rs-drive/src/query/drive_document_sum_query/executors/total.rs`:
- Around line 89-121: The function read_primary_key_sum_tree currently maps a
missing SumTree element (None) to 0 which masks a mis-created/missing tree;
instead, if grove_get_raw_optional returns None you should return an Err
indicating the primary-key sum tree is missing/corrupt so callers fail fast;
update read_primary_key_sum_tree to check element.is_some() and return a
descriptive Error (referencing the primary-key sum tree for document_type_name
and contract_id) rather than mapping None to zero (do not change the existing
success path that calls e.sum_value_or_default()).
In `@packages/wasm-sdk/src/queries/document.rs`:
- Around line 646-650: The current scaffolded failure uses
WasmSdkError::generic(...) in the getDocumentsSum scaffold (and the similar
scaffold around lines 664-667); add and use a typed "not implemented" error
variant instead so JS can detect this case: add a
WasmSdkError::NotImplemented(String) (and a helper constructor like
WasmSdkError::not_implemented(msg)) to the WasmSdkError enum/impl, then replace
the two generic(...) usages in the get_documents_sum/getDocumentsSum scaffold
with WasmSdkError::not_implemented("getDocumentsSum") (or an appropriate
message) so the typed variant is returned consistently.
---
Duplicate comments:
In
`@packages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rs`:
- Around line 313-327: The stateless sizing calls in
add_document_to_primary_storage v0 are using plain-item sizing
(Element::required_item_space) and omitting the sum-item overhead when
documents_summable/primary_key_sum_property is present; update all such
callsites (e.g., where PathKeyUnknownElementSize is constructed for
contract_documents_keeping_history_primary_key_path_for_unknown_document_id and
other QueryTargetValue/target computations) to use the sum-aware helper
(Element::required_item_with_sum_item_space or the sum-aware sizing function
used elsewhere) and include the sum-item overhead in the target computation when
primary_key_sum_property.is_some(); find and replace required_item_space ->
required_item_with_sum_item_space (or call the sum-aware API) for the code paths
that depend on documents_summable and ensure QueryTargetValue/target additions
include the extra sum-item bytes.
🪄 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: 49b133e1-5b43-4208-8bdb-cd832e4bfcad
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (43)
book/src/drive/document-sum-trees.mdbook/src/drive/indexes.mdbook/src/drive/sum-index-examples.mdpackages/rs-dpp/Cargo.tomlpackages/rs-dpp/src/data_contract/document_type/accessors/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/index/mod.rspackages/rs-dpp/src/data_contract/document_type/index_level/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/accessors.rspackages/rs-drive-abci/Cargo.tomlpackages/rs-drive-abci/src/query/document_query/v1/mod.rspackages/rs-drive-proof-verifier/src/proof/document_split_sum.rspackages/rs-drive-proof-verifier/src/proof/document_sum.rspackages/rs-drive/Cargo.tomlpackages/rs-drive/benches/document_average_worst_case.rspackages/rs-drive/benches/document_sum_worst_case.rspackages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rspackages/rs-drive/src/drive/document/delete/remove_reference_for_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_reference_for_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/mod.rspackages/rs-drive/src/drive/document/primary_key_tree_type.rspackages/rs-drive/src/query/drive_document_sum_query/drive_dispatcher.rspackages/rs-drive/src/query/drive_document_sum_query/execute_range_sum.rspackages/rs-drive/src/query/drive_document_sum_query/executors/per_in_value.rspackages/rs-drive/src/query/drive_document_sum_query/executors/range_aggregate_carrier_proof.rspackages/rs-drive/src/query/drive_document_sum_query/executors/total.rspackages/rs-drive/src/query/drive_document_sum_query/grovedb_pr_670.rspackages/rs-drive/src/query/drive_document_sum_query/index_picker.rspackages/rs-drive/src/query/drive_document_sum_query/mod.rspackages/rs-drive/src/query/drive_document_sum_query/path_query.rspackages/rs-drive/src/verify/document_sum/mod.rspackages/rs-drive/src/verify/document_sum/verify_aggregate_count_and_sum_proof/mod.rspackages/rs-drive/src/verify/document_sum/verify_carrier_aggregate_count_and_sum_proof/mod.rspackages/rs-platform-version/Cargo.tomlpackages/rs-platform-wallet/Cargo.tomlpackages/rs-sdk-ffi/src/document/queries/mod.rspackages/rs-sdk-ffi/src/document/queries/sum.rspackages/rs-sdk-ffi/src/system/queries/path_elements.rspackages/rs-sdk/Cargo.tomlpackages/rs-sdk/src/mock/requests.rspackages/wasm-drive-verify/src/state_transition/state_transition_execution_path_queries/token_transition.rspackages/wasm-sdk/src/queries/document.rs
✅ Files skipped from review due to trivial changes (2)
- packages/rs-drive/src/query/drive_document_sum_query/grovedb_pr_670.rs
- book/src/drive/indexes.md
🚧 Files skipped from review as they are similar to previous changes (23)
- packages/rs-dpp/Cargo.toml
- packages/rs-platform-version/Cargo.toml
- packages/rs-sdk/Cargo.toml
- packages/rs-dpp/src/data_contract/document_type/v2/accessors.rs
- packages/rs-drive-proof-verifier/src/proof/document_sum.rs
- packages/rs-drive-proof-verifier/src/proof/document_split_sum.rs
- packages/rs-dpp/src/data_contract/document_type/index/mod.rs
- packages/rs-drive/src/verify/document_sum/mod.rs
- packages/rs-drive/src/verify/document_sum/verify_aggregate_count_and_sum_proof/mod.rs
- packages/rs-sdk-ffi/src/document/queries/sum.rs
- packages/rs-drive/src/verify/document_sum/verify_carrier_aggregate_count_and_sum_proof/mod.rs
- packages/rs-drive/src/query/drive_document_sum_query/executors/range_aggregate_carrier_proof.rs
- packages/rs-dpp/src/data_contract/document_type/accessors/v2/mod.rs
- packages/rs-drive/src/drive/document/primary_key_tree_type.rs
- packages/rs-drive/src/query/drive_document_sum_query/index_picker.rs
- packages/rs-drive/src/drive/document/delete/remove_reference_for_index_level_for_contract_operations/v0/mod.rs
- packages/rs-dpp/src/data_contract/document_type/index_level/mod.rs
- packages/rs-drive/src/query/drive_document_sum_query/drive_dispatcher.rs
- packages/rs-drive/src/query/drive_document_sum_query/executors/per_in_value.rs
- packages/rs-drive/src/query/drive_document_sum_query/mod.rs
- packages/rs-drive/src/query/drive_document_sum_query/execute_range_sum.rs
- packages/rs-drive/benches/document_average_worst_case.rs
- packages/rs-drive/src/query/drive_document_sum_query/path_query.rs
`drive-proof-verifier` (verify-only feature set) imports `drive::query::SumEntry`, but the re-export was gated `cfg(feature = "server")` only. The wasm-sdk JS build failed with E0432 unresolved import. Mirror the count-side split: shareable types (SumEntry/SumMode/DriveDocumentSumQuery) re-exported under either feature; server-only executor inputs (DocumentSumRequest/DocumentSumResponse/RangeSumOptions) stay server-gated.
- executors/total.rs: fail fast on missing primary-key sum tree
(insert_contract_operations_v0 creates [doctype, 0] unconditionally
for every applied document type with documents_summable; None means
corruption, not "fresh contract")
- benches/document_{sum,average}_worst_case.rs: include
PlatformVersion::latest().protocol_version in fixture_marker so
cached fixtures auto-invalidate across version bumps
- benches/document_count_worst_case.rs: add AggregateSumOnRange /
AggregateCountAndSumOnRange match arms in display_query_items
(newly-added grovedb PR 670 QueryItem variants)
- book/document-sum-trees.md: label tree-selection snippet as
"sum-only projection"; reference the combined count+sum branches
covered in "Choosing What to Set"
Skipped (tracked follow-ups):
- ReferenceWithSumItem / ItemWithSumItem stateless size accounting
via required_item_with_sum_item_space — helper not yet in grovedb.
- WasmSdkError::NotImplemented typed variant for scaffolded sum APIs
— minor cosmetic on code paths that already fail with a clear msg.
Address codecov/patch threshold miss with real unit tests on the genuinely testable code added in this PR: - drive_document_sum_query/tests.rs: 11 picker tests covering find_summable_index_for_where_clauses (strict coverage, property mismatch, In acceptance, range-operator rejection) and find_range_summable_index_for_where_clauses (terminator binding, prefix-Equal + terminator-range, property mismatch, non-range-summable rejection, range-not-on-terminator). - rs-dpp v2/mod.rs: 4 setter-invariant tests covering the set_range_summable normalization and set_documents_summable cascade-clear behavior added in this PR. - rs-dpp index/mod.rs: 4 summable-parser tests covering Some(string), null, empty-string rejection, and non-string rejection. drive lib 3165/3165, dpp lib 3461/3467 (6 ignored).
Contributor
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "d6d77eef539bf7d21c83f2cf41af2b204e0375d6eaf448452665534bde0e1bc0"
)Xcode manual integration:
|
Adds the SELECT AVG wire format end-to-end so callers can encode AVG queries today; execution returns NotYetImplemented in the same way SUM landed first as a wire-stable surface ahead of executor bodies. Both features unblock on the same grovedb PR 670 dependency (`AggregateCountAndSumOnRange` + PCPS element variant). Why no pre-computed average on the wire? The response carries the `(count, sum)` pair the underlying PCPS primitive yields in one root-hash-committed traversal, and the client divides. This preserves full precision and lets each caller pick its own representation — integer-truncated, floating-point, decimal. Pre-dividing on the server forces a choice that loses information for callers that wanted a different one. Changes: - proto: AverageEntry / AverageAggregate / AverageEntries / AverageResults messages + averages variant in ResultData (field #4). - rs-drive: drive_document_average_query module with DocumentAverageRequest/Response, AverageMode, AverageEntry types and a dispatcher stub returning NotYetImplemented. - rs-drive-abci: RoutingDecision::Average + dispatch_average_v1; SelectFunction::Avg now routes (was previously rejected at validation). - rs-drive-proof-verifier: DocumentAverage (count + sum) + helper `as_f64()`, DocumentSplitAverages + SplitAverageEntry + helper `into_flat_map() -> BTreeMap<key, (count, sum)>`. FromProof stubs return NotImplemented (same as DocumentSum / DocumentSplitSums). - rs-sdk: Fetch impls + MockResponse roundtrip for DocumentAverage / DocumentSplitAverages. - rs-sdk-ffi: dash_sdk_document_average extern "C" stub. - wasm-sdk: getDocumentsAverage + getDocumentsAverageWithProofInfo stubs returning typed not-implemented errors. drive 3165/3165, dpp 3461/3467 (6 ignored), drive-proof-verifier 225/225, drive-abci 2466/2475 (9 ignored), dash-sdk 117/123 (6 ignored). clippy --workspace --all-features -D warnings clean.
QuantumExplorer
commented
May 18, 2026
- document-meta v1: strip "Only effective from protocol version 12" / "Per-protocol-version 12+; rejected on earlier protocol versions" from every property description (lines 447, 453, 457, 535, 539, 545, 549). The top-level `$comment` already states the meta is v12+ as a whole; per-property restatements were redundant. - documentsCountable / rangeCountable / documentsSummable / rangeSummable: add a "rarely useful on its own" note steering authors toward per-index `countable` / `summable` / `rangeCountable` / `rangeSummable` instead. The primary-key flags only make sense when you genuinely need the unfiltered global total / sum.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/wasm-sdk/src/queries/document.rs (1)
646-650:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse typed not-implemented errors for all scaffolded aggregate WASM methods.
Line [646], Line [664], Line [705], and Line [724] still return
WasmSdkError::generic(...)even though these APIs are documented as typed not-implemented scaffolds. Please return a dedicated not-implemented variant so JS callers can branch reliably.Suggested patch
- Err(WasmSdkError::generic( - "getDocumentsSum — not yet wired through the wasm-sdk layer. \ - The rs-drive primitives are available; plumbing them up to the \ - browser-facing API is the pending SDK fan-out follow-up.", - )) + Err(WasmSdkError::not_implemented("getDocumentsSum")) @@ - Err(WasmSdkError::generic( - "getDocumentsSumWithProofInfo — not yet wired through the \ - wasm-sdk layer. See getDocumentsSum for the same note.", - )) + Err(WasmSdkError::not_implemented("getDocumentsSumWithProofInfo")) @@ - Err(WasmSdkError::generic( - "getDocumentsAverage — not yet wired through the wasm-sdk layer. \ - The rs-drive primitives are available; plumbing them up to the \ - browser-facing API is the pending SDK fan-out follow-up, same as \ - getDocumentsSum.", - )) + Err(WasmSdkError::not_implemented("getDocumentsAverage")) @@ - Err(WasmSdkError::generic( - "getDocumentsAverageWithProofInfo — not yet wired through the \ - wasm-sdk layer. See getDocumentsAverage for the same note.", - )) + Err(WasmSdkError::not_implemented("getDocumentsAverageWithProofInfo"))Also applies to: 664-667, 705-710, 724-727
🤖 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/wasm-sdk/src/queries/document.rs` around lines 646 - 650, Replace the generic error return used in the scaffolded aggregate WASM methods with a dedicated "not-implemented" WasmSdkError variant so JS callers can reliably branch; specifically, update the error returned in the getDocumentsSum scaffold (currently Err(WasmSdkError::generic(...))) and the other scaffolded aggregate methods at the other affected sites to return a typed not-implemented variant (e.g., WasmSdkError::not_implemented or a new WasmSdkError::scaffold_not_implemented) carrying a clear message that the API is intentionally unimplemented in the wasm-sdk layer; if the variant does not yet exist, add it to the WasmSdkError enum/impl and use it consistently across getDocumentsSum and the other scaffolded aggregate functions.
🤖 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/dapi-grpc/protos/platform/v0/platform.proto`:
- Around line 1098-1105: Update the AVG bullets in the query result comment to
mark them as scaffold-only (i.e., indicate they are not implemented/stubbed
fan-out) so generated docs don't advertise a working AVG surface; specifically
change the two lines referencing "select=AVG" and
"result.data.averages.aggregate_average"/"result.data.averages.entries" to note
"(scaffold-only)" or "not implemented / stubbed fan-out" while keeping the rest
of the list intact.
In `@packages/rs-dpp/schema/meta_schemas/document/v1/document-meta.json`:
- Around line 455-457: Add a JSON Schema conditional that enforces the
prerequisite when "rangeSummable" is true: use an "if" checking
properties.rangeSummable.const = true and a "then" that requires either
"summable" or "documentsSummable" (use a oneOf with required ["summable"] and
required ["documentsSummable"] to allow either). Apply the same conditional
guard for the other "rangeSummable" occurrence noted in the file so the schema
will reject objects with rangeSummable: true but missing the required
summable/documentsSummable flag.
---
Duplicate comments:
In `@packages/wasm-sdk/src/queries/document.rs`:
- Around line 646-650: Replace the generic error return used in the scaffolded
aggregate WASM methods with a dedicated "not-implemented" WasmSdkError variant
so JS callers can reliably branch; specifically, update the error returned in
the getDocumentsSum scaffold (currently Err(WasmSdkError::generic(...))) and the
other scaffolded aggregate methods at the other affected sites to return a typed
not-implemented variant (e.g., WasmSdkError::not_implemented or a new
WasmSdkError::scaffold_not_implemented) carrying a clear message that the API is
intentionally unimplemented in the wasm-sdk layer; if the variant does not yet
exist, add it to the WasmSdkError enum/impl and use it consistently across
getDocumentsSum and the other scaffolded aggregate functions.
🪄 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: daf4146d-f2ec-4f3b-8d91-b63f71949b52
📒 Files selected for processing (17)
packages/dapi-grpc/protos/platform/v0/platform.protopackages/rs-dpp/schema/meta_schemas/document/v1/document-meta.jsonpackages/rs-drive-abci/src/query/document_query/v1/mod.rspackages/rs-drive-proof-verifier/src/lib.rspackages/rs-drive-proof-verifier/src/proof.rspackages/rs-drive-proof-verifier/src/proof/document_average.rspackages/rs-drive-proof-verifier/src/proof/document_split_average.rspackages/rs-drive/src/query/drive_document_average_query/drive_dispatcher.rspackages/rs-drive/src/query/drive_document_average_query/mod.rspackages/rs-drive/src/query/mod.rspackages/rs-sdk-ffi/src/document/queries/average.rspackages/rs-sdk-ffi/src/document/queries/mod.rspackages/rs-sdk/src/mock/requests.rspackages/rs-sdk/src/platform/documents/document_average.rspackages/rs-sdk/src/platform/documents/document_split_averages.rspackages/rs-sdk/src/platform/documents/mod.rspackages/wasm-sdk/src/queries/document.rs
- proto: mark SUM / AVG wire-shape bullets explicitly "(scaffold-only)"
and add a paragraph clarifying the dispatcher returns
NotYetImplemented today, so generated docs don't advertise a working
surface.
- document-meta.json: add `dependentRequired` enforcing the
prerequisite at both levels:
* per-index: `rangeCountable` → `countable`, `rangeSummable` →
`summable`.
* top-level (doctype): `rangeSummable` → `documentsSummable`.
The top-level `rangeCountable` → `documentsCountable` guard is
omitted because the existing Rust parser auto-promotes
`documents_countable = documents_countable || range_countable` —
schema-level requirement would break the minimal `{rangeCountable:
true}` form that contracts use today.
- wasm-sdk: add `WasmSdkErrorKind::NotImplemented` + helper
`WasmSdkError::not_implemented(api_name)` and switch all four
scaffolded aggregate methods (getDocumentsSum,
getDocumentsSumWithProofInfo, getDocumentsAverage,
getDocumentsAverageWithProofInfo) from `generic` to `not_implemented`.
JS callers can now branch on `error.kind === "NotImplemented"`.
drive 3165/3165, dpp 3461/3467 (6 ignored), drive-proof-verifier
225/225. clippy --workspace --all-features -D warnings clean.
The previous commit applied the "rarely useful on its own" note to documentsCountable, rangeCountable, documentsSummable, AND rangeSummable for consistency. That was wrong — the user's review comment was specifically about `rangeSummable` (primary-key range-sum without a where-clause filter is unusual). The other three primary-key toggles are legitimate ways to opt in and don't need the steer. Reverts the "rarely useful" wording on documentsCountable, rangeCountable, documentsSummable; keeps it (and tightens the wording) on rangeSummable only.
Addresses Codex findings #2 (partial), #3 (full), #5 (full): ## #3 Sum aggregation overflow (P1) — full fix The proof and no-proof paths used inconsistent overflow semantics that were both wrong for a deterministic protocol: - `execute_range_sum.rs:113` used `saturating_add` (silent clamp) - `execute_point_lookup.rs:48` used iterator `.sum::<i64>()` (panics in debug, wraps in release) Replaced both with `checked_add` + a typed `QuerySyntaxError::Unsupported` error so an overflowed aggregate fails deterministically with a clear user-facing message at the same point in both paths. ## #5 Stale gRPC clients (P2) — full fix `yarn build` in `packages/dapi-grpc/` regenerates the static clients for web, Node.js, Objective-C, and Python (Java is service-stub only). All five now include `SumResults`, `AverageResults`, `sums`, `averages`, `aggregate_sum`, `aggregate_average`, and the entry types. The Rust client uses `tonic_prost_build` at compile time so it was never stale. ## #2 U64 summable values (P1) — partial fix The deeper issue (DPP accepts U64 summable; values > i64::MAX would silently overflow grovedb's i64 sum aggregator) needs either schema- inference restructuring or a document-level validator — neither small enough for this PR. The symptom is mitigated here: `read_document_sum_contribution` now returns `DriveError::InvalidInput` (user-facing) instead of `CorruptedCodeExecution` (internal corruption signal) on i64 conversion failure, with the property name and the underlying conversion error interpolated into the message. A user submitting a document whose sum-bearing property value exceeds i64::MAX now gets a clean "value cannot be represented as i64" error rather than an internal corruption error. The DPP-level rejection of U64 is tagged as a TODO with the design context preserved. drive 3165/3165, dpp 3461/3467 (6 ignored). clippy --workspace --all-features -D warnings clean. ## Skipped (tracked follow-ups) - #1 cost estimation paths still use `Element::required_item_space` — needs grovedb's `required_item_with_sum_item_space` helper which doesn't exist upstream yet. - #4 mixed count/sum shared-prefix indexes — needs a failing repro test to demonstrate the path is reachable from valid contracts.
…ue in cost estimation Bumps grovedb pin to develop head `e47626e96288b0052614cffd0451cefcf4b1103d` which lands grovedb#673 (`required_item_with_sum_item_space` + `required_reference_with_sum_item_space` helpers), then wires the new helpers at the per-element stateless-cost / dry-run estimation sites this PR added. ## Sites wired All three sites are v12+ gated (no v11 consensus baseline), so the switch from `required_item_space` → sum-aware helper is unconditional: - `add_document_to_primary_storage/v0/mod.rs:591` — `DocumentEstimatedAverageSize` arm now picks `required_item_with_sum_item_space` when `primary_key_sum_property.is_some()`. Also adds +10 to the `StatelessBatchInsert` target size for the same predicate so the full insert cost (element + target) reflects the i64 sum bytes. - `add_reference_for_index_level_for_contract_operations/v0/mod.rs:236` (non-unique) and `:280` (unique) — both `DocumentEstimatedAverageSize` arms now pick `required_reference_with_sum_item_space` when `sum_property_name.is_some()`. ## Tracked follow-ups (need either grovedb API extensions or version-gated workarounds) Codex finding #1 also called out two layer-level estimation sites that can't be cleanly fixed with the current grovedb API: - `address_funds/estimated_costs/for_address_balance_update/v0/mod.rs:92` uses `EstimatedLayerSizes::AllItems(key, value, flags)` — grovedb's enum has no `AllItemsWithSumItem` variant. The only workaround is bumping `AVERAGE_NONCE_SIZE + AVERAGE_BALANCE_SIZE` from 14 to 24 via v0/v1 dispatch (consensus-relevant; v11 grandfathered). - `add_estimation_costs_for_contract_insertion/v1/mod.rs:154` reports `sum_trees_weight: 0` / `count_sum_trees_weight: 0` because the surrounding loop only counts `range_countable` terminators — doesn't visit `range_summable` / `range_countable + range_summable` shapes. Both are tracked for a separate PR once we decide between extending grovedb's `EstimatedLayerSizes` enum or shipping the constant-bump workaround. drive 3165/3165, dpp 3461/3467 (6 ignored). clippy --workspace --all-features -D warnings clean.
Bumps grovedb pin to #674 head `49e17ce47744915878fcf1ef49de916e9d3ddde3` and wires the new `EstimatedLayerSizes::AllItemsWithSumItem` / `AllReferencesWithSumItem` variants + the four `provable_*_trees_weight` fields on `EstimatedSumTrees::SomeSumTrees` through dash-platform's estimation paths. ## Address-funds estimation (v0/v1 split — consensus-safe) `for_address_balance_update` now dispatches on `drive_version.methods.address_funds.cost_estimation.for_address_balance_update`: - **v0** (unchanged, locked to protocol v11 production): uses `EstimatedLayerSizes::AllItems(...)` for the CLEAR_ADDRESS_POOL leaf layer. Undercharges by ~10 bytes per insert (the i64 sum_value on `ItemWithSumItem` isn't accounted for), but the undercharge is consensus-locked. - **v1** (active at protocol v12+): switches the leaf layer to `AllItemsWithSumItem(...)` so dry-run fees match applied fees. Method-version table: new `DRIVE_ADDRESS_FUNDS_METHOD_VERSIONS_V2` bumps `for_address_balance_update: 0 → 1`. Wired into `DRIVE_VERSION_V7` (protocol v12). V1 table (`for_address_balance_update: 0`) stays selected for v11 (`DRIVE_VERSION_V6`). ## Contract-insertion estimation refactor (v12-only, no consensus risk) `add_estimation_costs_for_contract_insertion_v1` previously tallied all count-bearing children (CountTree + ProvableCountTree) into a single `count_trees_weight`. With grovedb #674's finer-grained weights, the loop now: - Categorizes each doctype-children tree via `property_name_tree_type_from_flags` (a 9-way matrix matching the selection in `add_indices_for_index_level_for_contract_operations`). - Tallies into a `TreeTypeWeights` struct keyed by every TreeType variant the contract apply path can write. - Maps the tally to `SomeSumTrees` with the right weight per slot — including `provable_sum_trees_weight`, `provable_count_sum_trees_weight`, and `provable_count_provable_sum_trees_weight` for the v12 sum-tree feature's `range_summable` / `range_summable + range_countable` indexes. Existing test `range_countable_index_contract_counts_both_pk_and_index_as_count_children` updated to reflect the new split: CountTree primary-key counts as `count_trees_weight: 1`, ProvableCountTree index counts as `provable_count_trees_weight: 1` (was previously both lumped into `count_trees_weight: 2`). ## Per-element estimation already wired The three v12-only doc-storage sites continue to use the per-element sum-aware helpers from grovedb #673 ( `required_item_with_sum_item_space` / `required_reference_with_sum_item_space`) added in the previous commit. ## Snapshot updates Two v12 fee snapshots updated for the +99_260 processing-fee shift that grovedb #674's new cost formula introduces: - `identity::balance::update::should_add_to_balance_latest_version_estimated`: 4_278_840 → 4_378_100. v1 variant (`should_add_to_balance_first_version_estimated`) still pins 4_278_840 — grovedb #674's v0/v1 formulas are byte-stable. - `tokens::balance::update::should_estimate_costs_without_state`: same delta, same explanation. ## Struct-field plumbing All 25 construction sites of `EstimatedSumTrees::SomeSumTrees` and `EstimatedLayerSizes::Mix` in rs-drive updated to fill the new fields with `0` / `None` respectively (preserving today's behavior since no caller has data to populate them yet). drive 3165/3165, dpp 3461/3467 (6 ignored), drive-abci 2466/2475 (9 ignored). clippy `--workspace --all-features -D warnings` clean.
Bumps grovedb pin from #674's PR-branch SHA `49e17ce47744915878fcf1ef49de916e9d3ddde3` → develop head `ad2492dcdc869a1452b0b10fbed8f9b0de1634c6` which is the merged form plus the follow-up Mix-arm fixes CodeRabbit caught during review: - `value_with_feature_and_flags_size` Mix arm: pre-existing bug where mixed layers averaged `Σ size_i / Σ weight_i` instead of the proper weighted `Σ (size_i · weight_i) / Σ weight_i`. New v1 formula at grove v3+ (v0/v1 stay byte-stable for v11/v12 consensus baseline). - `add_average_case_merk_propagate` Mix arms: pre-existing `cost / nodes_updated²` underestimate. New v2 formula at grove v3+ matches the homogeneous `AllItems` / `AllSubtrees` arms; v1 stays byte-stable for grove v2 (platform v12 production cost basis). - Macro hygiene fix on `check_grovedb_v0_v1_or_v2!` and a v0 divisor guard for `EstimatedSumTrees::estimated_size`. - Refactor: per-function dispatchers split into per-version files (dash-platform-style layout). No behavior change. drive 3165/3165, dpp 3461/3467 (6 ignored). clippy clean. No additional dash-platform fee snapshot shifts — the Mix arms aren't hit by the identity / token balance estimation paths the previous commit pinned.
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Wire grovedb's sum-tree and dual-axis (count+sum) proof primitives — landed in grovedb#670, now merged to grovedb develop (head
e98bab5f) — through the full Drive → SDK stack. Builds reproducible benchmarks against two contracts (tip-jar for pure-sum, grades for combined count+sum/averages) and lands the worked-example chapters they populate.What was done?
This PR is broader than the original commit's title suggested — it includes the schema-layer work, the contract-creation write-side, the read-side query family, the SDK fan-out scaffolds, the two benches, and the chapter pipeline:
1. DPP schema + DataContract layer
documentsSummable: \"<prop>\", per-indexsummable: \"<prop>\", andrangeSummable: true(both per-index and at the document-type level).Index::summable/Index::range_summablefields with v2 accessors + the matchingDocumentTypeV2Gettersextensions.IndexLevel::find_first_summability_changemirrors the count-side change-detection logic for sum-axis flag transitions.2. Drive write side (contract apply + document insert + delete)
SumTree,ProvableSumTree,CountSumTree,ProvableCountSumTree, andProvableCountProvableSumTree(PCPS) for combined per-node count+sum.batch_insert_empty_provable_sum_tree,batch_insert_empty_provable_count_sum_tree,batch_insert_empty_provable_count_provable_sum_treeoperations;for_known_path_key_empty_tree_under_aggregating_parentselects the rightNotCounted/NotSummed/NotCountedOrSummedwrapper based on the parent's TreeType.Element::ItemWithSumItemat primary storage andElement::ReferenceWithSumItemat index references whensummableis set, propagating per-doc sum contributions up to ancestor sum trees.make_document_reference_with_sum_item+read_document_sum_contributionextract the sum value from the document.remove_indices_for_*andremove_reference_for_*.3. Drive read side —
DriveDocumentSumQueryfamilypath_query.rs(instance + static): point-lookup, aggregate-range, distinct-range, and carrier path-query builders for both pure-sum and combined count+sum (PCPS).execute_point_lookup.rs/execute_range_sum.rs: prove + no-prove executors calling the matching grovedb primitives (verify_query,verify_aggregate_sum_query,verify_aggregate_sum_query_per_key,verify_aggregate_count_and_sum_query,verify_aggregate_count_and_sum_query_per_key).executors/*modules (per-mode dispatch):point_lookup_proof,range_proof,range_no_proof,range_distinct_proof,range_aggregate_carrier_proof,per_in_value,total.drive_dispatcher.rsmode-detection + request routing mirroring count's dispatcher.index_picker.rsresolves which summable / rangeSummable index covers a given where-clause shape.4. Drive verifier surface
verify/document_sum/module tree with verifiers for leaf-PCPS (verify_aggregate_count_and_sum_proof) and both carrier shapes (verify_carrier_aggregate_sum_proof,verify_carrier_aggregate_count_and_sum_proof).DriveVerifyDocumentSumMethodVersionsregistering all three v0 verifiers.5. SDK fan-out scaffolds (rs-sdk / rs-sdk-ffi / wasm-sdk / drive-abci)
rs-sdk:DocumentSum+DocumentSplitSumstypes withFromProofimpls (currently return typed errors pending follow-up plumbing) +MockResponseimpls for round-tripping in test fixtures.rs-sdk-ffi:dash_sdk_document_sumC entry-point scaffolded;path_elements.rsextended to format the newElement::ProvableCountProvableSumTreevariant.wasm-sdk:getDocumentsSum/getDocumentsSumWithProofInfobrowser entry-points scaffolded.drive-abci:query_documents_sum_v1routing in place;dispatch_sum_v1is scaffolded (returns typednot_yet_implementeduntil the SDK-fan-out follow-up lands the actual byte plumbing).wasm-drive-verify:AggregateCountAndSumOnRangeQueryItemvariant covered in the match exhaustiveness.6. Two reproducible benches
packages/rs-drive/benches/document_sum_worst_case.rs— 100 000-row tip-jar fixture covering Q1-Q9 (primary-key total + point lookup + In-grouped + range + compound + carrier).packages/rs-drive/benches/document_average_worst_case.rs— 50 000-triple grades-contract fixture (filtered to 31 620 actual grades via deterministic per-class enrollment popularity, 500 students × 10 classes × 10 semesters). Three-axis realistic score model: per-class(mean, spread)profile, deterministic per-student skill via FNV-1a hash + sum-of-three-uniforms CLT bell distribution, per-grade noise. Covers Q1-Q7 including PCPS-carrier.7. Two book chapters
book/src/drive/sum-index-examples.md— 9 worked queries against the tip-jar contract with proof_size, median µs,proof_ast<details>blocks (with visualizer-widget URLs encoded via gzip + base64url of the AST), per-layer mermaid diagrams.book/src/drive/average-index-examples.md— 7 worked queries against the grades contract, same format. Independent Python validation reproduces every per-bucket(count, sum)byte-for-byte.8. Grovedb dep bump
Cargo.tomlfiles now pin grovedb to develop heade98bab5f(PR feat(dpp): AbstractConsensusError tests and extensions #670 has merged). All workspace tests + clippy-D warningspass at the new pin.How Has This Been Tested?
cargo check -p drive— clean (bothserverandverifyfeatures)cargo clippy --workspace --all-features --locked -- --no-deps -D warnings— clean across the entire workspacecargo test -p drive --lib— 3 154 / 3 154 pass (5 new tests from the sum + average feature; no regressions)cargo test -p dpp --lib— 3 453 / 3 453 passcargo bench -p drive --bench document_sum_worst_case --no-run— buildscargo bench -p drive --bench document_average_worst_case --no-run— buildsDASH_PLATFORM_SUM_BENCH_REBUILD=1 cargo bench -p drive --bench document_sum_worst_case -- --test— Q1–Q9 emit reproducible[proof-size],[display]ASTs,[matrix]rowsDASH_PLATFORM_AVERAGE_BENCH_REBUILD=1 cargo bench -p drive --bench document_average_worst_case -- --test— 31 620 grades from 50 000 possible triples; Q1–Q7 verified end-to-end against shared root hash85e5b9e8…847e6is_enrolled/student_skill/grade_noise/compute_scorereproduces every per-bucket count + sum for Q2–Q7 byte-for-bytecd book && mdbook build— cleanBreaking Changes
None. The feature is purely additive: pre-existing contracts without
documentsSummable/summable/rangeSummableflags produce bit-identical grovedb layouts to what they produced before this change. The DPP schema additions are optional fields withadditionalProperties: falsealready covering rejection of unknown flags. The new drive method versions are registered as v0 (initial), and platform version v12 already gates the count side, so the dispatcher arms only activate on v12+.Known follow-ups (out of scope for this PR)
distinct_sum_path_queryport — ~280-line port deferred; blocks theGroupByRangemode + the 4th criterion case on the sum bench.documentsCountable + documentsSummableprimary-key tree promotion — the doctype root doesn't promote toCountSumTreeat insert time. The read path is correct (proof verifies, 614 B); only the stored(count, sum)is zero. Surfaces in Q1 of both the sum + averages chapters with explicit known-bug callouts.rangeSummablepromotion — same write path. Compound indexes (e.g.byClassSemester) work correctly; top-level singles (e.g.bySentAt) don't get promoted toProvableSumTree.rs-sdk/rs-sdk-ffi/wasm-sdk/drive-abciall have the surface scaffolded but return typednot_yet_implemented/DriveErrorfrom their bodies. Filling those bodies in is the natural next PR; the rs-drive primitives they'll call are all wired up here.Checklist:
Summary by CodeRabbit
New Features
Documentation
Tests
Chores