[romchornyi]

Basic explanation

What this does: The app keeps two records for money it holds: one for each transaction and one for each coin. In some cases the transaction record is updated to "in a block" while its coins' records are not, and the wallet then treats those coins as unconfirmed forever. When the wallet loads, it now checks each coin against its own transaction record and treats the coin as confirmed (or InstantSend-locked) when its transaction is.

Value: Wallets that already have a stuck coin get it back without a reset and restore. The known case is the change of an asset lock that was resumed after a restart (fixed going forward in #5716): it was counted in the balance but refused by every send ("Insufficient balance", Max below the balance).

Risks: Low. No consensus or protocol code. The load only ever raises a coin's flags, only from the store's own transaction record, and only for a block the wallet has already processed. The stored coin rows are not rewritten.

Issue being fixed or feature implemented

The FFI stores (SwiftData, Room) keep is_confirmed / is_instantlocked / height on each coin row, next to the transaction rows. Coin rows are written only from an emitted record's output_details; transaction rows from the record itself. A record that names none of its coins (dashpay/rust-dashcore#1291; the synthetic asset-lock record fixed in #5716) therefore updates the transaction row and leaves the coin rows behind. At the next load the engine restores the coin from its row, the block is already behind the synced height, and nothing ever repairs it.

#5716 stops new occurrences. This PR repairs wallets that already hit it. The SQLite store has no such gap: it already derives each coin's confirmation from its transaction's height at load (rs-platform-wallet-storage core_state.rs).

What was done?

• rs-platform-wallet/src/wallet/core/stored_coin.rs (new): StoredParentState and reconcile_with_parent(utxo, parent, processed_height). Transaction in a block at a height this wallet has processed → coin confirmed at that height (a confirmed coin keeps its own height unless it has none). Transaction InstantSend-locked → coin locked. Otherwise nothing changes. It never lowers a flag.
• rs-platform-wallet-ffi: UtxoRestoreEntryFFI gains parent_context and parent_block_height (appended). build_wallet_start_state decodes them (stored_parent_state, new TX_CONTEXT_RAW_INSTANT_SEND; unknown values mean no evidence) and applies the rule before routing the coin, against last_processed_height.
• Swift buildUtxoRestoreBuffer: copies record.transaction?.context / blockHeight as-is, and prefetches \.transaction (already read per row for the txid).
• Kotlin buildUtxoRestoreData: one chunked projection query (TransactionDao.getConfirmationsByTxids: txid, context, blockHeight) for the coins' creating transactions; UtxoRestoreData gains parentContext / parentBlockHeight. JNI reads them (u32::try_from, a negative value means no evidence).

Notes:

• The coin rows in the store are not rewritten. Every load re-applies the rule; spendability comes from the engine. Store-only readers (storage explorers) keep showing the stored flag.
• A coin can only be raised from a transaction row in the same store, keyed by txid. On v5.0-dev neither transaction nor coin rows are lowered on a reorg, so this adds no state the normal write path does not already produce.
• The new JNI field reads, like the existing ones, are exercised only by instrumented tests.

How Has This Been Tested?

• stored_coin.rs: 8 unit tests (block / chain-locked / locked / unconfirmed parent, a block above the processed height, a confirmed coin with and without its own height, a locked coin under an unconfirmed parent).
• rs-platform-wallet-ffi should_restore_a_coin_as_final_as_its_stored_transaction: drives build_wallet_start_state with coins under parent contexts 2, 1, 0, 3, an unknown value, and a block above the processed height. Fails without the change.
• Swift CoreTxoReconcileTests: testRestoreCarriesTheCreatingTransactionsStoredContext, testRestoreCarriesAnInstantLockedTransactionsContext (loadWalletList hands over the parent's context/height).
• Kotlin PlatformWalletPersistenceHandlerTest.shouldCarryTheCreatingTransactionsStoredContextOnARestoredCoin.
• cargo test -p platform-wallet --all-features (1529), cargo test -p platform-wallet-ffi --all-features (440), cargo clippy -p platform-wallet -p platform-wallet-ffi -p rs-unified-sdk-jni --all-features --all-targets -- -D warnings, ./build_ios.sh --target sim, swift test --filter CoreTxoReconcileTests (21), ./gradlew :sdk:testDebugUnitTest --tests "*PlatformWalletPersistenceHandlerTest*" (162).

Breaking Changes

No behaviour or persisted-format change: no Room or SwiftData migration, and the coin rows are not rewritten. One host-allocated C struct grows: UtxoRestoreEntryFFI gains the appended parent_context / parent_block_height (152 → 160 bytes on 64-bit), read as an array from on_load_wallet_list_fn. A host must be built against the matching generated header, which the in-tree Swift package and JNI crate always are; an out-of-tree C consumer of the struct must be rebuilt. This is the same shape as WalletRestoreEntryFFI in #5026.

Kotlin binary compatibility. UtxoRestoreData gains two primary-constructor properties. Source-compatible, but already-compiled Kotlin/Java code that constructs it must be rebuilt against the new SDK.

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

For repository code-owners and collaborators only

• I have assigned this pull request to a milestone

🤖 Generated with Claude Code

PR Hygiene · 8c7d806

• Bots — thepastaclaw 1 thread unresolved — resolve it
• Self-review — post /self-reviewed
• Reviewer requests paused — your 5 review slots are occupied; this PR is excluded from reviewers' queues. Required approvals still count without a slot.
• Build green
• Approvals
  • kotlin-sdk (packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/NativePersistenceBridge.kt, packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt, packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/TransactionDao.kt and 1 more) — HashEngineering
  • rs-platform-wallet-ffi (packages/rs-platform-wallet-ffi/src/persistence.rs, packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs) — HashEngineering or ZocoLini or llbartekll
  • rs-platform-wallet (packages/rs-platform-wallet/src/wallet/core/mod.rs, packages/rs-platform-wallet/src/wallet/core/stored_coin.rs) — HashEngineering or ZocoLini or llbartekll
  • files with no dedicated owner (packages/rs-unified-sdk-jni/src/persistence.rs) — QuantumExplorer or shumkov
  • swift-sdk — you own it

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: v5.0-dev
• File: .coderabbit.yaml
• Commit: a304409
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 2c627c53-5082-4282-870f-e4305c77a151

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

[github-actions]

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

[thepastaclaw]

✅ Final review complete — no blockers (commit 8c7d806)

[thepastaclaw]

Final validation — Phase 1 + Phase 2

Updated after discussion: 1 finding(s) of this review were settled in the inline threads; no blocking finding remains.

• ✅ Withdrawn: 🔴 Negotiate the extended restore-row layout before reading it

Static verification confirms that the reconciliation policy is conservative, belongs in platform-wallet, and runs before balance reconstruction. One blocking issue remains: the extended restore-row layout is read through the unchanged persistence callback without ABI negotiation. Current CI shows Kotlin build/tests and the Swift frozen-schema check passing; Rust workspace validation remains pending.

🔴 1 0 blocking

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

• Triage: normal by gpt-6.1-sol (effort low) — The cross-language restore change spans several components, but the reconciliation rule is narrowly bounded and does not constitute a large or intricate change to funds movement or coin selection.
• 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)
• 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

🤖 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-ffi/src/wallet_restore_types.rs`:
- [BLOCKING → WITHDRAWN in discussion; do not fix] packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs:470-473: Negotiate the extended restore-row layout before reading it
  Appending these fields changes UtxoRestoreEntryFFI's 64-bit size and array stride from 152 to 160 bytes, but on_load_wallet_list_fn and the existing manager constructors remain unchanged. A persistence host compiled against the previous header can still register and return its old allocation; build_wallet_start_state then constructs a slice with the new stride at persistence.rs:5665–5668. A single row now permits reads beyond its allocation, while subsequent rows are decoded at incorrect offsets, potentially interpreting unrelated bytes as script pointers and lengths. Updating the bundled Swift/JNI producers protects those rebuilt producers, but the documented rebuild requirement does not reject an older host before Rust dereferences its rows. This conflicts with the compatibility handling already present in PersistenceCallbacksExtension: its sidecars explicitly avoid changing bare-pointer payload strides, and manager.rs reads extension slots only after size/version negotiation. Keep the legacy restore row unchanged and transport parent evidence through a negotiated sidecar, or introduce a versioned restore interface that distinguishes the layouts before reading the allocation.

[thepastaclaw]

✅ Withdrawn at 8c7d8062; see the replies below.

🔴 Blocking: Negotiate the extended restore-row layout before reading it

Appending these fields changes UtxoRestoreEntryFFI's 64-bit size and array stride from 152 to 160 bytes, but on_load_wallet_list_fn and the existing manager constructors remain unchanged. A persistence host compiled against the previous header can still register and return its old allocation; build_wallet_start_state then constructs a slice with the new stride at persistence.rs:5665–5668. A single row now permits reads beyond its allocation, while subsequent rows are decoded at incorrect offsets, potentially interpreting unrelated bytes as script pointers and lengths. Updating the bundled Swift/JNI producers protects those rebuilt producers, but the documented rebuild requirement does not reject an older host before Rust dereferences its rows. This conflicts with the compatibility handling already present in PersistenceCallbacksExtension: its sidecars explicitly avoid changing bare-pointer payload strides, and manager.rs reads extension slots only after size/version negotiation. Keep the legacy restore row unchanged and transport parent evidence through a negotiated sidecar, or introduce a versioned restore interface that distinguishes the layouts before reading the allocation.

_{source: gpt-6.1-sol (phase2-reviewer: ffi-engineer)}

[romchornyi]

Agreed that the stride changes. I've handled it as a breaking change, not a sidecar, and the PR now says so: the title has !, and the Breaking Changes section names the 152 → 160 byte growth, the rebuild requirement, and the Kotlin UtxoRestoreData constructor change.

Why not negotiate the layout:

• The struct's only producers are in this repo: the Swift package's PlatformWalletPersistenceHandler and the JNI crate. The Swift package ships with its xcframework, and the JNI crate is compiled into the same native library as this FFI crate. Neither can pair an old header with this Rust.
• fix(platform-wallet)!: persist DashPay coreHeight backfill coverage so a relaunch resumes instead of rewinding again #5026 grew WalletRestoreEntryFFI the same way. That struct is read through this same on_load_wallet_list_fn array, and the PR was marked ! with the same rebuild note.
• A sidecar would add a load callback and an outpoint lookup to both hosts to guard against a pairing the shipped artifacts cannot produce.

If a maintainer prefers the sidecar shape here (as #4799 did for the identity rows), I'll switch to it.

[thepastaclaw]

You're right: I treated cross-version binary compatibility as a requirement without establishing that it is supported here. I checked that Swift allocates these rows using the header from its XCFramework, JNI constructs them using its direct Rust dependency, and #5026 likewise extended WalletRestoreEntryFFI. The mixed-version external-host case I described remains unsafe, but the documented rebuild requirement explicitly excludes that pairing; it does not demonstrate a defect in the matched producers shipped here. I withdraw the blocker—layout negotiation is not required merely because this deliberately breaks the old ABI.

Withdrawing this finding.

[github-actions]

Your move: thepastaclaw left review threads unresolved; resolve them.
Full checklist in the description.

[thepastaclaw]

Re-review after discussion — commit 8c7d806

Standing review was CHANGES_REQUESTED; this re-review is APPROVE: no blocking findings remain.

Approved: the discussion withdrew or deferred every finding the full review of 8c7d8062 (run 8295) raised, and the approval rests on that review.

No longer blocking:

• Negotiate the extended restore-row layout before reading it

Same commit as the standing review; the inline threads above carry the per-finding outcome. This follow-up exists only to correct the verdict.

Review provenance

Source: conversation lane gpt-6.1-sol (agent: conversation); no reviewer or verifier lanes ran for this follow-up

• Verdict moved because the inline discussion withdrew or deferred findings of the standing review of this commit; the code was not re-reviewed