Repository navigation
feat(platform)!: delete and erase lifecycle for keep-history documents - #4657
Merged
Merged
Conversation
Contributor
|
Important Review skippedToo many files! This PR contains 230 files, which is 130 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry.
 |
Collaborator
|
✅ Final review complete — no blockers (commit 84ac1e7) · triage: critical |
shumkov
force-pushed
the
keep-history-lifecycle
branch
from
September 11, 2026 00:47
492f4b5 to
feea7d5
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## keep-history-storage-v2 #4657 +/- ##
===========================================================
- Coverage 75.60% 73.52% -2.09%
===========================================================
Files 2849 2888 +39
Lines 421354 438776 +17422
===========================================================
+ Hits 318567 322604 +4037
- Misses 102787 116172 +13385
🚀 New features to boost your workflow:
|
thepastaclaw
suggested changes
Sep 11, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
The lifecycle and erasure implementation is generally well covered, but two correctness issues remain. The WASM/JavaScript API cannot represent the documented continuation case where an identity other than the document owner submits a later erase chunk, and the history proof verifier accepts incomplete metadata proofs that can be interpreted as false lifecycle states.
🔴 1 blocking | 🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
criticalbygpt-6-astra(effort low) — This is a large, intricate cross-cutting change that directly modifies consensus validation and state-transition serialization, funds accounting/refunds, peer-facing protobuf/query deserialization, and storage migration/lifecycle deletion behavior. - Phase 1 reviewers: not run (skipped for throughput: 15 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-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/wasm-sdk/src/state_transitions/document.rs`:
- [SUGGESTION] packages/wasm-sdk/src/state_transitions/document.rs:597-625: Expose the erase submitter separately from the document owner
The WASM wrapper derives `owner_id` from the target document (`doc_inner.owner_id()` or the plain object's documented `ownerId`) and passes it as the final argument to `DocumentEraseTransitionBuilder::new`. On the Rust side, that field is the identity submitting and paying for the erase: it is used to fetch the contract nonce and to construct the outer batch transition. Therefore a continuation signed by an unrelated identity is built as though it were submitted by the original document owner, so the signer and transition owner do not match and validation rejects the continuation. Add a separately named submitter or payer identity to `DocumentEraseOptions`, use it for the builder and nonce lookup, and retain the document owner only as target metadata where needed. Defaulting the submitter to the document owner can preserve the first-owner call.
In `packages/rs-drive/src/drive/document/history/mod.rs`:
- [BLOCKING] packages/rs-drive/src/drive/document/history/mod.rs:569-575: Require absence proofs for lifecycle metadata lookups
`metadata_query` performs exact-key lookups for the current document pointer, the lifecycle record, and the per-document history tree, but verification sets `absence_proofs_for_non_existing_searched_keys` to `false`. With that option, GroveDB may return no row for a requested exact key without proving that the key is absent. `query.lifecycle()` then treats omitted metadata as absent and can derive `ABSENT`, `DELETED`, or another incorrect lifecycle state even when the authenticated root contains the omitted pointer or lifecycle record. A compromised DAPI server can consequently make clients accept a false lifecycle result. Verify this metadata query with absence proofs enabled and give the merged query a finite limit sufficient for all three exact terminal-key lookups.
shumkov
force-pushed
the
keep-history-lifecycle
branch
from
September 12, 2026 03:03
6e27f84 to
25959ad
Compare
thepastaclaw
suggested changes
Sep 12, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
The lifecycle implementation fixes both previously reported issues, but the reviewed range still contains four in-scope defects. Protocol-14 activation can reject valid legacy contracts, revision-based history queries can return authenticated false-empty results after deleting gapped histories, migration work is unbounded during activation, and SDK mocks lose lifecycle states and metadata. These issues require changes before merge.
🔴 3 blocking | 🟡 2 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
criticalbygpt-6-astra(effort low) — This large, intricate diff changes consensus validation and state-transition serialization, peer-facing gRPC/proof deserialization, persistent document storage/migration behavior, and accounting/refunds for deleting and erasing document revisions. - Phase 1 reviewers: not run (skipped for throughput: 15 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-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/class_methods/try_from_schema/v3/mod.rs`:
- [BLOCKING] packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v3/mod.rs:320: Preserve deserialization of valid legacy contested-history contracts
`reject_contested_keep_history` is called unconditionally, including when `full_validation` is false. Protocol 13 permits a document type with retained history and a contested index, but protocol-14 activation loads every stored contract using the new parser with `full_validation = false`. A valid legacy contract with this combination therefore fails to deserialize while the migration inventories contracts, causing the activation hook to abort before the migration can run. Keep the registration-time restriction separate from legacy-state deserialization, or provide an explicit migration-compatible parsing path, and cover registration under protocol 13 followed by protocol-14 activation.
In `packages/rs-drive/src/drive/document/history/mod.rs`:
- [BLOCKING] packages/rs-drive/src/drive/document/history/mod.rs:353-360: Keep revision-gap detection after a history document is deleted
Revision and `StartAtRevision` queries use an offset of `revision - 1`, but the gap check is only performed while `active` is true. A migrated history can legitimately retain revisions such as `[1, 3]`; while active, querying revision 3 is rejected because the latest revision differs from the retained count. Once deletion removes the current pointer, `active` becomes false, the check is skipped, and querying revision 3 applies offset 2 to the two retained entries and returns an empty page. The empty result contains no entries for `decode_entries` to inspect, so both the fetch path and proof verifier accept a false absence. Retain authenticated metadata sufficient to establish contiguity after deletion, or reject revision-position queries when contiguity cannot be proven.
In `packages/rs-drive/src/drive/document/migration/mod.rs`:
- [BLOCKING] packages/rs-drive/src/drive/document/migration/mod.rs:471-493: Bound and paginate legacy history migration work
`history_migration_entries` constructs a `Query` with `insert_all()` and materializes the complete history subtree into a vector. The caller retains those entries while constructing migration copies and applies the work in the single protocol-activation transaction. Pre-protocol-14 keep-history documents have no bound on the number of retained revisions, so a contract can accumulate a sufficiently large paid history before activation. During activation every validator must read, deserialize, duplicate, and rewrite that entire history synchronously; a large legal history can exhaust validator memory or exceed activation processing limits and prevent the upgrade from completing. Migration needs bounded resumable work, or a pre-activation bound that is enforced before such histories can accumulate.
In `packages/rs-sdk/src/mock/requests.rs`:
- [SUGGESTION] packages/rs-sdk/src/mock/requests.rs:273-306: Preserve all lifecycle states and timestamps in SDK mocks
The mock codec serializes lifecycle state as only `state == Active` and stores only `remaining_revisions`; it omits all four lifecycle timestamps. Deserialization consequently maps both `Deleted` and `Erasing` to `Absent` and resets lifecycle metadata to defaults. `expect_fetch` passes responses through this codec, so mocked callers cannot faithfully exercise deleted or erasing document behavior. The same lossy representation affects nested `DocumentHistoryProofInfo` data. Serialize the complete lifecycle state and all lifecycle fields, and add round-trip coverage for active, deleted, erasing, and absent results with non-default metadata.
In `packages/rs-drive/src/drive/document/lifecycle/tests.rs`:
- [SUGGESTION] packages/rs-drive/src/drive/document/lifecycle/tests.rs:1568-1573: Exercise proof verification when testing tampered lifecycle claims
The tampered lifecycle value is created after verification and only compared with the original using `assert_ne!`; it is never passed back through the proof verifier. This tests value inequality rather than rejection of a forged lifecycle claim, and would still pass if the verifier's new lifecycle-field checks were removed. Extend the response-level verifier harness with deleted and erasing results, mutate each lifecycle field independently while leaving the proof unchanged, and assert that `DocumentHistory::maybe_from_proof` rejects every tampered response.
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
The lifecycle changes are coherent, and the previously reported proof, mock, revision-gap, parser, and WASM submitter issues are fixed at the current head. One in-scope API correctness issue remains: the new WASM history query accepts JavaScript Numbers for exact u64 selectors, allowing values above 2^53−1 to be silently rounded before Rust parses them.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
criticalbygpt-6-astra(effort low) — This is a large, intricate diff that changes consensus validation and peer-facing state-transition deserialization, moves funds through lifecycle erasure refunds, and introduces storage migrations affecting document history. - Phase 1 reviewers: not run (skipped for throughput: 14 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-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/wasm-sdk/src/queries/document.rs`:
- [SUGGESTION] packages/wasm-sdk/src/queries/document.rs:178-183: Reject unsafe JavaScript Numbers for exact history selectors
The public TypeScript API accepts `number` for `startAtMs`, both fields of `startAfter`, `startAtRevision`, and `revision`, while the Rust boundary deserializes these values as `u64` at lines 257–269. JavaScript has already rounded any Number greater than `Number.MAX_SAFE_INTEGER` before Rust receives it; for example, `9007199254740993` becomes `9007199254740992`. The SDK can therefore query and verify a different timestamp or revision than the caller requested, without reporting an error. Require `bigint` for these exact selectors, or retain Number compatibility only after rejecting unsafe values at runtime.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Bound and paginate legacy history migration work — The migration helper still uses an unrestricted
insert_all()query and materializes complete subtrees in memory, so the resource-exhaustion concern remains technically valid. However, the migration implementation was introduced by the stacked PR before this PR's stated boundary at commitd4f6776fd, and the PR explicitly limits this PR to commits after that boundary. This should be tracked with the stacked keep-history migration work rather than blocking this lifecycle PR.- Follow-up: Track bounded or resumable activation migration separately in the keep-history storage migration work.
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
Revalidated all eight prior findings at 9da4990: seven are fixed, and activation-migration capacity remains deferred work belonging to the stacked storage PR. Two client-side bugs are confirmed; the contested-ID failure already exists in the declared stacked base and is retained only as a separate high-value follow-up. Verification was source-based; no tests were run in this verifier pass.
🟡 2 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
criticalbygpt-6-astra(effort low) — This large, intricate change directly modifies consensus authorization in packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/action_validation/document/document_erase_transition_action/state_v0/mod.rs and destructive, refund-bearing storage operations in packages/rs-drive/src/drive/document/delete/erase_document_for_contract_operations/v0/mod.rs, requiring scr - Phase 1 reviewers: not run (skipped for throughput: 18 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-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/wasm-sdk/src/state_transitions/document.rs`:
- [SUGGESTION] packages/wasm-sdk/src/state_transitions/document.rs:645-649: Forward erase signing settings from JavaScript to the builder
The wrapper decodes PutSettings, including userFeeIncrease and stateTransitionCreationOptions, but forwards them only through with_settings. DocumentEraseTransitionBuilder::with_settings stores broadcasting settings without populating the separate fields that sign reads. Consequently, documents.erase({ ..., settings: { userFeeIncrease: 250 } }) signs with a zero fee increase, and supplied creation options are also ignored. Broadcasting cannot repair an already-signed transition. Forward both signing settings to their builder methods and add coverage asserting that non-default JavaScript settings reach transition construction.
In `packages/rs-sdk/src/platform/documents/transitions/erase.rs`:
- [SUGGESTION] packages/rs-sdk/src/platform/documents/transitions/erase.rs:127-134: Reject unsupported erase versions before reserving a nonce
With an SDK pinned to protocol 12 or 13, a valid contract/type and successful nonce fetch reach this call with bump_first = true, advancing the cached contract nonce. Only afterward does DocumentEraseTransition::from_document reject the unavailable erase version, and the error returns without broadcasting or undoing the increment. Repeated rejected calls can advance the cache beyond the protocol's 24-revision window and cause subsequent valid document transitions to fail. Refreshing does not lower the reserved value because NonceCache::get_or_fetch_nonce preserves max(cached, platform). Validate erase availability and deterministic creation-version constraints before reserving the nonce, and add a regression asserting that unsupported construction leaves the cache unchanged.
Out-of-scope follow-up suggestions (2)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Track activation inventory limits and resumable migration separately — The migration still materializes complete history subtrees and type-wide index inventories in one activation transaction. However, packages/rs-drive/src/drive/document/migration/mod.rs is unchanged between the declared stacked base d4f6776 and this head, and the PR explicitly separates storage-migration release work from this lifecycle change. The reported testnet rehearsal measures one inventory rather than enforcing a bound on subsequent growth.
- Follow-up: Track activation-capacity acceptance with PR #4652 or release preparation: refresh the inventory and resource rehearsal before activation, define acceptance thresholds, and use bounded or resumable processing if those thresholds are exceeded.
- Handle pending-contest document-ID collisions in the storage-v2 work — The contested-ID collision is a concrete storage-v2 concern, but not a newly introduced failure in this lifecycle PR. In stacked base d4f6776, add_document_to_primary_storage_v1 already calls batch_insert_if_not_exists for the current pointer and returns CorruptedDocumentAlreadyExists when it exists. Thus an ordinary document occupying a pending contender's ID already makes award_document_to_winner fail with override_document = false; the newly added history-tree guard merely returns the same class of error earlier. The comparison with the protocol-13 writer misses this existing protocol-14 behavior in PR #4652.
- Follow-up: Track this with PR #4652: reproduce a legacy conditional-contest contract with a pending contender and an ordinary document sharing its ID, then cover protocol-14 award processing and safe handling of collisions already present before activation.
shumkov
force-pushed
the
keep-history-lifecycle
branch
4 times, most recently
from
September 13, 2026 12:35
3d02cc9 to
4f6537b
Compare
thepastaclaw
suggested changes
Sep 13, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Phase 1 blocker gate
Two prior findings remain valid at the exact head. The lifecycle tamper test still checks only in-memory inequality rather than verifier rejection, and legacy history migration still loads an entire per-document subtree into memory in one activation transaction. The other eight prior findings are fixed by the current code.
Validated blockers were found by the Phase-1 review and confirmed by a fresh verifier. Phase 2 is deferred until a fresh same-head revalidation clears the blocker gate.
🔴 1 blocking
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: ffi-engineer); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-gate-verifier, role: verifier)
- Triage:
criticalbygpt-6-astra(effort low) — This is a large, intricate cross-cutting change that directly modifies consensus state-transition validation, peer-facing protobuf deserialization, funds-refunding accounting, cryptographic proof verification, and persistent document storage/migration behavior. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— ffi-engineer (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— security-auditor (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 11% left, 5h 100% left),glm-5.3-flash(zai below 15% reserve: 5h 100% left, weekly 13% left) - Fresh verifier:
gpt-6-astra— verifier; agentastra-gate-verifier - Phase 2 reviewers: not run (deferred by blocker gate)
🤖 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/lifecycle/tests.rs`:
- [SUGGESTION] packages/rs-drive/src/drive/document/lifecycle/tests.rs:1559-1573: Exercise proof verification when testing tampered lifecycle claims
(existing thread: https://github.com/dashpay/platform/pull/4657#discussion_r3995256902)
The test mutates a clone of `verified.lifecycle` and asserts only that the clone differs from the verified value. It never serializes the altered lifecycle into a response and passes that response, with the original proofs unchanged, through `DocumentHistory::maybe_from_proof` or the document-history verifier. Consequently, the test would remain green if the verifier stopped checking the claimed lifecycle fields. Add response-level coverage for deleted and erasing pages that mutates each lifecycle field independently and asserts verifier rejection.
In `packages/rs-drive/src/drive/document/migration/mod.rs`:
- [BLOCKING] packages/rs-drive/src/drive/document/migration/mod.rs:471-493: Bound and paginate legacy history migration work
(existing thread: https://github.com/dashpay/platform/pull/4657#discussion_r3995256896)
`history_migration_entries` constructs an unrestricted `Query` with `insert_all()` and materializes the complete history subtree into a `Vec` via `to_key_elements()`. The caller then retains these entries while constructing migration copies and applying the work in the single protocol-activation transaction. Legacy pre-protocol-14 keep-history documents have no retained-revision bound, so a single legally accumulated history can force every validator to read, deserialize, duplicate, and rewrite an unbounded amount of data synchronously. A testnet snapshot and timing measurement do not bound future mainnet state or growth before activation; sufficiently large state can exhaust memory or exceed activation limits and prevent the upgrade. Bound the migration with resumable pagination/streaming, or enforce a pre-activation limit before such histories can accumulate.
Store immutable revisions in per-document provable count trees with composite time and revision keys, and keep the current pointer in primary storage. Select the new write, estimate, reference, and ordinary-read layout at protocol 14; preserve older replay behavior. Test would have caught this in CI: ✖ before fix, should_retain_both_replacements_in_the_same_block retained [1, 3]; ✔ after fix it retains [1, 2, 3]. cargo test -p drive --lib: 3579 passed, 5 existing ignored tests. cargo test -p drive --lib history: 84 passed.
Authenticate history pages and raw count-tree metadata separately, preserve complete cursors, and verify revision offsets and subtree absence at protocol 14. Keep released protocol selectors unchanged. Test would have caught the pointer-size underestimate: RED before fix (57 estimated bytes versus 59 serialized, then 316 versus 317 at a flags encoding threshold), GREEN after including reference hops and the flags-length allowance. Selector, absence, deep-history fee, and migration checks pass; workspace all-features clippy passes.
Move legacy revision Items into per-document provable count trees with deterministic staged batches, preserve flags and sums, rewrite index references, and retain the existing cache refresh. Add a rollback-only offline-copy inventory benchmark and an actual 13-to-14 signed-operation upgrade/restart strategy. Migration matrix and rollback/retry checks pass, including overwritten-revision gaps, empty types and deep summable histories. Upgrade strategy passes; activation dispatch, signed lifecycle writes and legacy repair tests pass. The parallel-suite repair test failed before its queries explicitly selected protocols 13 and 14 and passed after the fixture correction. Synthetic 256-revision migration: 686.34 ms in a loaded debug build; authentic testnet inventory and validator budget remain the release gate.
Add time/cursor and revision selectors to the versioned wire API and expose ordered entries with authenticated lifecycle counts across the Rust, WASM and JavaScript SDKs. Verify both GroveDB proofs and quorum signatures, including absent documents and empty pages, while preserving the legacy wire format for released protocols. Validation: signed quorum/API round trip and tampering tests pass; proof verifier 267, DPP 4114 and Rust SDK 189 tests pass; version activation/replay tests pass. WASM Rust history tests pass (2), WASM unit suites pass (424 each in Node and Chrome), and Evo SDK unit suites pass (222 each in Node and Chrome); TypeScript lint/build and workspace all-features clippy pass.
Declare each history-root child as AllProvableCountTrees so propagation includes its authenticated count. Pin the four rewritten ancestors to an extra eight bytes each in should_charge_count_tree_overhead_when_propagating_history_roots. Test would have caught this in CI: red before fix (568 replaced bytes versus 600 required), green after fix (1 passed).
Route history v1 through the four existing versioned entry points with known versions [0, 1], retaining explicit legacy adapters for replay. A legacy proof at protocol 14 now reports an unsupported query shape instead of UnknownVersionMismatch. Authenticate the latest sequence from the resolved primary document before accepting ordinal revision offsets. Both Revision(3) and StartAtRevision(3) reject retained [1,3], including a valid proof of the otherwise-empty positional page. Pin positive selector results and the sixteen-byte decoder gate. Test would have caught this in CI: should_report_legacy_history_as_unsupported_after_activation failed before dispatch routing and passed after; should_migrate_revisions_and_indexes_without_recovering_overwritten_revisions failed when fetch/prove/verify all accepted the empty gap page and passed after. All nine history tests and the migration gap regression passed in the Drive library suite. BREAKING CHANGE: SDKs older than this release cannot read keep-history documents after protocol 14 activates. History v0 and block_time_ms point-in-time reads for keep-history types are rejected at that boundary.
Enumerate contracts through ascending fetch_contract_ids pagination. Allow only the reserved document-type keys 0, 1, and 2 with their permitted shapes, or declared index roots, and reject any other child before migrating that type. Assert that each document rewrites every collected index reference. Expose the largest per-type index inventory and a conservative owned-buffer bound in the benchmark output. Structural-flag assertions now distinguish contract flags from writer flags for document trees and both populated and empty history roots. Test would have caught this in CI: should_reject_unrecognised_type_children_during_migration_inventory failed before the explicit inventory validation and passed after. All four migration tests passed, including rollback/retry, overwritten revisions, depth measurements, and distinct structural flags.
Contributor
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
thepastaclaw
suggested changes
Nov 12, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The PR still has three in-scope issues: the new canBeErased keyword breaks the Rust workspace compatibility-inventory test, erase handling was added to a shipped v0 validation generation even though the PV15 path reaches those arms, and consensus-relevant erase refund pricing has no platform-version selector. The public raw Drive batch converter also fails to carry sibling operations, so multiple first-container lifecycle deletes can emit duplicate tree insertions. The branch additionally conflicts with the current v5.1-dev tip and must preserve the independent PV15 contest changes during integration.
🔴 3 blocking | 🟡 1 suggestion(s)
Still open from earlier rounds (1)
- 🔴 Blocking: Keep erase processing out of shipped validation generations —
packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/validation/validate_basic_structure/v0/mod.rs:179-213(thread)
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: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); 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); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This large, intricate change directly modifies consensus authorization in document_erase_transition_action/state_v0/mod.rs and persistent deletion and erasure semantics in rs-drive/src/drive/document/delete/erase_document_for_contract_operations/v0/mod.rs, alongside proof verification and fee accounting. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),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 verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review (PastaClaw). How to work through them:
- Verify each finding against the code; fix only what is real, as narrowly as the finding.
- If a finding is wrong, decline it on its thread in one or two sentences; the bot answers once and stops.
- To defer one, link a tracking issue on its thread (or reply asking the bot to file one).
- Suggestions don't block the merge, but most are worth fixing in this PR; judge each on its merits.
- Keep the PR's scope; don't grow it to satisfy hardening suggestions.
- Carried items were raised in an earlier round: read the thread first, and don't re-fix what you already fixed, declined or deferred there. Disputed and deferred items are already handled and are not listed.
In `packages/rs-drive/src/util/batch/drive_op_batch/drive_methods/convert_drive_operations_to_grove_operations/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/util/batch/drive_op_batch/drive_methods/convert_drive_operations_to_grove_operations/v0/mod.rs:56-63: Deduplicate lifecycle containers at the Drive batch boundary
The application route now passes accumulated sibling operations, but the public raw converter still calls into_low_level_drive_operations_after_ttl_drain with previous_batch_operations set to None for every operation. At PV15, generation 1 delegates to this v0 implementation. Converting two deletes of different documents of the same type before its lifecycle container exists therefore allows both operations to emit the same empty-tree insertion at the lifecycle container key, and the resulting GroveDB batch can be rejected for duplicate operations. Thread the accumulated low-level operations through this converter as well, or explicitly reject this batch shape, and cover it through convert_drive_operations_to_grove_operations.
In `packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/native_schema/document_type.rs`:
- [BLOCKING] packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/native_schema/document_type.rs:29-41: Account for canBeErased in the schema compatibility inventory
Adding canBeErased to ROOT_KEYS makes the native-schema inventory admit a keyword that the selected schema compatibility generation does not classify. Its should_have_a_rule_for_every_keyword_the_meta_schema_admits test iterates the native root keys and fails at validate_schema_compatibility/v1/mod.rs:939 because neither the shared rules nor the dedicated exemptions contain canBeErased. The Rust workspace CI job at this head confirms the failure. Add a version-safe compatibility classification or an explicit dedicated-validator exemption for canBeErased, while preserving the immutability check owned by validate_update_v2; do not remove or weaken the inventory assertion.
In `packages/rs-drive/src/drive/document/lifecycle/refund_recipients.rs`:
- [BLOCKING] packages/rs-drive/src/drive/document/lifecycle/refund_recipients.rs:36-47: Version the consensus-relevant erase refund-recipient fee calculation
erase_refund_recipient_cost is called during ABCI state validation to add a pre-calculated fee for the balance updates caused by erasure refunds. It determines the charged processing and storage fees by multiplying the measured balance-update cost by the protocol's erase chunk size, but the helper itself has no OptionalFeatureVersion or other method-table selector. This makes a consensus-visible fee rule unversioned and prevents future protocol generations from preserving or changing it through the platform-version tables. Add a versioned selector under the document-delete method versions, keep it inactive before lifecycle erasure, dispatch the PV15 implementation through that selector, and ensure estimation and validation use the same versioned path.
In `packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/validation/validate_basic_structure/v0/mod.rs`:
- [BLOCKING] packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/validation/validate_basic_structure/v0/mod.rs:179-213: Keep erase processing out of shipped validation generations
(carried; thread: https://github.com/dashpay/platform/pull/4657#discussion_r4079213485)
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Reconcile PV15 lifecycle tables with current contest-index changes — The current v5.1-dev tip e6b3006 conflicts with this branch in dpp_contract_versions/v7.rs and version/v15.rs. Its canonical_contested_index_values selection and related PV15 validation changes are independent of the lifecycle selectors and must not be lost during integration.
- Follow-up: Rebase or merge v5.1-dev before publication, resolve both PV15 table conflicts while retaining the lifecycle and contest selections, and rerun the relevant compatibility and consensus checks.
Give the top-level canBeErased flag the existing frozen compatibility rule. Schemas without the flag keep the original options, including released schemas and contract-definition comparisons. The CI keyword-rule inventory failed before the fix; the 18 focused compatibility tests pass afterward, including the released-definition pin and the flag addition, removal, change and unchanged matrix. Test would have caught this in CI: ✖ before fix, ✔ after.
Describe the lazy, unflagged per-type lifecycle container and its versioned DPP record items from protocol 15. Populate real deleted and erasing documents so conformance checks the container and both record states. The new conformance regression fails without the description and passes with it. Regenerate the structure export: only the two lifecycle nodes are added, with all 281 existing nodes and 34 layer shapes unchanged. Test would have caught this in CI: ✖ before fix, ✔ after.
Merge the current v5.1-dev changes while preserving the lifecycle parser and transition selectors. Keep upstream contested-index canonicalization and notes 38/39; move the lifecycle note and its pointers to note 40.
Contributor
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
github-actions
Bot
added
waiting-self-review
Contributor
|
🌳 GroveDB structure This pull request changes the described GroveDB structure. Open it in the structure viewer: new nodes glow, removed ones stay as ghosts, and the tour walks through each change. Added (2 nodes)
Compared |
thepastaclaw
suggested changes
Nov 12, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The lifecycle implementation still has two in-scope defects: the erase refund-recipient fee calculation is consensus-relevant but lacks versioned dispatch, and the public Drive batch converter drops the sibling-operation context required to deduplicate lifecycle containers. The Rust workspace CI check also fails because the added DocumentTypeV2 field changes a shielded characterization hash without updating its expected pin; other reviewed lifecycle, SDK, WASM, proof, and compatibility findings are resolved or appropriately withdrawn/deferred.
🔴 2 blocking | 🟡 1 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🔴 Blocking: Reconcile the Rust characterization pins with the added document-type field
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/shielded/nullifier_batch_guard.rs:2187-2189
The Rust workspace test should_preserve_single_entry_actions_errors_operations_and_settled_state fails on this head in the valid-payment case. CI reports the validation hash as d927dadd5153feea26a476e6db2abfabefedfd10b239366ed27efc97a2806d44, while the source still expects 9388d5cf63c4a571b9fb009679e2e34d0b681acc88f0f5195e7287c7efbd92f7 at the cited pin. The change adds documents_can_be_erased to the derived Debug representation of DocumentTypeV2, and this characterization hashes fetched contract types, so the failure is caused by this PR rather than infrastructure. Audit the expected semantic changes and update the affected pin, or replace the incidental Debug-shape hash with an explicit semantic projection while retaining the fee, root, and nonce assertions.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, platform-versioning, rust-quality, security-auditor)
Still open from earlier rounds (2)
- 🔴 Blocking: Version the consensus-relevant erase refund-recipient fee calculation —
packages/rs-drive/src/drive/document/lifecycle/refund_recipients.rs:36-47(thread) - 🟡 Suggestion: Deduplicate lifecycle containers at the Drive batch boundary —
packages/rs-drive/src/util/batch/drive_op_batch/drive_methods/convert_drive_operations_to_grove_operations/v0/mod.rs:56-63(thread)
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: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); 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); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This large, intricate diff directly changes consensus authorization and state validation in rs-drive-abci's document_erase_transition_action/state_v0/mod.rs, persisted deletion and erasure behavior in rs-drive, and temporal proof verification. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),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 verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review (PastaClaw). How to work through them:
- Verify each finding against the code; fix only what is real, as narrowly as the finding.
- If a finding is wrong, decline it on its thread in one or two sentences; the bot answers once and stops.
- To defer one, link a tracking issue on its thread (or reply asking the bot to file one).
- Suggestions don't block the merge, but most are worth fixing in this PR; judge each on its merits.
- Keep the PR's scope; don't grow it to satisfy hardening suggestions.
- Carried items were raised in an earlier round: read the thread first, and don't re-fix what you already fixed, declined or deferred there. Disputed and deferred items are already handled and are not listed.
In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/shielded/nullifier_batch_guard.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/shielded/nullifier_batch_guard.rs:2187-2189: Reconcile the Rust characterization pins with the added document-type field
The Rust workspace test `should_preserve_single_entry_actions_errors_operations_and_settled_state` fails on this head in the `valid-payment` case. CI reports the validation hash as `d927dadd5153feea26a476e6db2abfabefedfd10b239366ed27efc97a2806d44`, while the source still expects `9388d5cf63c4a571b9fb009679e2e34d0b681acc88f0f5195e7287c7efbd92f7` at the cited pin. The change adds `documents_can_be_erased` to the derived `Debug` representation of `DocumentTypeV2`, and this characterization hashes fetched contract types, so the failure is caused by this PR rather than infrastructure. Audit the expected semantic changes and update the affected pin, or replace the incidental Debug-shape hash with an explicit semantic projection while retaining the fee, root, and nonce assertions.
In `packages/rs-drive/src/drive/document/lifecycle/refund_recipients.rs`:
- [BLOCKING] packages/rs-drive/src/drive/document/lifecycle/refund_recipients.rs:36-47: Version the consensus-relevant erase refund-recipient fee calculation
(carried; thread: https://github.com/dashpay/platform/pull/4657#discussion_r4553004963)
In `packages/rs-drive/src/util/batch/drive_op_batch/drive_methods/convert_drive_operations_to_grove_operations/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/util/batch/drive_op_batch/drive_methods/convert_drive_operations_to_grove_operations/v0/mod.rs:56-63: Deduplicate lifecycle containers at the Drive batch boundary
(carried; thread: https://github.com/dashpay/platform/pull/4657#discussion_r4553004947)
Retain the upstream proposal processing fee and its protocol-15 note 40, and move the lifecycle note and its pointers to 41. The auto-merged batch action keeps both gas-sponsored lifecycle ownership and proposal counting.
Select erase_refund_recipient_cost only in the protocol 15 document-delete table and refuse inactive or unknown versions through the dispatcher. Move the recipient bound and existing fee algorithm into generation 0 without changing pricing. Red before the fix: a protocol 14 table with an erase chunk limit incorrectly priced the inactive method. The regression pins VersionNotActive and the active dispatcher pins UnknownVersionMismatch for generation 1; the complete test receipts are recorded in the verification report. Green after the fix: both dispatcher regressions pass in the complete changed-crate non-shielded phase, including the exact inactive/unknown errors and unchanged active pricing.
Select raw converter generation 2 at protocol 15 and retain pending operations for history deletes, including both force-delete forms. Keep generation 1 at protocol 14 and keep non-history deletes independent of pending siblings. Red before the fix: converting sibling history deletes emitted two lifecycle-container inserts, and force-delete and shared-index batches failed application. The regressions require one container, persisted Deleted records, equal roots between conversion and application, and identical non-history operations across generations. Exercise the released converter refusals at both protocol 14 and latest. Green after the fix: all four raw-conversion regressions pass in the complete changed-crate non-shielded phase, including persisted record read-back and shared-index root equality.
The valid-payment characterization differs from base 9d221db only because two fetched DocumentTypeV2 values now include documents_can_be_erased: false in their derived Debug output. All nine validation strings match after removing that exact default-false field, and all nine settled-state strings match byte for byte. Exclude that inactive field in this non-history fixture while retaining all eighteen original hash pins. Red before normalization: d927dadd differed from the original 9388d5cf validation pin; no actions, errors, charges, lowered operations or settled state are re-pinned. Green after normalization: the same characterization passes all nine cases with the original eighteen hashes in the full CI shielded shared-process phase.
List raw converter generation 2 beside the retained-history lifecycle selectors in protocol 15 note 41 and link the V10 selector comment to that note. Explain that sibling history deletes insert the lifecycle container once. Comment-only change; runtime code and selector values are unchanged, so no new behavioral regression test is needed.
Contributor
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
Collaborator
Author
|
Disposition of the non-inline blocking finding in review 5882588783 (shielded characterization pin in Exact base/head captures of the The two inline findings from that review are fixed in 5872857 (versioned |
thepastaclaw
approved these changes
Nov 13, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static review of the exact head found one in-scope non-blocking architecture suggestion: the SDK's nonce-protection preflight duplicates erase-construction version allowlists owned by DPP. No blocking correctness or security findings remain. Rust, wasm32, functional, SDK, and most E2E checks pass; the local-network E2E failure occurs during Core synchronization before lifecycle coverage, while the main test suite and one browser shard remain pending.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Approved: nothing here blocks the merge. The non-blocking findings are optional: fix them forward, in this PR or a follow-up, without waiting for another review round.
Follow-up candidates (optional) (1)
New non-blocking observations on code unchanged since the last full review. No inline threads are opened for them and later reviews do not repeat them: fix forward or track separately if they are worth it.
🟡 Suggestion: Share erase constructibility checks with DPP constructors
packages/rs-sdk/src/platform/documents/transitions/erase.rs:177-219
The preflight correctly runs before nonce reservation, but it independently duplicates the supported batch, erase-method, and document-base version allowlists already encoded by DPP's constructors. If DPP adds a constructible generation without this SDK helper being updated, the SDK will reject valid transitions; if the copies diverge in the other direction, construction can still fail after the cached nonce has advanced. Move the side-effect-free constructibility check into DPP and have both the constructors and SDK preflight use it, while keeping nonce reservation after that shared check.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, platform-versioning, rust-quality, security-auditor)
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: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); 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:
criticalbygpt-6.1-sol(effort low) — This is a large, intricate change that alters protocol-versioned consensus validation, funds and fee accounting, peer-facing serialization/proofs, and persistent document storage lifecycle behavior across DPP, Drive, and rs-drive-abci. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),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
- Code unchanged since the last final review (
70dd0591): 1 new suggestion(s) listed as follow-up candidates, not posted inline - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
Contributor
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
shumkov
added a commit
that referenced
this pull request
Nov 13, 2026
Merge v5.1-dev at d9d5689, which carries the squashed lifecycle PR (#4657) and the adaptive context provider work (#5882, #5883, #5884) plus #5830. Merge attributes were disabled; the merge is clean. Audit: the merge delta onto 1073d2e is byte-identical to the v5.1-dev delta onto the merged lifecycle head 84ac1e7 (47 files), so the squash reproduces the lifecycle content exactly and nothing of the mobile delta changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
6 of 15 tasks
QuantumExplorer
added a commit
that referenced
this pull request
Nov 13, 2026
…y-v51 Conflicts in packages/rs-platform-version/src/version/v15.rs, with #4657 (delete and erase lifecycle for keep-history documents): - imports: this branch's STATE_TRANSITION_METHOD_VERSIONS_V3 and the base's STATE_TRANSITION_SERIALIZATION_VERSIONS_V4 are both kept; - both sides added a note 41 to the PLATFORM_V15 doc comment. The base's note (retained-history lifecycle) keeps 41; the signature note becomes 42, with its reference on the state_transition_method_versions line. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Basic explanation
What this does: At protocol 15, a permitted delete removes a document from ordinary reads while retaining its history. A separately permitted erase removes that retained history in bounded chunks.
Value: Applications can keep revision history without making every document permanent. Historical queries distinguish active, deleted, erasing and absent documents.
Risks: This changes consensus validation, storage, proofs and fee accounting at protocol activation. Focused runtime, compatibility and build checks pass locally; full CI and validator-hardware capacity checks are separate verification layers.
Issue being fixed or feature implemented
Keep-history documents need an explicit lifecycle: removal from current reads must preserve revisions, while permanent removal of those revisions requires separate authorization. The lifecycle activates at protocol 15; protocols through 14 retain their existing behavior.
What was done?
Lifecycle and authorization
canBeEraseddefaults to false, requires both history and deletion, and cannot change on a contract update. New registration still refuses a keep-history type with a contested index; non-validating reads preserve legacy contracts. History types cannot be consumed through a document reference.[1]tree records the deletion. Multiple deletes share one lazy lifecycle-container insertion within a PV15 Drive batch.Reads and proofs
For a single-document lookup at time
T:T, with the existing history-time clamp; absent before the first revision.deleted_at; absent at or after it. The comparison uses the original, unclampedT, so a same-block delete wins.T, from the first erase chunk onward.T; no terminal receipt is kept.The PV15 temporal query authenticates the lifecycle record or its absence together with history. Whole-proof and subset verification share structural checks; typed verification also checks the returned document's id and revision. The raw serialized verifier keeps its public signature. Generation 0 remains available for cross-version execution proofs.
DriveDocumentQuerywith a block time remains refused for keep-history types.GetDocumentHistoryv1 reportsACTIVE,ABSENT,DELETEDandERASING, withremaining_revisions,deleted_at_ms,erasing_started_at_ms,erasing_from_time_msanderasing_from_revision. Pages continue to expose surviving revisions during erasure. Proved responses use one merged GroveDB proof; lifecycle fields are derived from authenticated state. Released history v0 remains unchanged.The Rust, WASM and JavaScript SDK surfaces expose erase and a separate current-lifecycle read. Erase confirmation authenticates affected state, rather than proving that this particular transition executed.
Fees and storage ownership
Protocol 15 version slots
The lifecycle selectors are documented in
v15.rs, note 41:try_from_schema4 /validate_update2add_document_for_contract_operations2 /delete_document_for_contract_operations1Some(0)Some(0)Some(0..=0)/Some(100)The PV15 tables are
CONTRACT_VERSIONS_V7,DPP_VALIDATION_VERSIONS_V6,STATE_TRANSITION_SERIALIZATION_VERSIONS_V4, document/state-transition method tables V5, verify table V4, ABCI validation V11 andSYSTEM_LIMITS_V5. Erase handlers, bounds and limits are absent in released tables.DocumentTypeV2gains the erasability flag; schema version 3 and compiled writer generation 0 are retained.StoredDeletionvariants 0/1 preserve the previous false/true bytes; variant 2 represents erasability and requires history.How Has This Been Tested?
Local macOS verification at
a9c0e1ae82:should_end_a_poll_of_the_most_contenders_a_contest_accepts); it was not executed. This includes Drive lifecycle 49, delete 28, client costs 23, temporal/single-document verification 33, history verification 14, history decoder 19, migration 10 and contested 83; ABCI erase 19, references 8, activation 3, genesis 5 and commit-reveal 59; DPP contested 105 and lifecycle parser 20; and all 3 committed-byte compatibility tests.cargo fmt --check --all.cargo clippy --workspace --all-targets --all-features --locked -- --no-deps -D warnings.cargo check -p wasm-sdk --target wasm32-unknown-unknown --locked; wasm-dpp was also built for wasm32 and used by the JavaScript tests.Follow-up verification at
d6347691f2: the compatibility-v1 suite passes all 18 cases; structure lint/conformance/export tests pass all 12 cases; export freshness, workspace formatting and DPP/Drive clippy pass. The generated structure adds only the lifecycle container and record nodes, preserving all 281 existing nodes and 34 layer shapes. The requested changed-crate non-shielded runs completed 17,968 selected cases using the CI filter and--no-fail-fast: 17,967 passed initially and one reference-registration case aborted with the local stack setting unset. That case passes on both the frozen base and this branch with CI’sRUST_MIN_STACK=4194304; no test or production stack-size code was changed. Exact group counts, skips and passing process-leak warnings are recorded in the follow-up report.Actual total delete-fee checks include:
These assert total estimates cover applied fees. Processing has wide slack; storage margins are tight.
Verification at
70dd05914a5c: 2,047 focused Rust cases passed, including all 45 new upstream Rust tests for schema patterns, contact requests, proposal descriptions, contest poll identity and wallet addresses. The compatibility keyword inventory, signed lifecycle reference matrix, lifecycle/delete/history and proposal/contest groups pass with CI’sRUST_MIN_STACK=4194304; 16,224 cases were skipped by the focused selection. One passing process-leak warning was recorded and the case passed cleanly in isolation. All 46 decision-proposal JavaScript tests, workspace formatting and exact workspace clippy pass. The JavaScript run uses a WASM dependency rebuilt from this source. Swift wrapper tests remain CI-only because the required SDK framework is absent locally.Local verification at
6edc4c051dc8:dpp,drive,drive-abci,dash-sdk,drive-proof-verifier,platform-versionand nativewasm-dpp2pass 16,521 non-shielded cases with the CI filter and--no-fail-fast(1,214 skipped). The complete CI shielded shared-process phase passes 1,029 cases, including all 418 ABCI cases; three existing fixtures are ignored. Both useRUST_MIN_STACK=4194304. All six converter/refund-dispatch regressions pass, and workspace fmt, exact workspace clippy and all 38 platform-version cases pass. The shielded characterization keeps all eighteen original hashes: exact base/head captures differ only by two inactive default-false erasure fields in contract-type Debug output, which the non-history fixture excludes. Actions, errors, charges, lowered operations and settled state retain their original pins.Final comment-only documentation commit
84ac1e73775calso passes workspace fmt and all 38 platform-version tests.CI-only verification: the full-workspace non-shielded and full-chain phases,
cargo test -p wasm-sdk --lib --locked -j 2, and workspace doctests. Those remaining phases are not established by these local runs. The network functional lifecycle test, representative validator-hardware activation rehearsal and maximum-block erase benchmarking are not established by these local checks. Client erase-refund modelling remains a separate follow-up.In-place changes to shipped generations
Erase is appended to existing document/batch enums with existing discriminants and batch wire formats preserved. Released protocols publish no Erase bounds and refuse it in basic structure before executable handlers. Existing-input branches remain unchanged. This follows the same per-kind admission pattern as index-only deletion and shielded-payment branches.
The shipped versioned code gaining exhaustive Erase handling is:
methods/v0,resolvers/v0,v0/v0_methods,v1/v0_methods, andvalidation/validate_basic_structure/v0.advanced_structure/v0,advanced_structure/v1,state/v0(includingaccepted_token_pool_nullifiers) andtransformer/v0(includingproperty_constraint_aggregates).verify_state_transition_was_executed_with_proof/v0: Erase is explicitly classified as affected state, not execution-proved.Unversioned exhaustive enum/accessor/dispatch updates accompany those arms. The complete appended-arm inventory is below.
Files with appended-kind handling
packages/rs-dpp/src/data_contract/document_type/action_fees/mod.rspackages/rs-dpp/src/state_transition/mod.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/document_transition.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/document_transition_action_type.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/resolvers.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_transition.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/methods/mod.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/methods/v0/mod.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/resolvers/v0/mod.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/v0/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/v1/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/validation/validate_basic_structure/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/common/validate_document_not_expired.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/advanced_structure/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/advanced_structure/v1/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/state/v0/accepted_token_pool_nullifiers.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/state/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/transformer/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/transformer/v0/property_constraint_aggregates.rspackages/rs-drive/src/query/filter.rspackages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/document/document_transition.rspackages/rs-drive/src/state_transition_action/batch/batched_transition/document_transition/document_transition_action_type.rspackages/rs-drive/src/state_transition_action/batch/batched_transition/document_transition/mod.rspackages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rspackages/wasm-dpp/src/document/state_transition/batch_transition/document_transition/mod.rspackages/wasm-dpp2/src/enums/batch/batch_enum.rspackages/wasm-dpp2/src/state_transitions/batch/document_transition.rspackages/rs-platform-wallet/src/wallet/identity/network/state_transition_summary.rsOther shared-generation edits preserve released behavior:
canBeErasedflag. Released grammars refuse that flag; comparisons without it, including contract definitions, keep the original compatibility rules.DocumentTypeV2and compiled format v0 gain the flag using byte-compatible stored deletion variants; existing fixture bytes are pinned.update_keywords/v0carry fullBlockInfoand optional deleter context into version dispatch. Delete conversion 0 supplies no deleter, and generation 0 continues using the same timestamp and existing operations. Sponsorship sets the new deleter field, which only delete generation 1 reads.apply_drive_operations0/1 andconvert_drive_operations_to_grove_operations0/1 pass no accumulated sibling operations; their PV15 generation 2 passes them for lifecycle-container deduplication. Shared force-delete lowering forwards siblings only for history types, which released force-delete handlers refuse; non-history lowering remains unchanged.Breaking Changes
DocumentTypeV2gains an erasability field; exhaustive matches and direct struct construction may require updates. NoDocumentTypeV3or new batch wire format is introduced.DocumentOperationType::DeleteDocumentgainsdeleter_id, andEraseDocumentis added.DELETED,ERASINGand the lifecycle fields. Generated gRPC clients are updated. Erase confirmation remains an affected-state observation.Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
PR Hygiene ·
84ac1e7/self-reviewedjs-wasm-sdk— you own itdpp— you own itrs-drive-abci— you own itrs-drive— you own itrs-platform-wallet(packages/rs-platform-wallet/src/wallet/identity/network/state_transition_summary.rs) — HashEngineering or ZocoLini or llbartekll or romchornyirust-sdk— you own itWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.