[Claudius-Maginificent]

TL;DR

When the blockchain replaces a few recent blocks, the wallet already corrects itself in memory. This PR makes the saved wallet follow, so the balance and history are still right after the app restarts.

User story

As a wallet user, I want my balance and transaction history to match the real chain after a short chain reorganization, so that I do not see coins that no longer exist, or miss coins that came back, the next time I open the app.

Scenario

Actual behavior

A payment arrives and is confirmed in a block. Shortly after, the network replaces that block with a different one. While the app stays open the wallet shows the right picture. After a restart it shows the old one: the payment from the replaced block is still listed as confirmed, coins it spent are still missing, and the wallet does not look again at the blocks that replaced the old ones.

Expected behavior

After a restart the wallet shows the same balance and history as a wallet that was never closed: transactions of replaced blocks are gone or back to unconfirmed, the coins they spent are available again, and scanning resumes from the point where the chain changed.

Detailed discussion

Issue being fixed or feature implemented

Fixes #5380. Stacked on #5150 (fix/pr-5126), which pins the rust-dashcore revision that emits WalletEvent::ChainTruncated; merge that first, then retarget this PR.

key-wallet rewinds its in-memory state on a fork (truncate_above) and reports what it did. #5307 mapped that event to an empty persistence round, because changeset merging and every backend only move heights upwards and none can delete the records of a dropped block. The stored wallet therefore stayed on the replaced branch.

What was done?

• Changeset. CoreChangeSet gains truncations: Vec<ChainTruncation> (height, removed txids, kept unconfirmed_txids, restored_outpoints). A backend applies them before the rest of the round. Merge reduces earlier content by an incoming truncation (records, coins and heights above the fork), so one merged round stores the same wallet as one round per step.
• Capability. PersistenceCapabilities::CORE_CHAIN_TRUNCATION (bit 15; bit 13 belongs to asset-lock operation IDs and bit 14 is reserved for CORE_HISTORY_RESTORE in fix(platform-wallet)!: replay recorded transaction history on load for every persister #5220). A persister that does not attest it is not handed the truncation; the rest of the round is stored and one error is logged. A truncation a persister rejected is offered again with the next round; a backend that keeps a failed round owns its truncations until they are applied.
• SQLite. apply_truncation runs inside the round's write transaction: it deletes the records of dropped blocks with their coins, returns InstantSend-locked ones to unconfirmed, releases the coins the removed records spent and lowers the stored heights to the fork. No schema change. When a fatal flush makes the store drop its buffer, it keeps that buffer's truncations and applies them ahead of the next write, each once.
• FFI. A new trailing, size-negotiated slot on_persist_wallet_changeset_truncations_fn in PersistenceCallbacksExtension delivers ChainTruncationFFI values before the other callbacks of the round. A transaction a same-round truncation keeps is projected to the host as InstantSend-locked, so the round's ordinary callbacks do not overwrite that state.
• JNI / Kotlin. NativePersistenceBridge.onWalletChangesetChainTruncated; the Room handler applies the truncation in the round's transaction.
• Swift. The SwiftData handler applies the truncation in the round's changeset. A removed row is set aside and deleted at the end of the round unless the same round stores the transaction again, because txid and outpoint are unique attributes and a delete followed by an insert of the same key in one save was not confirmed to be safe.
• Shared transaction rows (Swift, Kotlin). Those stores keep one transaction row for all wallets while rounds commit per wallet, so a truncation takes only the truncating wallet's coins, spend links, pending claims and account involvement; the shared row is deleted only when nothing references it.
• DashPay payments. A sent payment whose block was dropped returns from confirmed to pending. If the store rejected the round that carried that change, it is offered again with the wallet's next round.
• Spend fences. A coin a truncation hands back is fenced again until the undone spend is seen again, so it is not selected for a second spend while the first may still confirm. The fence is set where the wallet is truncated, under the wallet-manager write lock, because fork recovery truncates between two runs of the SPV client, when no event handler is listening.
• The InstantSend lock of a detected transaction is stored when the transaction is detected, so a fork after a restart keeps a locked transaction as unconfirmed instead of removing it.

Asset locks need no change: a truncation never goes below the best ChainLock, and their statuses depend on finality only.

Known limitations

• A persister without the capability keeps the replaced branch until it rescans. A rejected truncation is retried from memory only, so a restart or a panic of the committing thread loses it. A truncation the store cannot apply is retried ahead of every later round of that wallet, so that wallet's writes fail until it applies; it is never skipped. The retry also follows a failure whose outcome the backend could not vouch for; over a partly applied round it removes only that round's content above the fork, which the next start rescans. No in-tree backend can leave a round partly applied, and this convergence is reasoned from the code, not covered by a test.
• A payment returned to pending and not yet accepted by the store is held in memory only, so a restart before the store accepts it leaves the stored payment confirmed. It is offered to the store again after a rejected round, but given up when the round that carries it is refused a second time with a failure that is not transient; the stored payment then stays confirmed. On the Swift store every failure is reported as permanent, so such a row gets one retry.
• When a payment reversal a round reports itself is the cause of a refusal that is not transient, it is carried once and makes the next round fail as well before it is given up.
• A point read of a stored transaction (get_core_tx_record) returns the record as written and does not consult the stored InstantSend locks. A transaction locked after it was stored, or kept by a truncation, therefore reads as a mempool transaction there, while the loaded wallet holds it as InstantSend-locked; sent-payment history built from that read can show it as pending until it is mined. Coins and balances are not affected. This predates the PR for transactions locked after they were stored.
• If the identity that owns such a payment is removed and added again before the retry, the row is written into the new identity's payment history, where it shows as a pending sent payment.
• On the Swift store the pending-input lookup used when a truncation hands a coin back does not report a failed read. If that read fails while a swept-input tombstone exists for the coin, the tombstone survives and can later mark the restored coin spent in the store; the wallet itself still holds the coin. The same lookup serves the sweep path and predates the PR.
• The Swift host's changeset-end callback reports every failed round as a failed rollback, so all of them are classified as fatal and a Swift host never receives a transient verdict for a round in which a callback failed. This predates the PR; separating the cases needs new FFI return codes.
• A transaction row is shared by all wallets of a Swift or Kotlin store, so between two wallets' rounds its block fields can show the branch the other wallet last wrote; coins and balances are per wallet and are not affected.
• Under FlushMode::Manual (not the default; no in-tree host sets it) a fatal flush drops the buffered round. The truncation is kept and applied with the next write, but a sent payment the fork returned to pending is not, so its stored status can stay confirmed.
• The sent-payment reconcile pass confirms from the stored transaction record. If it reads a pre-fork record just before the truncation is stored and writes its confirmation just after, the payment the fork returned to pending is stored and shown as confirmed again; unless the transaction is mined again, nothing corrects it. After a rejected truncation round the same pass can instead leave the payment confirmed in memory and pending in the store until the next start.
• On the Kotlin store a coin spent by an InstantSend-locked transaction that is not in a block is not marked spent, and unconfirmed sends are not replayed at load. When the mining of such a transaction and the truncation of its block reach the store in one round, the coin reloads as unspent after a restart until the transaction is mined again and scanned. The Swift store replays unconfirmed sends at load and is not affected.
• BalanceUpdateHandler takes the post-truncation balance from the SPV event monitor (rs-platform-wallet/src/wallet/core/balance_handler.rs:76-78), which does not receive ChainTruncated during fork recovery, so the lock-free balance read by the UI is not refreshed until the next wallet event. This predates the PR.
• A send that selects a coin handed back by a truncation fails with InputMidBroadcast until the undone spend is seen again, at the latest until the wallet restarts.
• FFI hosts (SwiftData, Room): a fork that happens after a restart is not undone, because the FFI restore path does not replay ordinary transaction records, so key-wallet has nothing above the fork to rewind. The stored state is corrected by the next rescan. fix(platform-wallet)!: replay recorded transaction history on load for every persister #5220 adds the replay of recorded history on the FFI load path for SwiftData; the Kotlin host does not supply history yet. Hosts also store no InstantSend lock bytes (see the known limitations of fix(platform-wallet)!: replay recorded transaction history on load for every persister #5220); a kept transaction is stored in the InstantSend context without them.
• When the detection of an unconfirmed payment and its removal by a conflicting confirmed spend reach the store in one round, SQLite can keep the coin as unspent although the running wallet has it spent. This predates the PR and does not involve a fork; it is marked TODO(sweep-of-unstored-loser).
• After a conflict was settled and the winner's block is then dropped, key-wallet does not hand the coin back (upstream behaviour, related to dash-spv: build a reorg/disconnect pipeline so key-wallet can retract non-chainlocked observed-spent entries rust-dashcore#854); the store follows the wallet.

Out of scope

• Sent-payment status has three writers — the adapter's verdicts, the reversals it carries after a rejected round, and the reconcile pass — and each decides on its own. They need one shared rule for freshness (a writer does not act on evidence older than the last truncation that is not yet stored) and for identity lifecycle (a row belongs to the identity it was decided for, and is dropped when that identity is removed, also if it is added again).
• The point read of a stored transaction (get_core_tx_record) should resolve the stored InstantSend locks the way the full load does, so every reader sees a locked transaction as locked, whether it was locked after it was stored or kept by a truncation.

How Has This Been Tested?

• 304 targeted all-feature tests passed across wallet, storage, wallet FFI and unified JNI, including the 48-case merged-versus-stepwise persistence property test. Clippy passed for the same four packages with --all-targets --all-features --locked -- --no-deps -D warnings; formatting and whitespace checks passed.
• The truncation checkpoint regression failed before its fix and passes afterward. The checkpoint is bounded by the observed live height and fork height, remains subject to the normal epoch checks, and cannot override a later contact-history rewind.
• tests/sqlite_chain_truncation.rs drives a real WalletManager through the adapter into SqlitePersister, reopens the store and compares the loaded wallet with the one that never restarted, with one round per step and with one merged round: a spent coin restored when its spender is not mined again, a transaction stored once when mined again, heights lowered to the fork, an InstantSend-locked transaction kept unconfirmed (also when the fork follows a restart), and a store left untouched when a truncation cannot be applied.
• A property test stores generated sequences of blocks, receipts, spends, syncs and forks both ways and requires the same stored wallet. It does not generate conflicting spends.
• The new tests were shown to fail without the change; disabling the merge reduction fails every merged-round variant.
• Tests also cover a kept transaction projected to FFI hosts as InstantSend-locked, a truncation kept across a dropped buffer and applied once, a payment returned to pending offered again after a rejected round and given up once the store refuses it for good, a restored coin fenced by a truncation of a real manager with no event delivered, and, per host, two wallets sharing a transaction (removed, kept, and the shared row's lifetime).
• Not run locally: the Swift and Kotlin code and their tests (ChainTruncationPersistTests.swift, PlatformWalletPersistenceHandlerTest.kt). No Swift or Android toolchain was available; they run in CI only.

Breaking Changes

None for consensus, persisted data or the C layout. Rust source only: CoreChangeSet gains the public field truncations, PersistenceCallbacksExtension gains on_persist_wallet_changeset_truncations_fn, and PersistenceExtensionCallbacks gains wallet_changeset_truncations. Code that builds any of them with a full struct literal must set the new field; ..Default::default() keeps the previous behaviour for CoreChangeSet and PersistenceExtensionCallbacks, while PersistenceCallbacksExtension has no Default and takes None. For C and Swift/Kotlin hosts the slot is trailing and size-negotiated, and the capability bit is additive.

Checklist

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

_{🤖 Co-authored by Claudius the Magnificent AI Agent}

🤖 Generated with Claude Code

https://claude.ai/code/session_01PSi8cX229kW2fmLxKW6ir4

[coderabbitai]

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

• Configuration used: Repository: dashpay/platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: ec296c28-3f3f-48f9-a5a3-db338f25d4ee

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[thepastaclaw]

⛔ Final review complete — 7 blocking finding(s) (commit cfa14d0) · triage: critical

[thepastaclaw]

Final validation — Phase 1 + Phase 2

Static verification at head 3a1aa90 confirms three in-scope persistence defects and one targeted test-coverage gap. These affect client-wallet persistence rather than consensus and are classified as suggestions under the supplied severity policy. No local builds or tests were run; the supplied CI snapshot reports successful Rust workspace, Swift, and Kotlin checks, with the dedicated Rust wallet job skipped.

🟡 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: ffi-engineer); 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 persistent wallet state and coin restoration/spend fencing across Rust, SQLite, JNI, Kotlin, Swift, and FFI, directly affecting funds movement and coin selection in functions such as apply_truncation and spend-observer handling.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 14% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
• Fresh 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 — ffi-engineer (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-platform-wallet/src/changeset/changeset.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/changeset.rs:731-735: Preserve InstantSend state when projecting a merged truncation to FFI
  A merged block-observation/truncation round demotes retained transaction records and account slices to Mempool here, while separately preserving lock evidence and marking new_utxos InstantSend-locked. WalletChangeSetFFI::from_changeset does not project those fields: record_new_utxos_ffi derives its lock flag exclusively from the account record's context, and tx_record_to_ffi likewise emits context 0 for the demoted transaction. FFIPersister delivers the truncation callback first, then this ordinary changeset. Swift's upsertTransaction/upsertUtxo and Kotlin's transaction/TXO upserts therefore overwrite the truncation handler's context-1 and locked-output updates with context 0 and is_instantlocked=false. If the transaction was not previously stored, the truncation callback is a no-op and the ordinary callbacks create it unlocked. Separate rounds end with the truncation's locked state, so persistence now depends on batching and loses the retained transaction's finality classification. Preserve retained-lock evidence in the shared FFI projection without overriding a later block observation, and add a merged mining/truncation regression through FFIPersister. This does not require storing lock bytes and is distinct from the documented FFI restore limitation.
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/changeset.rs:327-335: Cover the merged sweep-plus-truncation safety guarantee at the store layer
  This newly documented exception guarantees that a merged sweep/truncation can withhold a coin but cannot hand it out twice when the winner and funding transaction were both dropped, only the funding transaction is re-mined, and no surviving spender exists. should_release_a_restored_coin_from_a_sweep_recorded_before_the_truncation checks the rewritten release list, not the persisted result. The SQLite truncation suite's conflict scenario runs only per-step and explicitly defers merged coverage. Add a focused store-level test applying this merged shape through SQLite apply, then reload and assert the documented withheld end state. This pins the safety direction where sweep ordering and surviving-claim vetoes actually decide it, without requiring exact merged/per-step equality for the acknowledged exception.

In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt:1307-1309: Prevent another wallet's truncation from deleting already re-mined outputs
  These DAO deletions are global by txid, but core_bridge folds events into a BTreeMap<WalletId, WalletBatch> and commits each wallet's entire round separately. For a transaction T paying distinct outputs to wallets A and B, buffer its truncation and replacement-branch re-mining in both wallets' rounds. A's round deletes T and recreates A's valid output. B's round then deletes T and every output again, including A's newly recreated output, before recreating only B's account slices. Both rounds can succeed and advance their watermarks while A's coin is missing. Swift has the same ownership mismatch: vacateTransaction vacates every shared row.outputs entry and clears all involvedAccounts, while deleteVacatedRows deletes outputs not re-emitted by the current wallet. A txid-only removal is not idempotent across another wallet's intervening replacement writes. Make invalidation distinguish old-branch state from already-applied replacement state, or coordinate shared invalidations before replacement writes; wallet-scoped cleanup must also avoid cascading deletion of other-wallet rows. Add a two-wallet shared-transaction truncation/re-mining backlog regression for both hosts.

In `packages/rs-platform-wallet/src/changeset/core_bridge.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/core_bridge.rs:893-898: Preserve retry ownership when a backend later drops its buffered truncation
  This branch avoids duplicating a backend-held truncation, but also stops tracking it before durability is established. SqlitePersister restores the complete buffered changeset after SQLITE_BUSY, and its default store_transient_is_reissuable=false makes kept_by_backend true here. A later non-truncation store merges into that buffer. If the resulting flush fails fatally—for example, an unrelated incoming transaction update encounters a corrupt stored record during strict decoding—handle_flush_error drops the entire merged buffer, including the earlier truncation. On that call offered_truncations contains only the incoming round's truncations and is empty, so the adapter saves nothing for retry. Later successful writes leave the old branch's removed records in storage; freezing the existing watermark does not recreate the lost one-shot truncation or lower the old height to the fork. Retain explicit responsibility for backend-held truncations until their successful application is known, or have the backend preserve/return them when discarding a failed buffer. Do not blindly reissue while the backend still holds a copy. Add a SQLite regression covering transient retention, subsequent fatal buffer disposal, and successful recovery.

[thepastaclaw]

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

The truncation persistence implementation addresses the four prior findings and correctly propagates truncations through the merge, storage, FFI, Kotlin, and Swift layers. Two in-scope retry-state issues remain: a rejected truncation round can lose its DashPay payment reversal, and unknown persistence outcomes are treated as permission to replay only the truncation. Static review only; CI still has Kotlin and policy checks pending.

🟡 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: ffi-engineer); 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: muse-spark-1.3-contributor (agent: phase1-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: ffi-engineer); 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 persistent wallet storage and cross-language persistence boundaries that directly changes restoration and availability of coins after chain reorganizations, making funds movement and stored wallet state critical surfaces; specifically the SQLite truncation logic in packages/rs-platform-wallet-storage/src/sqlite/persister.rs and the corresponding Kotlin/Swift/FF
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 14% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (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 — ffi-engineer (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-platform-wallet/src/changeset/core_bridge.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/core_bridge.rs:893-898: Preserve payment reversals when retrying a rejected truncation
  When a ChainTruncated event changes a stored Confirmed sent payment to Pending, sent_payment_verdicts mutates the in-memory entry and places the reversal only in the current WalletBatch's payment overlay. If store() rejects the round, this code retains only the CoreChangeSet truncations in unapplied_truncations and drops the overlay. A later round therefore retries the truncation without the payment reversal: the removed transaction and spent coin are corrected, but the persisted payment can remain Confirmed after restart. Retain the payment overlay together with the owed truncation, or otherwise rederive and persist the reversal when the truncation is retried.
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/core_bridge.rs:893-898: Do not replay truncations after an unknown persistence outcome
  The branch treats every non-transient store error as permission to replay the truncation. That is not equivalent to knowing that the failed round was unapplied: FFIPersister reports a failed rollback from its changeset-end callback as Fatal because the round's disposition is unknown. If a failed round contained a truncation followed by replacement-branch records and some writes were applied before rollback failed, replaying only the truncation in a later round can remove those replacement records without recreating them. The persistence contract should return a disposition that distinguishes known-unapplied, backend-retained, and unknown outcomes; the adapter should replay only a known-unapplied truncation and require recovery for an unknown outcome.

[thepastaclaw]

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

Verified the complete changeset and reviewer claims against head 3d6aeca. Four in-scope correctness suggestions remain concerning spend-fence timing, buffered payment recovery, reconciliation ordering, and Android retained-spend restoration; five prior findings are fixed and the persistence-disposition distinction remains intentionally deferred. This was a static review: the supplied CI snapshot shows Kotlin checks passing and Rust workspace tests pending; Swift schema validation passed but does not establish behavioral-test success.

🟡 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: ffi-engineer); 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: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 13: 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 wallet persistence and truncation handling that directly changes stored coin and transaction state during chain reorganizations in rs-platform-wallet-storage/src/sqlite/persister.rs and cross-language persistence handlers, affecting funds movement and recovery of spendable coins.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 14% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (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 — 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 — ffi-engineer (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-platform-wallet/src/wallet/core/spend_observer.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/spend_observer.rs:125-129: Re-arm restored-input fences before releasing the wallet mutation lock
  The restored coin becomes selectable before this handler necessarily installs its fence. PlatformWalletInfo::truncate_above delegates directly to core_wallet.truncate_above, while the pinned dash-spv dependency delivers WalletEvent callbacks through a separately spawned broadcast monitor, not synchronously inside the wallet mutation. After the wallet-manager write guard is released, a concurrent transaction build can select the restored input and pass in_broadcast_conflict before the monitor reaches ChainTruncated. A lagged monitor can also exit without delivering the event. Install the fence at the WalletInfoInterface::truncate_above mutation boundary, before exposing restored inputs, and coordinate spend observations so queued pre-fork events cannot retire the newly installed fence. Add a real-manager rewind regression that withholds event delivery and immediately attempts to build using the restored coin; the current test invokes the event handler directly and misses this interval.

In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/persister.rs:1194-1201: Retain payment reversals with backend-owned truncations
  Fatal flush recovery retains core.truncations but discards the corresponding Confirmed-to-Pending payment overlay. In FlushMode::Manual, store returns Ok after buffering both, so the adapter no longer retains the reversal. If a subsequent flush or commit_writes fails fatally, this replacement PendingWrites contains only the truncations. After the error is repaired, recovery can delete the old-branch transaction and restore its inputs while leaving the authoritative identity blob's payment Confirmed. The fork event does not repeat, and reconcile_sent_payments only processes Pending entries, so reloading does not repair that verdict. Preserve the final reversal overlay with the retained truncations, respecting newer verdicts and identity lifecycle boundaries. Add a buffered acceptance → fatal flush → successful recovery → reload regression that verifies the payment status as well as core state.

In `packages/rs-platform-wallet/src/changeset/core_bridge.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/core_bridge.rs:744-748: Coordinate retried payment reversals with confirmation reconciliation
  The newer-wins merge only sees verdicts produced by this adapter, although reconcile_sent_payments writes the same payment independently. After a truncation changes the live payment to Pending and its store round fails, reconciliation can read SQLite's still-committed pre-fork confirmed transaction and successfully write the live payment back to Confirmed. The reconciliation path has no outstanding-truncation or sync-fault guard. If a later adapter round re-mines that transaction, sent_payment_verdicts emits no overlay because Confirmed + Final is explicitly a no-op. This merge then replays the retained Pending row over the newer reconciliation write, leaving live state Confirmed and persisted state Pending. Later reconciliation and finality events skip the already-Confirmed live entry. Coordinate reconciliation with outstanding truncations/reversals or apply a shared freshness check across payment writers. Exercise the actual reconciliation and verdict paths in a regression; supplying a newer Confirmed overlay directly does not cover this interleaving.

In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt:1324-1325: Persist retained transaction input debits in merged truncation rounds
  The kept-transaction updates preserve spent inputs only when an earlier round already marked them spent. Start with an unspent funding coin F persisted, then merge the mining of an InstantSend-locked transaction T spending F with the truncation of T's block before T reaches storage. CoreChangeSet::rewind_to demotes T to Mempool, and the FFI projection correctly sends it as context 1. These truncation updates initially affect no rows. The subsequent transaction and spent-outpoint callbacks both call linkSpender, which sets isSpent only for context >= CONTEXT_IN_BLOCK, so F remains unspent while T's locked change is stored. buildUtxoRestoreData also excludes linked inputs only for context >= CONTEXT_IN_BLOCK, and JNI supplies no unconfirmed outgoing replay records. Reload therefore restores both F and T's change, unlike separate mining/truncation rounds. Carry the round's retained-txid evidence into input debit processing after the records exist, or transport equivalent debit evidence. Add a merged retained-spend regression beginning with only F persisted and assert that reload excludes F.

Out-of-scope follow-up suggestions (1)

These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.

• Distinguish rollback success from unknown round disposition in the persistence ABI — Swift's endChangeset returns false after a successful rollback, and its C shim converts that to the same nonzero result FFIPersister interprets as rollback failure. The in-tree backends apply rounds atomically, so this does not establish the claimed partial-write defect here, but the ABI cannot express the distinction needed by an independently implemented host with a genuinely unknown outcome. The PR explicitly defers that coordinated contract change.
  • Follow-up: Track a separate persistence-disposition contract change covering Rust error semantics, FFI return codes, mobile callbacks, and recovery fault-injection tests.

[thepastaclaw]

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

Verified the complete base-to-head changes at 22d050f and revalidated all ten prior findings. Six are fixed; four non-consensus correctness suggestions remain, covering Android retained-spend restoration, payment-reconciliation ordering, reversal retry lifecycle, and Swift truncation error propagation. This was static verification only: the supplied CI snapshot reports Kotlin build/tests passing, Rust workspace tests pending, the dedicated Rust wallet job skipped, and no Swift runtime-test result.

🟡 4 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: ffi-engineer); 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: ffi-engineer); 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 wallet persistence and coin state across Rust, SQLite, Kotlin, JNI, and Swift, directly affecting which coins are restored or considered spendable after chain reorganizations.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 14% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (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 — ffi-engineer (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-platform-wallet/src/changeset/core_bridge.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/core_bridge.rs:745-748: Cancel retained payment reversals when their owning identity is removed
  unstored_reversals is adapter-private and is merged into the next round without checking identity lifecycle. After a truncation round is rejected, IdentityManager::remove_identity can independently persist a successful terminal removal while this cache still owns the deleted identity's Pending payment. The next adapter round carries that obsolete overlay into SQLite: dashpay::apply attempts an insert whose identity foreign key rejects the write, rolling back the entire round, including otherwise valid core changes. The failure path at lines 922–925 retains the same reversal again, so subsequent adapter writes continue failing after the original storage problem is repaired. Re-adding the same identity before retry instead allows the obsolete payment to be patched into the newly blank identity history. Invalidate retained payment rows when their owning identity lifecycle ends, independently of the still-owed core truncation; checking only whether the identity currently exists does not cover removal followed by re-addition. Add rejected reversal → successful identity removal → successful core persistence and removal/re-addition regressions.
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/core_bridge.rs:1554-1555: Coordinate retried payment reversals with confirmation reconciliation
  (existing thread: https://github.com/dashpay/platform/pull/5384#discussion_r4251865308)
  The restart argument holds when the retried Pending overlay is the final write, but the independent reconciliation writer also permits the opposite ordering without any persistence failure. sent_payment_verdicts changes the live payment to Pending and releases the manager lock before committing. reconcile_sent_payments can then read the still-stored pre-fork InBlock record at payments.rs:758; the adapter successfully commits the truncation and Pending overlay; reconciliation subsequently acquires the manager write lock and calls confirm_sent_payment_by_txid using its earlier evidence. That function checks only that the payment is still Pending and independently persists Confirmed. Both memory and disk now report the removed transaction as confirmed, and restart does not heal this ordering because reconciliation skips Confirmed entries and the accepted truncation has no queued reversal. After a rejected round, the existing retry-last ordering can instead leave live state Confirmed and storage Pending. Coordinate confirmation with shared rewind/evidence-generation state and validate freshness at the payment mutation boundary; merely rereading storage under the manager lock is insufficient while the adapter's rewind is not yet durable. Add an interleaving regression using the real reconciliation path, including a successful truncation commit between evidence acquisition and confirmation.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:1691-1693: Propagate pending-input lookup failures through the truncation callback
  restoreTruncatedCoin is throwing, but this required tombstone lookup cannot propagate a read failure. pendingInputRows uses try? backgroundContext.fetch in both fetch paths, substitutes an empty result, and, during a round, registers that result as successfully fetched. If the lookup fails while a detached swept tombstone exists, the deletion is skipped; a later successful save nevertheless makes persistWalletChangesetTruncations acknowledge success to Rust. The adapter then relinquishes the one-shot truncation, although a subsequent drainPendingInputs can consume the surviving tombstone and mark the restored coin spent again. The helper's best-effort behavior predates this PR, but its use in the new authoritative truncation acknowledgment introduces this failure path and contradicts the callback's stated fail-the-round lookup contract. Use a throwing lookup here, retaining the saved/staged-row merge behavior without caching failed reads, so the surrounding catch rejects and rolls back the round. Add a regression with a detached tombstone and a failed pending-input fetch.

In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt:1322-1323: Persist retained transaction input debits in merged truncation rounds
  (existing thread: https://github.com/dashpay/platform/pull/5384#discussion_r4251865317)
  These updates preserve an existing debit but cannot establish one when the retained spender has not reached storage. With only funding coin F persisted, merge the mining of InstantSend-locked spender T and the truncation retaining T. The truncation callback runs before T exists; the following transaction and spent-outpoint callbacks receive context 1 and call linkSpender, which sets isSpent only for context >= CONTEXT_IN_BLOCK. F therefore remains unspent alongside T's change. buildUtxoRestoreData also excludes linked inputs only for an in-block spender, and JNI explicitly supplies null/0 for unconfirmed_outgoing_tx_records, so restarting restores the consumed input without replaying its reservation. Separate mining/truncation rounds keep F debited. The ordinary unconfirmed-spend restore limitation explains the implementation, but does not make this new batching-dependent result equivalent or safe-sided. Carry retained-spender evidence through the subsequent input callbacks and establish the debit before committing. Add a merged retained-spend reload regression starting with only F stored; the existing kept-transaction test begins after the mining debit is already durable. This finding is narrowed to Kotlin because Swift already supplies unconfirmed outgoing replay records.

Out-of-scope follow-up suggestions (3)

These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.

• Complete ordinary confirmed-history restoration for mobile hosts — The existing FFI load path restores UTXOs and selected unconfirmed outgoing transactions, but not ordinary confirmed transaction history. A fork detected after restart can therefore lack the records needed to populate key-wallet's truncation lists. This is concrete restore-path debt already present at the base, distinct from the new round-local persistence defects.
  • Follow-up: Continue the Swift history-restoration work tracked in #5220 and track equivalent Kotlin/JNI restoration separately, with restart-then-reorg coverage for each host.
• Separate persistence failure classification from round disposition — Swift endChangeset(success: false) performs rollback but returns false, which its C shim converts to a nonzero result. FFIPersister interprets a nonzero end result on an already-failed round as an unknown rollback outcome. These unchanged return semantics prevent Rust from distinguishing completed rollback from genuinely unknown disposition.
  • Follow-up: Track a separate compatible FFI disposition-contract change covering completed rollback, failed commit, and unknown outcome across Swift, Kotlin, and Rust.
• Make pending-spend protection durable across process restarts — Outside this PR: generation.rs documents the same process-lifetime fence limitation at both base and head. Persisting dispatched transactions or introducing a durable pending-spend ledger is a separate broadcast/recovery change, not necessary to adjudicate the new truncation implementation.
  • Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.

[thepastaclaw]

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

The complete diff at e9b2aae preserves consensus boundaries and fixes six previously reported issues, including wallet-scoped cleanup, truncation retry ownership, FFI finality projection, and mutation-boundary fencing. Six non-consensus correctness suggestions remain around payment reversal ownership, retained-spend restoration, required lookup failures, and transaction point-read finality. Validation was static only: current CI reports Kotlin build/tests passing, but the visible checks do not independently establish Rust or Swift validation.

🟡 6 suggestion(s)

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

• Triage: critical by gpt-6.1-sol (effort low) — The intricate cross-backend reorg implementation directly changes spend eligibility in packages/rs-platform-wallet/src/wallet/core/generation.rs::fence_restored and restores or removes spendable coins through changeset rewinding and persistence.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 14% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (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 — ffi-engineer (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 — ffi-engineer (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-platform-wallet/src/changeset/core_bridge.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/core_bridge.rs:926-929: Retain fresh reversals when abandoning previously carried rows
  carries_reversals records whether any reversal was cached for this wallet before the current overlay was merged, whereas offered_reversals contains both carried and fresh rows. Retain reversal A after a rejected round, remove A's identity, and let a later fork newly reverse payment B belonging to a surviving identity. A's missing-owner constraint rejects the combined round, and this condition discards B along with A even though B has received only its first refusal. A subsequent successful round applies the owed truncations without B's reversal, leaving its stored payment Confirmed. B's live Pending status prevents Pending + Unmined from generating the row again. The documented abandonment policy for a refused carried row does not justify abandoning fresh rows in the same wallet round. Track carried keys separately and preserve newly generated reversals for their own retry; extend the existing three-round test with distinct carried and fresh payment keys.
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/core_bridge.rs:1731-1735: Coordinate retried payment reversals with confirmation reconciliation
  (existing thread: https://github.com/dashpay/platform/pull/5384#discussion_r4251865308)
  The reversal becomes visible in memory before its storage round commits. reconcile_sent_payments can snapshot that Pending entry and read the still-stored pre-fork confirmed record at payments.rs:758. After the adapter successfully commits the truncation and Pending overlay, reconciliation can acquire its confirmation write lock and persist Confirmed again. confirm_sent_payment_by_txid checks only Pending/Sent status, not whether the record read before the fork remains valid. Both memory and storage then retain the stale confirmation across restart because reconciliation skips Confirmed entries. This needs no persistence failure, so an outstanding-retry guard alone is insufficient. Although the displayed symptom existed before this PR, the new reversal and its independent confirmation writer cannot reliably deliver the promised post-fork history without shared evidence freshness. Validate confirmation evidence across the read-to-write interval and outstanding truncation persistence, and test reconciliation reading before a successful truncation commit and confirming afterward.
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/core_bridge.rs:744-749: Cancel retained payment reversals when their owning identity is removed
  (existing thread: https://github.com/dashpay/platform/pull/5384#discussion_r4252358882)
  The retained overlay is keyed by identity ID without an identity-lifetime discriminator. After its original round is rejected, remove_identity can independently persist terminal removal, and add_identity can create the same identifier with blank payment history before this retry. The owner now exists, so SQLite accepts the obsolete Pending row and patch_payments_into_entry_blobs inserts it into the new identity's authoritative blob. No refusal reaches the new abandonment branch. The newly added in-memory identity remains blank, while reload resurrects the old payment, contrary to remove_identity's documented terminal-removal contract. Bounding missing-owner retries fixes the indefinite failure loop but not this successful-write case. Associate retained rows with an identity incarnation or invalidate them through the shared lifecycle owner, independently of the owed core truncation. Add removal/re-addition-before-retry coverage that checks both live and reloaded payment history.

In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:491-499: Resolve retained InstantSend finality for every storage reader
  Truncation rewrites a retained transaction's authoritative blob to Mempool while leaving its valid InstantSend lock in core_instant_locks. Full wallet loading upgrades that context in restore_recorded_transactions, but SqlitePersister::get_core_tx_record delegates to get_tx_record, which returns the raw blob without resolving the lock. A transaction previously returned as block-final is therefore returned as Pending after truncation even though its retained lock remains final. Sent-payment history reconstruction uses this point read to create payment statuses, and Pending-payment reconciliation uses it to decide confirmation, so those consumers can disagree with the fully restored wallet. Existing Confirmed entries are not directly downgraded by reconciliation; the defect affects reconstruction and entries still awaiting confirmation. Share validated lock/context resolution between point reads and full replay while preserving the strict txid/height checks, and add a post-truncation get_core_tx_record regression requiring InstantSend context.

In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt:1319-1323: Persist retained transaction input debits in merged truncation rounds
  (existing thread: https://github.com/dashpay/platform/pull/5384#discussion_r4251865317)
  With only funding coin F persisted, merge the mining of InstantSend-locked spender T with the truncation retaining T. These updates run before T exists in Room. The subsequent ordinary callbacks correctly insert T at context 1, but linkSpender sets isSpent only for block-context evidence or an existing spent/stamped flag. F therefore remains unspent while T's change is stored. buildUtxoRestoreData also excludes linked inputs only for block-context spenders, and JNI supplies null/0 for unconfirmed outgoing replay records. Reload returns both F and T's change; separate mining/truncation rounds exclude F because mining previously set its spent flag. The ordinary unconfirmed-send limitation predates this PR, but the new retained-transaction callback exposes a batching-dependent violation of its input-debit contract. Preserve wallet-scoped retained-spender evidence through the later input callbacks, or supply equivalent replay evidence, and add a merged-round reload regression starting with only F persisted.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:1691-1693: Propagate pending-input lookup failures through the truncation callback
  (existing thread: https://github.com/dashpay/platform/pull/5384#discussion_r4252358886)
  This required lookup uses pendingInputRows, whose two fetch paths suppress failures with try?; the in-round path also caches the failed read as successfully fetched. If a detached swept-input tombstone exists for the restored outpoint, a failed lookup leaves it undeleted while the truncation callback returns success and the round can save. Rust then relinquishes the one-shot truncation. A later drainPendingInputs can consume the surviving tombstone and stamp the restored coin spent again. This conservatively withholds a legitimate coin rather than creating funds, but still makes successful truncation acknowledgment unreliable. The helper's older sweep callers have the same error-handling debt; that does not make the new caller's required read optional. Introduce a throwing lookup core that preserves saved/staged-row merging and caches only successful reads, use it here, and retain a compatibility wrapper for older callers if their migration remains separate. Add a failed-fetch regression asserting round rejection.

Out-of-scope follow-up suggestions (1)

These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.

• Complete ordinary transaction-history restoration for mobile hosts — The existing FFI restore path does not reconstruct ordinary confirmed transaction history. A fork first encountered after reopening can therefore lack the records needed to enumerate removals, even though the new callback correctly applies a truncation delivered during the running session. This omission precedes the PR and is explicitly separated into the history-restoration work associated with #5220.
  • Follow-up: Track restart-then-reorg coverage with the separate mobile history-restoration work, including the Kotlin/JNI restore inputs.

[thepastaclaw]

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

Verified the complete diff at a20fdff, including the fresh-reversal fix and all 14 prior findings. Two host-persistence correctness gaps and incomplete Rust breaking-change metadata remain as suggestions; no consensus-blocking issue was confirmed. Validation was static only: the supplied CI snapshot shows Kotlin validation in progress and changed-package determination queued, without completed Rust or Swift validation results.

🟡 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: ffi-engineer); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); 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: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: critical by gpt-6.1-sol (effort low) — The intricate cross-backend reorg handling changes coin availability and spend fencing, notably in core_state.rs::apply_truncation and WalletGeneration::fence_restored, directly affecting which restored funds can be spent.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 14% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (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 — ffi-engineer (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 `CHANGELOG.md`:
- [SUGGESTION] CHANGELOG.md:32-34: Mark the public Rust struct additions as breaking changes
  CoreChangeSet gains a public field, and PersistenceCallbacksExtension and PersistenceExtensionCallbacks also gain fields. Downstream exhaustive Rust struct literals consequently stop compiling; serde(default), Default, and size-negotiated C layout do not preserve source compatibility for those callers. The PR description discloses the CoreChangeSet migration, but the title still lacks the required ! and this changelog entry does not identify the Rust API break. CLAUDE.md and the PR template explicitly require breaking-change title metadata. Mark the title accordingly and add a durable migration note covering the new fields and the ..Default::default() alternative where available. No consensus version or database migration is required.

In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt:1319-1323: Persist retained transaction input debits in merged truncation rounds
  (existing thread: https://github.com/dashpay/platform/pull/5384#discussion_r4251865317)
  These updates preserve a previously stored debit but cannot establish it when a retained transaction first reaches Room in this round. With only funding coin F persisted, merge the mining of InstantSend-locked spender T with truncation of its block. The truncation executes before T exists, and the following FFI callbacks correctly deliver T at context 1. Both transaction-input processing and the spent-outpoint callback use linkSpender, which leaves F unspent unless block context or an existing spend/stamp supplies the debit. buildUtxoRestoreData excludes linked inputs only for in-block spenders, and JNI supplies no unconfirmed outgoing replay records, so restart restores F alongside T's change. Separate mining/truncation rounds exclude F. Swift's outgoing replay does repair its corresponding state, but Android has no equivalent; restored-input fences also cannot repair this because T's retained inputs are not restored_outpoints. Preserve the round's retained-txid evidence through transaction/input processing and apply wallet-scoped input debits before commit. Add a merged-round reload regression beginning with only F persisted.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:1691-1693: Propagate pending-input lookup failures through the truncation callback
  (existing thread: https://github.com/dashpay/platform/pull/5384#discussion_r4252358886)
  This required cleanup can fail while the new truncation callback still acknowledges success. pendingInputRows suppresses fetch errors with try?, substitutes an empty result, and registers that result as successfully fetched for the round. A detached swept-input tombstone therefore survives a failed lookup, while restoreTruncatedCoin clears the coin's spent flag and the round can save successfully. Rust then relinquishes the accepted truncation. A subsequent successful drainPendingInputs stamps the restored coin spent again from the surviving tombstone. The helper and sweep callers predate this PR, and the consequence conservatively withholds a coin, but the new restoration acknowledgment still reports an incomplete correction as applied. The cached failure also prevents another lookup in the same round from repairing the omission. Make the shared lookup throwing for both sweep and truncation callers, preserve saved/staged-row merging, and cache only successful reads. Route failures through the existing callback failure/rollback path and cover a detached tombstone with a failed lookup.

Out-of-scope follow-up suggestions (2)

These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.

• Coordinate sent-payment evidence freshness and identity lifecycle — The independently verified residual cases are concrete: reconciliation can read pre-fork transaction evidence and persist Confirmed after a successful truncation reversal, while a retained reversal can enter a removed-and-recreated identity's blank history. Permanent-refusal abandonment prevents the missing-owner write wedge but does not prevent either successful stale write. The PR explicitly excludes the shared freshness and identity-lifecycle policy, so these remain follow-up work rather than canonical in-scope findings.
  • Follow-up: Track a shared payment mutation policy with deterministic read-before-truncation/write-after-truncation and identity removal/re-addition regressions.
• Coordinate retried payment reversals with confirmation reconciliation — INTENTIONAL_EXCLUSION: The race is real, but the PR's Out of scope section explicitly excludes shared sent-payment freshness coordination. payments.rs reads transaction evidence before the confirmation write lock and subsequently checks only Pending status; the current head does not fix that residual. It is reconciled as INTENTIONALLY_DEFERRED and retained in the concrete payment-policy follow-up, not as an in-scope finding.
  • Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.

[thepastaclaw]

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

Verified the complete PR diff at 307a662 and revalidated all 15 prior findings. Eight findings are fixed; three non-consensus correctness suggestions remain in the Kotlin retained-spend path, Swift truncation acknowledgment, and payment reconciliation ordering. Validation was static only: the supplied CI snapshot reports Kotlin build/tests successful, Rust workspace tests in progress, and the Swift frozen-schema check queued.

🟡 3 suggestion(s)

3 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: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: normal by gpt-6.1-sol (effort low) — This is a large, intricate cross-language wallet-persistence change affecting stored chain state and coin availability, but it does not modify consensus, cryptography, funds movement, coin selection, or a storage migration.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 14% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• 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 high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); 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/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt:1319-1323: Persist retained transaction input debits in merged truncation rounds
  (existing thread: https://github.com/dashpay/platform/pull/5384#discussion_r4251865317)
  These updates preserve a retained spender's input debit only when an earlier round already stored it. Start with funding coin F stored unspent, then merge the mining of InstantSend-locked spender T with the truncation that retains T. The truncation executes before T exists in Room, and the subsequent FFI record arrives at context 1. Both transaction-input processing and the spent-outpoint callback call linkSpender, whose isSpent expression requires block context for an unstamped input. buildUtxoRestoreData likewise excludes linked inputs only for block context, and JNI supplies null/0 for unconfirmed outgoing replay records. Restart therefore restores F alongside T's locked change, whereas separate mining/truncation rounds leave F spent. The existing ordinary-unconfirmed restoration limitation explains the mechanism, but the new merged truncation path loses an observed mined debit and violates the retained-input contract. Preserve retained-spender evidence through subsequent input callbacks, or apply its debits after the rows materialize within the atomic round. Add a merged-round reload regression starting with only F persisted; general transaction-history restoration is not required for this repair.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:1674-1676: Propagate pending-input lookup failures through the truncation callback
  (existing thread: https://github.com/dashpay/platform/pull/5384#discussion_r4252358886)
  This required tombstone lookup cannot propagate failure to the truncation callback's catch. pendingInputRows uses try? for both fetch paths, substitutes an empty result, and registers the failed indexed lookup as fetched. If a detached swept-input tombstone exists and the read fails, restoreTruncatedCoin skips its deletion while the round can still save and acknowledge success to Rust. Rust then relinquishes the truncation, but a later successful drainPendingInputs can consume the surviving tombstone and mark the restored coin spent again. The helper's sweep behavior predates this PR, and the error withholds rather than invents a spendable coin; nevertheless, its new use here can acknowledge an unapplied restoration effect. Make this authoritative lookup throwing, retaining the saved/staged-row merge and caching only successful fetches. A checked path for truncation can be introduced without changing every existing sweep caller. Add failed-fetch coverage proving that the callback rejects the round and its mutations roll back.

In `packages/rs-platform-wallet/src/changeset/core_bridge.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/core_bridge.rs:745-750: Coordinate retried payment reversals with confirmation reconciliation
  (existing thread: https://github.com/dashpay/platform/pull/5384#discussion_r4251865308)
  The newer-wins overlay merge orders adapter verdicts, not the independent reconciliation writer. sent_payment_verdicts changes the live entry from Confirmed to Pending and releases the manager lock before persistence. reconcile_sent_payments can then read the still-committed pre-fork record at payments.rs:758, allow the adapter to successfully commit the truncation and Pending overlay, and finally call confirm_sent_payment_by_txid using that old evidence. Its write-side check only requires the entry to remain Pending, which the reversal itself ensures. Reconciliation consequently persists Confirmed after the transaction was removed; both memory and disk remain wrong after restart because subsequent passes skip Confirmed entries. This ordering needs no failed store round. Although the wider shared-writer redesign is explicitly deferred, the newly introduced reversible confirmation edge needs an evidence-freshness check coordinated with the truncation commit. Add a regression pausing reconciliation between its record read and confirmation while a successful truncation round commits.

Out-of-scope follow-up suggestions (4)

These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.

• Invalidate retained payment overlays across identity lifetimes — The bounded retry policy prevents an absent identity's reversal from indefinitely rejecting core persistence, but the retained overlay still has no identity-incarnation marker. Removing and re-adding the same identity before retry can therefore insert the old payment into its newly blank history. This remaining lifecycle case is explicitly excluded from the selected payment-writer work and is distinct from the fixed persistence wedge.
  • Follow-up: Track identity incarnation or lifecycle invalidation in a separate payment-owner change, with removal/re-addition coverage for retained overlays.
• Restore recorded history for mobile forks after restart — The mobile FFI restore surface does not replay ordinary confirmed transaction history, so key-wallet can lack the records needed to produce a meaningful truncation after restart. This restoration limitation predates the new transport and is explicitly separated into Swift history-restoration work in #5220 and an equivalent Kotlin/JNI follow-up.
  • Follow-up: Complete the separately scoped recorded-history restoration paths and test a restart followed by a fork on each host.
• Preserve retained-spend input debits when mining and truncation share a round — Deduplicated into prior finding 78ad35e85c2e. The mechanism is verified, but its canonical severity is suggestion under the project's policy for correctness defects outside consensus; the fresh lane's blocking classification is not retained.
  • Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
• Resolve retained InstantSend finality for every storage reader — Outside the selected scope: get_tx_record returns the raw blob, while restore_recorded_transactions resolves separately stored locks. The same divergence occurs for ordinary post-write lock arrival without a fork. The author explicitly separates the shared reader repair; it is not an additional truncation-specific request.
  • Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.

[thepastaclaw]

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

The truncation persistence design is largely correct and the previously reported retry, fencing, merge, and wallet-isolation issues are fixed. Four in-scope suggestions remain: the changelog omits the public Rust API break, Kotlin can restore a retained spend's input as unspent after a merged round, payment reconciliation can overwrite a successful truncation reversal with stale evidence, and stale InstantSend locks can survive a transaction being mined again. No consensus or security blocker was identified.

🟡 4 suggestion(s)

1 finding(s) not shown inline (the lines are not part of this PR's diff)

🟡 Suggestion: Clear stale InstantSend locks when a transaction is mined again after truncation
packages/rs-platform-wallet/src/changeset/changeset.rs:871-875

A detected InstantSend transaction is stored in instant_locks_for_non_final_records, while a later BlockProcessed changeset emits the transaction as InBlock without removing that lock. rewind_to intentionally retains the lock when a truncation keeps the transaction in unconfirmed_txids, and Merge then extends the retained map with the later block changeset. The sequence detect → mine → truncate while retaining the transaction → mine again can therefore leave the old lock in the merged changeset. SQLite updates the transaction to its mined context but does not delete the separate lock row; on restart, rehydration replays the stale lock and can mark confirmed outputs InstantSend-locked. Treat an InBlock or InChainLockedBlock observation as a tombstone for an earlier non-final lock for the same txid, and add a persistence/restart regression for this sequence.

_{source: gpt-6.1-sol (phase2-reviewer: rust-quality)}

3 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: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); 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: ffi-engineer); 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 wallet persistence that directly alters stored transaction and coin state during chain reorganizations, including funds-related restoration and deletion across Rust, SQLite, Kotlin, Swift, and FFI boundaries.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 14% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (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 — ffi-engineer (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-platform-wallet/src/changeset/changeset.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/changeset.rs:871-875: Clear stale InstantSend locks when a transaction is mined again after truncation
  A detected InstantSend transaction is stored in `instant_locks_for_non_final_records`, while a later `BlockProcessed` changeset emits the transaction as `InBlock` without removing that lock. `rewind_to` intentionally retains the lock when a truncation keeps the transaction in `unconfirmed_txids`, and `Merge` then extends the retained map with the later block changeset. The sequence detect → mine → truncate while retaining the transaction → mine again can therefore leave the old lock in the merged changeset. SQLite updates the transaction to its mined context but does not delete the separate lock row; on restart, rehydration replays the stale lock and can mark confirmed outputs InstantSend-locked. Treat an `InBlock` or `InChainLockedBlock` observation as a tombstone for an earlier non-final lock for the same txid, and add a persistence/restart regression for this sequence.

In `CHANGELOG.md`:
- [SUGGESTION] CHANGELOG.md:42-48: Mark the public Rust struct additions as breaking changes
  (existing thread: https://github.com/dashpay/platform/pull/5384#discussion_r4258098852)
  The PR title and description now disclose the breaking API change, but the durable changelog entry still presents chain-truncation persistence only as a normal platform-wallet change. `CoreChangeSet::truncations`, `PersistenceCallbacksExtension::on_persist_wallet_changeset_truncations_fn`, and `PersistenceExtensionCallbacks::wallet_changeset_truncations` are public Rust fields that break downstream exhaustive struct literals. Add a breaking-change note and the available `..Default::default()` migration guidance to the changelog entry.

In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt:1203-1214: Persist retained transaction input debits in merged truncation rounds
  (existing thread: https://github.com/dashpay/platform/pull/5384#discussion_r4251865317)
  When an unspent funding coin F is already persisted and the detection, mining, and truncation of an InstantSend-locked spender T arrive in one round, the truncation callback runs before T's transaction and spent-outpoint callbacks. The later callbacks link F to T with InstantSend context, but `linkSpender` sets `isSpent` only for context at or above `CONTEXT_IN_BLOCK`. Kotlin's restore builder applies the same block-only threshold, and the JNI load path supplies no unconfirmed outgoing transactions to replay. After restart, both F and T's change can therefore be restored, whereas separate persistence rounds leave F spent. Carry retained-spend evidence through the round and mark the corresponding inputs consumed, or complete the native unconfirmed-spend replay path; add a merged-round reload regression beginning with only F persisted.

In `packages/rs-platform-wallet/src/changeset/core_bridge.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/core_bridge.rs:1561-1566: Coordinate retried payment reversals with confirmation reconciliation
  (existing thread: https://github.com/dashpay/platform/pull/5384#discussion_r4251865308)
  The new `Confirmed` to `Pending` transition is applied to the in-memory payment before the persistence round commits. `reconcile_sent_payments` can concurrently read the still-persisted pre-fork transaction, then call `confirm_sent_payment_by_txid` after the truncation and its `Pending` overlay have committed. That mutation checks only that the entry is still a `Pending` sent payment, so it can write `Confirmed` again without a persistence failure or a newer chain observation. The payment then remains confirmed in memory and storage, and later reconciliation skips it. Apply confirmation evidence only when it is newer than the wallet's outstanding truncation generation, and cover the stale-read, truncation-commit, confirmation-write ordering.

[Claudius-Maginificent]

Re the suggestion that is not shown inline, "Clear stale InstantSend locks when a transaction is mined again after truncation" (rs-platform-wallet/src/changeset/changeset.rs:871-875): no change, the premise does not hold.

The row does stay, but it is not stale and the loaded wallet does not differ. An InstantSend lock is permanent for its transaction: key-wallet latches the coin's flag when the transaction is processed again in a block (key-wallet/src/managed_account/managed_core_funds_account.rs:474-486), so the wallet that never restarted also holds that coin as confirmed and InstantSend-locked. On load the stored lock marks the coin the same way (rs-platform-wallet-storage/src/sqlite/rehydrate.rs:419-421) and leaves a mined record's context alone (:490-494).

Checked with a test that runs detect → mine, with and without a fork that keeps the transaction and a second block, both merged into one round and one round per step: in all four cases the restored coin, balance and record height equal the in-memory ones, and one lock row is stored. The row outliving the confirmation is not specific to a merged round or to a fork; the SQLite backend has stored locks this way since #3968.

The row is also needed. A truncation keeps a transaction only if the wallet knows its lock (key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs:596-610), and after a restart the stored row is the only source of that. Deleting it when the transaction is mined would make a relaunched wallet drop, on a later fork, a transaction the running wallet keeps; should_keep_an_instant_locked_transaction_when_the_fork_follows_a_restart covers that case.

The other three suggestions of this round are carried forward and answered in their threads.

_{🤖 Co-authored by Claudius the Magnificent AI Agent}

[thepastaclaw]

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

The head's SQLite capability withdrawal is fail-closed, but cumulative verification at cfa14d0 confirms six unresolved reorg-persistence defects; eight prior findings are fixed and two are withdrawn. The Kotlin CI failure is also attributable to the cumulative PR: its changed capability-mask assertion expects bit 14 although chain truncation uses bit 15. Verification was static, with current CI checks and the failed job log inspected; no local builds or tests were run.

🔴 7 blocking

6 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: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: critical by gpt-6.1-sol (effort low) — This is a large, intricate change to wallet persistence and coin-state restoration across Rust, SQLite, FFI, Kotlin, and Swift, directly affecting funds availability and transaction history.
• 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 (lane 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 — ffi-engineer (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/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandlerTest.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandlerTest.kt:223: Use bit 15 in the Kotlin capability-mask expectation
  The cumulative PR changes this assertion from 0x0bbf to 0x4bbf, but the handler now adds CORE_CHAIN_TRUNCATION at bit 15, producing 0x8bbf. The current Kotlin SDK build + tests job fails this exact assertion, with 547 tests completed and one failure. Although neither Kotlin file changes in the final five-file increment, both the capability addition and this incorrect expected value are introduced by the PR relative to dae60456ec08feb6baa52f4155670988961b271b, so this is not unrelated validation debt. Update the expectation to the intended bit-15 mask and obtain a passing Kotlin CI run; do not move the capability to bit 14, which is reserved for history restoration.

In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt:1355-1356: Persist retained transaction input debits in merged truncation rounds
  (existing thread: https://github.com/dashpay/platform/pull/5384#discussion_r4251865317)
  Starting with only funding coin F persisted, merge the mining of InstantSend-locked spender T and truncation of its block before T reaches storage. These truncation updates initially affect no T rows, and the subsequent ordinary callbacks store T at InstantSend context 1. Both input channels use linkSpender, which marks F spent only at block context or when already stamped; buildUtxoRestoreData likewise excludes linked inputs only at block context. JNI supplies null/0 for unconfirmed outgoing replay records at persistence.rs:2957-2958. Restart therefore restores F alongside T's change, exposing an already-consumed input as selectable; separate mining and truncation rounds do not have this result. Swift's outgoing replay avoids this particular failure, and #5220 explicitly excludes Android history restoration. Supply the existing Rust-owned outgoing replay contract through JNI, or transport authoritative debit evidence from Rust, and cover merged-round reload without introducing another Kotlin spendability rule.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:1770-1772: Propagate pending-input lookup failures through the truncation callback
  (existing thread: https://github.com/dashpay/platform/pull/5384#discussion_r4252358886)
  restoreTruncatedCoin throws, but pendingInputRows suppresses both fetch failures with try? at lines 2724 and 2733; inside a round it also registers the failed lookup as a fetched empty set. If a detached swept-input tombstone exists, the callback can leave it behind while acknowledging the truncation successfully. A later drainPendingInputs applies that surviving tombstone and marks the restored coin spent again, after Rust has relinquished the correction. The helper predating this PR and also serving sweeps supports repairing the shared helper, not acknowledging an incomplete new truncation operation. Make required pending-input lookups throwing, retain their saved/staged-row merge semantics, and propagate errors through the affected callbacks so the round rolls back instead of caching a failed read. Add a detached-tombstone regression with a failed lookup.

In `packages/rs-platform-wallet/src/changeset/core_bridge.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/changeset/core_bridge.rs:1194-1199: Coordinate retried payment reversals with confirmation reconciliation
  (existing thread: https://github.com/dashpay/platform/pull/5384#discussion_r4251865308)
  The adapter's newer-wins merge does not coordinate with reconciliation's independent writer. sent_payment_verdicts changes the live entry to Pending before committing; reconcile_sent_payments can then read the still-stored pre-fork confirmed record at wallet/identity/network/payments.rs:1043-1047. If the adapter commits the truncation and Pending overlay before confirm_sent_payment_by_txid runs, that function checks only Pending/Sent and persists Confirmed from the stale evidence. Memory and storage now permanently report confirmation for a dropped transaction, and restart does not repair it because reconciliation skips Confirmed entries. This requires no persistence failure. The opposite writer order also permits the retry overlay to disagree with live state. Establish a shared evidence-freshness rule in the Rust payment owner covering adapter verdicts, retained reversals and reconciliation, and test the actual read/truncation/confirmation interleaving rather than only overlay merging.
- [BLOCKING] packages/rs-platform-wallet/src/changeset/core_bridge.rs:1195-1198: Cancel retained payment reversals when their owning identity is removed
  (existing thread: https://github.com/dashpay/platform/pull/5384#discussion_r4252358882)
  The permanent-refusal fallback bounds retries for an absent identity, but the adapter-private reversal cache has no identity-incarnation ownership. IdentityManager::remove_identity explicitly defines removal as terminal in memory and storage, with re-addition starting blank. Remove and re-add that identity before this merge, and the old Pending payment overlay is accepted into the replacement identity's durable history even though its live payment collection is blank. No permanent refusal occurs, so the new fallback cannot prevent this stale successful write. Invalidate retained reversals when their owning identity lifecycle ends, or carry owner-issued lifecycle evidence validated by the shared Rust identity/payment owner. Cover removal followed by re-addition before retry, independently of the still-owed core truncation.

In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/persister.rs:1193-1202: Retain payment reversals with backend-owned truncations
  (existing thread: https://github.com/dashpay/platform/pull/5384#discussion_r4251865297)
  Fatal recovery preserves only core.truncations and drops their accepted Confirmed-to-Pending payment updates. With the public FlushMode::Manual, store has already returned Ok after buffering both, so the adapter no longer retains the reversal. A fatal flush followed by successful recovery can consequently correct the core transaction and coin state while leaving the authoritative identity payment Confirmed; the fork event does not repeat, and reconciliation skips that status after reload. Manual not being selected by an in-tree host limits exposure but does not make the newly introduced selective recovery coherent. Retaining every generic Pending row would indeed be unsafe because some belong to caller-rolled-back sends. Preserve explicit reorg-reversal provenance alongside backend-owned truncations, with newer-verdict and lifecycle handling, or reject this unsupported buffered combination. Add an acceptance, fatal-flush, recovery and reload regression that checks payment status as well as core state.

In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:493-500: Resolve retained InstantSend finality for every storage reader
  (existing thread: https://github.com/dashpay/platform/pull/5384#discussion_r4252965583)
  This rewrites a retained transaction's record_blob to Mempool while preserving its InstantSend lock. Full loading resolves that lock in restore_recorded_transactions, but SqlitePersister::get_core_tx_record delegates to get_tx_record and returns the decoded blob without lock resolution. Payment-history reconstruction can therefore persist a Pending entry for a still-final payment, and reconciliation cannot confirm that entry until another mined record arrives, although full wallet restoration recognizes the transaction as locked. Existing Confirmed entries are not directly downgraded by this reader. An equivalent late-lock inconsistency predating the PR does not eliminate the new deterministic route from previously mined records into this state. Share validated lock/context resolution between point reads and full restoration, preserving the transaction-body, txid and typed-height checks, and add a post-truncation point-read regression.

[thepastaclaw]

🔴 Blocking: Use bit 15 in the Kotlin capability-mask expectation

The cumulative PR changes this assertion from 0x0bbf to 0x4bbf, but the handler now adds CORE_CHAIN_TRUNCATION at bit 15, producing 0x8bbf. The current Kotlin SDK build + tests job fails this exact assertion, with 547 tests completed and one failure. Although neither Kotlin file changes in the final five-file increment, both the capability addition and this incorrect expected value are introduced by the PR relative to dae6045, so this is not unrelated validation debt. Update the expectation to the intended bit-15 mask and obtain a passing Kotlin CI run; do not move the capability to bit 14, which is reserved for history restoration.

Suggested change

	assertEquals(0x4bbfL, handler.persistenceCapabilitiesBits())
	assertEquals(0x8bbfL, handler.persistenceCapabilitiesBits())

_{source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, rust-quality, security-auditor)}