fix(platform)!: key unsigned integer index values in value order by QuantumExplorer · Pull Request #5350 · dashpay/platform

QuantumExplorer · 2026-10-08T21:44:33Z

Basic explanation

When a document property is an unsigned whole number (a schema integer with a minimum of 0 or more, or only a maximum), Platform stores it in its indexes as bytes that are supposed to sort like the numbers. They don't: the top bit is flipped as if the number were signed, so the upper half of the range (128 to 255 for a one-byte number) sorts below the lower half. Range queries, ordering, range counts and sums over such a property then give wrong answers whenever values on both sides of the middle exist.

From protocol version 14 these numbers are stored as plain big-endian bytes, which sort correctly. The first block of protocol version 14 moves every index key stored the old way to its new key, with GroveDB's new move operation, so existing data is fixed too. One kind of property keeps the old keys: an unsigned number that ends a rangeSummable index keeps them in every index of its document type (see below).

Issue being fixed or feature implemented

An unsigned integer document property (U8, U16, U32, U64) has been keyed in indexes with its top bit flipped, as if it were signed, since sized integer types arrived in protocol version 9. Values from the middle of the type's range up sort below the rest, so range queries, ordering, range counts and range sums across that point return wrong results, and a count or sum over a range whose bounds straddle it is refused.

Before and after

A grade type with "grade": {"type": "integer", "minimum": 0, "maximum": 255} (a one-byte unsigned integer) and a range-countable index on it, holding the grades 5, 100, 127, 128, 150, 200 and 255:

Query	Before (protocol version 13)	After (protocol version 14)
count where `grade > 100`	1 (only 127)	5 (127, 128, 150, 200, 255)
count where `grade between [100, 200]`	refused: "range lower bound must be <= upper bound"	5
documents where `grade > 100` order by `grade` asc	`[127]`	`[127, 128, 150, 200, 255]`
documents where `grade > 100` order by `grade` desc	`[127]`	`[255, 200, 150, 128, 127]`

The index key of a grade, before and after:

grade	Before	After
100	`0xE4`	`0x64`
127	`0xFF`	`0x7F`
128	`0x00`	`0x80`
200	`0x48`	`0xC8`

Signed integers, strings, identifiers, dates and the $ system fields keep their keys.

What was done?

Key generation 1 (dpp). serialize_value_for_key and deserialize_value_for_key 1, selected by CONTRACT_VERSIONS_V6 (protocol version 14, amended in place), key a property or derived index property of an unsigned integer type (U8, U16, U32, U64, U128, KeyIdWithReference) with DocumentPropertyType::encode_value_for_tree_keys_v1: its big-endian bytes, at the same width. Every other key comes from generation 0. Index writes, deletes, updates, uniqueness checks, query bounds, cursors, count, sum, average, ranked and having queries, and their proof verification all build keys through these two methods, so they all follow.

The features new in protocol version 14 key the new way from the start. An integerRange window start (IntegerRangeKeyType::encode / decode) is keyed like its source property, so an unsigned grid keys its windows by big-endian bytes. indexOnly synthesis reads keys through deserialize_value_for_key (it now takes the platform version), so a member key, row commitment and synthesized id all agree with the keys written.

Contest vote polls keep the old keys. A contest's vote poll paths, and the masternode vote records that point at them, are keyed by the contested index's values. A contest can run across the upgrade, so serialize_value_for_vote_poll_key / deserialize_value_for_vote_poll_key key them with generation 0 (serialize_value_for_key_v0 / deserialize_value_for_key_v0) at every version, and the contested insert converts the document's generation-1 keys back (tree_key_in_generation_0). Nothing is rewritten there. The one thing that keeps the old order is the listing of contested values (getContestedResources) of a contested index on an unsigned property.

A property ending a range-summable index keeps the old keys. A rangeSummable index keeps a sum in every node of the tree holding the values of its last property, and GroveDB refuses a node sum outside the signed 64-bit range. Values of both signs whose sums fit every node in the old key order can leave the range in every balanced tree of the new order (for example weights +M, +M, −M, −M with M = 2^62 under grades 1, 2, 3 and 128). A summed property's type does not rule out negative values either: before protocol version 14 an update can remove a summed property's negative minimum, making it unsigned while the tree still holds negative values. So an unsigned property that ends a rangeSummable index keeps generation 0's keys at every version, whatever the index sums, and in every index of its document type, since the keys are chosen by property name (Index::ends_range_summable_with, read by serialize_value_for_key 1 and everything that follows it). Those indexes keep the old order. Readers, writers and clients all derive this from the contract alone. Every other index is moved.

integerRange on a property keyed the old way is refused. An integerRange grid keys its windows in its source's value order, so document type parser generation 3 (protocol version 14, full validation only) refuses an integerRange whose source is an unsigned property that ends a rangeSummable index (validate_integer_range_sources_keyed_in_value_order, which asks the built type how it keys the source). Before: such a contract registered, with its grid and its other indexes keying the property differently. After: InvalidContractStructure, "integerRange.on ("amount") ... names a property that the rangeSummable index "byAmount" ends with: such a property keeps its earlier key encoding, which an integerRange grid does not read".

Preallocated trees are keyed as the referring type keys them. A referenced document's value is now carried through generation 0 into the referring type's key (referenced_tree_key_for_referring_property), in add_preallocated_index_tree_operations and the fee estimator, so a post whose tier keeps the old keys preallocates its likes' trees under the key the likes use (0x05 for tier 5, not 0x85).

Upgrade migration. Drive::rekey_unsigned_integer_index_values, called first in transition_to_version_14, walks every contract's document types (those that index an unsigned property) through their layout (document_type_layout) down to the trees whose children are unsigned value keys, deeper levels first, and moves each child to its new key with GroveDB's move_op. A moved subtree keeps its stored bytes, so its hashes, aggregates, Merk shapes and references are those it had; nothing below the moved key is copied, re-hashed or re-resolved, and levels whose keys stay (an identifier before an unsigned property, sibling index branches) are not touched. A key and the same key with its top bit flipped hold two values that trade keys, and the two moves go in the same batch. Moves gather across trees, document types and contracts into batches of at most SystemLimits::max_rekey_batch_moves moves (10,000 in SYSTEM_LIMITS_V4, None before; one more for a pair); a batch is applied early only when a tree is about to move its own keys while the batch holds moves below them, which GroveDB refuses. A tree whose children all have fixed keys is walked with point reads, one whose children fit a page is read with one query, and a bigger one in slices. Keys keep their width, so GroveDB charges and refunds nothing. A second run would flip the keys back; the transition runs once per chain. Memory: Drive holds, per tree level on the way down, at most two pages of keys (a page with the top bit clear and one with it set, so twins stay together) and their moves, and one batch of moves gathered across trees; GroveDB copies every record of a moved subtree to its new prefixes, and the block's transaction holds every write until it commits, so the activation block's memory grows with the bytes the moved subtrees hold. That is accepted: it is bounded by the transaction the block writes anyway, and what earlier protocol versions stored under unsigned index values is small.

GroveDB bump to b50a583377. Brings dashpay/grovedb#1015 (move_op, enabled from GROVE_V4, which only protocol version 14 selects, together with its change to how a reference resolves through a skipped InsertIfNotExists) and two fixes that refuse a malformed batch instead of panicking (#1016, #1017); a batch that applied before applies the same.

Devnets. Devnets that already run a 5.0 beta at protocol version 14 and hold documents indexed by an unsigned property keep generation-0 keys the new binary does not read: reset them, as for other protocol version 14 changes.

PV14 changelog: item 90 in v14.rs. The book's integer width section (contract-keywords/property-schemas.md) now says how integers are keyed.

In-place changes to shipped generations

Generation	Selected by	Why consensus cannot change there
`VotePollPaths::contenders_path` of `ContestedDocumentResourceVotePollWithContractInfo` and `ContestedDocumentResourceVotePollWithContractInfoAllowBorrowed` (`drive/votes/paths.rs`)	every protocol version	They now call `serialize_value_for_vote_poll_key`, which is `serialize_value_for_key_v0`: what `serialize_value_for_key` was before 14, so the bytes are the same there.
`ResolvedVotePollsByDocumentTypeQuery` (`query/vote_polls_by_document_type_query.rs`) and `verify_contests_proof` 0	every protocol version	The same pair, `serialize_value_for_key_v0` / `deserialize_value_for_key_v0`, in both directions: what the generic methods were before 14.
`verify_chained_documents_proof` 0 and `composite_document_query`'s `decode_document_trios`	every protocol version	They pass the platform version to `synthesize_index_only_document`, which they reach only for an indexOnly type, and indexOnly parses only from protocol version 14.
GroveDB batch application (#1016, #1017, not version-gated)	every protocol version	A malformed batch that panicked now returns `InvalidBatchOperation`. A batch that panicked never committed, and a batch that applied before applies the same.
`CONTRACT_VERSIONS_V6`, `SYSTEM_LIMITS_V4`, GroveDB's `GROVE_V4` (`move_element`, `reference_through_conditional_insert`), `IntegerRangeKeyType`, `synthesize_index_only_document`, `add_contested_indices_for_contract_operations` 1, `add_preallocated_index_tree_operations` and `document_create_cost`, document type parser generation 3 (`validate_integer_range_sources_keyed_in_value_order`), `add_version_items_to_all_contracts`, `transition_to_version_14`	protocol version 14 only	Protocol version 14 has not shipped.

should_keep_the_flipped_order_before_protocol_version_14 runs the queries at protocol version 13 and gets the old answers, and should_key_a_vote_poll_value_as_generation_0_in_every_version shows the poll keys are the same at 13 and 14.

How Has This Been Tested?

dpp: generation 1 keys every unsigned width in value order and reads them back; a property ending a range-summable index keeps generation 0's key whatever it sums, and the conversions between generations leave it alone, while one summed without range sums does not; an integerRange on an unsigned property ending a range-summable index is refused, on a signed one it is not; generation 0 keys map one to one to generation 1 keys; other types are keyed the same by both; serialize_value_for_key at 13 and 14 through the dispatcher; vote poll keys at 13 and 14; integerRange window keys.
drive (query::unsigned_index_key_order_tests): the table above, with every result also proved and verified, at 14 and at 13.
drive (rekey_unsigned_integer_index_values):
- documents written at 13 are moved, and the stored tree afterwards is the one before with only the unsigned value keys changed (elements, flags, wrappers, tree types and aggregates equal). Covers nested levels under a string, a unique index, null values, u64 values above i64::MAX and keys that trade places; queries, proofs, deletes, inserts and updates at 14 then work on the moved documents;
- a summed index with a range-summable continuation (a wrapped sum tree under each value) is moved the same way;
- an index range-summing a property that an update at 13 made unsigned while it held negative values keeps its keys (moved, the activation block would overflow a node sum);
- an index range-summing weights of ±2^62 keeps its keys, and at 14 equality reads, a delete of a document written at 13 and a new insert all find the keys where they are;
- moving one key per batch leaves the records and aggregates one batch does, the pair that trades keys kept together;
- two unsigned levels and a unique index's references, read in slices and moved in batches of 2 and 3, end as one batch does, and queries at 14 read them in value order;
- the tree of documents under a moved key keeps its root key and count;
- a token history shaped type (indexes led by an identifier, one continuing with a u64 amount, siblings with $createdAt and another identifier) moves only the amount keys;
- a contest on a u8 value started and voted on at 13 is joined at 14 under the same poll; the vote stays under it, and both the contested values listing and the voter's vote record prove the value 200, the listing at 13 too;
- a contest started and voted on at 13 ends at 14: found by its end date, tallied, its winner awarded under the new key, and its end date, contenders and vote cleaned up.
drive (document::cost): a post whose tier keeps the old keys preallocates its likes' trees under the likes' key, in the fee estimate and in what Drive writes, and a like lands in them.
drive: shared_prefix_aggregate_v13_and_v14_layouts_coexist now runs the upgrade's rewrite between its v13 writes and its v14 operations, as a chain does; the withdrawals query tests' pinned app hash moves (their fixture parses the withdrawal type with sized integers; the system withdrawals contract keeps i64).
drive-abci: a protocol version 13 platform with u8 documents answers the range counts correctly after transition_to_version_14.

Local runs on the final code: cargo test -p dpp --lib -- document_type, cargo test -p drive --lib on the rewrite, key order, cost, preallocation, contested, vote poll and integer range tests, cargo test -p drive-abci --lib on the token history and protocol change tests, the 13 to 14 chain upgrade strategy test, and cargo clippy on dpp, drive and platform-version. An earlier round of this PR ran the full dpp, drive and drive-abci suites, strategy tests included.

Breaking Changes

Consensus: from protocol version 14 an unsigned integer index value is keyed by its big-endian bytes (except an unsigned property ending a rangeSummable index), and the first block of protocol version 14 moves the keys stored before. Contract validation at protocol version 14 refuses an integerRange on an unsigned property ending a rangeSummable index. GroveDB is bumped to b50a583377. Clients see the new key bytes wherever they read raw index keys (the hex keys of count, sum, average and ranked group entries); a client that verifies proofs must use protocol version 14's tables, as for every other protocol version 14 change.

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
I have made corresponding changes to the documentation if needed
If I added or changed GroveDB structure, I described it in the area's structure.rs, regenerated grovedb-structure.json, and checked the structure viewer link posted on this pull request (no structure change: the same trees, rekeyed)

For repository code-owners and collaborators only

I have assigned this pull request to a milestone

🤖 Generated with Claude Code

PR Hygiene · `480c490`

Bots — coderabbitai not yet · thepastaclaw not yet — /skip-bots proceeds without the ones not yet reported
Self-review — address shumkov requested changes, then post /self-reviewed
Reviewer requests paused — your 5 review slots are occupied; this PR is excluded from reviewers' queues. Required approvals still count without a slot.
Build running
Approvals — you own every area touched; none needed

When every merge requirement is met, the PR Hygiene check passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.

Summary by CodeRabbit

Bug Fixes
- From protocol version 14, unsigned-integer indexes sort in numeric order, improving range queries, ascending and descending results, and aggregate counts. Earlier protocol versions retain their previous ordering.
- During the protocol version 14 upgrade, existing unsigned-integer index keys are updated in batches. Vote-poll paths, system fields, and indexes summing potentially negative values retain their existing key behavior.
- Pagination now returns all documents on a full page instead of using a result slot for an empty cursor.
Validation
- Configurations that combine an integer-range index with an unsigned property used by a range-summing index over potentially negative values are rejected.
Documentation
- Clarified unsigned-integer index ordering and integer-range registration requirements.

Unsigned integer document properties were keyed in indexes with their top bit flipped as if they were signed, so the values from the middle of the type's range up sorted below the rest: range queries, ordering, range counts and sums across that point came out wrong. From protocol version 14 they are keyed by their big-endian bytes (serialize_value_for_key 1). The protocol version 14 features that key integers themselves (integerRange window starts, indexOnly synthesis) use the new keys from the start, contest vote poll paths keep the generation 0 keys, and transition_to_version_14 rewrites every key stored before (Drive::rekey_unsigned_integer_index_values). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions · 2026-10-08T21:44:54Z

📖 Book Preview built successfully.

Download the preview from the workflow artifacts.
To view locally: download the artifact, unzip, and open index.html.

Updated at 2026-10-12T08:00:47.281Z

github-actions · 2026-10-08T21:44:56Z

Waiting for bot review — coderabbitai ✓ · thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

coderabbitai · 2026-10-08T21:45:33Z

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: dashpay/platform/.coderabbit.yaml
Review profile: CHILL
Plan: Advanced
Run ID: ceecba2f-91f2-4db5-95bb-59494fe3f870

📥 Commits

Reviewing files that changed from the base of the PR and between 83ee86d and b459330.

📒 Files selected for processing (1)

packages/rs-drive/src/drive/contract/migration/rekey_unsigned_integer_index_values.rs

🚧 Files skipped from review as they are similar to previous changes (1)

packages/rs-drive/src/drive/contract/migration/rekey_unsigned_integer_index_values.rs

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

Walkthrough

Protocol version 14 introduces generation-1 encoding for eligible unsigned integer index keys and rewrites existing eligible entries in bounded batches. Query synthesis decodes keys by platform version. Vote-poll paths and other specified keys retain generation 0.

Changes

Unsigned Index Key Ordering

Layer / File(s)	Summary
Define and activate key generations `packages/rs-dpp/src/data_contract/document_type/{index/integer_range.rs, methods/mod.rs, methods/versioned_methods.rs, property/mod.rs, class_methods/try_from_schema/*, index/mod.rs}`, `packages/rs-platform-version/src/version/{dpp_versions/dpp_contract_versions/v6.rs, v14.rs}`	Generation 1 encodes eligible unsigned integer keys as plain big-endian bytes. Platform version 14 selects this generation. Signed keys, system fields, and unsigned keys ending range-summable indexes over signed values retain generation 0. Full validation rejects the specified incompatible integer-range index configuration.
Rekey stored indexes during the version-14 transition `packages/rs-drive/src/drive/contract/migration/`, `packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/{v0/mod.rs, token_history_storage_flags_tests.rs}`, `packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/shared_prefix_aggregation_e2e_tests.rs`, `packages/rs-platform-version/src/version/system_limits/`	The migration traverses contracts and eligible index trees, then moves changed keys in bounded batches. The transition checks whether the rewrite has run before invoking it. Version 14 sets the batch limit to 10,000 moves.
Use the required generation in query and vote paths `packages/rs-drive/src/drive/document/insert_contested/.../v1/mod.rs`, `packages/rs-drive/src/drive/votes/paths.rs`, `packages/rs-drive/src/query/{composite_document_query/mod.rs, index_only_synthesis.rs, vote_polls_by_document_type_query.rs}`, `packages/rs-drive/src/verify/{chained_document/verify_chained_documents_proof/v0/mod.rs, voting/verify_contests_proof/v0/mod.rs}`, `book/src/drive/index-only-document-types.md`, `packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/common/mod.rs`	Index-only synthesis decodes values using the platform version. Contested-index insertion converts values to generation 0. Vote-poll paths use generation-0-specific serialization and deserialization.
Validate ordering and migration behavior `packages/rs-drive/src/query/{mod.rs, unsigned_index_key_order_tests.rs}`, `packages/rs-drive/tests/query_tests.rs`, `book/src/contract-keywords/{property-schemas.md, integer-range.md}`, `Cargo.toml`	Tests cover unsigned ordering, range counts, query results, proofs, and migration behavior. Documentation describes encoding rules and exceptions. Grovedb workspace dependencies use an updated revision.

Priority: ⬆️ High

Estimated code review effort: 4 (Complex) | ~60 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant ProtocolUpgrade
  participant DriveMigration
  participant ContractLayouts
  participant IndexTrees
  ProtocolUpgrade->>DriveMigration: Check transition state and invoke rekey
  DriveMigration->>ContractLayouts: Read contracts and document type layouts
  ContractLayouts->>DriveMigration: Identify eligible unsigned integer index trees
  DriveMigration->>IndexTrees: Rewrite eligible keys in bounded batches
  DriveMigration-->>ProtocolUpgrade: Return migration result

Suggested reviewers: shumkov

Merge Risk: ⚪ Minimal · up to b4593

The identified key-encoding mismatch is prevented by update validation, and the migration preserves compatibility for signed-sum indexes. No actionable merge-blocking risk remains after normal checks.

Pre-merge checks | 4 | 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 79.26% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 135 functions across 31 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: encoding unsigned integer index values in value order from protocol version 14.

Fix all pre-merge checks with AI

✨ Finishing Touches 💡 2

📝 Generate docstrings 💡

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Commit to this branch
Create a new PR

🛠️ Fix failing CI checks 💡

Commit to this branch
Create a new PR

Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

thepastaclaw · 2026-10-08T21:49:29Z

🕓 Review not started yet because the new head is waiting for the 30-minute push debounce.

Request normal review — click when the PR is ready for review.
Request priority review — click to move this review to the front of the queue.

Commit 480c490. Normal review starts when eligible; priority review starts as soon as a slot is available.

thepastaclaw

Final validation — Phase 1 + Phase 2

The versioned encoding and vote-poll compatibility changes are coherent, but the activation migration can fail on previously valid signed-sum indexes. One blocking issue and five non-blocking suggestions remain after source verification and deduplication. This was static verification at head 4a9c8f5; no builds or tests were run, and Rust workspace CI was still in progress in the supplied 2026-10-08T22:17:14Z snapshot.

🔴 1 blocking | 🟡 5 suggestion(s)

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 7: gemini-3.8-flash-high (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

Triage: critical by gpt-6.1-sol (effort low) — The large, intricate diff changes persisted index encoding and introduces Drive::rekey_unsigned_integer_index_values in packages/rs-drive/src/drive/contract/migration/rekey_unsigned_integer_index_values.rs, a storage migration activated at protocol version 14.
Phase 1 reviewers: gemini-3.8-flash-high — general (completed, effort high); agent phase1-reviewer, gemini-3.8-flash-high — rust-quality (completed, effort high); agent phase1-reviewer
Phase 1 model: gemini-3.8-flash-high — antigravity quota: weekly 78% left, 5h 67% left
Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer

🤖 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/contract/migration/rekey_unsigned_integer_index_values.rs`:
- [BLOCKING] packages/rs-drive/src/drive/contract/migration/rekey_unsigned_integer_index_values.rs:195-201: Rebuilding signed-sum indexes can halt PV14 activation
  Preserving the moved elements' sums does not ensure that the rebuilt Merk tree's internal aggregates remain representable. PV13 permits an unsigned u8 indexed property `grade` with `summable: "weight"` and `rangeSummable: true`, where the required signed i64 `weight` admits values from -2^62 to 2^62. Let M = 2^62 and insert separate documents in this order: `(grade, weight) = (1, M), (128, -M), (2, M), (3, -M)`. Generation 0 sorts the keys as `[128, 1, 2, 3]`; these insertions produce a valid balanced tree rooted at grade 1, with grade 128 on the left and grades 2 and 3 on the right. Its stored aggregates fit in i64 and its total is zero.

  The removal batch empties the property-name tree, and the insertion batch rebuilds it in generation-1 order `[1, 2, 3, 128]`. In the pinned GroveDB revision 9791d277207998b7789c85b0745e99d37040a12e, `merk/src/tree/ops.rs::build` selects `batch.len() / 2` as the root. The new tree therefore has grade 3 at its root and grades 1 and 2 in its left subtree. That subtree sums to 2^63, which `TreeNode::aggregate_data` rejects with checked i64 addition despite the complete index still summing to zero.

  This error propagates through `transition_to_version_14` into `run_block_proposal`, whose dispatcher performs upgrade events before processing state transitions. Previously valid permissionless state can therefore prevent every activation-block attempt from completing, including empty proposals or proposals containing corrective deletes. Make the migration support these valid historical signed-sum states, and add a PV13-to-PV14 regression covering contributions near the aggregate limits.
- [SUGGESTION] packages/rs-drive/src/drive/contract/migration/rekey_unsigned_integer_index_values.rs:167-169: Bound the migration's live query and operation buffers
  The insertion batch retains every recursively copied descendant until all changing children at this level have been processed. When an index's first property is unsigned, this includes operations for the entire non-null index, with owned destination paths for its descendants. Separately, `stored_children` executes an unlimited query and materializes its results into a vector. Contract-ID pagination does not bound either allocation, and the number of documents in a legal index is not bounded by its schema. A sufficiently large index can exhaust memory during the mandatory activation block instead of returning a typed error. Use bounded child reads and staged write batches within the transaction, preserving both members of each top-bit-flip collision pair before replacing either. Paginating reads alone will not fix the retained insertion batch.

In `packages/rs-drive/src/drive/document/insert_contested/add_contested_indices_for_contract_operations/v1/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/drive/document/insert_contested/add_contested_indices_for_contract_operations/v1/mod.rs:180-189: Exercise unsigned contest continuity across the v14 upgrade
  The new DPP test proves the generation-0 vote-poll codec invariant, but its schema declares an ordinary index and does not exercise this separate contested write-side conversion. The existing `should_keep_the_plain_tree_of_a_poll_started_before_protocol_version_14` test uses DPNS string values and does not run the migration, so it cannot detect disagreement between unsigned contender insertion, poll paths, vote records, and contest proof decoding. Add an integration case that creates an unsigned contested value and records a vote at PV13, performs the PV14 upgrade, and adds another contender or vote to the same poll. Verify that the original vote remains accessible, both contenders address the original poll, and contested-resource listing and proof verification recover the original numeric value.

In `packages/rs-dpp/src/data_contract/document_type/property/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/document_type/property/mod.rs:3430-3434: Compare key widths without truncating the slice length
  This new public helper promises to convert only keys of the unsigned type's exact width, but `key.len() as u16` makes the check wrap modulo 65536. For example, `DocumentPropertyType::U32` accepts a 65540-byte slice and flips its first bit rather than returning `None`. Normal stored index keys are much smaller, so this is not an established production exploit, but the helper's width guard does not enforce its documented contract. Convert the fixed width to `usize` and compare it directly with the slice length.

In `packages/rs-dpp/src/data_contract/document_type/methods/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/document_type/methods/mod.rs:831-834: Correct the generation-0 helper's indexOnly documentation
  This comment says indexOnly row commitments and synthesized IDs always use generation-0 bytes, but neither implementation calls this helper. Row commitments obtain property bytes through the active version's document key encoding, and synthesized IDs hash the actual encoded path and terminal bytes. These PV14-only features therefore use generation 1 from their introduction, as the PR description states. Document the actual cross-version invariant here—vote-poll paths and their vote records—rather than an indexOnly invariant the implementation does not follow.
- [SUGGESTION] packages/rs-dpp/src/data_contract/document_type/methods/mod.rs:852-856: Reuse already-owned keys for identity generation conversions
  `serialize_value_for_vote_poll_key` first allocates a key with `serialize_value_for_key`, then passes it to this helper, which allocates another vector even when generation 0 is already active or the property's encoding is unchanged. This introduces an avoidable second allocation in vote-poll path construction. Reuse the original owned buffer for these identity conversions, for example through an owned-key conversion variant that mutates unsigned keys in place. A borrowed `Cow` alone would not remove the extra copy if the vote-poll helper immediately converts it back into its required owned `Vec`.

github-actions · 2026-10-08T22:35:21Z

Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them.
Full checklist in the description.

… in range (PV14) The protocol version 14 rewrite of unsigned integer index value keys now: - moves the children of each first-level index tree in groups capped by SystemLimits::max_rekey_batch_insertions (10,000 in SYSTEM_LIMITS_V4), a key and its top-bit-flipped partner always in the same group; - keeps the earlier keys of an index tree whose sums could leave the signed 64-bit range in a rebuilt Merk shape, instead of failing the block; - runs only while the withdrawal limit trees the transition inserts last are absent, so a second run of transition_to_version_14 is harmless; - walks contracts through Drive::for_each_contract_in_state, shared with add_version_items_to_all_contracts. Vote poll keys call serialize/deserialize_value_for_key_v0 directly, tree_key_from_generation_0 has its own version dispatch, the unsigned key width check compares usize lengths, and generation 1 wraps generation 0. Docs and in-place comments corrected; tests cover a sum-bearing continuation, the sum-range guard, and a contest started and voted on at 13 and joined at 14. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…ndex-key-order-pv14 # Conflicts: # packages/rs-platform-version/src/version/v14.rs

github-actions · 2026-10-09T05:56:09Z

Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 1 thread unresolved — resolve it. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

thepastaclaw

Re-review — Final validation — Phase 1 + Phase 2

The PV14 codec dispatch and vote-poll compatibility changes are coherent, but the migration still has two blocking issues: skipped indexes become incompatible with active readers and writers, and descendant buffers remain unbounded during mandatory activation. Five prior findings are fixed; the buffer-bounding finding remains valid, and generation-1 encoding also introduces an avoidable allocation. Verification was static at the exact requested head; no builds or tests were run, and the supplied CI snapshot still had build/test prerequisites queued.

🔴 2 blocking | 🟡 1 suggestion(s)

1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 10: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 11: gemini-3.8-flash-high (agent: phase1-reviewer, role: rust-quality); reviewer 12: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

Triage: critical by gpt-6.1-sol (effort low) — The intricate change introduces a persisted index-key storage migration in packages/rs-drive/src/drive/contract/migration/rekey_unsigned_integer_index_values.rs and activates it during the protocol upgrade, while changing consensus-visible key encoding and query behavior.
Phase 1 reviewers: gemini-3.8-flash-high — general (completed, effort high); agent phase1-reviewer, gemini-3.8-flash-high — rust-quality (completed, effort high); agent phase1-reviewer
Phase 1 model: gemini-3.8-flash-high — antigravity quota: weekly 60% left, 5h 41% left
Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — general (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — security-auditor (completed, effort xhigh); agent phase2-reviewer
Model comparison: every Phase-2 reviewer also ran on gpt-6-astra; the verifier weighed both sets without knowing which model wrote which

🤖 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/contract/migration/rekey_unsigned_integer_index_values.rs`:
- [BLOCKING] packages/rs-drive/src/drive/contract/migration/rekey_unsigned_integer_index_values.rs:155-162: Keep skipped indexes compatible with PV14 readers and writers
  Returning success leaves generation-0 keys in storage, while CONTRACT_VERSIONS_V6 unconditionally selects generation 1 for serialization and deserialization. No index-generation marker or corresponding consumer dispatch records the exception. In the new ±2^62 regression, grade 1 remains under 0x81, but PV14 equality lowering in query/conditions.rs requests 0x01. Document inserts and removals obtain the same incompatible keys through get_raw_for_document_type; aggregate and proof query builders also select the active codec. The transition subsequently installs its completion marker, so the skipped index is not retried.

  This also defeats uniqueness, not just ordering. A unique u8 grade index with required signed i64 weight, summable: "weight", and rangeSummable: true can hold historical contributions (1, 2^62) and (2, -2^62). Both stored sums fit, but their magnitudes total 2^63, so this preflight skips the index. A PV14 create with grade 1 and weight 0 probes the empty 0x01 location: validate_uniqueness_of_data_v1 builds an equality query, and the terminal batch_insert_if_not_exists check targets the same new location. Neither detects the existing grade-1 document under 0x81. Queries can likewise prove absence at the location now interpreted as that existing value.

  Either migrate valid historical layouts without overflowing, or persist an authenticated encoding choice and honor it consistently in queries, proof verification, uniqueness validation, estimation, and mutations. Returning an error alone would restore the original activation blocker. Extend the retained-tree regression beyond dump equality to check PV14 proved reads, duplicate rejection, inserts, updates, and deletes.
- [BLOCKING] packages/rs-drive/src/drive/contract/migration/rekey_unsigned_integer_index_values.rs:226-239: Bound the migration's live query and operation buffers
  (existing thread: https://github.com/dashpay/platform/pull/5350#discussion_r4224822257)
  The 10,000-element threshold is checked only after copy_rekeyed has recursively buffered a complete first-level child and its collision partner. A legal non-unique u8 index whose documents all have grade 0 has one changing value subtree containing every document reference. All descendant insertions and their owned destination paths enter the batch before this check executes, so max_rekey_batch_insertions does not bound an individual move.

  The read side remains unbounded independently: stored_children uses SizedQuery::new(Query::new_range_full(), None, None), the initial results are retained in a BTreeMap, and sums_stay_in_range_in_any_shape uses the same unlimited reads even for trees without sums. That preflight can therefore materialize a large membership bucket before write batching begins. Permissionless state can grow incrementally through ordinary submissions, but every validator must traverse it together during transition_to_version_14, which run_block_proposal executes before block transitions. Sufficiently large historical state can exhaust activation-block memory; an empty proposal or corrective deletes in that block do not bypass the scan.

  Bound child reads, preflight traversal state, and retained insertion operations within individual subtrees. Preserve collision partners using bounded transactional staging rather than retaining their complete copies. Add coverage where one value bucket exceeds a deliberately small batch limit.

In `packages/rs-dpp/src/data_contract/document_type/property/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/document_type/property/mod.rs:3359-3362: Flip unsigned key bytes in the owned encoding buffer
  encode_value_for_tree_keys already returns an owned Vec, but unsigned_tree_key_in_other_generation calls to_vec() for every non-null unsigned key. Generation-1 encoding therefore allocates and copies a second buffer just to flip one bit, then discards the original allocation. This new path is used by ordinary index writes, removals, uniqueness probes, and query bounds. Mutate the owned buffer directly; successful legacy encoding guarantees fixed-width unsigned keys, with an empty key for null. Keep the borrowed conversion helper for callers that actually need a separate copy.

encode_value_for_tree_keys_v1 flips the top bit of the key generation 0 already returned instead of copying it into a second buffer. Same bytes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…ndex-key-order-pv14 # Conflicts: # packages/rs-platform-version/src/version/v14.rs

github-actions · 2026-10-09T12:41:16Z

Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 1 thread unresolved — resolve it. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

thepastaclaw

Re-review — Final validation — Phase 1 + Phase 2

Static verification at df20a95 confirms that six prior findings are addressed. Two remain: skipped indexes retain keys incompatible with PV14 operations, and migration batching does not bound allocations within a single large subtree. No builds or tests were run; the supplied CI snapshot from 2026-10-09T12:44:02Z had Rust workspace tests and Kotlin tests queued, with the JS build still running.

🔴 1 blocking | 🟡 1 suggestion(s)

2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 5: gemini-3.8-flash-high (agent: phase1-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

Triage: critical by gpt-6.1-sol (effort low) — The large, intricate diff changes consensus-visible index encoding and persisted-state migration in rekey_unsigned_integer_index_values.rs, activated by perform_events_on_first_block_of_protocol_change/v0/mod.rs.
Phase 1 reviewers: gemini-3.8-flash-high — general (completed, effort high); agent phase1-reviewer, gemini-3.8-flash-high — rust-quality (completed, effort high); agent phase1-reviewer
Phase 1 model: gemini-3.8-flash-high — antigravity quota: weekly 35% left, 5h 73% left
Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer

🤖 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/contract/migration/rekey_unsigned_integer_index_values.rs`:
- [BLOCKING] packages/rs-drive/src/drive/contract/migration/rekey_unsigned_integer_index_values.rs:155-162: Keep skipped indexes compatible with PV14 readers and writers
  (existing thread: https://github.com/dashpay/platform/pull/5350#discussion_r4227185552)
  This successful return preserves generation-0 keys without recording an encoding exception, while PV14's CONTRACT_VERSIONS_V6 unconditionally selects generation 1. Ordinary document insertion and removal reach that codec through get_raw_for_document_type, and query bounds and aggregate proof verification use the same globally selected generation.

  The existing near-limit regression provides a concrete affected state: with M = 2^62, its grade-1 contribution remains at 0x81 and its grade-128 contribution remains at 0x00. After activation, a grade-1 lookup addresses 0x01 instead, while grade 129 addresses the historical grade-1 bucket. More seriously, an aggregate sum over grade BETWEEN [0, 0] addresses 0x00 and selects the historical grade-128 contribution of -M despite there being no grade-0 document. verify_aggregate_sum_proof reconstructs the same generation-1 path query, so Merkle verification does not detect the incorrect logical predicate. Subsequent writes can mix different values in one bucket, and updates or deletes construct paths different from those containing the historical entries.

  The warning and unchanged-storage assertion prevent the rebuild failure but do not preserve index usability. Safely migrate these historical signed-sum states, or persist an authenticated encoding choice and honor it throughout queries, proof verification, insertion, uniqueness checks, updates, and deletion. Extend the skipped-index regression with post-upgrade queries, aggregate proofs, and mutations.
- [SUGGESTION] packages/rs-drive/src/drive/contract/migration/rekey_unsigned_integer_index_values.rs:239-245: Bound the migration's live query and operation buffers
  (existing thread: https://github.com/dashpay/platform/pull/5350#discussion_r4224822257)
  The threshold is checked only after copy_rekeyed has recursively appended every descendant operation for a complete child subtree and its collision partner. A legal non-unique u8 index can contain an arbitrarily large document population under one value, so migrating that single bucket retains all its insertion operations and owned destination paths before the first flush, regardless of max_rekey_batch_insertions = 10,000.

  Reads are independently unbounded: stored_children uses SizedQuery::new(Query::new_range_full(), None, None), rekey_index_tree retains the complete first-level result in a BTreeMap, and the sum preflight materializes complete child lists and pending descendant paths. Grouping improves accumulation across completed subtrees but does not establish a live-memory bound for the mandatory activation migration.

  Paginate child reads and enforce insertion-buffer limits inside descendant traversal, using transaction-backed staging or an equivalent relocation mechanism to preserve collision partners before replacement. Keep batch boundaries deterministic through the versioned limits, and add coverage for one moved subtree larger than the configured threshold.

github-actions · 2026-10-10T02:43:40Z

Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them.
Full checklist in the description.

shumkov

We agreed to postpone for 5.1 #5338

thepastaclaw

Re-review — Final validation — Phase 1 + Phase 2

At head b459330, no in-scope blockers remain: eleven prior findings are fixed, and the remaining transaction-wide migration memory growth is explicitly accepted and deferred. One verified, non-blocking performance improvement remains in unsigned scalar decoding. This assessment is static-only; the supplied CI snapshot reports Rust workspace tests pending and the JavaScript build failing, so it does not establish passing build or test results.

🟡 1 suggestion(s)

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

Triage: critical by gpt-6.1-sol (effort low) — The intricate, cross-cutting diff changes persisted index serialization and adds a protocol-activated storage migration in packages/rs-drive/src/drive/contract/migration/rekey_unsigned_integer_index_values.rs, meeting both the complexity and critical-surface requirements.
Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 14% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer

🤖 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-dpp/src/data_contract/document_type/property/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/document_type/property/mod.rs:3456-3465: Avoid allocating a temporary Vec for each decoded unsigned scalar
  The conversion helper copies the input into a new Vec solely to flip its first bit; this method then decodes that buffer and immediately drops it. Through deserialize_value_for_key_v1, index-only document synthesis executes this path for every non-null unsigned component of each returned or proof-verified row, introducing one avoidable heap allocation per component. All supported unsigned key types have fixed widths of at most 16 bytes. Use a stack buffer here while preserving the exact-width validation and existing decoder; retain the owned conversion helper for callers that need an owned key.

github-actions · 2026-10-12T04:56:51Z

Bots are done — your move: address shumkov requested changes, then post /self-reviewed.
Full checklist in the description.

decode_value_for_tree_keys_v1 flips the top bit in a 16-byte stack buffer instead of copying the key into a new Vec, keeping the exact-width check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions · 2026-10-12T04:58:39Z

Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

thepastaclaw

Re-review — Final validation — Phase 1 + Phase 2

At e38a3c8, the codecs, compatibility checks, migration traversal, and regression coverage address twelve prior findings; transaction-sized activation memory remains explicitly accepted and deferred, not eliminated. One in-scope performance suggestion remains in contested index insertion. This was a static verification: git diff --check passed, no builds or tests were run, and the supplied CI snapshot has Kotlin SDK and PR Hygiene checks pending without completed Rust-test evidence.

🟡 1 suggestion(s)

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

Triage: critical by gpt-6.1-sol (effort low) — The intricate cross-cutting diff changes persisted index encoding and implements a protocol-upgrade storage migration in packages/rs-drive/src/drive/contract/migration/rekey_unsigned_integer_index_values.rs, meeting both the complexity and critical-surface requirements.
Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 14% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer

🤖 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/insert_contested/add_contested_indices_for_contract_operations/v1/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/drive/document/insert_contested/add_contested_indices_for_contract_operations/v1/mod.rs:182-184: Reuse the owned buffer when converting contested index keys
  For concrete documents, DocumentInfo::get_raw_for_document_type returns an owned DriveKeyInfo::Key(Vec<u8>). This arm borrows that buffer and calls tree_key_in_generation_0, which allocates a replacement vector: unchanged keys use key.to_vec(), and changed unsigned keys are copied by unsigned_tree_key_in_other_generation. The original allocation is then discarded. This adds an allocation and full-key copy for every present contested index component, including DPNS string components whose encoding does not change. Add an ownership-taking conversion that preserves the version dispatch and exact-width checks, flips unsigned keys in the existing buffer, and returns unchanged keys directly. Keep the borrowed conversion for KeyRef and callers that need a separate owned result.

A referenced document's value was keyed with the referenced type's codec when preallocating a referring indexOnly type's trees. An unsigned property that ends an index range-summing values that can be negative keeps generation 0's keys, so the two types can key one value differently: the trees were created beside the ones entries use. The key is now carried through generation 0 into the referring type's codec, in the insert path and in the fee estimator. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

The unsigned index key rewrite applied one GroveDB batch per tree with moved keys, each propagating to the root. Moves now gather across trees, document types and contracts into batches of max_rekey_batch_moves; a batch is applied early only when a tree is about to move its own keys while it holds moves below them, which GroveDB refuses. Trees whose children all have fixed keys are walked by point reads, and a tree that fits one page is read with one query. Tests: two unsigned levels and a unique index's references read in slices and moved in small batches; the Merk tree under a moved key kept as stored; a contest started before protocol version 14 awarded and cleaned up after it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

DocumentTypeV0Methods::has_unsigned_integer_tree_key shared its name with DocumentPropertyType::has_unsigned_integer_tree_key but answered whether the generations key a property differently, which is false for an unsigned property that keeps generation 0's keys. It is now tree_keys_differ_between_generations. The docs that said every unsigned property gets generation 1 name the properties that keep generation 0, the rule test checks the generation conversions leave a kept property's keys alone, and the second in-place edit of verify_contests_proof v0 carries its own comment. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions · 2026-10-12T06:23:04Z

Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

…eSummable index The rule kept generation 0's keys only when the index range-summed a property whose type is not unsigned. Before protocol version 14 an update can remove a summed property's negative `minimum` (or turn on sized integer types), making it unsigned while its sum tree still holds negative values; the activation block then moved the keys and overflowed a node sum. An unsigned property that ends a `rangeSummable` index now keeps generation 0's keys whatever the index sums (`Index::ends_range_summable_with`), in every index of its type. The integerRange refusal moves from the shared parse_indices into document type parser generation 3 (`validate_integer_range_sources_keyed_in_value_order`) and asks the built type how it keys the source; the single-caller wrapper is inlined. A contested insert converts an owned key in place (`tree_key_into_generation_0`, `unsigned_tree_key_into_other_generation`) instead of copying it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions · 2026-10-12T06:50:18Z

Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

… last insert The nullifier backfill runs after them in transition_to_version_14. The rekey guard only needs the trees to come from this transition. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions · 2026-10-12T07:53:20Z

Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

transition_to_version_14 runs once per chain, in the block whose transaction moves its protocol version to 14; a block that is not committed drops that transaction. The check for the withdrawal limit trees guarded a second run that cannot happen, so the rekey runs unconditionally and the test runs the transition once. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions · 2026-10-12T08:02:22Z

Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

QuantumExplorer · 2026-10-12T08:06:26Z

Reviewed

Resolves serialize_value_for_key generation 1 between #5350 (unsigned integers keyed in value order) and this branch (a typed array keyed by its elements): one generation 1 resolves a typed array to its element type first, so an unsigned element takes the value-order key, and every other element keeps the element type's encoding. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Conflicts: - drive contract migration modules: #5350's `rekey_unsigned_integer_index_values` beside this branch's `apply_contract_rebuilding_document_types`. - v14 notes: #5350 took note 90 (unsigned integers keyed in value order), so the DPNS v3 note moves to 92, after the shorthands note 91; the upgrade's comment cites it as 92. #5369 gave the document reference targets `where_conditions`: the owner-keyed reveal test's expected `DeletableDocumentLookup` sets it empty. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions Bot added this to the v5.0.0 milestone Oct 8, 2026

github-actions Bot added the waiting-bots Waiting for the review bots to report on this head label Oct 8, 2026

thepastaclaw suggested changes Oct 8, 2026

View reviewed changes

github-actions Bot added waiting-self-review Waiting for the author to post /self-reviewed and removed waiting-bots Waiting for the review bots to report on this head labels Oct 8, 2026

QuantumExplorer and others added 2 commits October 9, 2026 07:44

Merge remote-tracking branch 'origin/v5.0-dev' into claude/unsigned-i…

05de508

…ndex-key-order-pv14 # Conflicts: # packages/rs-platform-version/src/version/v14.rs

github-actions Bot added waiting-bots Waiting for the review bots to report on this head and removed waiting-self-review Waiting for the author to post /self-reviewed labels Oct 9, 2026

thepastaclaw suggested changes Oct 9, 2026

View reviewed changes

Comment thread packages/rs-drive/src/drive/contract/migration/rekey_unsigned_integer_index_values.rs Outdated

Comment thread packages/rs-dpp/src/data_contract/document_type/property/mod.rs Outdated

QuantumExplorer and others added 2 commits October 9, 2026 14:25

perf(dpp): flip the unsigned key top bit in the owned generation-1 key

85737cc

encode_value_for_tree_keys_v1 flips the top bit of the key generation 0 already returned instead of copying it into a second buffer. Same bytes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Merge remote-tracking branch 'origin/v5.0-dev' into claude/unsigned-i…

df20a95

…ndex-key-order-pv14 # Conflicts: # packages/rs-platform-version/src/version/v14.rs

QuantumExplorer mentioned this pull request Oct 9, 2026

feat: batch operation to move a subtree to a new key, keeping its stored nodes dashpay/grovedb#1014

Closed

thepastaclaw suggested changes Oct 9, 2026

View reviewed changes

github-actions Bot added waiting-self-review Waiting for the author to post /self-reviewed bot-review-skipped A required review bot did not report; it was skipped by the window or by a person. and removed waiting-bots Waiting for the review bots to report on this head labels Oct 10, 2026

shumkov requested changes Oct 11, 2026

View reviewed changes

shumkov changed the title ~~fix(platform)!: key unsigned integer index values in value order (PV14)~~ fix(platform)!: key unsigned integer index values in value order [WIP] Oct 11, 2026

shumkov changed the base branch from v5.0-dev to v5.1-dev October 11, 2026 11:21

github-actions Bot removed this from the v5.0.0 milestone Oct 11, 2026

github-actions Bot added waiting-bots Waiting for the review bots to report on this head and removed waiting-self-review Waiting for the author to post /self-reviewed labels Oct 12, 2026

thepastaclaw reviewed Oct 12, 2026

View reviewed changes

Comment thread packages/rs-dpp/src/data_contract/document_type/property/mod.rs Outdated

github-actions Bot added waiting-self-review Waiting for the author to post /self-reviewed and removed waiting-bots Waiting for the review bots to report on this head labels Oct 12, 2026

perf(dpp): decode a generation-1 unsigned key through a stack buffer

e38a3c8

decode_value_for_tree_keys_v1 flips the top bit in a 16-byte stack buffer instead of copying the key into a new Vec, keeping the exact-width check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions Bot added waiting-bots Waiting for the review bots to report on this head and removed waiting-self-review Waiting for the author to post /self-reviewed labels Oct 12, 2026

thepastaclaw reviewed Oct 12, 2026

View reviewed changes

Comment thread .../src/drive/document/insert_contested/add_contested_indices_for_contract_operations/v1/mod.rs

QuantumExplorer and others added 3 commits October 12, 2026 09:22

shumkov changed the title ~~fix(platform)!: key unsigned integer index values in value order [WIP]~~ fix(platform)!: key unsigned integer index values in value order Oct 12, 2026

QuantumExplorer mentioned this pull request Oct 12, 2026

fix(drive)!: fee estimates size sum and count index trees as what they are (PV14) #5377

Merged

6 of 11 tasks

docs(drive-abci): the withdrawal limit trees are not the transition's…

efd69a0

… last insert The nullifier backfill runs after them in transition_to_version_14. The rekey guard only needs the trees to come from this transition. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

QuantumExplorer merged commit 976f827 into v5.0-dev Oct 12, 2026
12 of 13 checks passed

QuantumExplorer deleted the claude/unsigned-index-key-order-pv14 branch October 12, 2026 08:06

QuantumExplorer mentioned this pull request Oct 13, 2026

chore: merge v5.0-dev into v5.1-dev #5404

Merged

8 of 26 tasks

shumkov mentioned this pull request Oct 13, 2026

Track remaining devnet audit server fixes for v5.1 (PV15) #5410

Open

13 tasks

Conversation

QuantumExplorer commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Basic explanation

Issue being fixed or feature implemented

Before and after

What was done?

In-place changes to shipped generations

How Has This Been Tested?

Breaking Changes

Checklist:

PR Hygiene · 480c490

Summary by CodeRabbit

Uh oh!

github-actions Bot commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning)

Uh oh!

thepastaclaw commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Final validation — Phase 1 + Phase 2

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Oct 8, 2026

Uh oh!

github-actions Bot commented Oct 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Re-review — Final validation — Phase 1 + Phase 2

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Oct 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Re-review — Final validation — Phase 1 + Phase 2

Uh oh!

github-actions Bot commented Oct 10, 2026

Uh oh!

shumkov left a comment

Choose a reason for hiding this comment

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Re-review — Final validation — Phase 1 + Phase 2

Uh oh!

Uh oh!

github-actions Bot commented Oct 12, 2026

Uh oh!

github-actions Bot commented Oct 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Re-review — Final validation — Phase 1 + Phase 2

Uh oh!

Uh oh!

github-actions Bot commented Oct 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

QuantumExplorer commented Oct 8, 2026 •

edited

Loading

PR Hygiene · `480c490`

github-actions Bot commented Oct 8, 2026 •

edited

Loading

github-actions Bot commented Oct 8, 2026 •

edited

Loading

coderabbitai Bot commented Oct 8, 2026 •

edited

Loading

thepastaclaw commented Oct 8, 2026 •

edited

Loading

github-actions Bot commented Oct 9, 2026 •

edited

Loading

github-actions Bot commented Oct 9, 2026 •

edited

Loading

github-actions Bot commented Oct 12, 2026 •

edited

Loading

github-actions Bot commented Oct 12, 2026 •

edited

Loading

github-actions Bot commented Oct 12, 2026 •

edited

Loading

github-actions Bot commented Oct 12, 2026 •

edited

Loading

github-actions Bot commented Oct 12, 2026 •

edited

Loading