feat(drive): document sum + average proof primitives, with SDK fan-out scaffolding and reproducible benchmarks#3661
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) <noreply@anthropic.com>
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-05-19T23:16:25.992Z |
|
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)
|
|
✅ Review complete (commit 40e96bb) Review: #3661 (review) |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3661 +/- ##
============================================
- Coverage 88.06% 87.19% -0.88%
============================================
Files 2521 2580 +59
Lines 308995 314202 +5207
============================================
+ Hits 272122 273953 +1831
- Misses 36873 40249 +3376
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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.
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).
|
✅ 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: "873aaac9e42623db2a0f4187d2e10e99bbd46ecafe1f6f0e6462b846e040a41b"
)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.
- 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.
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.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Two blocking regressions in the update path: the function only handles plain Element::Item on reload (it fails for summable doctypes whose primary storage is now Element::ItemWithSumItem), and it rewrites all index entries with plain Element::Reference (stripping the ReferenceWithSumItem contributions the insert path wrote). A third issue — DPP accepting U64 as a summable property type while grovedb aggregates as i64 — is acknowledged in-tree as a deferred TODO but leaves a path from valid contract+document through to a non-consensus DriveError::InvalidInput at apply time. Several smaller cost-estimation drifts and defense-in-depth concerns round out the review.
Reviewed commit: 820cc4b
🔴 3 blocking | 🟡 4 suggestion(s) | 💬 1 nitpick(s)
8 additional findings
🔴 blocking: Update path rejects summable primary storage rows (`Element::ItemWithSumItem`)
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs (lines 152-165)
add_document_to_primary_storage now writes Element::ItemWithSumItem for any doctype with documents_summable: Some(_) (see add_document_to_primary_storage/v0/mod.rs:290, :393, :412, :437, :474, :501, :520, :546, :571). The reload here only matches Element::Item(...) and returns DriveError::CorruptedDocumentNotItem("old document is not an item") for anything else. Every update of a mutable document on a summable doctype will hit this error before any index work happens, so updates are unconditionally broken for summable mutable doctypes. Extend the match (and the documents_keep_history reload at lines 121-128) to also accept Element::ItemWithSumItem(bytes, _sum_value, flags) and feed bytes / flags into the same Document::from_bytes / map_some_element_flags_ref path.
🔴 blocking: Update path rewrites summable index entries as plain references, breaking SUM/AVG proof soundness
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs (lines 105-109)
The insert path explicitly branches on index_type.summable and emits Element::ReferenceWithSumItem for summable indexes so the per-document sum propagates into ancestor sum trees (insert/add_reference_for_index_level_for_contract_operations/v0/mod.rs:118-141, make_terminal_ref). The update path here builds a single Element::Reference via make_document_reference and reuses it for every reinsertion and refresh below (batch_insert at line 423-428, batch_insert_if_not_exists at line 435-441, batch_refresh_reference at lines 460-467 and 469-476). For any summable index this either re-inserts a non-summing reference after a key change or refreshes the existing ReferenceWithSumItem in place with a non-summing payload — in both cases the document's sum contribution disappears from ancestor sum trees while the document body remains queryable. This breaks proof soundness: a light client that verifies a SUM/AVG aggregate proof after an unrelated document update will accept an aggregate that excludes that document. An attacker who owns a document on a summable contract can intentionally trigger this with any benign no-op update. Mirror the insert dispatch: branch on index.summable (and index.range_summable) and construct make_document_reference_with_sum_item(...) when the index is sum-bearing, reading the sum value from the new document body via read_document_sum_contribution.
🔴 blocking: U64 summable property values > i64::MAX produce a non-consensus Drive error at apply
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs (lines 200-230)
The validator at lines 212-222 accepts U64 (and U32 / U16 / U8) as integer types for a summable property, but grovedb's SumValue = i64 — read_document_sum_contribution (packages/rs-drive/src/drive/document/mod.rs:185) calls value.to_integer::<i64>() and returns Error::Drive(DriveError::InvalidInput(...)) on any unsigned value above i64::MAX. A contract author can publish a contract whose summable property is JSON-schema type: integer with no maximum (DPP infers U64), and a user can submit a document with that property set to u64::MAX. The state transition passes DPP validation, passes basic + advanced structure validation, and only fails at apply time inside Drive with an internal DriveError::InvalidInput rather than a typed ConsensusError. The author has tagged this as a follow-up TODO at lines 200-211 and remapped the error tag from CorruptedCodeExecution to InvalidInput, but the underlying contract surface is still internally inconsistent — a contract that passes DPP can deterministically fail at Drive. Since protocol v12 is not yet activated, close the gap pre-merge with one of: (a) reject U64 from the summable-type allow-list, (b) preserve the schema's maximum on the inferred property type and require max <= i64::MAX here when summable is set, or (c) add a per-document validator that rejects summable values above i64::MAX before they reach apply.
🟡 suggestion: Two `DocumentEstimatedAverageSize` branches under-charge for summable inserts
packages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rs (lines 313-328)
When primary_key_sum_property.is_some() applied inserts store Element::ItemWithSumItem(...) (~10 worst-case varint bytes for the i64 sum_value over plain Element::Item). Only the trailing else arm (lines 585-606) accounts for this by branching to Element::required_item_with_sum_item_space when primary_key_sum_property.is_some(). Both the documents_keep_history() arm here (lines 313-328) and the insert_without_check arm (lines 451-462) call Element::required_item_space(*max_size, STORAGE_FLAGS_SIZE, ...) unconditionally. For doctypes with documentsKeepsHistory: true && documentsSummable: "<x>" or any summable doctype routed through the insert_without_check path, dry-run cost estimation under-charges by ~10 bytes/insert, so check_tx / prove=false fee pre-checks can succeed for inserts that fail at apply with insufficient funds. Fix mirrors the trailing-else arm: branch the element size on primary_key_sum_property.is_some().
🟡 suggestion: Delete-side cost estimation can't represent sum-bearing layer variants
packages/rs-drive/src/drive/document/delete/remove_indices_for_index_level_for_contract_operations/v0/mod.rs (lines 33-51)
The parent_value_tree_is_range_countable bool input cannot represent the four count×sum-axis combinations the insert side produces (SumTree / ProvableSumTree / CountSumTree / ProvableCountSumTree / ProvableCountProvableSumTree). The author's docstring is explicit that the actual delete operations remain correct (they compose flags via remove_reference_for_index_level_for_contract_operations's nine-arm dispatch), but the EstimatedLayerInformation this walker emits for cost estimation under-charges sum-bearing layers by a few bytes per affected node. Per the project rule that fee estimation and actual execution follow the same code paths, this is a fee-estimation drift between dry-run delete and applied delete on summable doctypes. Already noted as a focused follow-up in the comment; flagged for tracking.
🟡 suggestion: Structural summable invariants only enforced under `full_validation`
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs (lines 162-242)
The cross-validation at lines 162-242 is gated entirely behind if full_validation { ... }. The three checks inside — (1) all per-index summable declarations on a doctype name the same property, (2) the canonical summable property exists and is an integer type, (3) the property is in required — are structural invariants of the on-disk grovedb sum-tree layout, not optional schema-quality checks. If any future code path calls try_from_schema_v2 with full_validation: false on an attacker-controlled contract, mixed-property summables or non-required summable fields would silently corrupt on-disk sum aggregations. Today all production call sites pass true (verified at drive-abci/.../data_contract_create/mod.rs:3918+), so this is defense-in-depth, but the same gating pattern means a future caller could introduce the gap without obviously breaking tests. Consider lifting the mixed-property check and required-field check outside the full_validation block.
🟡 suggestion: Five-fold duplication of `primary_key_sum_property` element-construction match
packages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rs (lines 192-616)
Across the three top-level branches in this function (keeping-history, insert_without_check, and trailing else), the same match &primary_key_sum_property { Some(prop) => Element::new_item_with_sum_item_with_flags(...), None => Element::Item(...) } pattern is open-coded five times — once per DocumentInfo variant. The function grew from ~210 lines pre-PR to ~560 lines almost entirely from this duplication, and the missed sum-awareness in two DocumentEstimatedAverageSize arms (separate finding) is a direct symptom: a small helper fn build_primary_element(serialized: Vec<u8>, document: &Document, prop: Option<&str>, flags) -> Result<Element, Error> would have made the keep-history and insert_without_check arms impossible to forget. Not blocking — flagged for follow-up to lower future drift surface.
💬 nitpick: `find_first_summability_change` returns placeholder string without property name
packages/rs-dpp/src/data_contract/document_type/index_level/mod.rs (lines 290-314)
When a summability axis changes at the current IndexLevel, this returns a static "(summable changed)" / "(range_summable changed)" with no information about which property changed (old → new). The parallel find_first_countability_change has the same shape, so it matches the existing style, but including the from/to summable values would make this consensus-rejection error actionable for clients without forcing a schema diff. Low-impact and worth flagging for parity if/when the count-side analog is improved.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs`:
- [BLOCKING] lines 152-165: Update path rejects summable primary storage rows (`Element::ItemWithSumItem`)
`add_document_to_primary_storage` now writes `Element::ItemWithSumItem` for any doctype with `documents_summable: Some(_)` (see `add_document_to_primary_storage/v0/mod.rs:290`, `:393`, `:412`, `:437`, `:474`, `:501`, `:520`, `:546`, `:571`). The reload here only matches `Element::Item(...)` and returns `DriveError::CorruptedDocumentNotItem("old document is not an item")` for anything else. Every update of a mutable document on a summable doctype will hit this error before any index work happens, so updates are unconditionally broken for summable mutable doctypes. Extend the match (and the `documents_keep_history` reload at lines 121-128) to also accept `Element::ItemWithSumItem(bytes, _sum_value, flags)` and feed `bytes` / `flags` into the same `Document::from_bytes` / `map_some_element_flags_ref` path.
- [BLOCKING] lines 105-109: Update path rewrites summable index entries as plain references, breaking SUM/AVG proof soundness
The insert path explicitly branches on `index_type.summable` and emits `Element::ReferenceWithSumItem` for summable indexes so the per-document sum propagates into ancestor sum trees (`insert/add_reference_for_index_level_for_contract_operations/v0/mod.rs:118-141`, `make_terminal_ref`). The update path here builds a single `Element::Reference` via `make_document_reference` and reuses it for every reinsertion and refresh below (`batch_insert` at line 423-428, `batch_insert_if_not_exists` at line 435-441, `batch_refresh_reference` at lines 460-467 and 469-476). For any summable index this either re-inserts a non-summing reference after a key change or refreshes the existing `ReferenceWithSumItem` in place with a non-summing payload — in both cases the document's sum contribution disappears from ancestor sum trees while the document body remains queryable. This breaks proof soundness: a light client that verifies a SUM/AVG aggregate proof after an unrelated document update will accept an aggregate that excludes that document. An attacker who owns a document on a summable contract can intentionally trigger this with any benign no-op update. Mirror the insert dispatch: branch on `index.summable` (and `index.range_summable`) and construct `make_document_reference_with_sum_item(...)` when the index is sum-bearing, reading the sum value from the new document body via `read_document_sum_contribution`.
In `packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs`:
- [BLOCKING] lines 200-230: U64 summable property values > i64::MAX produce a non-consensus Drive error at apply
The validator at lines 212-222 accepts `U64` (and `U32` / `U16` / `U8`) as integer types for a summable property, but grovedb's `SumValue = i64` — `read_document_sum_contribution` (`packages/rs-drive/src/drive/document/mod.rs:185`) calls `value.to_integer::<i64>()` and returns `Error::Drive(DriveError::InvalidInput(...))` on any unsigned value above `i64::MAX`. A contract author can publish a contract whose summable property is JSON-schema `type: integer` with no `maximum` (DPP infers `U64`), and a user can submit a document with that property set to `u64::MAX`. The state transition passes DPP validation, passes basic + advanced structure validation, and only fails at apply time inside Drive with an internal `DriveError::InvalidInput` rather than a typed `ConsensusError`. The author has tagged this as a follow-up TODO at lines 200-211 and remapped the error tag from `CorruptedCodeExecution` to `InvalidInput`, but the underlying contract surface is still internally inconsistent — a contract that passes DPP can deterministically fail at Drive. Since protocol v12 is not yet activated, close the gap pre-merge with one of: (a) reject U64 from the summable-type allow-list, (b) preserve the schema's `maximum` on the inferred property type and require `max <= i64::MAX` here when summable is set, or (c) add a per-document validator that rejects summable values above i64::MAX before they reach apply.
- [SUGGESTION] lines 162-242: Structural summable invariants only enforced under `full_validation`
The cross-validation at lines 162-242 is gated entirely behind `if full_validation { ... }`. The three checks inside — (1) all per-index `summable` declarations on a doctype name the same property, (2) the canonical summable property exists and is an integer type, (3) the property is in `required` — are structural invariants of the on-disk grovedb sum-tree layout, not optional schema-quality checks. If any future code path calls `try_from_schema_v2` with `full_validation: false` on an attacker-controlled contract, mixed-property summables or non-required summable fields would silently corrupt on-disk sum aggregations. Today all production call sites pass `true` (verified at `drive-abci/.../data_contract_create/mod.rs:3918+`), so this is defense-in-depth, but the same gating pattern means a future caller could introduce the gap without obviously breaking tests. Consider lifting the mixed-property check and required-field check outside the `full_validation` block.
In `packages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rs`:
- [SUGGESTION] lines 313-328: Two `DocumentEstimatedAverageSize` branches under-charge for summable inserts
When `primary_key_sum_property.is_some()` applied inserts store `Element::ItemWithSumItem(...)` (~10 worst-case varint bytes for the `i64` sum_value over plain `Element::Item`). Only the trailing `else` arm (lines 585-606) accounts for this by branching to `Element::required_item_with_sum_item_space` when `primary_key_sum_property.is_some()`. Both the `documents_keep_history()` arm here (lines 313-328) and the `insert_without_check` arm (lines 451-462) call `Element::required_item_space(*max_size, STORAGE_FLAGS_SIZE, ...)` unconditionally. For doctypes with `documentsKeepsHistory: true && documentsSummable: "<x>"` or any summable doctype routed through the `insert_without_check` path, dry-run cost estimation under-charges by ~10 bytes/insert, so `check_tx` / `prove=false` fee pre-checks can succeed for inserts that fail at apply with insufficient funds. Fix mirrors the trailing-else arm: branch the element size on `primary_key_sum_property.is_some()`.
- [SUGGESTION] lines 192-616: Five-fold duplication of `primary_key_sum_property` element-construction match
Across the three top-level branches in this function (keeping-history, `insert_without_check`, and trailing `else`), the same `match &primary_key_sum_property { Some(prop) => Element::new_item_with_sum_item_with_flags(...), None => Element::Item(...) }` pattern is open-coded five times — once per `DocumentInfo` variant. The function grew from ~210 lines pre-PR to ~560 lines almost entirely from this duplication, and the missed sum-awareness in two `DocumentEstimatedAverageSize` arms (separate finding) is a direct symptom: a small helper `fn build_primary_element(serialized: Vec<u8>, document: &Document, prop: Option<&str>, flags) -> Result<Element, Error>` would have made the keep-history and insert_without_check arms impossible to forget. Not blocking — flagged for follow-up to lower future drift surface.
In `packages/rs-drive/src/drive/document/delete/remove_indices_for_index_level_for_contract_operations/v0/mod.rs`:
- [SUGGESTION] lines 33-51: Delete-side cost estimation can't represent sum-bearing layer variants
The `parent_value_tree_is_range_countable` bool input cannot represent the four count×sum-axis combinations the insert side produces (SumTree / ProvableSumTree / CountSumTree / ProvableCountSumTree / ProvableCountProvableSumTree). The author's docstring is explicit that the actual delete operations remain correct (they compose flags via `remove_reference_for_index_level_for_contract_operations`'s nine-arm dispatch), but the `EstimatedLayerInformation` this walker emits for cost estimation under-charges sum-bearing layers by a few bytes per affected node. Per the project rule that fee estimation and actual execution follow the same code paths, this is a fee-estimation drift between dry-run delete and applied delete on summable doctypes. Already noted as a focused follow-up in the comment; flagged for tracking.
Note: GitHub refused the PR diff API for this large PR (HTTP 406: diff exceeded 20,000 lines), so I posted the verified findings in the top-level review body instead of inline comments.
Three blocking regressions called out by thepastaclaw's review on PR #3661 + one closely-related estimation suggestion. ## #1 (blocking) Update path now accepts ItemWithSumItem on reload `add_document_to_primary_storage` writes `Element::ItemWithSumItem` for any doctype with `documents_summable: Some(_)` (11 write sites). The update path's reload at `update_document_for_contract_operations/v0/mod.rs:152` only matched plain `Element::Item(...)` and rejected anything else with `CorruptedDocumentNotItem`. Every update of a mutable document on a summable doctype hit this error before any index work happened — updates were unconditionally broken for summable mutable doctypes. Extended the reload to accept BOTH variants. The sum_value is discarded on reload because the new write below re-computes it from the freshly-supplied document. ## #2 (blocking, SOUNDNESS) Update path emits ReferenceWithSumItem for summable indexes The insert path branches on `index.summable` and emits `Element::ReferenceWithSumItem` for sum-bearing indexes so the per-document sum propagates into ancestor sum trees. The update path built one `Element::Reference` before the index loop and reused it for every `batch_insert` / `batch_insert_if_not_exists` / `batch_refresh_reference` call. For any summable index, an update would overwrite the existing `ReferenceWithSumItem` with a plain `Reference`, silently dropping the document's sum contribution from ancestor aggregations while the document body remained queryable. This is a SUM/AVG proof-soundness bug: a light client verifying an aggregate proof after an unrelated document update would accept an aggregate that excludes that document. An attacker who owned a document on a summable contract could trigger it with any benign no-op update. Fix: mirror the insert dispatch — build a per-index reference variant inside the loop. Summable indexes call `make_document_reference_with_sum_item` after reading the sum value via `read_document_sum_contribution`; non-summable indexes reuse the plain reference built once above. Both helpers promoted from `fn` to `pub(crate) fn` so the update path can call them. ## #3 (blocking) U64 rejected as summable property type DPP previously accepted U64 in the summable type allow-list. A contract with `type: integer, minimum: 1, no maximum` would infer U64, pass DPP validation, and a document with that property set to u64::MAX would fail at Drive apply time with `DriveError::InvalidInput` rather than a typed `ConsensusError` — a contract that passes DPP could deterministically fail at Drive. Fix: drop U64 from the summable accept-list in `try_from_schema/v2/mod.rs`. Accepted types are now I64 + I32/U32 + I16/U16 + I8/U8 — the set that fits losslessly in grovedb's i64 sum value. Authors who want positive-only integers as summable must pick u8/u16/u32 or set an explicit `maximum`. (The Codex-flagged TODO at this site is removed; the inference-restructuring follow-up remains tracked separately.) Two test contracts updated to add explicit `maximum: u32::MAX` so the inference picks U32 (an accepted summable type): - `tests/supporting_files/contract/tip-jar/tip-jar-contract.json` - `benches/document_sum_worst_case.rs` inlined fixture The grades contract's `score: {min: 0, max: 100}` already infers U8 and was unaffected. ## #4 (suggestion) keep-history + insert_without_check estimation arms Two of three `DocumentEstimatedAverageSize` arms in `add_document_to_primary_storage/v0` called `Element::required_item_space` unconditionally — under-charging keep-history-mutable and `insert_without_check`-routed summable inserts by ~10 bytes/insert. Both arms now branch on `primary_key_sum_property.is_some()` to match the trailing-else arm fixed earlier. drive 3165/3165, dpp 3461/3467 (6 ignored). clippy `--workspace --all-features -D warnings` clean. ## Not addressed (smaller suggestions / tracked follow-ups) - Suggestion: delete-side cost estimation can't represent sum-bearing layer variants. Already docstring-flagged as a follow-up in tree; needs a larger refactor to plumb the parent's count×sum axis combo through `remove_indices_for_index_level_for_contract_operations`. - Suggestion: structural summable invariants gated behind `full_validation`. Production call sites all pass true; defense-in- depth move worth doing but doesn't change observable behavior. - Suggestion: helper to dedupe the five-fold `primary_key_sum_property` element-construction match. Refactor with no behavior change; not in this commit. - Nitpick: `find_first_summability_change` placeholder string.
…okup arm) `execute_document_average_prove`'s point-lookup arm only matched `AverageMode::Aggregate`, so a no-range `GroupByIn` AVG + prove request fell through every arm and returned `QuerySyntaxError::Unsupported`. But the sum router (`mode_detection/v0`) resolves both `(Aggregate, !range, _, true)` and `(GroupByIn, !range, _, true)` to `DocumentSumMode::PointLookupProof`, and the SDK verifier (`verify_point_lookup_count_and_sum_proof`) is wired to that same resolved mode. So the SDK could encode + dispatch a perfectly valid no-range GroupByIn AVG proof request, drive would 500 with `Unsupported`, and verification never got bytes to chew on. Fix: extend the no-range point-lookup arm to accept both `Aggregate` and `GroupByIn`. The grovedb-side proof is identical in both cases — one walk through the point-lookup `subquery` per In key emits one count-sum-bearing element per branch. The folding shape (single aggregate vs per-branch entries) is decided client-side by the verifier picking `DocumentAverage` vs `DocumentSplitAverages`. Regression test `no_range_group_by_in_avg_prove_routes_to_point_lookup` builds a summable + countable (non-range) `byColor` index, inserts three widgets, and pins both halves of the contract: 1. Server returns `Proof(bytes)`, not `Unsupported`. 2. Bytes bincode-decode as a `GroveDBProof` (sanity check). End-to-end root-hash verification is exercised in the SDK FromProof tests; this dispatcher-level test just pins the routing decision so the gap can't silently come back. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@thepastaclaw Addressed in The no-range point-lookup arm of Validation: |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Confirmed the 81467ea fix for my carried-forward blocker: the no-range AVG prove point-lookup arm now accepts both Aggregate and GroupByIn, and the added no_range_group_by_in_avg_prove_routes_to_point_lookup regression test pins the public wire-valid shape that previously fell through to Unsupported. The latest delta is scoped to that dispatcher/test change, so this addresses my review finding.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Verified the latest head 81467eaf306f2daa2ae0ca1edf81926bf219e9ca against my prior 7d13f8f0 review.
The only remaining blocking finding I had open — no-range GroupByIn AVG prove falling through to Unsupported — is fixed: the point-lookup prove arm now accepts AverageMode::Aggregate | AverageMode::GroupByIn, and the new regression test no_range_group_by_in_avg_prove_routes_to_point_lookup covers the routed proof response.
Validation: inspected the latest one-file delta (drive_dispatcher.rs) and ran git diff --check on 7d13f8f0..81467eaf.
…, proto status Three review findings from the cumulative SUM/AVG PR review: P1 (drive) — summable-index updates with unchanged keys now fail. `update_document_for_contract_operations_v0` builds an `Element::ReferenceWithSumItem` for `summable` indexes and hands it to `batch_refresh_reference` when no index key changes (refresh arm). `batch_refresh_reference_v0` only matched `Element::Reference` and returned `CorruptedCodeExecution`, so a benign update that only rewrites the summed property (e.g. `byRecipient` summable on `amount`, change only `amount`) would fail at the server. Fix: `batch_refresh_reference_v0` now dispatches on the element variant and emits grovedb's `refresh_reference_with_sum_item_op` (sum-item override mode) for `ReferenceWithSumItem` inputs. Adds the `refresh_reference_with_sum_item_for_known_path_key_reference_info` helper next to its plain-reference sibling; rewrites the stale "Drive only refreshes plain (counted) references" comment in `fees/op.rs`. Pre-v12 paths are unaffected (no pre-v12 contract can carry sum flags so the new variant arm is dormant). Regression test `summable_index_update_keeps_unchanged_keys_via_refresh_path` builds a v12 contract with a `byColor` summable + countable index, inserts a widget at `amount=5`, updates to `amount=42` keeping `color="red"` (refresh arm, not insert-then-delete), then queries SUM by color="red" and asserts the aggregate is exactly 42 — pinning both that the call succeeds AND that the new sum_value reached the ancestor sum tree. P2 (dpp) — `documentsAverageable` shorthand was not equivalent to its longhand form when one range axis was set. The doctype-level parser at `try_from_schema/v2/mod.rs:234` merged both range axes as `merged_range = range_countable || range_summable || range_averageable` and assigned it to BOTH outputs. So `documentsAverageable + rangeSummable: true` silently set `range_countable: true`, while the longhand `documentsCountable + documentsSummable + rangeSummable: true` left it false. Different on-disk tree shapes for what authors expected to be equivalent forms. Fix: split the merge to `range_countable || range_averageable` and `range_summable || range_averageable` — only `rangeAverageable: true` promotes both. Matches the per-index parser at `index/mod.rs:941`. Two parity tests pin both directions. P3 (proto) — `platform.proto` comments still labeled SUM/AVG as "scaffold-only" returning `NotYetImplemented`, but this PR wires real execution end-to-end on the drive dispatcher. Refreshed the wire-shape table and the SUM/AVG status block to point at the resolved-mode tables and the executor modules instead of a not-implemented stub. Validation: `cargo fmt`, `cargo clippy -p drive -p dpp --all-targets -- -D warnings` clean, drive lib tests 3205 passed (was 3204; +1 for the P1 regression), dpp lib tests 3477 passed (+2 for P2 parity), `cargo check -p dapi-grpc -p drive-abci` OK. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed all three findings in P1 — P2 — split the merge in P3 — refreshed the wire-shape table and SUM/AVG status block in Re v10/v11 sync — agreed: Validation: |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Verified latest head 16d75074b911b5a09fbbdfad620463ec3304dcca against the three items in Sam's follow-up.
batch_refresh_reference_v0now handlesReferenceWithSumItemvia the sum-item refresh op, and the new end-to-end update regression covers unchanged index keys with a changed summable value.- The doctype-level
documentsAverageablerange-axis merge now preservesrangeCountableandrangeSummableindependently unlessrangeAverageableasks to promote both axes; the new longhand/shorthand parity tests cover both directions. - The proto status text now describes SUM/AVG as wired end-to-end instead of scaffold-only.
Validation: git diff --check 81467eaf..16d75074; cargo test -p dpp doctype_documents_averageable_with_range -- --nocapture; cargo test -p drive summable_index_update_keeps_unchanged_keys_via_refresh_path -- --nocapture.
…est side
Carried-forward review nit on the same PR: the request-side
`selects` comment at platform.proto:1023 still said `SUM` / `AVG`
were rejected at the per-function gate, even though the dispatcher
now wires both end-to-end. The `Select` message-level docstring
(lines 914-918) had the same staleness — claiming only `DOCUMENTS`
and `COUNT(*)` are evaluated.
Refreshed both blocks to reflect today's capability table:
- DOCUMENTS, COUNT(*), SUM(<field>), AVG(<field>) → evaluated.
- COUNT(<field>), MIN(<field>), MAX(<field>) → wire-stable but
rejected at the per-function gate.
MIN/MAX in particular note the underlying blocker (no grovedb
min/max primitive on count or sum trees; the order-by-then-LIMIT-1
emulation has the wrong proof shape for the cryptographic verifier)
so client authors reading the proto as the source of truth aren't
left guessing why those two are still rejected when SUM/AVG aren't.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed in
Caught this on first pass thanks to your earlier P3 nit — same staleness pattern but on the request side instead of the response side. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review — supplemental blocker
Cumulative review at 16d7507. All prior reviewed findings from 81467ea and earlier are resolved (AVG prove dispatcher no-range GroupByIn arm, summable-index batch_refresh_reference_v0 accepting ReferenceWithSumItem, doctype-level documentsAverageable shorthand fix, proto status doc refresh). One new blocking finding in the latest delta scope: the update path still creates new aggregate-index branches as TreeType::NormalTree, inconsistent with the v1 insert helpers that pick the proper aggregate TreeType from countable/summable/range_countable/range_summable.
Reviewed commit: 16d7507
Note: an exact-SHA approval was posted while this full reviewer pass was still running. This review adds a newly verified blocker from the completed pass rather than duplicating that approval.
🔴 1 blocking
blocking: Key-changing updates still build new aggregate index branches as NormalTree
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs (line 266)
When a document update moves into a previously unseen index branch, every missing subtree is created with TreeType::NormalTree at lines 266-271 (top-level value branch), 339-344 (next property-name level), 371-376 (next value branch), and 451-453 (terminator [0] subtree). This contradicts the insert path: add_indices_for_top_index_level_for_contract_operations_v1 (packages/rs-drive/src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/v1/mod.rs lines 130-151, 195) and the recursive add_indices_for_index_level_for_contract_operations_v1 (v1/mod.rs lines 186-252) explicitly select ProvableCountTree / ProvableSumTree / CountSumTree / ProvableCountSumTree / ProvableCountProvableSumTree from each sub-level's countable / range_countable / summable / range_summable flags, and add_reference_for_index_level_for_contract_operations_v0 mirrors that table when terminating. Nothing in document type validation prevents an aggregate index on a mutable document, and update_document_for_contract_operations_v0 already takes care to emit Element::ReferenceWithSumItem for index.summable at lines 216-226 — but the tree-type call sites were not updated. The result: a valid update that changes the top-level or any sub-level index key for a mutable document under an aggregate index can materialize a NormalTree branch beneath a ProvableCount*/ProvableSum* parent. Subsequent inserts/refreshes under that branch will not contribute count/sum bytes the parent expects, so COUNT/SUM/AVG queries and proofs become wrong for any document that reaches the branch via update rather than insert, and nodes that take different code paths diverge — a consensus break. The new regression test in this PR only covers the unchanged-key refresh path on insert; the update-key-change path is uncovered and broken. Fix by computing the per-sub-level property_name_tree_type and value_tree_type the same way insert v1 does (from has_index_with_type() countable/range_countable/summable/range_summable) and threading them through each batch_insert_empty_tree_if_not_exists call here. Also add an integration test that updates a document's indexed field on an aggregate index and asserts COUNT/SUM/AVG over the new branch.
Source: codex; confidence: 0.95
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Verified the 779822d request-side proto comment refresh. The delta is comment-only in platform.proto, and it aligns the Select/selects capability tables with the current v1 routing: DOCUMENTS, COUNT(*), SUM, and AVG are evaluated, while COUNT(field)/MIN/MAX remain rejected at the per-function gate. git diff --check 16d75074..779822da32 is clean.
…the proper aggregate TreeType
The four `batch_insert_empty_tree_if_not_exists` call sites in
`update_document_for_contract_operations_v0` (top-level value tree,
inner property-name tree, inner value tree, and the terminator
`[0]` reference bucket) hardcoded `TreeType::NormalTree`. The
insert path's v1 helpers
(`add_indices_for_{top_index_,index_}level_for_contract_operations_v1`
and `add_reference_for_index_level_for_contract_operations_v0`)
pick the aggregate variant — `CountSumTree` / `ProvableCount*` /
etc. — from each level's countable / range_countable / summable /
range_summable flags. The mismatch means a valid update that
moves a document into a previously-unseen branch under an
aggregate index materializes a `NormalTree` beneath a
`ProvableCountTree` / `ProvableSumTree` parent: subsequent
inserts / refreshes on that branch don't contribute count / sum
bytes the parent expects, COUNT / SUM / AVG queries and proofs
become wrong for any document that reached the branch via update
rather than insert, and nodes that took different code paths
diverge on the merk root — a consensus break.
Pre-v12 contracts can't trigger this (DPP v11's `try_from_schema`
doesn't parse the sum / count flags), but v12+ mutable doctypes
with summable or rangeCountable indexes do.
Fix: walk the doctype's `IndexLevel` tree in lockstep with each
index's properties chain, and at each branch we materialize, pick
the matching `TreeType` via the same dispatch tables the insert
helpers use. The walk uses `has_index_with_type()` on the
descended `IndexLevel` node so the choice reflects ALL indexes
sharing the prefix — a shared depth-1 level under `[recipient]`
and `[recipient, sentAt]` upgrades to whichever terminator
demands more aggregate carriers, exactly the way the insert path
picks it.
Three helper fns inlined at the top of the file (one per
dispatch table: value-tree, property-name-tree, terminator
reference-bucket); each cites the canonical insert-side source so
a future drift between insert and update gets caught at review.
Regression test
`summable_index_update_changes_key_into_new_branch_materializes_aggregate_tree_type`
exercises the bug end-to-end: a v12 contract with a
`byColor` (summable + countable, no range) index, insert at
color="red", update to color="blue" (forces the update path to
materialize the unseen "blue" branch), then SUM(amount) where
color="blue". Pre-fix the branch lands as `NormalTree` and the
dispatcher errors with `GroveDB(InvalidQuery("path_queries can
not refer to trees"))` because there's no count + sum carrier;
post-fix it lands as `CountSumTree` and the aggregate equals 17
(the updated value). Companion to
`summable_index_update_keeps_unchanged_keys_via_refresh_path`,
which covers the no-key-change refresh arm.
Validation: drive lib tests 3206 passed (was 3205; +1 for this
regression). `cargo fmt`, `cargo clippy -p drive --all-targets
-- -D warnings` clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@thepastaclaw Addressed in Fix walks the doctype's
The walk uses Regression test Validation: drive lib tests 3206 passed (+1), |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental review at 779822d (delta from 16d7507 is proto-comment-only). Prior blocking finding about the document update path creating new aggregate index branches as TreeType::NormalTree is STILL VALID and carried forward. Independently confirmed a separate blocking issue in the verified-proof flat-map helpers used by the wasm SUM/AVG bindings: unchecked i64/u64 arithmetic while collapsing compound (In + range) entries, in contrast to the rs-sdk's checked_add path. The FFI NotImplemented stub for dash_sdk_document_sum/_average is intentional and documented (waits on grovedb PR 670), so it is filtered as not a defect.
Reviewed commit: 779822d
🔴 2 blocking
2 additional finding(s) omitted from inline comments; included below.
1. [blocking] Key-changing updates create new aggregate-index branches as NormalTree under aggregate parents — consensus break for SUM/AVG/COUNT
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs:260-460
Carried forward from review 4323825204 and STILL VALID at head 779822d (delta touched only platform.proto comments). When a document update changes an indexed field so the document moves into a previously unseen branch, every freshly materialized subtree is created with TreeType::NormalTree at all four sites in this function: top-level value branch (lines 266–271), next property-name level (lines 339–344), next value branch (lines 371–376), and the non-unique terminator [0] subtree (lines 451–453).
The insert path no longer does this. add_indices_for_top_index_level_for_contract_operations/v1 and the recursive add_indices_for_index_level_for_contract_operations/v1 derive property_name_tree_type and value_tree_type from each sub-level's has_index_with_type() countable / range_countable / summable / range_summable flags and dispatch to CountTree, SumTree, ProvableCountTree, ProvableSumTree, CountSumTree, ProvableCountSumTree, or ProvableCountProvableSumTree accordingly. This file already takes care of the leaf-reference variant (Element::ReferenceWithSumItem when index.summable.is_some() at lines 216–226), proving the aggregate split was considered for the terminal reference but missed for the branch tree-type call sites.
Impact: a valid update on a mutable document under an aggregate index — nothing in document-type validation forbids this — can materialize a NormalTree branch beneath a ProvableCount*/ProvableSum*/CountSum*/PCPS parent. Subsequent inserts/refreshes into that branch will not maintain the per-doc count/sum bytes the parent's merk-layer aggregation expects, so any COUNT/SUM/AVG query or proof that traverses the branch returns wrong totals. Worse, when the same value lands by a separate document create via insert v1, the post-state tree types diverge across nodes that reached the branch by insert vs. update — a chain-split risk.
The new regression test summable_index_update_keeps_unchanged_keys_via_refresh_path covers only the unchanged-key refresh path; the update-key-change branch creation path that goes through these four NormalTree call sites is uncovered.
Fix: compute per-sub-level property_name_tree_type and value_tree_type from the same has_index_with_type() dispatch table used by insert v1 and thread them into each batch_insert_empty_tree_if_not_exists call at lines 266–271, 339–344, 371–376, and 451–453. Bump update_document_for_contract_operations to v1 in the platform-version method table so this stays consensus-versioned. Add an integration test that updates an indexed field of an aggregate (sum / range-sum / count+sum / count+range-sum) document to a previously-unseen value and asserts a SUM/AVG/COUNT prove query over the new branch matches an independently recomputed value.
2. [blocking] into_flat_map uses unchecked i64/u64 arithmetic when collapsing compound In+range branches — debug-panic / release-wrap on overflow
packages/rs-drive-proof-verifier/src/proof/document_split_sum.rs:39-47
DocumentSplitSums::into_flat_map (lines 39–47) and DocumentSplitAverages::into_flat_map (lines 58–68 in document_split_average.rs) merge per-(in_key, key) entries into a flat BTreeMap<key, _> using bare += on i64 (sum) and u64/i64 (average count/sum). For compound (In, range) queries, multiple verified branches share the same terminator key and are merged here before the wasm bindings expose them to JS as bigints (packages/wasm-sdk/src/queries/document.rs:817–860). If the merged total exceeds the integer bound, debug builds panic and release builds wrap silently — JS receives a corrupted aggregate.
The rest of the Rust stack already treats this as a real overflow hazard and surfaces a typed error: DocumentSum::fold_sum_entries in packages/rs-sdk/src/platform/documents/document_sum.rs:34–51 uses checked_add and returns RequestError (with guidance to fall back to DocumentSplitSums for per-branch i64s and i128 folding). The flat-map helpers reintroduce the unchecked behavior at exactly the boundary the SDK was trying to harden, so any consumer who reaches them — notably the wasm SUM/AVG path — loses that protection.
Fix: change the signatures to return Result<BTreeMap<…>, Error> (or an inherent Try* variant) using checked_add, surface the same overflow error message as the SDK aggregate path, and propagate the Result through split_sums_to_js_map / split_averages_to_js_map so the wasm caller throws a real error rather than corrupting data. Add unit tests over compound-In merges at the i64/u64 boundary.
Supplemental full cumulative review: a lightweight same-SHA approval existed, but the verifier found carried-forward/new blocking issues that were not covered by that approval.
…ECT projection docs Carried-forward P3 nit on the same PR: two doc blocks for the SQL-shaped SELECT surface still claimed SUM/AVG were rejected. - `packages/rs-drive/src/query/projection.rs` module docstring + the `SelectFunction::Sum` / `::Avg` variant docstrings. - `packages/rs-sdk/src/platform/documents/document_query.rs`'s `with_select` docstring. Refreshed both to the current capability table: `Documents`, `COUNT(*)`, `SUM(<field>)`, and `AVG(<field>)` route end-to-end through the drive count / sum / average dispatchers; `COUNT(<field>)`, `MIN(<field>)`, and `MAX(<field>)` remain wire-stable but rejected at the per-function gate. The variant docstrings on `Sum` / `Avg` now point at the dispatcher modules (`crate::query::drive_document_sum_query` / `drive_document_average_query`) so callers tracing routing from the SDK API down to the executor have a direct breadcrumb. (The earlier proto-side update in 779822d covered platform.proto:914 and platform.proto:1023; this commit closes out the Rust-side doc surface.) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed in Note:
Did a broader grep for other stale "SUM/AVG rejected" / "not yet implemented" claims — the remaining matches are either accurate ( |
…um / average results `DocumentSplitSums::into_flat_map` and `DocumentSplitAverages::into_flat_map` used bare `+=` on `i64` (sum) and `u64` + `i64` (count + sum) while collapsing per-(in_key, key) compound entries into a flat per-key `BTreeMap`. For compound `(In, range)` queries that resolve to multiple verified In-fork branches at the same terminator key, the merged total could exceed the integer bound — debug builds panic, release builds wrap silently, and the wasm SUM/AVG bindings that call these helpers (`split_sums_to_js_map` / `split_averages_to_js_map` in `packages/wasm-sdk/src/queries/document.rs`) surface a corrupted aggregate to the JS caller. The SDK aggregate path was already hardened on this boundary: `DocumentSum::fold_sum_entries` and `DocumentAverage::fold_average_entries` both use `checked_add` and surface overflow as `RequestError`. The flat-map helpers re-introduced the unchecked behavior at the exact point the SDK was trying to harden. Fix: rename the two helpers `into_flat_map` → `try_into_flat_map` and make them return `Result<BTreeMap<…>, Error>`, using `checked_add` on each accumulator step. Error messages match the SDK's wording and direct the caller back to iterating `DocumentSplitSums.0` / `DocumentSplitAverages.0` directly so they can fold per-branch i64s with their own arithmetic (e.g. i128). Wasm consumer changes (`split_sums_to_js_map` / `split_averages_to_js_map`): return `Result<Map, WasmSdkError>`, convert the proof-verifier error to `WasmSdkError::generic`, and propagate via `?` in the four exported wasm-bindgen entry points (`get_documents_sum`, `get_documents_sum_with_proof_info`, `get_documents_average`, `get_documents_average_with_proof_info`). JS callers now see a structured error rather than a panic or a wrapped value. Five overflow regression tests per helper: - pass-through of a single-branch entry, - sums across In-forks, - `i64`/`u64` overflow surfaces `RequestError` (not panic / wrap), - underflow on the i64 side surfaces symmetrically, - `None` (proven-absent) entries are skipped without poisoning. Plus an avg-side `i64::MAX`/`u64::MAX` boundary pair so both axes are independently exercised. The matching `DocumentSplitCounts::into_flat_map` u64 fold is not touched here — overflow there would require summing 2^64 verified counts, which is structurally unreachable from a single grovedb walk. Worth revisiting if a future protocol surface aggregates across many queries client-side; out of scope for this PR. Validation: drive-proof-verifier lib tests 235 passed (was 226; +9 new), dash-sdk lib 128 passed. `cargo clippy -p drive-proof-verifier -p dash-sdk --all-targets -- -D warnings` clean; `cargo check -p wasm-sdk --target wasm32-unknown-unknown` clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@thepastaclaw Two findings in this supplemental: Finding 1 (key-changing updates as NormalTree): Already addressed in Finding 2 (unchecked arithmetic in flat-map helpers): Real and now fixed in
Note on Validation: drive-proof-verifier lib 235 passed (was 226; +9 new), dash-sdk lib 128 passed. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Verified the latest two-addressed commits at ab8b8a9b34.
Resolved:
254e5e6367fixes the key-changing update path blocker: the four branch-materialization sites now deriveTreeTypefrom the matchingIndexLeveldispatch instead of hardcodingNormalTree, and the new regressionsummable_index_update_changes_key_into_new_branch_materializes_aggregate_tree_typepasses.ab8b8a9b34refreshes the Rust-side SELECT projection docs:projection.rsanddocument_query.rsnow correctly sayDOCUMENTS,COUNT(*),SUM, andAVGare evaluated end-to-end, whileCOUNT(field),MIN, andMAXremain rejected.
Validation:
git diff --check 779822da32fbfd584c6c7f28266a53dcff52a854..ab8b8a9b34cargo test -p drive summable_index_update_changes_key_into_new_branch_materializes_aggregate_tree_type -- --nocapture
Still blocking from review 4323924622:
DocumentSplitSums::into_flat_mapandDocumentSplitAverages::into_flat_mapstill use unchecked+=while flattening compound(In, range)entries (document_split_sum.rs:39-43,document_split_average.rs:58-64). Current HEAD still returns a plainBTreeMap, not aResult, and there is nochecked_add/overflow propagation at the wasm-facing helper boundary. That keeps the debug-panic / release-wrap issue valid for SUM/AVG flat-map collapse.
So the latest addressed commits look good, but I’m keeping changes requested until the flat-map overflow blocker is handled.
|
@thepastaclaw Thanks for confirming the first two are resolved. The flat-map overflow blocker was addressed in
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-reviewed the latest head 2e7dcdd6f84324a451467f110df4315b3725fbb8.
The remaining flat-map blocker from my prior review is resolved in 2e7dcdd6f8: SUM/AVG split flat-map collapse now goes through try_into_flat_map, uses checked arithmetic on the relevant axes (i64 for sums, u64 count + i64 sum for averages), and propagates a structured verifier/wasm error instead of debug-panicking or release-wrapping. The wasm helpers now propagate those errors through the exported SUM/AVG APIs.
Validation I ran locally:
git diff --check 779822da32fbfd584c6c7f28266a53dcff52a854..HEADcargo test -p drive summable_index_update_changes_key_into_new_branch_materializes_aggregate_tree_type -- --nocapturecargo test -p drive-proof-verifier try_into_flat_map -- --nocapturecargo check -p wasm-sdk
I also attempted cargo check -p wasm-sdk --target wasm32-unknown-unknown; that did not complete because the local clang toolchain cannot create wasm32-unknown-unknown objects, not because of a Rust compile error.
No remaining blocking findings from my side.
|
IT IS DONE! |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified at 254e5e6. Prior key-changing-update TreeType bug is FIXED (proper IndexLevel-driven dispatch + regression test). Two prior blocking findings remain STILL VALID: DocumentSplitSums/Averages::into_flat_map use unchecked i64/u64 += across compound In+range branches, exposed directly to JS via the wasm SUM/AVG bigint boundary. One new finding on the latest delta: the update path's inner property-name tree insertion skips the under-aggregating-parent wrapper that insert_v1 applies, producing wrong COUNT/SUM/AVG aggregates for contracts that combine an aggregating prefix-only index with a compound continuation.
Reviewed commit: 254e5e6
🔴 3 blocking
3 additional finding(s) omitted from inline comments; included below.
1. [blocking] into_flat_map uses unchecked i64 += when collapsing compound (In, range) branches
packages/rs-drive-proof-verifier/src/proof/document_split_sum.rs:39-47
Carried forward and STILL VALID at head 254e5e6 — this file is untouched by the latest delta. DocumentSplitSums::into_flat_map at lines 39–47 merges per-(in_key, key) entries into a flat BTreeMap<key, i64> with bare *out.entry(entry.key).or_insert(0) += sum;. For compound (In, range) queries multiple verified In-branches legitimately collide on the same flattened terminator key; a large or adversarial proof can drive the accumulator past i64::MAX/MIN. Debug builds panic (and panicking across the wasm-bindgen extern boundary is a WASM trap / UB); release builds silently wrap and emit a corrupted aggregate. The result flows directly to JS as Map<string, bigint> via split_sums_to_js_map in packages/wasm-sdk/src/queries/document.rs:817-825, so wasm SUM callers receive either a trap or attacker-controllable wrong bigints. The native SDK fold path (packages/rs-sdk/src/platform/documents/document_sum.rs:34-50) already uses checked_add and surfaces RequestError for exactly this hazard; the flat-map helper reintroduces the unchecked behavior at the last hop before JS. Fix: return Result<BTreeMap<Vec<u8>, i64>, Error> using checked_add, surface the same overflow error message as DocumentSum::fold_sum_entries, and thread the Result through split_sums_to_js_map (and the WithProofInfo variant) so JS sees a typed error rather than corrupt data. Add a regression test with two SplitSumEntrys sharing a terminator key and sums positioned to overflow on merge.
2. [blocking] into_flat_map uses unchecked u64/i64 += when collapsing compound (In, range) branches
packages/rs-drive-proof-verifier/src/proof/document_split_average.rs:58-68
Carried forward and STILL VALID at head 254e5e6 — file untouched by the latest delta. DocumentSplitAverages::into_flat_map at lines 58–68 does acc.0 += c (u64 count) and acc.1 += s (i64 sum) with no overflow handling. Compound (In, range) AVG proofs can legitimately produce multiple branches at the same flattened terminator key, and the merge can overflow either accumulator. Debug builds panic across the wasm-bindgen boundary (WASM trap); release builds silently wrap, yielding corrupted (count, sum) pairs. The result feeds split_averages_to_js_map in packages/wasm-sdk/src/queries/document.rs:842-857 as Map<string, {count: bigint, sum: bigint}>, and the JS caller then divides sum/count on already-corrupt data with no way to detect the overflow. The native SDK average path (packages/rs-sdk/src/platform/documents/document_average.rs:34-65) uses checked_add and returns a typed RequestError for this exact case; the flat-map helper bypasses that protection at the wasm boundary. Fix: return Result<BTreeMap<Vec<u8>, (u64, i64)>, Error> using checked_add on both components and propagate the error through split_averages_to_js_map. Add regression tests covering both the u64 count overflow and the i64 sum overflow paths.
3. [blocking] Update path inserts continuation property-name tree without the NotCounted/NotSummed wrapper that insert_v1 applies under aggregating parents
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs:486-510
The 254e5e6 fix correctly dispatches TreeType at all four call sites in update_document_for_contract_operations_v0 to match insert_v1's table, but it does not mirror insert_v1's wrapping policy for the depth-i+1 property-name tree. add_indices_for_index_level_for_contract_operations_v1 (lines 308–331) branches on parent_value_tree_aggregates and uses batch_insert_empty_tree_under_aggregating_parent_if_not_exists whenever the immediate parent value tree aggregates count, sum, or both — that wrap inserts Element::NonCounted / NotSummed / NotCountedOrSummed so the continuation contributes 0 to the parent's per-axis aggregate. The update path at line 494 unconditionally calls batch_insert_empty_tree_if_not_exists with no wrapping.
Failure shape: a v12 contract with both [color] (countable+summable terminator) AND [color, size] indexes on the same doctype. Initial inserts go through insert_v1, which wraps the size property-name tree under each color CountSumTree with NotCountedOrSummed. A key-changing update that moves a document to a previously-unseen color value goes through this update path, which now materializes the new color value tree as CountSumTree correctly, but creates the size continuation property-name tree as plain unwrapped NormalTree. That empty subtree contributes 1 to the new value tree's count_value (and skewed sum_value upstream), on top of the legitimate [0] ref-bucket contribution. COUNT/SUM/AVG queries pointing at the update-created color branch return inflated counts and corresponding wrong averages. Once the branch exists, future inserts hitting the same prefix skip via if_not_exists with the wrong wrapping persisted.
Not a chain split — all nodes apply the same update in the same canonical order, so every node lands the same wrong bytes — but the platform-reported aggregate is wrong, proofs verify against the wrong root, and the WASM SUM/COUNT/AVG bindings surface the inflated counts to JS with no error. The new regression test only exercises a single-property byColor index (no continuation tree at all), so this composition is uncovered. Fix: thread parent_value_tree_type through the index.properties walk in this function and call batch_insert_empty_tree_under_aggregating_parent_if_not_exists when the parent at each depth aggregates, mirroring insert_v1 exactly. Add an integration test on a doctype that combines a terminating-aggregate prefix with a compound continuation (e.g. [a] countable+summable AND [a, b]), do a key-changing update on a to a previously-unseen value, then assert SUM/COUNT/AVG under the new branch match what an equivalent fresh insert produces.
GitHub refused the PR diff API for this large PR during dry-run, so I posted the verified findings in the top-level review body.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
All three carried-forward blocking findings remain STILL VALID at ab8b8a9 — the latest delta is documentation-only (SUM/AVG SELECT projection comments) and does not address them. Unanimous convergence across six independent reviewers (general/security/rust-quality x claude/codex). No new findings in the doc-only delta.
Reviewed commit: ab8b8a9
🔴 3 blocking
3 additional finding(s) omitted from inline comments; included below.
1. [blocking] DocumentSplitSums::into_flat_map uses unchecked i64 += when collapsing compound (In, range) branches
packages/rs-drive-proof-verifier/src/proof/document_split_sum.rs:39-47
Verified at head ab8b8a9, lines 39–47:
pub fn into_flat_map(self) -> std::collections::BTreeMap<Vec<u8>, i64> {
let mut out = std::collections::BTreeMap::new();
for entry in self.0 {
if let Some(sum) = entry.sum {
*out.entry(entry.key).or_insert(0) += sum;
}
}
out
}
For compound (In, range) SUM queries, multiple verified In-branch totals can legitimately collide on the same flattened terminator key. Because document sums come from user-controlled stored values, an adversarial (or simply large) proof can drive the accumulator past i64::MAX/i64::MIN. Debug builds panic — and panicking across the wasm-bindgen extern boundary is a WASM trap; release builds silently wrap and emit a corrupted aggregate.
The corrupt or trapping result flows directly to JS as Map<string, bigint> via split_sums_to_js_map in packages/wasm-sdk/src/queries/document.rs, so wasm SUM callers either trap or receive attacker-influenced wrong bigints. The native SDK fold path (packages/rs-sdk/src/platform/documents/document_sum.rs:34–50) already uses checked_add and surfaces RequestError for this exact hazard; this helper bypasses that discipline at the last hop before JS.
Fix: change to try_into_flat_map(self) -> Result<BTreeMap<Vec<u8>, i64>, Error> using checked_add, surface the same overflow error as DocumentSum::fold_sum_entries, propagate the Result through split_sums_to_js_map (and the WithProofInfo variant), and add a regression test with two SplitSumEntrys sharing a terminator key positioned to overflow on merge.
2. [blocking] DocumentSplitAverages::into_flat_map uses unchecked u64/i64 += when collapsing compound (In, range) branches
packages/rs-drive-proof-verifier/src/proof/document_split_average.rs:58-68
Verified at head ab8b8a9, lines 58–68:
pub fn into_flat_map(self) -> std::collections::BTreeMap<Vec<u8>, (u64, i64)> {
let mut out = std::collections::BTreeMap::new();
for entry in self.0 {
if let (Some(c), Some(s)) = (entry.count, entry.sum) {
let acc = out.entry(entry.key).or_insert((0u64, 0i64));
acc.0 += c;
acc.1 += s;
}
}
out
}
Compound (In, range) AVG proofs can legitimately produce multiple verified branches at the same flattened terminator key, and the merge can overflow either accumulator. Debug builds panic across the wasm-bindgen boundary (WASM trap); release builds silently wrap, yielding corrupted (count, sum) pairs.
The result feeds split_averages_to_js_map in packages/wasm-sdk/src/queries/document.rs as Map<string, {count: bigint, sum: bigint}>, and the JS caller then divides sum/count on already-corrupt data with no way to detect the overflow. This hazard is amplified vs SUM because u64 wraps faster and the divide-by-count step further obscures the corruption. The native SDK average path (packages/rs-sdk/src/platform/documents/document_average.rs:34–65) uses checked_add and returns RequestError for the same hazard; this helper bypasses that protection at the wasm boundary.
Fix: change to try_into_flat_map(self) -> Result<BTreeMap<Vec<u8>, (u64, i64)>, Error> using checked_add on both axes, propagate the error through split_averages_to_js_map, and add regression tests covering both u64 count overflow and i64 sum overflow paths.
3. [blocking] Update path inserts continuation property-name tree without the under-aggregating-parent wrapper that insert_v1 applies
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs:486-510
Verified at head ab8b8a9. Lines 486–510 unconditionally call self.batch_insert_empty_tree_if_not_exists(...) for the inner property-name tree at depth i+1, with no branch on parent aggregation. Compare with the insert-side equivalent at packages/rs-drive/src/drive/document/insert/add_indices_for_index_level_for_contract_operations/v1/mod.rs:308–331, which branches on wrap_property_name_tree_under_aggregating_parent (= parent_value_tree_aggregates) and uses batch_insert_empty_tree_under_aggregating_parent_if_not_exists when the immediate parent aggregates — inserting Element::NonCounted / NotSummed / NotCountedOrSummed so the continuation contributes 0 to the parent's per-axis aggregate.
The 254e5e6 fix correctly dispatched TreeType for the value-tree call sites in this function, but did not mirror insert_v1's wrapping policy for the continuation property-name tree.
Failure shape: a v12 contract with [color] (countable+summable terminator) AND [color, size] indexes on the same doctype. Initial inserts go through insert_v1, which wraps the size property-name tree under each color CountSumTree with NotCountedOrSummed. A key-changing update that moves a document to a previously-unseen color value goes through this update path: the new color value tree is now materialized as CountSumTree correctly (per 254e5e6), but the size continuation property-name tree is created as unwrapped NormalTree. That empty subtree contributes 1 to the new value tree's count_value and a skewed sum_value upstream, on top of the legitimate [0] ref-bucket contribution. COUNT/SUM/AVG queries against the update-created branch return inflated counts and wrong averages. Once persisted, future inserts skip via if_not_exists with the wrong wrapping retained.
Not a chain split — all nodes apply the update deterministically — but proofs verify against the wrong root and the wasm-sdk / rs-sdk SUM/COUNT/AVG bindings silently surface inflated aggregates to clients with no error. The new regression test summable_index_update_changes_key_into_new_branch_materializes_aggregate_tree_type exercises only a single-property byColor index, leaving this composition uncovered.
Fix: thread parent_value_tree_type through the index.properties walk in update_document_for_contract_operations_v0 (analogous to insert_v1's plumbing) and call batch_insert_empty_tree_under_aggregating_parent_if_not_exists when the parent at each depth aggregates, mirroring insert_v1 exactly. Add an integration test combining a terminating-aggregate prefix [a] (countable+summable) with a compound continuation [a, b], perform a key-changing update on a to a previously-unseen value, and assert SUM/COUNT/AVG under the new branch match what an equivalent fresh insert produces.
GitHub refused the PR diff API for this large PR during dry-run, so I posted the verified findings in the top-level review body.
2 participants
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 (rs-sdk, rs-sdk-ffi, wasm-sdk, drive-abci), 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).documentsAverageable/rangeAverageablesyntactic sugar (desugar todocumentsCountable + documentsSummableon the same property).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.documentsKeepHistory: true + documentsSummable: "<prop>"(and itsdocumentsAverageablesugar) is supported — see §2 below for the per-doc-SumTree +ReferenceWithSumItem-on-0-key layout that makes the composition correct.2. Drive write side (contract apply + document insert + delete)
SumTree,ProvableSumTree,CountSumTree,ProvableCountSumTree, andProvableCountProvableSumTree(PCPS) for combined per-node count+sum. The v1primary_key_tree_typedispatch composes count × sum orthogonally across all nine cells of the resulting matrix.update_contract_operations_v0mirrorsinsert_contract_operations_v0's 4-way(range_countable, range_summable)dispatch on both the new-doctype branch AND the existing-doctype branch (the latter is a defense-in-depth alignment even thoughIndexLevel::validate_updatecurrently rejects new top-level indexes on existing doctypes).batch_insert_empty_provable_sum_tree,batch_insert_empty_provable_count_sum_tree,batch_insert_empty_provable_count_provable_sum_tree;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.SumTree, version bodies are plainItem(so historical versions don't double-count), and the0-key "current pointer" is aReferenceWithSumItemcarrying the current version'ssum_propertyvalue. Grovedb's merk machinery does aggregation propagation for free — same pattern used at the per-index sum-tree layer viamake_document_reference_with_sum_item. Updates rewrite the0-key's sum_value, propagating only the delta to ancestors.make_document_reference_with_sum_item+read_document_sum_contributionextract the sum value from the document.TreeType(not just abool) so dry-run cost estimation reports the correct sum-bearing layer types and matches applied delete fees byte-for-byte.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). Includesdistinct_sum_path_query(the ~280-line analog of count'sdistinct_count_path_query).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.mode_detection/v0/mod.rsversioned routing table —detect_sum_mode_from_inputs(where_clauses, mode, prove, platform_version)is the single source of truth that both the server (viadetect_sum_modewrapper that adds the doctype cross-check) and the SDK call.GroupByCompound + In + range + prove → RangeDistinctProof(per-(in_key, range_key)entries), mirroring count'srequires_distinct_walk()semantics — the carrier shape (one entry per In branch) is reserved forGroupByIn.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).verify_distinct_sum_proof,verify_distinct_count_and_sum_proof,verify_point_lookup_count_and_sum_proof.DriveVerifyDocumentSumMethodVersionsregistering all v0 verifiers.5. SDK fan-out (rs-sdk / rs-sdk-ffi / wasm-sdk / drive-abci)
rs-sdk:DocumentSum,DocumentSplitSums,DocumentAverage,DocumentSplitAveragestypes withFromProofimpls wired end-to-end.verify_sum_query/verify_average_queryroute through drive'sdetect_sum_mode_from_inputsfor byte-parity with the server. SUM/AVG single-aggregate folds usei64::checked_add/u64::checked_add(positive + negative directions) — overflow surfaces asRequestErrorhinting atDocumentSplit*for per-branch arithmetic.MockResponseimpls for round-tripping in test fixtures.rs-sdk-ffi:dash_sdk_document_sumC entry-point;path_elements.rsextended to formatElement::ProvableCountProvableSumTree+Element::ReferenceWithSumItem.wasm-sdk:getDocumentsSum/getDocumentsSumWithProofInfo/getDocumentsAverage/getDocumentsAverageWithProofInfoJS entry-points dispatch to the real Rust SDK;split_sums_to_js_map/split_averages_to_js_mapmappers produceMap<string, bigint>/Map<string, {count: bigint, sum: bigint}>. (Closes wasm-sdk: wire SUM/AVG fan-out (getDocumentsSum / getDocumentsAverage + proof-info variants) #3684.)drive-abci:query_documents_sum_v1/ AVG routing wired through the V1 wire surface (validate_and_routeshares logic with count).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-targets --locked -- -D warnings— clean across the entire workspace (including the wasm32-unknown-unknown target forwasm-sdk)cargo test -p drive --lib— 3 203 / 3 203 passcargo test -p platform-version --lib— 5 / 5 pass (including thehistorical_method_table_freezeregressions that pinDRIVE_DOCUMENT_METHOD_VERSIONS_V2.primary_key_tree_type = 0so v10/v11 method tables stay frozen — load-bearing for consensus-safe replay)cargo test -p dpp --lib— 3 475 / 3 475 passcargo test -p dash-sdk --lib— 128 / 128 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)
None.
Checklist:
Summary by CodeRabbit
New Features
Documentation
Tests
Chores