[HashEngineering]

Issue being fixed or feature implemented

#4638 fixed, for iOS, TXO rows the store writes as unspent for coins the engine never credited
(the rust-dashcore#992 shape: a CoinJoin collateral burn processed before the coin's funding transaction). A
restore then hands those rows back to the engine as spendable at the next launch. #4638 added two things: credit
verdicts delivered at save time (Part 1, prevention) and a post-sync store↔engine reconcile (Part 2). Its Rust
side is already in v5.1-dev. Android uses neither: the JNI layer never registers the verdict slot, and the
reconcile only exists in #4439, which cannot build on v5.1-dev because its address-pool hole repair needs
rust-dashcore#979's ensure_contiguous_to.

This PR is #4638 for Kotlin: the same Rust, the same rules, nothing that needs #979.

What was done?

• JNI bindings for the paged inventory and the outpoint classifier (from fix(kotlin-sdk): reconcile the TXO store against the engine and repair restored address pools #4439): wallet_utxos_page and
  classify_outpoints, both added by fix(swift-sdk): stop born-spent TXO rows at the persistence seam and reconcile the store after a full scan #4638.
• Post-sync TXO reconcile (port of fix(swift-sdk): stop born-spent TXO rows at the persistence seam and reconcile the store after a full scan #4638 Part 2, built on fix(kotlin-sdk): reconcile the TXO store against the engine and repair restored address pools #4439's Kotlin reconcile and brought to fix(swift-sdk): stop born-spent TXO rows at the persistence seam and reconcile the store after a full scan #4638's
  rules):
  • When it runs: on the transition into steady state (SYNCED, or waiting for events with the filter phase at
    its target) and every 30 minutes per wallet.
  • Gates, each a logged skip: a latched sync fault; the wallet's own engine watermark more than 6 blocks
    behind the scan tip; a run for the wallet already in flight. Closing the manager or removing the wallet stops
    a run between pages.
  • Heal pass (engine → store, insert-only): inserts a coin the store lacks only when it is well-formed,
    confirmed by the engine, at least 100 confirmations deep, and owned by an account the store can resolve from
    the seven-field tuple. An unresolvable owner is counted, never filed unowned.
  • Classify pass (store → engine): asks about the wallet's unspent rows a page at a time. Marks a row spent
    only on KnownUncredited (a mined spender on record), and drops pending-input claims on it. The page is read
    with a count of committed persistence rounds; if a round committed before the apply, nothing is written and
    the page is classified again.
  • Never deletes a row, never un-marks a spent row, never acts on absence.
  • One summary line per run, counts and reference digests only (no txid, outpoint, address or script).
• One new JNI export, walletManagerCoreWalletSyncedHeight: a single call to
  platform_wallet_core_wallet_state, so the reconcile gates on the engine's watermark as Swift does. The
  persisted copy in Room trails it by a round, which is where the SYNCED transition lands.
• Credit verdicts at save time (new; port of fix(swift-sdk): stop born-spent TXO rows at the persistence seam and reconcile the store after a full scan #4638 Part 1):
  • rs-unified-sdk-jni registers on_persist_wallet_changeset_utxo_verdicts_fn and passes the round's
    verdicts to NativePersistenceBridge.onWalletChangesetUtxoVerdicts as flat arrays, before the changeset
    callback.
  • PlatformWalletPersistenceHandler keeps them in the round's buffer. upsertUtxoRow writes an
    observed_spent / doomed output spent at creation, with no spender link. Any verdict vetoes the
    redelivery recovery clear (both the flag and the sweep stamp). uncredited changes nothing else. A
    rolled-back round leaves no verdict behind.
  • One log line per round, counts only.
• Not carried from fix(kotlin-sdk): reconcile the TXO store against the engine and repair restored address pools #4439: the address-pool hole repair (needs rust-dashcore#979) and the "netAmount suspects"
  log. Both can follow separately.

Kotlin-specific choices, same behaviour:

• The classify pass walks the store by outpoint rather than by offset, so its own flips never shift the walk.
• A reconcile store step takes the same lock as the persistence callbacks, so it never has to wait behind an
  open round.
• Contact (DIP-15 watch-only) coins are also excluded on the Kotlin side, by account tag and by address.

How Has This Been Tested?

On this branch (v5.1-dev, rust-dashcore e4208c90, no #979):

check	result
cargo test -p platform-wallet --lib	1,185 passed
cargo test -p platform-wallet-ffi --lib	388 passed, 1 ignored (pre-existing)
cargo test -p rs-unified-sdk-jni	44 passed
cargo clippy … -D warnings	clean
cargo fmt --check	clean
Kotlin SDK unit tests (--rerun-tasks)	571 passed, 0 failed

The Kotlin tests port BornSpentTxoPersistTests (observed_spent / doomed / uncredited, the redelivery veto,
round scope, rollback, and a restart over a reopened on-disk store that restores zero coins for a verdict-spent
row) and the relevant cases of CoreTxoReconcileTests, CoreTxoReconcileShutdownTests and
CoreTxoReconcilePrivacyTests:

• flip on proof, and the stale-round refusal with re-classification;
• the heal gates (immature, engine-unconfirmed, unresolved owner, malformed), and healed-spent counted
  separately;
• idempotence, wallet scope, paging, read failures, and cancellation;
• the steady-state and watermark gates;
• every reconcile log line checked for txids in either byte order, outpoints, addresses, scripts, wallet ids,
  and long hex or Base58 runs.

On an Android emulator (Pixel 9 AVD), with the same commits on the integration line, restored from seed and
then relaunched:

• Wallet A: 168.96778705, 1,081 coins, 7,461 transactions, all identical to the build without this PR. 13,460
  rows written spent at creation. The reconcile ran 2 s after SYNCED on the first launch and 1 s after on the
  relaunch: 1,081 engine coins, 1,081 store rows, all unspent, zero mutations. 0 collateral burns left unspent.
• Wallet B: 58.05157585, 779 coins, 3,273 transactions, all identical to the build without this PR. 4,564
  rows written spent at creation. The reconcile ran 1 s after SYNCED on the first launch and on the relaunch: 779
  engine coins, 779 store rows, all unspent, zero mutations.
• An earlier revision on an engine with rust-dashcore#979 removed: balances, coins and the reconcile were
  unchanged and Part 1 fired the same way. (Transaction history is a different matter; that belongs to fix: same block core chain lock height #979.)

Breaking Changes

None. One additive bridge method with a no-op default, and one additional extension slot wired.

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

🤖 Generated with Claude Code

PR Hygiene · d7a2674

• Bots — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 2 threads unresolved — resolve them — /skip-bots proceeds without the ones not yet reported
• Self-review — post /self-reviewed
• Build failed
• Approvals
  • files with no dedicated owner (Cargo.lock, packages/rs-unified-sdk-jni/Cargo.toml, packages/rs-unified-sdk-jni/src/persistence.rs and 1 more) — QuantumExplorer or shumkov
  • kotlin-sdk — you own it
  • rs-platform-wallet-ffi (packages/rs-platform-wallet-ffi/src/lib.rs, packages/rs-platform-wallet-ffi/src/manager.rs, packages/rs-platform-wallet-ffi/src/manager_diagnostics.rs and 1 more) — ZocoLini or llbartekll or romchornyi
  • rs-platform-wallet (packages/rs-platform-wallet/src/manager/accessors.rs, packages/rs-platform-wallet/src/manager/mod.rs, packages/rs-platform-wallet/src/manager/txo_reconcile.rs and 2 more) — ZocoLini or llbartekll or romchornyi

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

[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

• Source mode: Auto
• Repository: dashpay/platform
• Branch: feat/kotlin-sdk-txo-reconcile-4638-port
• File: .coderabbit.yaml
• Commit: d7a2674
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 1f29c644-a5c3-4484-ab75-69ac4f930806

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 — 2 blocking finding(s) (commit d7a2674) · triage: critical

[github-actions]

Bots are done — your move: post /self-reviewed.
Full checklist in the description.

[HashEngineering]

/self-review

[thepastaclaw]

Final validation — Phase 1 + Phase 2

Verified the supplied findings against head 420996c and the stated Android reconciliation scope. Four non-blocking suggestions remain: restore/reconcile ownership parity, diagnostic-log privacy, strict inventory decoding, and transport-contract coverage. This was a static review; the supplied CI snapshot reports Kotlin build/tests passing, with Rust workspace tests and several builds still pending.

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

• Triage: critical by gpt-6.1-sol (effort low) — The intricate reconciliation and save-time verdict logic in PlatformWalletPersistenceHandler.kt, including upsertUtxoRow and classifyStoreRows, changes which coins are persisted as spendable and restored into the engine, directly affecting coin-selection eligibility.
• 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 15% 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, 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 — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — security-auditor (completed, effort xhigh); agent phase2-reviewer
• Model comparison: every Phase-2 reviewer also ran on gpt-6-astra; the verifier weighed both sets without knowing which model wrote which

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/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:2043-2047: Resolve the same TXO owners during reconciliation and restore
  The new resolver returns null when coreAddressId is absent, but buildUtxoRestoreData resolves the account through txo.address in that case. This difference is reachable: upsertUtxoRow leaves coreAddressId null when the address row does not yet exist, and the address-pool callback subsequently inserts that address without backfilling the TXO relationship. An unspent legacy row can therefore be eligible for restoration into Rust while every reconciliation run skips it as unresolved, even when the engine has positive evidence that it was spent. Share the restore path's address fallback with reconciliation and add a regression test where the address arrives after the TXO and a KnownUncredited verdict removes the row from the restore set.
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt:2130-2134: Avoid recoverable outpoint references in reconciliation logs
  Each mutation logs an outpoint reference alongside a stable wallet reference. logReference computes an unkeyed SHA-256 digest truncated to six bytes, so a recipient of diagnostic logs can enumerate public blockchain outpoints, hash their wire-order txid plus little-endian vout, and match the reference. The wallet reference then groups those matched outputs into one wallet's history; truncation and the absence of literal identifiers do not prevent this lookup. Remove the outpoint-derived reference and retain aggregate diagnostics, or use a randomly keyed process-local reference whose key is never logged. Extend the privacy test beyond excluding raw identifiers and long hexadecimal strings. This is a non-blocking diagnostic-data exposure under the supplied severity policy, rather than a consensus or authentication violation.
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt:1358-1362: Require inventory account fields to enforce fail-loud decoding
  EngineUtxoRow and engineJson explicitly promise that missing routing fields fail decoding rather than healing a row under a defaulted owner. However, standardTag, index, registrationIndex, and keyClass default to zero. A page missing index can therefore decode and resolve to account zero instead of failing. The current Rust producer emits all four fields, so this is a contract-hardening issue rather than a demonstrated failure with the current producer. Remove these defaults, retain the intentionally optional identity halves, and add a missing-routing-field decode test.

In `packages/rs-unified-sdk-jni/src/wallet_manager.rs`:
- [SUGGESTION] packages/rs-unified-sdk-jni/src/wallet_manager.rs:3354-3376: Test the actual Rust/Kotlin reconciliation JSON contract
  The new Kotlin persistence tests use FakeEngine, which supplies handwritten inventory JSON, represents cursors as ordinal strings, and validates classification fields against a separately maintained RUST_REQUIRED_QUERY_KEYS list. No Rust tests exercise the new serde models, identity conversion, or account/outpoint cursor reconstruction. These tests consequently cannot detect disagreement between the actual Rust and Kotlin codecs, despite the documented zero-valued-field omission previously rejecting classification batches. Add shared golden payloads decoded by Rust's Vec<OutpointQuery> and Kotlin's EngineUtxoPage serializer, plus cursor round-trip tests through the actual conversion code. Cover zero-valued standard accounts, non-default account fields and DashPay identity halves, inventory flags, and malformed identity/txid 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.

• Track the same public-outpoint disclosure in existing Swift diagnostics — Existing Swift reconciliation logs also emit outpoint references, and SDKLogger.reference uses unkeyed SHA-256 truncated to six bytes. The same public-outpoint lookup applies, but those implementations are unchanged in this PR and do not expand its merge requirements.
  • Follow-up: Track a separate privacy issue to remove public-outpoint-derived references or replace them with secret-keyed diagnostic references.

[github-actions]

Ready for review — files with no dedicated owner: QuantumExplorer or shumkov.
Full checklist in the description.

[github-actions]

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

[github-actions]

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

[thepastaclaw]

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

At head 9e7711d, seven prior findings are fixed and the pagination-index suggestion remains unresolved. The new destructive classification pass can apply a spent verdict after an engine reorg has invalidated it, creating a blocking persisted-wallet-state defect; three additional nonblocking scheduling and claim-lifetime issues are confirmed. This was static verification only: current CI reports Kotlin/emulator and Rust workspace tests passing, with Swift SDK build/tests and PR Hygiene still pending.

🔴 1 blocking | 🟡 4 suggestion(s)

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

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: 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) — The intricate reconciliation and save-time verdict logic in PlatformWalletPersistenceHandler.kt, especially upsertUtxoRow, changes which persisted coins are considered spent or spendable and therefore directly affects coin selection after restore.
• 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 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/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:1946-1950: Fence destructive verdicts against engine reorgs, not only Room commits
  This fence detects host persistence changes, but not changes to the engine snapshot that supplied the verdict. `classify_outpoints` accepts any mined spender, including one above the last chainlock, and releases the wallet read lock before returning. The pinned dash-spv can then handle a fork and call `truncate_above`, removing that spender and restoring its input coin. `build_core_changeset` returns an empty changeset for `ChainTruncated`, so this interleaving need not open or commit a persistence round: both checks here can still pass and mark the now-live coin spent. Later reconciliation cannot undo the flag because healing skips existing rows and classification skips spent rows, so restoration subsequently omits the coin. The broader truncation-persistence gap is tracked in https://github.com/dashpay/platform/issues/5380, but this PR adds the stale destructive write. Coordinate classification and apply with engine mutations through a Rust-owned consistency mechanism, or restrict actionable verdicts to evidence an allowed reorg cannot invalidate. Add a regression that truncates the spender between classification and apply without committing a host round.

In `packages/rs-platform-wallet/src/manager/txo_reconcile.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/txo_reconcile.rs:363-370: Release provisional reconcile claims when begin is cancelled
  `begin` inserts the wallet into `in_flight` before awaiting `gate`, but releases it only when that await returns an error. Dropping this public Rust future while it waits for SPV progress or the wallet-manager read lock skips cleanup without returning the successful result that obligates the caller to call `finish`. Subsequent begins return `AlreadyRunning`, `due_wallets` excludes the wallet, and `forget` leaves the claim intact. The blocking C adapter and Kotlin's non-cancellable handoff protect the current Android path, so this is nonblocking native API hardening rather than an Android cancellation failure. Hold a provisional rollback guard across the await and disarm it only when successfully transferring the claim to the caller. Add a regression that drops a blocked `begin` future and verifies the wallet can be claimed again.
- [SUGGESTION] packages/rs-platform-wallet/src/manager/txo_reconcile.rs:313-316: Prevent an old run from restoring a removed wallet's cadence
  Wallet removal calls `forget_txo_reconcile`, but an already-running reconciliation can finish afterward and reinsert the timestamp that `forget` removed. This is reachable through Kotlin's epoch-based stop: the handler returns an incomplete report after removal, and `reconcileTxoStore` still calls `finish` with `ran = true`. Re-adding the same deterministic wallet ID while SPV remains steady then waits for the 30-minute cadence instead of being due immediately. Preserve the in-flight exclusion until the old run exits, but prevent its completion from restoring scheduling history for a forgotten lifecycle, for example by associating the claim with a lifecycle generation. Cover `try_begin → forget → finish → due_wallets` in the schedule tests.

In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt:2415-2418: Do not consume cadence for wallets Kotlin skips
  The native `due_wallets` call stamps `last_run` for every returned wallet before this code filters the result against Kotlin's loaded wrappers. Native creation and restoration publish wallets before Kotlin publishes the corresponding wrapper: creation still performs Keystore and Room work, and restoration reconstructs pending keys before wrapping the wallets. If a steady-state poll lands in that gap, `loaded[key] ?: continue` discards the scheduled run but leaves its timestamp intact. Publishing the wrapper afterward does not retry it, so the wallet waits up to 30 minutes despite never having reconciled. Have the native scheduling API account for the host-loaded set, or explicitly release skipped reservations without erasing completed-run history. Add coverage for native publication followed by a poll before Kotlin wrapper publication.

In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/TxoDao.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/TxoDao.kt:48-52: Add an index matching the unspent pagination query
  (existing thread: https://github.com/dashpay/platform/pull/5259#discussion_r4236385885)
  `TxoEntity` has separate indexes for walletId and the outpoint primary key, but none combining this query's wallet/spent predicates with its cursor and ordering. SQLite can repeatedly filter and sort a wallet's history, or walk the outpoint index while filtering unrelated and spent rows; LIMIT bounds returned rows, not the work needed to find them. Each automatic classification-page read holds `callbackExclusion`, which synchronous persistence callbacks also acquire, so large histories can delay durable sync progress. Add a `(walletId, isSpent, outpoint)` index with the Room migration and schema update. Keeping this PR schema-neutral explains the proposed follow-up, but the supplied discussion contains no linked tracking issue or PR; link that work before treating the suggestion as deferred.

[github-actions]

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

[github-actions]

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

[thepastaclaw]

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

The Android reconciliation policy is now largely centralized in rs-platform-wallet, and the previously reported cadence, held-verdict, owner-resolution, transport, and open-round issues are fixed. Two blocking lifecycle/ownership issues remain: a JNI marshalling failure can permanently strand a native reconcile claim, and the existing Swift reconciler still bypasses the newly introduced Rust-owned policy. The Room pagination index and several fail-loud transport and diagnostic hardening items also remain; CI checks covering Rust and Kotlin are still in progress.

🔴 2 blocking | 🟡 8 suggestion(s)

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

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); 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); 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 TXO persistence, credit verdict handling, and engine/store reconciliation that directly affects which coins are considered spendable and therefore changes a funds/coin-selection critical surface.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
• Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — 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-unified-sdk-jni/src/wallet_manager.rs`:
- [BLOCKING] packages/rs-unified-sdk-jni/src/wallet_manager.rs:3515-3549: Release the reconcile claim when outTipHeight marshalling fails
  This JNI export claims the wallet before writing `out_tip_height`. It checks only for a null array, so an `IntArray` with the wrong length, or a JNI failure from `set_int_array_region`, returns `-1` after the native claim has been installed. Kotlin enters the cleanup `finally` only after a successful begin return, leaving the wallet permanently `AlreadyRunning` for the manager lifetime. Validate the array length before calling the native begin, and explicitly finish the claim on every post-begin marshalling failure.
- [SUGGESTION] packages/rs-unified-sdk-jni/src/wallet_manager.rs:3782-3809: Initialize outHeld for an empty classification batch
  The JNI contract documents `outHeld[0]` as zero when no guard is held, but the empty-query path returns without writing it. A caller that reuses a `LongArray(1)` can retain an opaque token from an earlier call; later cleanup may free a stale token or associate it with the wrong classification call. Validate the array length and write the zero sentinel before the empty return.

In `packages/rs-platform-wallet/src/manager/txo_reconcile.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/txo_reconcile.rs:1-27: Make all mobile callers use the Rust-owned TXO reconcile policy
  The new Rust policy is used by Kotlin, but `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerTxoReconcile.swift` still owns its own cadence, start gates, heal eligibility, stale retry loop, and direct classify/apply flow. That caller bypasses `TxoReconciler::classify_held` and its engine consistency guard, leaving two mobile policy implementations that can diverge and preserving the old classify-to-apply behavior for iOS. Route Swift through the Rust policy using thin bindings, including the held-verdict lifecycle, instead of leaving policy decisions in the Swift wrapper.
- [SUGGESTION] packages/rs-platform-wallet/src/manager/txo_reconcile.rs:537-541: Read the SPV run session after the steady-state read
  `due_wallets` reads `run_session()` before awaiting `scan_progress()`. If SPV stops and restarts between those operations, the poll can pair the old session with the new steady state; the schedule then sees the old session as already steady and misses the rising edge until the normal cadence expires. Reading the session after the steady-state read fails safe: a restart in that gap may cause one extra idempotent run rather than silently skipping the edge.

In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt:1381-1394: Do not let a stale run claim a re-added wallet
  The coroutine captures `txoReconcileEpoch` before waiting for `teardownGate`, but invokes the native `begin` without checking that epoch again. If it is delayed before acquiring the gate while `removeWallet` forgets the old lifecycle and the same wallet ID is re-added, the old coroutine can claim the new wallet. It then observes the stale epoch and returns `MANAGER_CLOSED`; `try_begin` has already stamped `last_run`, while `finish(ran = false)` leaves that stamp in place, suppressing the newly added wallet for the cadence interval despite no reconciliation having run. Reject the stale caller before the native claim and associate the claim with the wallet lifecycle generation so the race after that check cannot claim a replacement wallet.

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:1398-1419: Make all inventory state fields required at the JNI boundary
  The transport comments promise strict decoding, but `address`, `scriptHex`, and all engine flags still have defaults. A payload can therefore omit these fields and still decode; an omitted script becomes an empty byte array, which is not rejected by `healEngineRow`, and an eligible row can be inserted with malformed address/script data or guessed flags. Keep only the intentionally optional DashPay identity halves nullable and require the remaining inventory fields.
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt:1457-1461: Reject inventory pages with incomplete continuation metadata
  `EngineUtxoPage` defaults `utxos`, `cursor`, and `hasMore`, so a malformed or drifted payload can decode as a terminal page. `healMissingTxos` treats either `!hasMore` or a null cursor as completion, meaning an omitted continuation field can silently stop the heal walk and leave later engine coins unrepaired. Make the page fields required and reject the inconsistent state where `hasMore` is true without a cursor.
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt:2189-2201: Keep reconcile diagnostics free of wallet amounts
  The summary described as counts and keyed references logs `insertedDuffs` and `flippedDuffs`, which are exact aggregate values of coins healed into or removed from the restore set. Android logcat, support collection, OEM tooling, or a compromised logging path can therefore disclose wallet financial activity even though individual identifiers are protected. Retain counts, completion state, and keyed references, but remove the amount totals from the diagnostic line.

In `packages/rs-platform-wallet-ffi/src/txo_reconcile.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/txo_reconcile.rs:80-127: Catch panics in the new reconcile C exports before crossing into JNI or Swift
  The new `platform-wallet-ffi` reconcile exports call `block_on_worker` and blocking Tokio lock operations directly from `extern "C"` functions. `block_on_worker` uses `expect("tokio worker panicked")`, and `blocking_read()` can panic in an incompatible runtime context. JNI's `guard` is on the far side of this C ABI, so it cannot catch a panic that occurs inside the C export; Swift callers have no such guard either. Wrap each new C entry point with the established `catch_panic_to_code` pattern and return a `PlatformWalletFFIResult` error while keeping output initialization and allocated-resource cleanup inside the guarded body.

In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/TxoDao.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/TxoDao.kt:48-52: Add an index matching the unspent pagination query
  (existing thread: https://github.com/dashpay/platform/pull/5259#discussion_r4236385885)
  `pageUnspentByWallet` filters by `walletId` and `isSpent`, then seeks and orders by `outpoint`, but `TxoEntity` has only separate single-column indexes. SQLite may scan and filter a wallet's full history or walk the global outpoint index for every page. This query now runs automatically under `callbackExclusion`, so large histories can delay persistence callbacks. Add a composite `(walletId, isSpent, outpoint)` Room index and the corresponding schema migration.

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.

• Persist engine truncations that occur after a reconcile write — The held read guard fixes the pre-apply fork race, but a later engine truncation can still restore a coin without a persistence round that makes the corresponding Room row live again. This broader truncation-persistence gap is concrete and is already tracked separately.
  • Follow-up: #5380

[thepastaclaw]

🔴 Blocking: Release the reconcile claim when outTipHeight marshalling fails

This JNI export claims the wallet before writing out_tip_height. It checks only for a null array, so an IntArray with the wrong length, or a JNI failure from set_int_array_region, returns -1 after the native claim has been installed. Kotlin enters the cleanup finally only after a successful begin return, leaving the wallet permanently AlreadyRunning for the manager lifetime. Validate the array length before calling the native begin, and explicitly finish the claim on every post-begin marshalling failure.

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

[thepastaclaw]

🔴 Blocking: Make all mobile callers use the Rust-owned TXO reconcile policy

The new Rust policy is used by Kotlin, but packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerTxoReconcile.swift still owns its own cadence, start gates, heal eligibility, stale retry loop, and direct classify/apply flow. That caller bypasses TxoReconciler::classify_held and its engine consistency guard, leaving two mobile policy implementations that can diverge and preserving the old classify-to-apply behavior for iOS. Route Swift through the Rust policy using thin bindings, including the held-verdict lifecycle, instead of leaving policy decisions in the Swift wrapper.

_{source: gpt-6.1-sol (phase2-reviewer: architecture-layering)}

[thepastaclaw]

🟡 Suggestion: Do not let a stale run claim a re-added wallet

The coroutine captures txoReconcileEpoch before waiting for teardownGate, but invokes the native begin without checking that epoch again. If it is delayed before acquiring the gate while removeWallet forgets the old lifecycle and the same wallet ID is re-added, the old coroutine can claim the new wallet. It then observes the stale epoch and returns MANAGER_CLOSED; try_begin has already stamped last_run, while finish(ran = false) leaves that stamp in place, suppressing the newly added wallet for the cadence interval despite no reconciliation having run. Reject the stale caller before the native claim and associate the claim with the wallet lifecycle generation so the race after that check cannot claim a replacement wallet.

_{source: gpt-6.1-sol (phase2-reviewer: architecture-layering)}

[thepastaclaw]

🟡 Suggestion: Make all inventory state fields required at the JNI boundary

The transport comments promise strict decoding, but address, scriptHex, and all engine flags still have defaults. A payload can therefore omit these fields and still decode; an omitted script becomes an empty byte array, which is not rejected by healEngineRow, and an eligible row can be inserted with malformed address/script data or guessed flags. Keep only the intentionally optional DashPay identity halves nullable and require the remaining inventory fields.

Suggested change

	data class EngineUtxoRow(
	val typeTag: Int,
	val standardTag: Int,
	val index: Int,
	val registrationIndex: Int,
	val keyClass: Int,
	val userIdentityId: String? = null,
	val friendIdentityId: String? = null,
	val txid: String,
	val vout: Int,
	val amount: Long,
	val address: String = "",
	val scriptHex: String = "",
	val height: Int,
	/** The engine's own flags for the coin. Stamped onto a healed row
	* as given: guessing them (a coinbase row filed as non-coinbase)
	* would misstate maturity in the mirror the engine reloads from. */
	val isConfirmed: Boolean = false,
	val isInstantlocked: Boolean = false,
	val isCoinbase: Boolean = false,
	val isLocked: Boolean = false,
	val healVerdict: Int,
	val address: String,
	val scriptHex: String,
	val height: Int,
	val isConfirmed: Boolean,
	val isInstantlocked: Boolean,
	val isCoinbase: Boolean,
	val isLocked: Boolean,
	val healVerdict: Int,

_{source: gpt-6.1-sol (phase2-reviewer: architecture-layering)}

[thepastaclaw]

🟡 Suggestion: Reject inventory pages with incomplete continuation metadata

EngineUtxoPage defaults utxos, cursor, and hasMore, so a malformed or drifted payload can decode as a terminal page. healMissingTxos treats either !hasMore or a null cursor as completion, meaning an omitted continuation field can silently stop the heal walk and leave later engine coins unrepaired. Make the page fields required and reject the inconsistent state where hasMore is true without a cursor.

_{source: gpt-6.1-sol (phase2-reviewer: architecture-layering)}

[thepastaclaw]

🟡 Suggestion: Keep reconcile diagnostics free of wallet amounts

The summary described as counts and keyed references logs insertedDuffs and flippedDuffs, which are exact aggregate values of coins healed into or removed from the restore set. Android logcat, support collection, OEM tooling, or a compromised logging path can therefore disclose wallet financial activity even though individual identifiers are protected. Retain counts, completion state, and keyed references, but remove the amount totals from the diagnostic line.

Suggested change

	private fun logReconcileSummary(report: TxoReconcileReport, walletRef: String) {
	val counts = "engineRows=${report.engineRows} storeRows=${report.storeRows} " +
	"inserted=${report.inserted} insertedDuffs=${report.insertedDuffs} " +
	"healedSpent=${report.healedSpent} " +
	"flipped=${report.flipped} flippedDuffs=${report.flippedDuffs} " +
	"alreadyPresent=${report.alreadyPresent} unspent=${report.unspent} " +
	"unknown=${report.unknown} notOwned=${report.notOwned} " +
	"skippedImmature=${report.skippedImmature} " +
	"skippedUnresolvedAccount=${report.skippedUnresolvedAccount} " +
	"skippedInvalid=${report.skippedInvalid} " +
	"unresolvedAccount=${report.unresolvedAccount} staleRetries=${report.staleRetries} " +
	"transportFailures=${report.transportFailures} storeFailures=${report.storeFailures} " +
	"completed=${report.completed} wallet=$walletRef"
	val counts = "engineRows=${report.engineRows} storeRows=${report.storeRows} " +
	"inserted=${report.inserted} healedSpent=${report.healedSpent} " +
	"flipped=${report.flipped} alreadyPresent=${report.alreadyPresent} " +
	"unspent=${report.unspent} unknown=${report.unknown} notOwned=${report.notOwned} " +
	"skippedImmature=${report.skippedImmature} " +
	"skippedUnresolvedAccount=${report.skippedUnresolvedAccount} " +
	"skippedInvalid=${report.skippedInvalid} unresolvedAccount=${report.unresolvedAccount} " +
	"staleRetries=${report.staleRetries} transportFailures=${report.transportFailures} " +
	"storeFailures=${report.storeFailures} completed=${report.completed} wallet=$walletRef"

_{source: gpt-6.1-sol (phase2-reviewer: architecture-layering)}

[thepastaclaw]

🟡 Suggestion: Read the SPV run session after the steady-state read

due_wallets reads run_session() before awaiting scan_progress(). If SPV stops and restarts between those operations, the poll can pair the old session with the new steady state; the schedule then sees the old session as already steady and misses the rising edge until the normal cadence expires. Reading the session after the steady-state read fails safe: a restart in that gap may cause one extra idempotent run rather than silently skipping the edge.

Suggested change

	pub async fn due_wallets(&self) -> Vec<WalletId> {
	let session = self.spv.run_session();
	let steady = self.scan_progress().await.is_some_and(|p| p.is_steady());
	let wallets: Vec<WalletId> = self.wallets.load().keys().copied().collect();
	lock_schedule(&self.schedule).due_wallets(steady, session, wallets, Instant::now())
	let steady = self.scan_progress().await.is_some_and(|p| p.is_steady());
	let session = self.spv.run_session();
	let wallets: Vec<WalletId> = self.wallets.load().keys().copied().collect();
	lock_schedule(&self.schedule).due_wallets(steady, session, wallets, Instant::now())

_{source: muse-spark-1.3-contributor (phase1-reviewer: rust-quality)}

[thepastaclaw]

🟡 Suggestion: Catch panics in the new reconcile C exports before crossing into JNI or Swift

The new platform-wallet-ffi reconcile exports call block_on_worker and blocking Tokio lock operations directly from extern "C" functions. block_on_worker uses expect("tokio worker panicked"), and blocking_read() can panic in an incompatible runtime context. JNI's guard is on the far side of this C ABI, so it cannot catch a panic that occurs inside the C export; Swift callers have no such guard either. Wrap each new C entry point with the established catch_panic_to_code pattern and return a PlatformWalletFFIResult error while keeping output initialization and allocated-resource cleanup inside the guarded body.

_{source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, rust-quality, security-auditor)}

[thepastaclaw]

🟡 Suggestion: Initialize outHeld for an empty classification batch

The JNI contract documents outHeld[0] as zero when no guard is held, but the empty-query path returns without writing it. A caller that reuses a LongArray(1) can retain an opaque token from an earlier call; later cleanup may free a stale token or associate it with the wrong classification call. Validate the array length and write the zero sentinel before the empty return.

_{source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: rust-quality)}