Skip to content

feat(platform)!: destroy a frozen identity's escrowed listings with its funds (PV15) - #5532

Open
QuantumExplorer wants to merge 83 commits into
claude/token-market-historyfrom
claude/token-market-frozen-escrow
Open

QuantumExplorer wants to merge 83 commits into
claude/token-market-historyfrom
claude/token-market-frozen-escrow

Conversation

@QuantumExplorer

@QuantumExplorer QuantumExplorer commented Oct 22, 2026 •

Copy link
Copy Markdown
Member

Basic explanation

What this does: Closes a loophole the marketplace opened in freezing. A token issuer can freeze an account and then destroy the frozen funds. But tokens an account has listed for sale sit in escrow, not in its balance, so until now they escaped the destroy. From protocol version 15 destroying frozen funds also destroys what the frozen account's open listings hold, and deletes those listings.

Value: Freezing and destroying keep working as issuers expect: someone holding stolen or sanctioned tokens cannot shield them from destruction by listing them before the freeze lands.

Risks: Low-medium. A destroy at protocol version 15 reads the frozen account's listings (a small extra read, charged) and, when there are any, destroys at most 16 per transition, oldest first, so its work stays bounded. Before 15 nothing changes: the read and the new conversion are selected only by protocol version 15's tables, and the previous conversion's operations are byte-for-byte the same. The account's offers hold credits, not tokens, and are left alone.

Issue being fixed or feature implemented

The sixth PR of the token marketplace stack (PV15), on top of #5531 (history). A frozen account's listings cannot be filled (the listing PR refuses fills of a frozen maker), but nothing could remove the tokens they hold short of the account cancelling them itself.

What was done?

Frozen identity: balance 5,000, listings of 3,000 and 2,000
Before (14's rules): TokenDestroyFrozenFunds burns 5,000; 5,000 stays in escrow, untouchable
After (15):          it burns all 10,000, deletes both listings, records destroyedAmount 10,000
  • Read (DriveTokenOrderEscrowMethodVersions.fetch_frozen_listings, None before 15, Some(0) in DRIVE_TOKEN_METHOD_VERSIONS_V3): Drive::fetch_frozen_token_listings reads the frozen identity's open ask documents of the token through the byOwner index ($ownerId, tokenId, $createdAt), oldest first, at most SystemLimits::max_frozen_listings_destroyed_per_transition (16). With None it refuses with VersionNotActive; the transformer does not call it there (read_frozen_listings returns no listings before 15), and from 15 calls it, and bills the read, only for a signature that performs the destroy or proposes it as a group action: a group signature that does neither records and destroys nothing.
  • Action: TokenDestroyFrozenFundsTransitionActionV0 carries frozen_listings: Option<FrozenListings>, the listings with the token market contract their documents are deleted from, None when there are none. destroyed_amount() is the balance plus every listing's remaining, one checked sum, which the converter, the history and the group event all use.
  • Conversion (token_destroy_frozen_funds_transition 1, in the new DRIVE_STATE_TRANSITION_METHOD_VERSIONS_V5 that DRIVE_VERSION_V10 selects): without listings it is generation 0. With listings, one new op TokenDestroyFrozenHolding burns the balance, removes each listing's escrow and lowers the supply once by the destroyed total the converter passes it (so the repeated-write guard sees each key once; it refuses a burn generation other than token.update.burn 0, so a new burn generation has to decide this case too); a DeleteDocument per listing deletes its ask document, each built against the batch's operations before it (below), so index trees the listings share (a price bucket, a $createdAt bucket, the owner's bucket) are removed and refunded to whoever paid for them; the history and group-action events record the total.
  • Raw converter (convert_drive_operations_to_grove_operations 2, protocol version 15, new in this PR): builds each document operation against the batch's earlier ones as apply_drive_operations 2 does, and prepares the batch as apply_drive_operations does: it refuses a batch writing one token balance, supply, pool total or escrow twice (DriveOperation::refuse_repeated_token_balance_writes) and merges repeated identity balance, fee pot and prefunded balance writes (DriveOperation::merge_balance_writes), which each set a value computed from state before the batch. No platform code calls it at protocol version 15: the proposer payout credits through apply_drive_operations from 14.
  • Shared read: both transformer routes read and bill the listings through one helper, read_frozen_listings.
  • Batch boundary (apply_drive_operations 2, the protocol 15 generation from feat(platform)!: token marketplace escrow trees and token market contract (PV15) #5526, extended in place as it is unreleased): every document operation of a batch is built against the operations of its GroveDB batch built before it, and every operation is appended to them; other operation kinds (token history documents, a token's escrow tree, group actions) are still built without the batch, so max_transitions_in_documents_batch stays load-bearing for them (DriveOperation::append_low_level_drive_operations_after_ttl_drain). A document insert, update or deletion therefore sees the batch's earlier writes as pending: one batch destroying a frozen identity's listings of two tokens removes the owner's shared byOwner bucket and refunds it, and an insert into a tree an earlier deletion of the batch empties keeps the tree. Grouped document operations (MultipleDocumentOperationsForSameContractDocumentType) are built the same way. So are the consume operations of v5.1-dev's single-read consume (perf(platform)!: delete a consumed document from what its lookup read (PV15) #5360, also protocol 15): AddDocumentAndDeleteReadConsumed and ForceDeleteReadDocument are built as their AddDocumentAndDeleteConsumed and ForceDeleteDocument siblings, each deletion from what the lookup read when the read was kept; its commit_reveal_lookup and dpns_v3 tests pass through them. This is the latent defect batched_group_drain and SystemLimits::max_transitions_in_documents_batch document (unreachable through a transition while that cap is 1): its five specification cases, #[ignore]d until now, run and pass at the latest version, and the phantom-group characterization is pinned to protocol version 14. Protocol version 14 and earlier keep generation 1.
  • Groups: a group destroy reads the listings at every signature, and the one that completes it destroys what the frozen identity holds then, as generation 0 already does with the balance (its state validation compares only the frozen identity, not the amount). A frozen identity can still cancel a listing, but that moves the escrow into the balance, so a cancel between the signatures can only make the destroy larger: the balance takes the cancelled escrow and the next listing joins the oldest 16. Binding the completion to the proposed amount would let the frozen identity block every group destroy with one cancel.
  • Limit: max_frozen_listings_destroyed_per_transition is Option<u16>: None in the shipped tables, Some(16) in SYSTEM_LIMITS_V5 (protocol version 15). More listings need one more destroy each 16.
  • Docs: book "Frozen accounts" and "History" sections (a destroy closes the listings it takes without a record per order); v15 note 18, the DRIVE_VERSION_V10 and slot comments for the batch-context change.

How Has This Been Tested?

  • cargo test -p drive-abci --lib -- batch::tests::token::marketplace::frozen:

    • a frozen identity with a 5,000 balance and listings of 3,000 and 2,000: one destroy leaves balance 0, both escrows and listings gone, supply down 10,000 and balanced, destroyedAmount 10,000 recorded;
    • 17 listings at increasing block times: the first destroy takes the 16 oldest, the newest waits, the second takes it, supply balanced each time;
    • a frozen identity's offer keeps its credits.
  • cargo test -p drive-abci --lib -- destroy_frozen_funds batch::tests::token perform_events_on_first_block_of_protocol_change (every existing destroy test now goes through generation 1 with no listings).

  • cargo test -p drive --lib -- verify::state_transition drive_op_batch state_transition_action::batch order_escrow; cargo test -p platform-version.

  • Review round 1:

    • marketplace::frozen::should_remove_and_refund_the_index_trees_destroyed_listings_shared: two listings at one price in one block, which share index buckets: the 24 ask trees and items they make shrink, after the destroy, to the same 3 that cancelling the two one by one leaves, and the frozen owner's refund is at least what those cancels refund (the destroy also shrinks the burned balance item). (Before the batch-context change below, deletes built blind to each other left 16 trees and about half the refund.)
    • converter dispatcher tests: at protocol version 14 (generation 0) the nonce, the burn of the balance and its history, also for an action carrying listings, and a group proposal that does not perform (the group action with its event, no burn) and the completing signature (closes it, burns); at 15 the holding destroy, the deletes of both listings in order and the history of everything destroyed;
    • fetch_frozen_token_listings::tests::should_refuse_to_read_frozen_listings_before_protocol_version_15: at protocol version 14 the read refuses with VersionNotActive; the destroy there does not call it (review round 6 runs the destroy itself at 14).
  • Review round 2, apply_drive_operations::v2::tests::batch_document_writes:

    • one owner's asks of two tokens deleted by two operations of one batch leave the ask trees that deleting them in two batches leaves, with no tree keyed by the owner;
    • a deletion emptying a bucket and an insert into the same bucket in one batch apply, and leave the trees of the new ask alone;
  • cargo clippy on platform-version, drive, drive-abci.

  • Review round 3:

    • batched_group_drain: the five drain specifications (two deletes in either order, under the shipped batching config, batched against sequential, a two-document move) run un-ignored at the latest version; a_batched_drain_leaves_a_phantom_group_that_verifies_and_proves applies its batch at protocol version 14 and still finds the phantom;
    • batch_document_writes::should_insert_through_a_group_into_a_tree_an_earlier_deletion_empties: a deletion and a grouped insert into the same bucket apply and leave the new document's trees alone (it failed with an insertion order error when grouped operations started a fresh accumulator);
    • converter at the latest version in a group with listings: a non-performing proposal records the balance plus escrow (500) and destroys nothing; the completing signature closes the action, destroys the holding, deletes both listings and records the total.
  • Review round 4, marketplace::frozen::should_destroy_at_least_what_a_group_proposed_after_the_frozen_identity_cancels: a group of two must sign the destroy; the frozen identity has 17 listings of 10, 20, ... 170 and 100 in its balance. The proposal destroys nothing; the frozen identity then cancels its oldest listing (balance 110); the completion leaves balance 0, destroys the 16 listings left (the newest, past the limit at the proposal, included), lowers the supply by everything the identity held (1,630, more than the 1,460 proposed) and records 1,630.

  • Review round 5: apply_drive_operations::v2::tests::should_refuse_repeated_token_writes_through_the_raw_converter: at the latest version the raw converter refuses a frozen holding destroy naming one listing twice, and two destroys of one holding. The converter dispatcher tests (token_destroy_frozen_funds_transition::tests) pass with generation 0 in its own module (cargo test -p drive --lib -- token_destroy_frozen_funds_transition should_refuse_repeated_token_writes_through_the_raw_converter, 37 tests).

  • Review round 6 (a max code review of this PR):

    • destroy_frozen_funds::should_destroy_frozen_funds_through_the_transformer_at_protocol_version_14: a destroy at protocol version 14 goes through the shared transformer, reads no listings and burns the balance;
    • convert_drive_operations_to_grove_operations::v2::tests: the repeated token write refusal, matched by its message (the earlier test passed on any CorruptedCodeExecution, which the missing token tree also raised), and two credits to one identity: generation 1 (protocol version 14) writes the balance twice, generation 2 once with both;
    • summable_off_count_index_e2e_tests::should_refuse_two_likes_only_from_the_convert_generation_that_checks_counter_moves refuses at 14 and 15;
    • batched_group_drain: the two inverse-shape cases run at protocol version 14 (generation 1) and the latest;
    • the group destroy test reads the proposal's recorded amount (1,460) instead of comparing two constants;
    • converter tests: at 15 without listings the operations equal generation 0's, alone and in a group; with listings a DeleteDocument per listing;
    • test fixtures shared: one market builder with an optional group member, the existing transfer and freeze helpers, one subtree walker in drive::util::test_helpers, asks built by TokenOrder::new_document.

In-place changes to shipped generations

  • The converter: generation 0's body moved as it was from the dispatcher into token_destroy_frozen_funds_transition/v0/mod.rs; generation 1 is a separate implementation. The dispatcher refuses, at generation 0, an action carrying listings (CorruptedCodeExecution), so the amount the shared history builders derive from the action always equals what generation 0 burns. No transformer builds such an action before protocol version 15 (fetch_frozen_listings is None), so no earlier block reaches the refusal.
  • SystemLimits V1 to V4 and DriveTokenOrderEscrowMethodVersions in DRIVE_TOKEN_METHOD_VERSIONS_V1 gain a field (max_frozen_listings_destroyed_per_transition: None, fetch_frozen_listings: None) that nothing before protocol version 15 reads.
  • TokenDestroyFrozenFundsTransitionActionV0 (in memory, never serialized) gains frozen_listings, None before 15.
  • The destroy transformer (TokenDestroyFrozenFundsTransitionActionV0's two transformer routes), which every protocol version runs: it calls read_frozen_listings, which returns no listings at once where fetch_frozen_listings is None (every version before 15), so no read is made or billed and the action is what it was. should_destroy_frozen_funds_through_the_transformer_at_protocol_version_14 runs a destroy through it at 14.
  • TokenTransitionAction::associated_token_event, reached at every version by build_historical_document: for a destroy it records destroyed_amount(), which equals the frozen balance wherever the action carries no listings (every version before 15). Its only consensus caller (token_history_operation) runs for the marketplace transitions alone.

Breaking Changes

Consensus, from protocol version 15: TokenDestroyFrozenFunds also destroys the frozen identity's escrowed listings and reads them first. Earlier protocol versions are unchanged.

Rust API of the drive crate (source-breaking for code that builds or reads these actions itself):

  • TokenTransitionAction::associated_token_event returns Result<TokenEvent, Error>: a destroy's event records destroyed_amount(), a checked sum that fails only on overflow. A caller handles the error.
  • TokenDestroyFrozenFundsTransitionActionV0 gains the public field frozen_listings: Option<FrozenListings> (the listings and the token market contract). A struct literal sets it to None before protocol version 15; the transformer fills it from 15.
  • TokenOperationType gains the public variant TokenDestroyFrozenHolding { token_id, holder_id, balance_amount, order_ids, destroyed_amount }, which removes the frozen balance and each listing's escrow and lowers the total supply once by destroyed_amount. The enum is not #[non_exhaustive], so an exhaustive match on it needs a new arm.
  • convert_drive_operations_to_grove_operations merges repeated balance writes from protocol version 15.

Rust API of the platform-version crate (source-breaking for struct literals of these tables, such as a custom or test PlatformVersion):

  • DriveTokenOrderEscrowMethodVersions gains the public field fetch_frozen_listings (None before protocol version 15, Some(0) from 15).
  • SystemLimits gains the public field max_frozen_listings_destroyed_per_transition: Option<u16> (None before protocol version 15, Some(16) in SYSTEM_LIMITS_V5).

These are also listed in the Unreleased section of CHANGELOG.md.

Checklist:

  • I have performed a self-review of my own code
  • My Rust, FFI, WASM, Swift or Kotlin changes follow the coding conventions
  • 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

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

🤖 Generated with Claude Code

…ts funds (PV15)

A frozen identity's open listings hold its tokens in escrow, out of the
balance a TokenDestroyFrozenFunds burns. From protocol version 15 the
destroy also reads the frozen identity's open listings of the token
(order_escrow.fetch_frozen_listings, byOwner index, oldest first, at most
max_frozen_listings_destroyed_per_transition = 16), destroys what they hold
with the balance through TokenDestroyFrozenHolding (one supply decrement),
deletes their ask documents and records the whole amount. Converter
generation 1 (DRIVE_STATE_TRANSITION_METHOD_VERSIONS_V5) does this; v0's
operations are unchanged. Offers hold credits and are left alone.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@github-actions

github-actions Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

📖 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-11-01T09:16:09.850Z

@coderabbitai

coderabbitai Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • Trigger review
  • Autofix · 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

@thepastaclaw

thepastaclaw commented Oct 22, 2026 •

Copy link
Copy Markdown
Collaborator

🕓 Queued for automated review — 2nd in line, estimated start in ~5 min (commit ca2be73)
Estimated review time once started: ~25 min (two-phase automated review; median of recent runs).

  • Request priority review — click to move this review to the front of the queue.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final validation — Phase 1 + Phase 2

The PV15 gates and combined balance/escrow burn appear correct on inspection, but the listing deletions lose pending batch context, leaving empty index structures and withholding their storage refunds. Historical-path coverage and generation isolation also need improvement. This was static verification at head 2a7d869; current reported CI checks pass, but none reports a dedicated Rust test result.

🔴 1 blocking | 🟡 4 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: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); 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: critical by gpt-6.1-sol (effort low) — This is a large, intricate change to consensus-critical token destruction, escrow removal, supply accounting, state-transition version dispatch, and persisted storage operations in files such as token_destroy_frozen_funds_transition.rs and the Drive batch operation code.
  • 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 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; 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/state_transition_action/action_convert_to_operations/batch/token/token_destroy_frozen_funds_transition.rs`:
- [BLOCKING] packages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/token/token_destroy_frozen_funds_transition.rs:146-155: Preserve pending operations across the listing deletions
  Each listing becomes a separate DeleteDocument, whose lowering in util/batch/drive_op_batch/document.rs passes None for previous_batch_operations. apply_drive_operations_v1 lowers all operations before applying the batch, so each deletion still sees the other listings in storage. With two listings at the same price, neither deletion prunes their shared depth bucket; byOwner ancestors also remain, and listings sharing a creation timestamp leave their shared timestamp buckets behind. The documents and escrow disappear, but empty index trees remain and the identities that paid for those trees do not receive their storage refunds. Both the pinned and current GroveDB implementations already support pending-operation-aware pruning. Build these deletions together in Drive's document-operation lowering and pass the accumulated low-level operations to each deletion, following AddDocumentAndDeleteConsumed, without changing historical protocol paths. Add a regression asserting shared-index cleanup and the associated refunds, not only document absence and token conservation.
- [SUGGESTION] packages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/token/token_destroy_frozen_funds_transition.rs:35-38: Add behavioral coverage for the pre-PV15 dispatcher path
  Generation 0 now delegates to the shared helper, and both transformers call the new version-gated listing fetch. The existing destroy integration tests and the new marketplace tests use PlatformVersion::latest(), so they exercise generation 1; the action accessor tests do not exercise conversion. Comparison with the base revision shows the generation-0 operation sequence is currently preserved, but no behavioral test protects that claim. Add a dispatcher-level test pinned to PlatformVersion::get(14) that verifies nonce, burn, history and group-action operations, including perform_action=false. Also verify that the historical listing-fetch gate returns an empty result at zero cost without marketplace state. This supplies the historical regression evidence required by the book's in-place-change checklist.
- [SUGGESTION] packages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/token/token_destroy_frozen_funds_transition.rs:35-38: Keep the converter generations structurally separate
  The version tables correctly select generation 1 only for PV15, and the false branch currently preserves generation 0's output. However, both generations execute the same listing-aware body through a destroy_listings flag. book/src/versioning/versioned-dispatch.md explicitly calls for separate generation implementations and identifies a shared helper with a flag as the pattern that separation replaces. Retain the balance-only implementation in generation 0 and put the listing-aware implementation in generation 1, selected by the existing table bump. This makes historical replay independent of later edits to PV15 operation ordering, history and group events. This is a structural replay-safety concern, not evidence of a current PV14 behavioral difference.
- [SUGGESTION] packages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/token/token_destroy_frozen_funds_transition.rs:64-68: Avoid cloning the owned action's listing vector
  This helper owns self but clones every escrowed TokenOrder into a new vector. The following code only reads those orders and collects their id/remaining pairs, so the allocation and full-order copies are unnecessary on each listing-aware destroy. Borrow the listing slice until its final use, adjusting the loop accordingly, or move the vector out of the owned action. The borrow can end before public_note_owned consumes self.

In `packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/v0/transformer.rs`:
- [SUGGESTION] packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/v0/transformer.rs:151-166: Share the new listing-fetch and billing sequence
  The owned constructor's fetch, processing-fee update and optional market-contract lookup are repeated in the borrowed constructor at lines 318–333. These operations have identical semantics; only the frozen identity argument needs dereferencing. Keeping this new sequence in two places makes later changes to fee accounting or contract handling liable to diverge between transformation routes. Extract a helper used by both constructors that returns the listings, optional contract and read cost, or updates the FeeResult. This is a maintainability suggestion rather than a layer-ownership or current correctness violation.

QuantumExplorer and others added 3 commits October 22, 2026 10:16
…rozen-escrow

fetch_frozen_listings joins the escrow methods as an OptionalFeatureVersion (None in V1, Some(0) in V3); DRIVE_VERSION_V10 keeps both sides' changes.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
…eration

- New document operation DeleteDocuments: several documents of one type
  deleted in one conversion, each delete built against the ones before it,
  so index trees they share are removed and refunded. Generation 1 of the
  destroy-frozen-funds converter deletes the listings through it; separate
  DeleteDocuments each read the others as stored and left shared trees
  behind, unrefunded.
- The converter's generation 0 is restored to its shipped implementation;
  generation 1 is its own function, and collects only the (id, remaining)
  pairs it needs instead of cloning the orders.
- Both transformer routes read and bill the listings through one helper.
- Tests: two listings sharing index buckets destroyed together leave the
  ask trees cancelling them one by one leaves and refund at least as much;
  dispatcher tests at protocol version 14 (nonce, burn, history, group
  proposal and completion) and 15; the listing read at 14 returns none at
  zero cost.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

At head 086cbf0, four prior findings are fixed, but pending-operation context still stops at each DeleteDocuments operation, leaving shared index trees and their refunds behind when one batch destroys holdings of two tokens. Current reported CI checks pass, although they provide no dedicated Rust test or clippy results; this verification was static only.

🔴 1 blocking

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); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — The intricate changes in token_destroy_frozen_funds_transition.rs and the TokenDestroyFrozenHolding batch operation alter consensus-critical destruction of balances, escrow holdings, and token supply, together with batch-aware listing deletion and storage refunds.
  • 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 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; 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/util/batch/drive_op_batch/document.rs`:
- [BLOCKING] packages/rs-drive/src/util/batch/drive_op_batch/document.rs:635-644: Preserve pending operations across the listing deletions
  The grouped deletion fixes shared-tree pruning within one destroy, but this fresh vector excludes deletions from other operations in the same batch. A valid BatchTransition can destroy one frozen identity's holdings of two tokens from distinct contracts controlled by the signer: their contract nonces, token balances, supplies and escrows are distinct, so the nonce validation and repeated-token-write guard permit this. Conversion emits two DeleteDocuments operations, and apply_drive_operations_v1 lowers both before applying either. Their listings share the marketplace's byOwner/$ownerId index bucket. Each group sees the other token's subtree still in storage, so neither prunes the shared owner bucket; execution removes the listings but leaves empty index structure and withholds its storage refund. Your regression verifies the single-token case, not this enclosing-batch boundary. Thread relevant accumulated operations through document lowering, or coalesce compatible deletion groups in a PV15-only path. Both pinned and current GroveDB already support pending-aware pruning. Add a two-token, two-contract destroy-batch regression comparing remaining index paths and owner refunds with sequential destroys.

QuantumExplorer and others added 5 commits October 22, 2026 11:20
…atch's earlier operations

apply_drive_operations 2 (protocol version 15, unreleased, extended in
place) builds every operation against the operations of its GroveDB batch
built before it and appends its own
(DriveOperation::append_low_level_drive_operations_after_ttl_drain). Every
document insert, update and deletion, DeleteDocuments included, gets them as
previous_batch_operations: one batch destroying a frozen identity's
listings of two tokens removes the owner's shared byOwner bucket and refunds
it, and an insert into a bucket an earlier deletion of the batch empties
keeps the tree. Protocol version 14 and earlier keep generation 1.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Verified the complete diff at 5429998 and confirmed that all six prior findings are fixed. One blocking issue remains: pending-aware batch lowering fixes behavior that an existing non-ignored test still asserts is defective; two non-blocking gaps concern grouped-operation context and PV15 group-action coverage. Validation was static only; current CI checks pass, but none reports the native Drive test suite.

🔴 1 blocking | 🟡 2 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: 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: critical by gpt-6.1-sol (effort low) — The large, intricate diff changes consensus-critical token destruction and escrow accounting in token_destroy_frozen_funds_transition.rs and shared pending-write batch semantics in apply_drive_operations/v2/mod.rs.
  • 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 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
  • 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/util/batch/drive_op_batch/drive_methods/apply_drive_operations/v2/mod.rs`:
- [BLOCKING] packages/rs-drive/src/util/batch/drive_op_batch/drive_methods/apply_drive_operations/v2/mod.rs:134-138: Update the ranked-drain test that still asserts the pre-PV15 defect
  Passing the enclosing batch's pending operations also fixes the ranked-index drain characterized by the non-ignored `a_batched_drain_leaves_a_phantom_group_that_verifies_and_proves` test in `packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/batched_group_drain.rs:577`. Its helpers select `PlatformVersion::latest()` and call `apply_drive_operations`, so they now use this path. The second deletion sees the first pending deletion and prunes the empty `beta` group, but the assertion at lines 594–599 still requires both `alpha` and `beta` to remain. This leaves a failing test in the native Drive suite, outside the targeted filters listed in the PR description. Retain the old characterization under PV14 if historical coverage is needed, and update or enable the desired-behavior drain cases for PV15.

In `packages/rs-drive/src/util/batch/drive_op_batch/document.rs`:
- [SUGGESTION] packages/rs-drive/src/util/batch/drive_op_batch/document.rs:590-601: Preserve enclosing batch context through grouped document operations
  The new append path preserves enclosing batch context for individual document operations but drops it for `MultipleDocumentOperationsForSameContractDocumentType`. That variant reaches this fallback, and its existing lowering starts a fresh accumulator at line 1011. Its inserts and updates therefore see only their group's predecessors. Expressing the replacement insert in `should_insert_into_a_tree_an_earlier_deletion_of_the_batch_empties` as a one-member grouped `AddOperation` exposes the gap: the earlier deletion schedules removal of shared index trees, while the grouped insert sees those trees in storage but cannot cancel their pending removal. The marketplace's current `DeleteDocuments` path is fixed, but the shared API's pending-context guarantee still depends on how equivalent writes are packaged. Pass the enclosing accumulator through grouped lowering and add the grouped equivalent of the delete-then-insert regression. This is a completeness gap in the shared batch abstraction, not a violation of layer ownership.

In `packages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/token/token_destroy_frozen_funds_transition.rs`:
- [SUGGESTION] packages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/token/token_destroy_frozen_funds_transition.rs:428-430: Exercise PV15 group actions with escrowed listings
  The listing-aware dispatcher test constructs only an ungrouped, performing action. The new group-action dispatcher tests select PV14, and the existing latest-version group integration tests contain no listings. No test therefore protects generation 1's combination of a listing-inclusive group event and deferred execution: recording only the liquid balance in that event would pass the current tests. Add latest-version cases with nonempty listings for a non-closing proposal and a completing signature. Assert that the proposal records the balance-plus-escrow total without emitting holding destruction, document deletion or history, and that completion closes the group action and emits the listing destruction and total-valued history.

QuantumExplorer and others added 2 commits October 22, 2026 14:26
…erations see the batch

- apply_drive_operations 2 builds the operations of a
  MultipleDocumentOperationsForSameContractDocumentType group against the
  enclosing batch's operations too, instead of a fresh accumulator.
- batched_group_drain: the five drain specifications, #[ignore]d as a latent
  defect while the documents batch cap is 1, run and pass at the latest
  version; the phantom-group characterization applies its batch at protocol
  version 14 (generation 1). The module, case and SystemLimits docs say
  protocol version 15 removes the hazard and that the cap's other
  dependencies still stand.
- Tests: a deletion and a grouped insert into the same bucket in one batch;
  the frozen-funds converter at the latest version in a group with
  listings (proposal records balance plus escrow, completion destroys and
  records the total).

Co-Authored-By: Claude Opus 5.5 <[email protected]>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

The complete diff at the requested head has no blocking consensus, accounting, security, or historical-replay defects. Two non-blocking improvements remain: make the new batch append match exhaustive for the currently delegated variants, and add coverage for destroying partially filled listings. Available CI checks pass, although the supplied snapshot does not include native Rust test results.

🟡 2 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: 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: critical by gpt-6.1-sol (effort low) — This is a large consensus-critical change that alters protocol-versioned state-transition logic, token destruction and escrowed-fund movement, listing deletion, supply accounting, and persistent GroveDB operations.
  • 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 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
  • 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/util/batch/drive_op_batch/document.rs`:
- [SUGGESTION] packages/rs-drive/src/util/batch/drive_op_batch/document.rs:654-665: Match remaining document operations explicitly instead of a wildcard
  The new append path handles inserts, updates, deletions, and grouped writes with the enclosing batch as pending context, but the trailing wildcard delegates `AddContestedDocument`, `AddWithdrawalDocument`, and `DocumentHistory` to the standalone lowering path. That is the current intended behavior for those variants, but the wildcard removes exhaustiveness checking: a future `DocumentOperationType` variant would silently bypass the pending-aware path. Enumerate the three remaining variants explicitly so every new document operation requires an intentional pending-context decision.

In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/marketplace/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/marketplace/mod.rs:2593-2602: Exercise destruction after a listing has been partially filled
  The new frozen-listing tests create each order with `TokenOrder.amount` equal to `TokenOrder.remaining`, so they cannot distinguish burning the original listing amount from burning only the escrow still present. The implementation currently uses `remaining`, but both fields share the same type and a regression to `amount` would pass these tests. Add a partial-fill case before freezing—such as filling 1,000 tokens from a listing—and assert that destruction lowers supply and records history by the remaining amount, preserves the counterparty's purchased tokens, and removes the remaining escrow and document.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

At c832240, the PV15 frozen-listing destruction path preserves historical dispatch and propagates pending operations through document application; all 11 prior findings are addressed in source. One non-blocking consistency gap remains in the alternate raw batch-conversion API. Current reported CI checks pass, but they do not establish native Drive or Drive-ABCI test success; this verification was static-only.

🟡 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: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 11: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 15: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 16: 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) — This large, intricate diff changes consensus-critical protocol-version dispatch, token supply destruction, escrowed funds movement, and persistent document/index deletion in functions such as token_destroy_frozen_funds_transition and apply_drive_operations.
  • 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 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
  • 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 — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — 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
  • 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/util/batch/drive_op_batch/drive_methods/apply_drive_operations/v2/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/util/batch/drive_op_batch/drive_methods/apply_drive_operations/v2/mod.rs:134-141: Share pending-aware lowering with the raw batch converter
  The application path now preserves enclosing batch context, but the public convert_drive_operations_to_grove_operations API still selects generation 1 at PV15, delegates to generation 0, and lowers each operation independently. In particular, each newly added DeleteDocuments operation starts a fresh accumulator in document.rs. Converting two such operations for one owner's asks of different tokens therefore still leaves their shared byOwner bucket behind: neither group sees the other's pending deletions. Existing RawConversion document tests establish that document batches are supported by this API. This is a non-blocking completeness gap in Drive's shared batch abstraction, not a misplaced-layer violation or a blocker for marketplace execution: the current production caller converts identity payouts, not documents. Add a PV15 raw-converter generation that uses the pending-aware accumulator, preserves the existing debt and ephemeral-operation refusals, and exercises the two-token cleanup case through both entry points.

QuantumExplorer and others added 2 commits October 22, 2026 23:19
… batch's earlier ones at protocol 15

convert_drive_operations_to_grove_operations 2, selected by DRIVE_VERSION_V10,
is generation 1 (repeated counter moves refused, TTL preparation) with every
operation appended through append_low_level_drive_operations_after_ttl_drain,
as apply_drive_operations 2 does, so two deletions emptying a shared index
tree remove it on this entry point too. Credits that repaid a debt and
ephemeral operations are refused as before. New test: one owner's asks of
two tokens deleted through the converter leave the trees deleting them
separately leaves.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
…grows the destroy

The completing signature reads the frozen identity's listings again and
destroys what it holds then, as generation 0 does with the balance. A
frozen identity can only cancel a listing, which moves its escrow into the
destroyed balance, so the completion never destroys less than proposed.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

The protocol-15 frozen-listing behavior is correctly version-gated and the prior correctness, history, batch-context, and API-documentation findings are resolved. Three non-blocking issues remain: CheckTx performs a new stateful multi-document listing scan during approximation, the PV15 raw converter omits the repeated token-write guard used by application execution, and the historical generation-0 implementation remains embedded in the dispatcher instead of its own version module. The supplied CI snapshot reports all listed checks successful.

🟡 3 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: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: muse-spark-1.3-contributor (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); 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) — This large, intricate diff changes consensus-critical protocol-version dispatch and token destruction operations that move funds, alter supply, and delete persisted escrow listings in the TokenDestroyFrozenFunds conversion and batch application paths.
  • 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 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
  • 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/util/batch/drive_op_batch/drive_methods/convert_drive_operations_to_grove_operations/v2/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/util/batch/drive_op_batch/drive_methods/convert_drive_operations_to_grove_operations/v2/mod.rs:27-32: Retain repeated token-write protection in the PV15 raw converter
  `apply_drive_operations_v2` rejects batches that write a token holder balance, supply, or escrow key more than once, but the newly added `convert_drive_operations_to_grove_operations_v2` only calls `refuse_repeated_counter_moves`. `TokenDestroyFrozenHolding` expands into an absolute holder write, a supply write, and one escrow write per listing; two such operations, or duplicate listing IDs, can therefore produce conflicting absolute writes in the raw conversion path. The public converter accepts arbitrary `DriveOperation` values, so it should fail closed with the same invariant before lowering the batch.

In `packages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/token/token_destroy_frozen_funds_transition/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/token/token_destroy_frozen_funds_transition/mod.rs:27-100: Isolate the historical converter generation in a v0 module
  The dispatcher currently contains the complete generation-0 balance-only implementation in its `0` match arm, while only generation 1 is in `v1/mod.rs`. The repository's versioning convention requires `mod.rs` to dispatch to generation-specific implementation modules, and the coding conventions state that each generation must be structurally isolated to make replay safety independent of later edits. Move the existing balance-only body and its generation-0 tests into `token_destroy_frozen_funds_transition/v0/mod.rs`, leaving the dispatcher with only the version match. This is a structural replay-safety issue even though the current generation-0 output is unchanged.

In `packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/v0/transformer.rs`:
- [SUGGESTION] packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/v0/transformer.rs:350-377: Avoid stateful frozen-listing reads during CheckTx cost approximation
  (existing thread: https://github.com/dashpay/platform/pull/5532#discussion_r4372880155)
  The destroy transformer receives `approximate_without_state_for_costs` and uses it for the frozen balance read, but `read_frozen_listings` has no approximation parameter and unconditionally calls `fetch_frozen_token_listings`. That query uses `dry_run = false`, traverses the `byOwner` index, and deserializes up to 16 attacker-selected ask documents before the later destroy authorization and frozen-account state checks. A correctly signed transition with a valid nonce can therefore make CheckTx and recheck perform repeated, uncharged multi-document reads even when block validation rejects the destroy. Existing point reads in other marketplace transitions do not remove the additional range-scan-and-decode workload introduced by this PR. Thread the approximation mode through the helper and use a deterministic bounded fee estimate without resolving listings on the non-authoritative path, while retaining the stateful lookup for validator execution.

…es; destroy generation 0 in its own module

convert_drive_operations_to_grove_operations 2 refuses a batch writing one
token balance, supply, pool total or escrow twice before lowering it, as
apply_drive_operations does. The destroy-frozen-funds converter's
generation 0 moves unchanged from the dispatcher into v0/mod.rs.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

The PV15 frozen-listing destruction implementation is correctly version-dispatched, preserves historical behavior, maintains token and escrow accounting, and includes the requested pending-operation and raw-converter safeguards. One in-scope security/performance concern remains: CheckTx and RecheckTx perform a real bounded listing query and decode before authorization, creating repeatable unpaid mempool I/O; no failed code checks are present, though book-preview and runner-selection checks remain queued.

🟡 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: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: muse-spark-1.3-contributor (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); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 12: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 13: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 14: gpt-6-astra (agent: phase2-reviewer, role: tenderdash-consensus-security); reviewer 15: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 16: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 17: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 18: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 19: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 20: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — This is a large cross-cutting change to consensus-critical token destruction, escrowed-funds movement, supply updates, document deletion, and protocol-versioned Drive operation conversion in functions including token_destroy_frozen_funds_transition and convert_drive_operations_to_grove_operations.
  • 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 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
  • 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 — tenderdash-consensus-security (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, gpt-6-astra — tenderdash-consensus-security (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, gpt-6.1-sol — tenderdash-consensus-security (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/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/v0/transformer.rs`:
- [SUGGESTION] packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/v0/transformer.rs:371-377: Avoid stateful frozen-listing reads during CheckTx cost approximation
  (existing thread: https://github.com/dashpay/platform/pull/5532#discussion_r4372880155)
  `transform_token_transition_v0` passes `approximate_for_costs = true` when ordinary Batch CheckTx or RecheckTx runs, but `read_frozen_listings` does not receive or honor that mode. Once protocol 15 is active, these lines call `fetch_frozen_token_listings`, whose implementation performs a real `query_documents` read with `dry_run = false` and decodes up to 16 `ask` documents. Ordinary CheckTx does not perform the destroy action's full frozen-account and authorization state validation until later, so a correctly signed, funded transition can name arbitrary token and frozen-identity IDs and trigger this bounded indexed read before being rejected. The estimated fee is not charged for a rejected admission, allowing repeated valid-nonce submissions or multiple identities to amplify RocksDB traversal and document decoding. Apply a common CheckTx policy here: either use a deterministic stateless upper-bound estimate while deferring listing resolution to validator execution, or move this read behind the relevant authorization stage; the policy should eventually cover the analogous marketplace reads as well.

…rozen-escrow

The changelog keeps v5.1-dev's compiled contract storage entry and the
5.0.1 release beside this PR's API entries. The frozen listings note is
v15 note 14.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

The PV15 implementation correctly isolates historical behavior, destroys the selected listing escrow, preserves offer credit escrow, and carries pending batch operations through both conversion paths. One non-blocking resource-amplification concern remains: approximate CheckTx transformation performs a stateful indexed query and decodes up to 16 listings before the destroy authorization and frozen-account checks run.

🟡 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: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); 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) — This is a large, intricate change to consensus-critical token destruction and storage operations that changes funds movement by burning escrowed holdings and deleting marketplace documents in protocol version 15.
  • 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 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
  • 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/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/v0/transformer.rs`:
- [SUGGESTION] packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/v0/transformer.rs:350-377: Avoid stateful frozen-listing reads during CheckTx cost approximation
  (existing thread: https://github.com/dashpay/platform/pull/5532#discussion_r4372880155)
  `read_frozen_listings` does not receive `approximate_without_state_for_costs` and always calls `fetch_frozen_token_listings`, whose implementation queries GroveDB with `dry_run = false` and decodes up to 16 indexed `ask` documents. The destroy transformer already avoids a state read for the frozen balance when approximation is enabled, but this new listing query still runs before the later state-validation stage checks that the account is frozen and that the caller is authorized to destroy it. Consequently, a correctly signed and funded transition naming arbitrary token and identity IDs can repeatedly cause bounded index traversal and document decoding during CheckTx/recheck even when block validation will reject it. The existing marketplace reads do not remove the additional indexed multi-document work introduced here. Thread the approximation mode through this helper and use a deterministic bounded cost-only estimate, or resolve the listings only after the applicable authorization gate; the broader policy for other marketplace reads can be handled separately.

QuantumExplorer and others added 4 commits October 29, 2026 07:19
… one listings field, versioned tables

- convert_drive_operations_to_grove_operations 2 merges repeated balance
  writes as apply_drive_operations does, with tests of its own and a v1/v2
  dispatcher test; it has no production caller at protocol 15
- the docs say only document operations are built against the batch
- TokenDestroyFrozenFundsTransitionActionV0 carries one
  frozen_listings: Option<FrozenListings>; destroyed_amount is one
  unversioned checked sum and associated_token_event takes no version
- the converter's generation 1 delegates to generation 0 without listings,
  deletes each listing with DeleteDocument (DeleteDocuments is removed:
  at protocol 15 separate deletes see each other), and passes the
  destroyed total to TokenDestroyFrozenHolding, which refuses a burn
  generation it does not know
- a group signature that neither proposes nor completes reads no listings
- max_frozen_listings_destroyed_per_transition is Option<u16>, Some(16)
  only in SYSTEM_LIMITS_V5; DRIVE_STATE_TRANSITION_METHOD_VERSIONS_V5 uses
  struct update syntax; the version docs name the batch-context change
- tests: the destroy through the transformer at protocol 14, the drain
  inverse shapes at 14 and 15, the proposal's recorded amount, shared
  test fixtures

Co-Authored-By: Claude Opus 5.5 <[email protected]>
…rozen-escrow

The frozen listings note is v15 note 16, beside v5.1-dev's yes/no vote poll
note 10 that came through the stack; DRIVE_VERSION_V10 keeps both the
yes/no bullet and the frozen listings and batch-context bullets. The
converter test that compares generation 1 without listings to generation 0
converts one action twice, as the fixture's contract id is random.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

The PV15 frozen-listing destruction path is correctly version-gated and preserves token, escrow, document, and history accounting. Three non-blocking issues remain: CheckTx performs a stateful listing range read before authorization, destroyed-amount history derivation is not dispatched by protocol generation, and the PV15 converter clones an owned listing vector. The supplied CI snapshot contains no failing code checks; the listed checks passed.

🟡 3 suggestion(s)

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: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); 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) — This large, intricate diff changes protocol-versioned consensus state transitions that destroy token funds, mutate escrowed listings and supply, and delete persisted documents.
  • 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 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
  • 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/state_transition_action/action_convert_to_operations/batch/token/token_destroy_frozen_funds_transition/v1/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/token/token_destroy_frozen_funds_transition/v1/mod.rs:24-33: Avoid cloning the owned action's listing vector
  The generation-1 converter consumes `self` but begins with `self.frozen_listings().cloned()`, copying the `FrozenListings` container and every `TokenOrder` before the code only reads each order's ID and remaining amount. The limit of 16 makes the cost bounded and this is a cold administrative path, but the allocation and full-order copies are still unnecessary. Destructure the owned action or move the owned listings out after extracting the other fields so only the required IDs and amounts are copied or moved.

In `packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/v0/transformer.rs`:
- [SUGGESTION] packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/v0/transformer.rs:352-388: Avoid stateful frozen-listing reads during CheckTx cost approximation
  (existing thread: https://github.com/dashpay/platform/pull/5532#discussion_r4372880155)
  `read_frozen_listings` does not receive `approximate_without_state_for_costs` and always performs `fetch_frozen_token_listings`, whose query reads and decodes up to 16 listing documents before the later state-validation stage checks the frozen account and signer authorization. Ordinary CheckTx and recheck transformations can therefore perform an attacker-selected `byOwner` range scan for a correctly signed but ultimately unauthorized destroy; the estimated fee is not charged for a rejected mempool transaction. The existing single-order reads in other marketplace transitions do not remove this new range-scan amplification. Thread the approximation mode through this helper and use a deterministic stateless upper-bound estimate, or defer the actual listing resolution until validator execution while preserving conservative admission accounting.

In `packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/mod.rs:22-40: Keep destroyed-amount history logic in versioned generation modules
  (existing thread: https://github.com/dashpay/platform/pull/5532#discussion_r4371810198)
  `destroyed_amount()` is an unversioned, consensus-visible calculation that always adds `frozen_listings`, while the protocol-14 converter selected by generation 0 deliberately records only `amount`. The normal protocol-14 transformer currently enforces the implicit invariant by leaving `frozen_listings` empty, but the public action can carry listings and the shared `associated_token_event` and `build_historical_document` callers do not receive or dispatch on `PlatformVersion`. A listing-bearing action used through a pre-PV15 shared history path can therefore produce a balance-plus-escrow event even though generation 0 conversion is balance-only, and future PV15 changes can affect historical builders. Restore a versioned destroyed-amount dispatcher with generation-owned v0/v1 implementations and pass the active platform version through the shared callers.

…nverter

Generation 1 of the token destroy frozen funds converter cloned the whole
FrozenListings (every TokenOrder) to read each listing's id. It now takes
the ids and the token market contract handle, and builds the same
operations in the same order.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

The PV15 frozen-listing destruction path is correctly version-gated and the pending-operation, repeated-write, history, group, and regression-test changes are present. Two review issues remain: CheckTx performs a new stateful multi-document listing query before authorization, and destroyed-amount history/event calculation is unversioned despite differing protocol-generation semantics. The supplied CI snapshot contains no failed checks.

🟡 3 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: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); 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) — This large, intricate diff changes consensus-critical token destruction, escrow removal, supply accounting, and historical protocol-version dispatch in the state-transition and storage layers.
  • 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 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
  • 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/state_transition_action/action_convert_to_operations/batch/token/token_transition.rs`:
- [SUGGESTION] packages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/token/token_transition.rs:212-216: Keep shared action history consistent with listing destruction
  `associated_token_event()` and the historical-document path call the unversioned `destroyed_amount()` without a `PlatformVersion`, while generation 0 of the converter intentionally emits a balance-only destroy. For a listing-bearing action supplied at protocol 14, the converter records only the balance but the shared event/history builder records balance plus escrow. The normal transformer prevents that shape before protocol 15, but the public action API and the existing direct converter tests do not enforce the invariant at the shared boundary. Pass the active platform version through the shared event/history construction and select the same generation-specific amount calculation used by the converter.

In `packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/v0/transformer.rs`:
- [SUGGESTION] packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/v0/transformer.rs:381-387: Avoid stateful frozen-listing reads during CheckTx cost approximation
  (existing thread: https://github.com/dashpay/platform/pull/5532#discussion_r4372880155)
  `read_frozen_listings` unconditionally calls `fetch_frozen_token_listings`, whose query uses `dry_run = false` and decodes up to 16 ask documents. The transformer already receives `approximate_without_state_for_costs` and uses it to avoid stateful token-balance reads, but that mode is dropped before this helper. During ordinary CheckTx/recheck, the listing query therefore runs before the later state-validation stage checks whether the target identity is frozen and whether the signer is authorized. A correctly signed and funded transition can name arbitrary token and identity IDs and repeatedly cause bounded RocksDB/index reads and document decoding without being charged at admission. The existence of similar single-order or group reads does not remove the additional multi-document scan introduced here. Pass the approximation mode through this helper and use a deterministic bounded estimate without fetching documents during admission, or establish one shared policy for all such transformation reads.

In `packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/mod.rs:31-40: Keep destroyed-amount history logic in versioned generation modules
  (existing thread: https://github.com/dashpay/platform/pull/5532#discussion_r4371810198)
  `destroyed_amount()` is again a single unversioned helper that always adds `frozen_listings`. The high-level converter dispatches protocol 14 to generation 0, which records and destroys only `amount`, while protocol 15 generation 1 includes listing escrow. Although transformer-produced pre-PV15 actions normally contain no listings, the action field is public and the converter tests explicitly construct a listing-bearing action at protocol 14; the shared event/history API would then report a different amount from the generation-0 converter. More importantly, future edits to listing handling can change replay-sensitive history and group-event behavior without changing the protocol-version table. Restore a dispatcher using the converter's version slot and place the balance-only and listing-aware calculations in generation-owned implementations.

Generation 0 of the destroy frozen funds conversion burns the frozen
balance alone, while the shared history builders derive the destroyed
amount from the action, listings included. The dispatcher now refuses
an action carrying listings at generation 0, so the two always agree.
No transformer builds such an action before protocol version 15
(fetch_frozen_listings is None), so no earlier block reaches it.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

The PV15 frozen-listing destruction flow is correctly version-gated, batch-aware, and covered by targeted regressions. Two suggestions remain: the new listing query performs a stateful scan during approximate CheckTx transformation, and the consensus-visible destroyed amount remains an unversioned shared helper rather than generation-owned logic. CI has no completed code failure, but policy and book-preview checks are still pending.

🟡 2 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: 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: critical by gpt-6.1-sol (effort low) — This is a large, intricate change to consensus-critical token destruction and storage operations that moves funds, deletes escrowed listings, updates supply, and introduces protocol-versioned behavior across Drive and state-transition execution.
  • 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 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
  • 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/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/v0/transformer.rs`:
- [SUGGESTION] packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/v0/transformer.rs:352-388: Avoid stateful frozen-listing reads during CheckTx cost approximation
  (existing thread: https://github.com/dashpay/platform/pull/5532#discussion_r4372880155)
  `read_frozen_listings` does not receive or inspect `approximate_without_state_for_costs`. Once protocol 15 is active, it always calls `fetch_frozen_token_listings`, whose query performs a real stateful `query_documents` call and decodes up to 16 attacker-selected listing documents. The destroy transformer is used during approximate CheckTx transformation before the full state-validation path verifies that the signer may destroy the target frozen identity, so a correctly signed but ultimately rejected transition can repeatedly force bounded RocksDB reads and document decoding without being admitted or charged. Keep the actual listing resolution for validator execution, and use a deterministic stateless/worst-case estimate or another shared admission policy during approximation; if the policy is changed globally, track and apply it consistently to the sibling marketplace reads as well.

In `packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/mod.rs:23-40: Keep destroyed-amount history logic in versioned generation modules
  (existing thread: https://github.com/dashpay/platform/pull/5532#discussion_r4371810198)
  `destroyed_amount` is a single unversioned implementation that sums all attached listings, even though its result is consensus-visible through `associated_token_event`, group-action events, and token history documents. Generation 0 now refuses listing-bearing actions during conversion, which prevents the normal pre-PV15 transformer path from producing the inconsistent shape, but the shared action/event API still computes its amount without selecting a platform-version generation. A future change to the listing-aware behavior could therefore alter replay-sensitive history construction or public action-derived results without an explicit generation change. Restore a dispatcher keyed by the destroy-converter version with generation-owned v0 and v1 calculations, or pass the active platform version through the shared event/history builders.

QuantumExplorer and others added 6 commits October 30, 2026 13:29
…rozen-escrow

# Conflicts:
#	packages/rs-platform-version/src/version/drive_versions/drive_state_transition_method_versions/v5.rs
#	packages/rs-platform-version/src/version/drive_versions/v10.rs
#	packages/rs-platform-version/src/version/v15.rs
v5.1-dev's consume single read (#5360) adds
AddDocumentAndDeleteReadConsumed and ForceDeleteReadDocument. At
protocol version 15 apply_drive_operations 2 builds every document
operation against the batch's earlier ones, so both get an arm there:
as their AddDocumentAndDeleteConsumed and ForceDeleteDocument siblings,
deleting from what the lookup read when the read was kept.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
# Conflicts:
#	packages/rs-platform-version/src/version/drive_versions/v10.rs
#	packages/rs-platform-version/src/version/v15.rs
# Conflicts:
#	packages/rs-drive/src/util/batch/drive_op_batch/drive_methods/apply_drive_operations/v2/mod.rs
#	packages/rs-platform-version/src/version/drive_versions/v10.rs
…rozen-escrow

# Conflicts:
#	packages/rs-drive/src/util/batch/drive_op_batch/drive_methods/apply_drive_operations/v2/mod.rs
#	packages/rs-platform-version/src/version/v15.rs

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

The PV15 frozen-listing destruction path is correctly gated, preserves pre-PV15 behavior, and uses pending-aware document lowering with checked accounting. Two non-blocking issues remain: the new listing query performs stateful work during approximate CheckTx transformation, and destroyed-amount history calculation remains an unversioned shared helper. The available CI snapshot shows no relevant Rust build or test failure; the cancelled duplicate Kotlin run is superseded by a successful run.

🟡 2 suggestion(s)

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: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); 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) — This is a large, intricate change to consensus-critical state-transition, token destruction, escrow, storage, version-dispatch, and funds-movement logic, including deleting listings and burning their held tokens.
  • 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 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
  • 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/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/v0/transformer.rs`:
- [SUGGESTION] packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/v0/transformer.rs:352-387: Avoid stateful frozen-listing reads during CheckTx cost approximation
  (existing thread: https://github.com/dashpay/platform/pull/5532#discussion_r4372880155)
  `read_frozen_listings` does not receive or inspect `approximate_without_state_for_costs`, so once protocol 15 is active it performs a real `fetch_frozen_token_listings` query and decodes up to 16 listing documents even during CheckTx and recheck transformation. The destroy transformer's state authorization runs later, allowing a correctly signed and funded but unauthorized request to trigger this bounded RocksDB and document-decoding workload before block validation rejects it. Existing marketplace reads do not make this newly introduced path harmless; either use a deterministic approximation for this read and its bounded maximum cost, or establish a tracked shared policy that deliberately permits these stateful CheckTx reads across all marketplace transformations.

In `packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/mod.rs:22-40: Keep destroyed-amount history logic in versioned generation modules
  (existing thread: https://github.com/dashpay/platform/pull/5532#discussion_r4371810198)
  `destroyed_amount()` is consensus-visible because its result is written into token-history documents and group-action events, and it is also used by the PV15 converter. The helper has no `PlatformVersion` dispatch and derives its behavior directly from the optional listing field, while `associated_token_event()` and historical-document construction also call it without selecting a generation. The generation-0 converter refusal protects the normal PV14 transformer path, but it does not make the shared public history API generation-aware. Keep the balance-only and listing-inclusive calculations behind generation-owned implementations selected by the existing converter version, or thread the active platform version through the shared history builders.

QuantumExplorer and others added 2 commits October 31, 2026 05:13
…ken-market-frozen-escrow

The frozen listings note becomes v15 note 19, and its slot comments and the
batch estimate note cite it as 19.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

At the reviewed head, the PV15 activation gates, balance-plus-escrow accounting, history totals, and pending-aware document lowering address the prior blocking findings. One non-blocking CheckTx read-amplification suggestion remains applicable. This was a static review: current listed CI checks pass, but no targeted native Drive or Drive-ABCI test job is listed, and no builds or tests were run locally.

🟡 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: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 11: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 12: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 15: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 16: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 17: 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 changes in token_destroy_frozen_funds_transition/v1/mod.rs and TokenOperationType::TokenDestroyFrozenHolding alter consensus-executed token destruction, escrow removal, total supply accounting, and batched document deletion.
  • 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 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
  • 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, 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
  • 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/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/v0/transformer.rs`:
- [SUGGESTION] packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/v0/transformer.rs:381-388: Avoid stateful frozen-listing reads during CheckTx cost approximation
  (existing thread: https://github.com/dashpay/platform/pull/5532#discussion_r4372880155)
  Keeping concrete listings for accurate deletion estimates is justified, but the current admission pipeline amplifies this new read: ordinary unsponsored Batch CheckTx transforms for advanced-structure validation, discards that action when full state validation is skipped, then transforms again for fee admission. This helper therefore performs two real index queries and decodes up to 32 listing documents before destroy authorization; recheck performs another query. Recording those costs in FeeResult does not collect or reserve them during admission. The fill/cancel point lookups are a relevant precedent for shared policy, but the group-action-info lookup already uses StatelessDirectQuery during approximation, so it does not establish the same precedent. The 16-document cap bounds each query, making this a hardening suggestion rather than a demonstrated denial-of-service vulnerability. Preserve the listing-dependent deletion estimate and protect the fan-out centrally in Drive-ABCI with a shared CheckTx work budget or equivalent admission control. If that policy is handled separately, link a tracking issue; none is present in the supplied discussion.

…e/token-market-frozen-escrow

SYSTEM_LIMITS_V5 sets both max_private_document_entry_size and
max_frozen_listings_destroyed_per_transition. The frozen listings note becomes
v15 note 20, and its slot comments and the batch estimate note cite it as 20.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Verified the complete diff at fe2d450 and revalidated all 31 prior findings. No actionable in-scope defect remains: PV15 activation, checked destruction totals, historical compatibility, and pending-aware document lowering are consistent with the implementation and regression assertions. This was a static review only; current PR checks pass, but the listed checks do not establish native Drive or Drive-ABCI regression-suite success.

🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(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: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); 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 change directly alters consensus-critical token destruction and escrow accounting in token_destroy_frozen_funds_transition/v1/mod.rs and convert_drive_operations_to_grove_operations/v2/mod.rs, including supply reduction and batched listing deletion.
  • 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 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
  • 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 the current code and confirm that no unresolved issues remain.

No unresolved findings remain from the prior review on this head.

…into claude/token-market-frozen-escrow

apply_drive_operations 2 keeps this branch's document operations built against
the batch (append_low_level_drive_operations_after_ttl_drain) and lowers the
whole batch into EstimatedLayers::keeping_every_description, resolved once the
batch is lowered. The batch-document-write tests stay beside the estimate
tests; v10 and v15 note 14 keep this branch's word that picking the
description does not change how a batch is applied, as note 20 changes it.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Independently inspected the complete diff at fee5083 and found no remaining actionable in-scope findings. All 31 prior findings were revalidated: 28 are fixed, two are withdrawn, and one is outdated; PV15 activation, historical behavior, checked destruction accounting, and pending-aware document lowering are consistent with the source. Current published CI checks pass, but they provide no dedicated native Drive or Drive-ABCI test result; this verification was static, with no builds or tests run.

🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(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: 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: critical by gpt-6.1-sol (effort low) — The intricate changes in token_destroy_frozen_funds_transition/v1/mod.rs and apply_drive_operations/v2/mod.rs alter consensus-executed token destruction, escrow removal, supply accounting, and batched document deletion.
  • 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 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
  • 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 the current code and confirm that no unresolved issues remain.

No unresolved findings remain from the prior review on this head.

…de/token-market-frozen-escrow

SYSTEM_LIMITS_V5 sets the private document store's chunk limits beside
max_frozen_listings_destroyed_per_transition.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants