You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(kotlin-sdk): port #4638 to Android — credit verdicts at save time and the post-sync TXO reconcile - #5259
#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.
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.
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.
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
Rust only: platform_wallet::changeset::CoreChangeSet gains a public restored_utxos field. The struct has public fields and is not #[non_exhaustive], so code that builds it by listing every field, or destructures it without .., must add restored_utxos (an empty Vec keeps the old behaviour) or use ..Default::default() / ... Serialized changesets without the field still decode (serde(default)). No PlatformVersion change.
Kotlin and Swift: none. One additive bridge method with a no-op default, one additional extension slot wired, and SDKLogValue keeps its cases.
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
Wallet transaction outputs are reconciled with engine records: eligible outputs missing from storage can be restored, and outputs confirmed as uncredited can be marked spent.
Reconciliation runs automatically for eligible wallets and reports completed, skipped, or incomplete runs.
Kotlin and Swift SDKs expose reconciliation outcomes and skip reasons.
Bug Fixes
Reconciliation stops safely when wallet data changes or work is cancelled, and releases held resources after failures.
Uncredited-spend decisions require evidence covered by the wallet’s ChainLock.
Reconciliation logs omit sensitive value totals and use keyed references for identifiers.
…lassifier
#4638 landed the wallet-wide paged UTXO inventory and the outpoint
classifier in platform-wallet, and stopped at the C boundary — the Swift
host reaches them, Kotlin does not. These two exports carry them across
JNI and no further.
Each is a serialization shim over exactly ONE platform-wallet call. The
account ordering, the cursor semantics, the page bound, the address
rendering and every classification rule stay in `platform-wallet`, so
both hosts walk the identical inventory and get identical verdicts.
Nothing here decides anything: serde shapes the JSON against the same
field names the Kotlin side decodes, so a renamed key fails that decode
loudly instead of healing a defaulted row.
Paged rather than swept whole because inventory size is chain-controlled:
anyone who knows a watched address can keep sending dust to it, and a
periodic full-inventory read would let them decide how much a phone
allocates on every pass.
Classification queries carry the account tuple the STORE files each coin
under and the script it recorded. The engine checks that claim against
its own pools rather than trusting it, which is what lets it answer
`not-owned` — so a bare outpoint cannot ask the question.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Room `txos` mirror is write-behind with no feedback loop: a changeset
that fails to deliver an owned output leaves a permanent hole. Because
the engine is REBUILT from this mirror on restart, that hole graduates to
a fund loss on the next launch — observed in the field as the job-flower
106.43 -> 86.33 restart drop, where a rescan nondeterministically drops
the change outputs of sends funded from CoinJoin-account outputs.
Two bounded passes, sharing only the watch-only contact accounts they
both exclude:
* the insert pass walks the ENGINE's paged inventory and inserts the
rows the store lacks, one Room transaction per page. Insert-only and
idempotent, so a sweep is many small commits rather than one large
one. Only outputs at least `minConfirmations` deep are healed;
immature holes age into the next sweep. The engine's own isConfirmed
/ isInstantLocked / isCoinbase travel with each row and are stamped
as given — a coinbase output filed as an ordinary one would misstate
maturity in the mirror the engine reloads from.
* the classification pass is inverted: it pages the STORE and asks the
engine about one page at a time, so neither side builds a set over a
whole inventory. Every verdict is log-only. `known-uncredited` is the
one class that is positive evidence (the owning account knows the
funding txid, owns the script, does not hold the coin, and a funds
account holds a MINED record spending it), and even that is only
counted: flipping spent state is the one direction where a
reconciler bug spends a coin the user still owns. Acting on it is a
separate, reviewable change.
Watch-only DIP-15 contact coins are excluded on both sides — they are the
contact's money. The engine's inventory omits those accounts outright;
the store-side check resolves ownership through both the explicit account
link and the `core_addresses` projection production writes actually use,
because an accountId-only check silently classifies every contact row.
Triggered inside the SDK on the SPV SYNCED transition and every 30
minutes while synced, rather than left to each host: the hole it repairs
becomes a fund loss on the next engine reload if any host forgets to call
it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…eries
kotlinx's default Json omits a property equal to its default value, and
OutpointQuery defaulted standardTag/index/registrationIndex/keyClass to 0.
For the common BIP44 account-0 store row every one of those is 0, so the
query went out as {"typeTag":0,"txid":…,"vout":…,"scriptHex":…} and
rs-unified-sdk-jni's OutpointQuery (whose flattened UtxoAccountTuple
requires those fields) rejected every batch:
malformed outpoint queries: missing field `standardTag` at line 1 column 162
The reverse (store→engine) half of reconcileTxoStore therefore never
classified a single row on device. Drop the defaults so the fields are
always encoded; only the identity halves stay optional, matching the
native tuple. The test FakeEngine now rejects a batch missing any key the
native deserializer requires, which is why the existing reconcile tests
did not catch this.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The heal pass counted and logged healed coins whose transaction record
might carry a short netAmount. It was log-only and is not part of
#4638, which this branch ports; it can follow on its own.
The heal still never touches netAmount, and the tests keep checking that.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Register the persistence-extension slot
on_persist_wallet_changeset_utxo_verdicts_fn that #4638
added and the JNI layer left unset. The new trampoline packs the round's
verdicts into flat arrays (36-byte outpoint keys, verdict codes, observed-
spent heights) and calls NativePersistenceBridge.onWalletChangesetUtxoVerdicts,
whose default body ignores them, so a bridge that does not override it
writes exactly what it wrote before. The descriptor is in the bridge
verification table and pinned by a test.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ation
Kotlin port of #4638 Part 1. The handler stages the
round's credit verdicts (they arrive before the changeset callback) and
upsertUtxoRow applies them as Swift's upsertUtxo does:
- observed_spent / doomed: the row is written spent, with no spender link;
- any verdict vetoes the redelivery recovery clear (flag and sweep stamp);
- uncredited changes nothing else;
- verdicts live in the round buffer, so a rolled-back round leaves none.
One log line per round that applied a verdict, counts only. Tests port
BornSpentTxoPersistTests, including a reopened on-disk store restoring
zero coins for a verdict-spent row.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cargo fmt --check failed on the inventory/classifier bindings carried from
#4439 (f5fed8c). Formatting only.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… TXOs
Groundwork for the #4638 store reconcile, which asks the
engine about a page of store rows outside the callback lock and applies
the verdicts under it:
- committedRoundGeneration, bumped when a changeset round's Room
transaction commits (and on a standalone callback write), so a verdict
read before a round that may have re-credited the coin is refused;
- TxoDao.pageUnspentByWallet, the outpoint-cursor walk over a wallet's
isSpent = 0 rows that the classify pass pages through.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Brings PlatformWalletPersistenceHandler.reconcileTxos in line with Swift's
runCoreTxoReconcile from #4638 Part 2; the Kotlin pass
was carried over from #4439 and differed from it.
Heal pass (engine -> store), Swift's gates in Swift's order: malformed
row (txid not 32 bytes, no script, no address) is invalid; a coin the
engine does not call confirmed, or under 100 confirmations, is immature;
a coin already in the store is present; a coin whose seven-field account
tuple names no store account is refused (no longer inserted unowned).
Inserted rows go through upsertUtxoRow with isConfirmed = true, and a row
the pending-input drain wrote spent is counted as healedSpent. The
contact (DashpayExternalAccount) skip stays.
Classify pass (store -> engine): only the wallet's isSpent = 0 rows are
asked about, by outpoint cursor. A KnownUncredited verdict marks the row
spent (no spender link, lastUpdated bumped) and drops the pending-input
rows on it; Unspent, Unknown and NotOwned are counted only. Each page is
read with committedRoundGeneration, classified outside the callback
lock, and applied under it only if no round committed in between;
otherwise the page is re-read, at most 5 times in a row.
Every store step re-checks a caller cancellation flag under the lock.
A failed engine read, the first page included, now yields an incomplete
report instead of null; a failed store step rolls back and is counted.
The report mirrors Swift's CoreTxoReconcileReport, and the logs carry
counts and SHA-256 reference digests only: one summary line per run
("txos reconcile: mirror consistent (...)" when nothing changed) and one
item line per healed / healed-spent / flipped row.
Tests updated to the new semantics and ported from CoreTxoReconcileTests,
CoreTxoReconcileShutdownTests and CoreTxoReconcilePrivacyTests.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PlatformWalletManager.reconcileTxoStore(walletId) now follows Swift's
reconcileCoreTxoStore(for:) from #4638 and returns a
TxoReconcileOutcome: Skipped(reason) or Reconciled(report). Gates, in
order: manager closed; wallet not loaded; a run for it already in
flight; SPV not in steady state (SYNCED, or WAIT_FOR_EVENTS with the
filter phase at a non-zero target); no scan tip (filter height, else
header height); syncFaultDetected(); the wallet's durable watermark (the
Room wallet row's syncedHeight) more than 6 blocks behind the tip.
The automatic trigger runs on the rising edge into steady state and then
every 30 minutes per wallet, stamped at schedule time, never while that
wallet's run is in flight. closeSuspending() and removeWallet() bump a
reconcile epoch, and the run stops at its next page or store step with
completed = false; the run also holds a teardown-gate borrow so the
native manager is never destroyed under an engine read.
Skip, transport-failure and run-failure lines carry the reason or the
exception type and a wallet reference digest only; the cursor and native
error messages are no longer logged.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The reconcile skipped itself at the SYNCED edge of a restore
(WALLET_BEHIND_TIP) and, stamped at schedule time, did not try again for
30 minutes. The gate read the wallet's persisted Room syncedHeight, which
trails the engine by up to one persistence round — exactly the window the
SYNCED transition lands in.
Swift gates on the engine's own value (platform_wallet_core_wallet_state).
Bind it — one JNI export over that one FFI call — and gate on it, through
a pure TxoReconcileGates.walletCaughtUp with its own test.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.
No actionable comments were generated in the recent review.
Walkthrough
The change adds native TXO reconciliation scheduling, run gates, paging, classification, and held-engine-guard APIs. It exposes these APIs through FFI and JNI, and integrates guarded reconciliation into Kotlin and Swift persistence and wallet managers.
Changes: TXO Reconciliation
Layer / File(s)
Summary
Native reconciliation policy and lifecycle packages/rs-platform-wallet/src/manager/*, packages/rs-platform-wallet/src/spv/runtime.rs
Adds wallet scheduling, run gates, heal verdicts, ChainLock-qualified spend classification, held engine guards, stale-page retries, and lifecycle cleanup.
FFI, JNI, and persistence bridge transport packages/rs-platform-wallet-ffi/src/*, packages/rs-unified-sdk-jni/src/*, packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/*
Exposes reconciliation operations, serializes pages and queries, manages guard ownership, contains FFI panics, and forwards changeset UTXO credit verdicts.
Kotlin persistence and manager flow packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/*, packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/*
Swift reconciliation and persistence flow packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/*, packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/SDKLogger.swift
Uses native scheduling and verdict decisions, applies guarded pages with cancellation and stale-generation checks, and adds keyed log references.
Validation, fixtures, and schema migration packages/*/test*, packages/swift-sdk/SwiftTests/*, packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/*
sequenceDiagram
participant WalletManager
participant NativeReconciler
participant JNI
participant PersistenceHandler
WalletManager->>NativeReconciler: request due wallets and begin run
NativeReconciler-->>WalletManager: return gate and scan tip
WalletManager->>JNI: request heal page or classify outpoints
JNI->>NativeReconciler: read engine data
NativeReconciler-->>JNI: return page or classifications with held guard
JNI-->>WalletManager: return transport data and guard
WalletManager->>PersistenceHandler: apply results
WalletManager->>JNI: release held guard
WalletManager->>NativeReconciler: finish run
Loading
Merge Risk: Minimal · previous review 50e49
This change adds paged, ChainLock-qualified TXO reconciliation to the Kotlin and Swift SDKs, along with a Room v16 index to keep the paging queries fast. The reviewed material shows no unresolved concrete risk, and the Room migration, exported schema and migration tests are in place.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 74.58% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…
Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed checks: 4 passed
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title clearly identifies the Kotlin SDK port and the two main changes: save-time credit verdicts and post-sync TXO reconciliation. It is specific and related to the primary changeset.
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
Explanation
Docstring coverage is 74.58% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 118 functions across 34 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
Generate docstrings
Commit to this branch
Create a new PR
🧪 Generate unit tests (beta)
Commit to this branch
Create a new PR
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.
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
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.
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.
The reason will be displayed to describe this comment to others. Learn more.
Re-review — Final validation — Phase 1 + Phase 2
At a8752bb, the canonical merge fix corrects SQLite's restoration precedence, but mobile transaction callbacks still recreate the superseded spend, leaving one blocking persistence inconsistency. The nonblocking key-wallet capability follow-up also remains untracked; the other prior findings are addressed or superseded, and the previously withdrawn performance request remains withdrawn. Verification was static only: current CI passes the Swift frozen-schema check, while Rust workspace runner selection and the separate runner-selection check remain pending.
🔴 1 blocking | 🟡 1 suggestion(s)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Triage: critical by gpt-6.1-sol (effort low) — The intricate reconciliation and credit-verdict changes alter spendable-coin state in PlatformWalletPersistenceHandler.kt and manager/txo_reconcile.rs, while DashDatabase.kt also introduces a storage migration.
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/core_wallet_types.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/core_wallet_types.rs:674-677: Resolve restored-coin precedence in the canonical Rust changeset
(existing thread: https://github.com/dashpay/platform/pull/5259#discussion_r4439191040)
The merge correction removes the older explicit spend, but this transaction projection still sends the same spend through an independent channel. In `a_restored_coin_drops_an_older_spend_of_it_from_the_bucket`, the older InBlock spender remains in `cs.records`; `tx_record_to_ffi` copies every raw transaction input into `input_outpoints` without consulting `cs.restores`. JNI delivers transactions before added UTXOs, so Kotlin's `onWalletChangesetTransaction` links the restored coin to that spender and sets `isSpent=true`. The restored addition then preserves that state because `upsertUtxoRow` sees a spender link. Swift's `upsertTransaction`/`resolveInputOutpoint` and settled-spender recovery guard produce the same result. An initially missing TXO also fails: the transaction callback creates a pending input that the addition drains. Thus the regression's batch persists unspent in SQLite but spent in both mobile backends, excluding a live coin from subsequent restoration. Carry Rust's authoritative restored disposition across every spend-observation channel, including obsolete links and pending claims, without changing transaction bytes or original input indices. Extend coverage through the actual mobile callback sequence and reopened restore set; asserting only `utxos_spent_count == 0` does not establish backend agreement.
In `packages/rs-platform-wallet/src/wallet/platform_wallet_traits.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_wallet_traits.rs:281-294: Track finalized-spend retention in key-wallet
(existing thread: https://github.com/dashpay/platform/pull/5259#discussion_r4433815913)
Reading key-wallet's `input_details` removes ownership reconstruction, but Platform still mirrors `keep-finalized-transactions` and intercepts both ChainLock promotion and first-seen ChainLocked arrivals to retain finalized spend evidence. I verified the pinned `aca020263990` and supplied default `a75c9d08e912`: spent membership remains crate-private, and `ApplyChainLockOutcome` returns promoted transaction IDs rather than finalized input evidence. This leaves the adapter coupled to key-wallet's eviction lifecycle. Your reply correctly identifies the upstream owner; the remaining step is to link a narrowly scoped dashpay/rust-dashcore issue or PR for a finalized spent-outpoint query or equivalent finalization outcome that can replace this bookkeeping. The linked #992 discovery issue and #1196 snapshot proposal do not specify that capability, and the upstream searches did not reveal a dedicated replacement. This remains nonblocking, but needs a tracking reference before it can be treated as deferred.
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 the broader chain-truncation persistence mirror — The ChainTruncated projection still explicitly omits abandoned transaction removal or demotion and the complete persisted height rollback. These broader pre-existing gaps are distinct from the emitted-batch restoration inconsistency above and are already tracked by #5380.
Follow-up: Continue #5380 separately, covering abandoned records and outputs, linked spenders, height rollback, and restart behavior across persistence backends.
…ts link
A batch folding a spend of a coin and the truncation that gives it back
still carried the spender's record. Kotlin and Swift apply transactions
before coins, so its input linked the restored coin to the dropped
spender and marked it spent, and both hosts keep a linked row spent on
re-delivery. The same held for a link an earlier round had saved, and a
coin with no row yet drained the record's pending claim into that link.
The verdict slot now carries a restored code (4) for every coin in
CoreChangeSet::restored_utxos, and fires on rounds that only restore.
On it, Kotlin and Swift write the coin unspent with no spender link or
stamp and delete ordinary pending claims; a swept tombstone on a coin
the store never held still drains, as the SQLite placeholder holds.
Merge orders a restoration against a credit verdict on the same coin,
newest wins.
Tests drive the real callback order on both hosts (same-round spend,
pending claim, earlier-round link; Swift across a restart) and check the
restore set; the Kotlin ones fail with the handling disabled.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This reverts commit fb745f7, which added the (walletId, isSpent,
outpoint) index on txos and Room schema v16. The index was a
non-blocking suggestion already accepted as deferred, and a schema
version is one-way for installed apps, so it goes in a follow-up PR
stacked on this one instead of riding with this change.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
The reason will be displayed to describe this comment to others. Learn more.
Re-review — Final validation — Phase 1 + Phase 2
Most earlier corrections are present, but restored coins can still be persisted as spent through later sibling-account transaction callbacks or an older buffered sweep. Two nonblocking follow-ups also remain untracked: the key-wallet finalized-spend API and the Room pagination index. This was static validation of f70815e; build-preparation checks remain pending, and the supplied base-drift evidence reports no merge conflicts.
🔴 2 blocking | 🟡 2 suggestion(s)
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Triage: critical by gpt-6.1-sol (effort low) — The intricate cross-language changes directly alter coin spendability in PlatformWalletPersistenceHandler.upsertUtxoRow and the Rust wallet reconciliation and changeset logic, determining which restored outputs can be treated as spendable funds.
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`:
- [BLOCKING] packages/rs-platform-wallet/src/changeset/changeset.rs:721-725: Prevent an older sweep from spending a restored coin again
Restoration removes older `spent_utxos` entries and credit verdicts, but leaves older sweep holds intact. A reachable sequence is an already-persisted unconfirmed transaction L spending an output O whose funding is not yet recognized, a mined conflicting transaction W sweeping L, late arrival of O's funding, then truncation dropping W above the ChainLock. The pinned key-wallet retains the late-funded coin under W in `unsettled_spends` and can return O in `restored_outpoints`. When the sweep and restoration are buffered together, merge retains both `sweeps` and `restored_utxos`. Kotlin first writes the restored coin unspent, then `onChangesetEnd` runs `applySweepRound`; decoding L's stored inputs holds O again because the old sweep did not release the winner's input. SQLite has the same ordering: it applies `new_utxos` before `apply_sweep`, which sets that input spent. The resulting live coin is excluded from restoration, and insert-only healing does not repair its existing row. Resolve restoration precedence against older sweep holds in the canonical operation and propagate the effective disposition through the persistence consumers, while retaining loser deletion and genuinely newer spend effects. Cover sweep → late funding → truncation in one buffered drain and verify the reopened restore set.
In `packages/rs-platform-wallet-ffi/src/core_wallet_types.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/core_wallet_types.rs:688-692: Resolve restored-coin precedence in the canonical Rust changeset
(existing thread: https://github.com/dashpay/platform/pull/5259#discussion_r4439191040)
The canonical merge and RESTORED verdict fix direct spend precedence and transaction links written before the owning account's UTXO callback, but a later account callback can reverse that correction. `from_changeset` emits the full folded transaction into every involved account's bucket, while the restored coin appears only in its owner's bucket. For a buffered transaction spending account A's coin and paying account B, followed by truncation restoring A's input, the callback order can be A transaction → A restored UTXO → B transaction. The last callback processes every serialized input again: Kotlin's `onWalletChangesetTransaction` calls `linkSpender`, and Swift's `upsertTransaction` calls `resolveInputOutpoint`. Neither path consults the restored disposition, so the orphaned block transaction marks A's coin spent again, with no subsequent A UTXO callback to clear it. Both mobile restore sets can therefore omit a coin that the engine and canonical SQLite changeset retain. Make Rust's final restored disposition survive the entire wallet projection, including later account records—for example, deliver authoritative final dispositions after all account callbacks. Preserve transaction bytes and input indices, and add a two-account regression using the actual callback order followed by a reopened restore.
In `packages/rs-platform-wallet/src/wallet/platform_wallet_traits.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_wallet_traits.rs:281-294: Track finalized-spend retention in key-wallet
(existing thread: https://github.com/dashpay/platform/pull/5259#discussion_r4433815913)
Reading account-local `input_details` correctly removes ownership reconstruction, but Platform still predicts record eviction during `apply_chain_lock` and separately captures first-seen-ChainLocked transactions in `check_core_transaction`. Both the pinned aca020263990 and current upstream a75c9d08e912 keep the account's spent-membership query crate-private and expose no finality-qualified spent-outpoint result replacing these hooks. The owning project is rust-dashcore/key-wallet, where transaction finalization and spent-state retention already live. The linked snapshot proposal #1196 covers persistence of operational spent state, not this finalized-outpoint query or finalization-outcome contract. Link a dedicated upstream issue or PR before deferring this follow-up; the current adapter can remain until that capability is available.
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)
The revert returns Room to v15 and removes the composite index, leaving this automatic query without an index combining `walletId`, `isSpent`, and the outpoint cursor/order. SQLite can filter and sort a wallet's history or scan outpoints while filtering unrelated and spent rows; LIMIT bounds returned rows, not examined rows. `readUnspentPage` runs under `callbackExclusion`, so that work also delays persistence callbacks. The native 16-page budget bounds requests per run but not the database work needed for each request. Keeping the index and schema migration in a separate PR is reasonable, but the latest reply supplies no tracking link. Link the follow-up covering the `(walletId, isSpent, outpoint)` index, migration, and migration test before treating this suggestion as deferred.
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 the broader chain-truncation persistence operation — The existing ChainTruncated projection still cannot remove orphaned transaction/output rows, demote confirmations, or fully represent the durable scan rewind. That broader pre-existing operation is tracked separately in #5380; the newly added restored-coin precedence failures are covered by the blocking findings above.
Follow-up: Continue #5380 with atomic persistence and restart coverage across SQLite, Room, and SwiftData.
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.
… in a round
A coin a chain truncation restored could still end its round spent:
- Rust sends a transaction to every account it involves but the restored
coin only to its owner. On Kotlin and Swift, a later account's copy of
the dropped spender linked the spend again after the coin was written.
- A sweep folded into the same drain as the restoration held the coin
again from its loser's stored inputs, on every store. A sweep persisted
in an earlier round left a placeholder (SQLite) or tombstone (Room,
SwiftData) that kept the coin spent through its re-delivery.
The restoration is the wallet's last word on the coin: merge drops it when
a newer spend follows, and a newer sweep taking the coin spends a coin the
wallet holds, so it arrives with that spend. Each store now applies the
round's restored coins last, after every account's callbacks and every
sweep: unspent, unlinked, unstamped, with every pending claim on them
dropped. The restored write in the UTXO upsert also drops tombstones now.
Tests cover a sweep -> late funding -> truncation sequence in one drain and
across rounds on SQLite, Room and SwiftData, and a two-account transaction
in the order the callbacks arrive, each checked against a reopened restore
set and confirmed failing without the fix.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
The reason will be displayed to describe this comment to others. Learn more.
Re-review — Final validation — Phase 1 + Phase 2
At f80233a, the end-of-round restoration passes fix the previously reported account-callback and sweep ordering failures, but two remaining restoration-merging defects can persist an obsolete spend disposition or stale confirmation metadata. Three nonblocking suggestions remain concerning upstream finalized-spend ownership, Room pagination indexing, and a temporary allocation during finalization. This was static review only: current Rust workspace, Kotlin SDK, Swift SDK build/tests, and Swift frozen-schema checks pass; diff hygiene is clean, and the supplied base-drift analysis reports no merge conflicts.
🔴 2 blocking | 🟡 3 suggestion(s)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Triage: critical by gpt-6.1-sol (effort low) — The intricate cross-language changes in PlatformWalletPersistenceHandler.kt (upsertUtxoRow) and rs-platform-wallet/src/manager/txo_reconcile.rs directly change which coins are persisted or restored as spendable, affecting coin selection and funds safety.
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`:
- [BLOCKING] packages/rs-platform-wallet/src/changeset/changeset.rs:735-743: Retire restoration overrides for newer record-only spends
A newer spend does not necessarily appear in other.spent_utxos: build_core_changeset populates that list from BlockProcessed.inserted, but not updated, even though updated records retain input_details and both transaction callbacks and the FFI interpret those inputs as spends. A reachable sequence is to project a truncation restoring O, then detect a new mempool spender T, and confirm and ChainLock T before its queued TransactionDetected is projected. Pinned key-wallet retains the confirmation record in the block event but prunes its live record; the detection projection consequently reads an empty slice, while the block projection contains T only in updated. Neither projection retires O's restoration here. The resulting batch carries a genuinely newer spender together with an obsolete restoration: from_changeset suppresses T's debit through cs.restores, and the final mobile restoration passes erase the spender link, persisting an engine-consumed coin as unspent. Resolve restoration lifetime against newer record input evidence as well as explicit spent_utxos, in the canonical Rust changeset/bridge. Add a detection → confirmation → pruning regression after restoration and verify the reopened restore set.
In `packages/rs-platform-wallet-ffi/src/core_wallet_types.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/core_wallet_types.rs:688-692: Do not overwrite newer confirmation data with a retained restoration snapshot
Restoration entries are appended after record-derived UTXOs, but CoreChangeSet::merge retains their old payload when a newer changeset only updates the funding record. For example, pinned key-wallet's truncate_above can demote an InstantSend-locked funding transaction to unconfirmed while dropping its non-locked spender, restoring the coin with height 0 and is_confirmed false. If that funding transaction is re-mined before the batch commits, BlockProcessed.updated carries its new confirmation height and flags, without replacing the retained restoration payload. This projection emits the correctly confirmed UTXO first and the stale restored snapshot last. Both Room's upsertUtxoRow and Swift's upsertUtxo assign height and confirmation flags directly, so the stale snapshot wins; the end-of-round passes only repair spend disposition, and insert-only healing does not update the existing row. Refresh surviving restoration payloads from newer observations in canonical Rust, or separate restoration disposition from mutable coin metadata. Add a restoration → funding re-confirmation merge/projection regression that checks height and flags after reopening.
In `packages/rs-platform-wallet/src/wallet/platform_wallet_traits.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_wallet_traits.rs:328-341: Stream finalized outpoints directly into the retained set
Both production callers immediately pass owned_spends into finalized_spends.extend, but this helper first materializes every matching input in a temporary Vec. The apply_chain_lock caller spans the entire promotion batch, not a bounded reconciliation page, so a large promotion temporarily holds another copy of all those outpoints while the engine write lock is held. Return an iterator and let HashSet::extend consume it directly; the test that sorts the result can collect explicitly. The existing core_bridge::spent_outpoints helper already implements the same checked input_details traversal and can be reused after filtering contact-owned records.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_wallet_traits.rs:281-294: Track finalized-spend retention in key-wallet
(existing thread: https://github.com/dashpay/platform/pull/5259#discussion_r4433815913)
Reading key-wallet's input_details correctly removes ownership re-derivation, but Platform still mirrors record eviction here and in check_core_transaction, including the keep-finalized-transactions condition, to maintain finalized_spends. In both pinned aca020263990 and upstream a75c9d08e912, spent_outpoints remains private and is_outpoint_spent remains crate-private; that predicate also lacks the finality qualification this classifier needs. The replacement therefore belongs in dashpay/rust-dashcore: finalized spent-outpoint membership or finalized owned-input evidence in the finalization outcome. The latest reply leaves filing that capability undecided, and neither the linked input-discovery issue #992 nor snapshot proposal #1196 specifies this query/outcome contract. Link a focused upstream issue or PR before treating this ownership follow-up as tracked deferral. This remains nonblocking; it does not require moving the implementation into a binding.
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)
The revert leaves Room at version 15 and TxoEntity without an index combining walletId, isSpent, and outpoint. SQLite therefore cannot seek directly through this wallet's unspent cursor range using one matching index: it must filter an outpoint walk or filter and sort wallet history. Each read holds callbackExclusion, so growing transaction history can delay persistence callbacks even though LIMIT bounds returned rows. Rust's 16-page budget bounds calls, not the rows examined or sorted within each call. Keeping the migration in a separate PR is a reasonable nonblocking scope decision, but the latest reply identifies no tracking issue or PR. Link the follow-up carrying the composite index, Room migration, exported schema, and migration coverage before treating the deferral as tracked.
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 durable chain-truncation handling under platform#5380 — The new restored-coin path addresses one truncation consequence, but the ChainTruncated bridge still explicitly leaves orphan transaction/output removal, confirmation demotion, and durable watermark rollback unimplemented. This broader persistence gap predates this PR and is already concretely tracked by #5380.
Follow-up: Continue #5380 with canonical truncation changesets, atomic application across SQLite, Room, and SwiftData, and restart coverage.
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.
… refresh it from a newer funding
- BlockProcessed lists the spends of its updated records in spent_utxos
- merge drops a restoration when a newer record's inputs spend the coin
- merge refreshes a restored coin's height and confirmation flags from a newer funding record
- owned_spends returns an iterator and reuses core_bridge::spent_outpoints
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
The reason will be displayed to describe this comment to others. Learn more.
Re-review — Final validation — Phase 1 + Phase 2
At head 084e421, the 49 prior findings reconcile to 46 fixed, one withdrawn, and two still-valid suggestions. One additional blocker is confirmed: a later truncation can remove a restored coin's funding without invalidating its newly spendable persisted disposition. This was a static review; current CI reports Rust workspace tests and Kotlin emulator build/tests passing, with Swift build/tests still pending and the frozen-schema check passing.
🔴 1 blocking | 🟡 2 suggestion(s)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Triage: critical by gpt-6.1-sol (effort low) — The intricate changes in PlatformWalletPersistenceHandler.kt and rs-platform-wallet/src/manager/txo_reconcile.rs alter which coins are persisted and restored as spendable, directly affecting coin-selection eligibility through credit verdicts and spent-state reconciliation.
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`:
- [BLOCKING] packages/rs-platform-wallet/src/changeset/core_bridge.rs:1886-1895: Invalidate restorations when a later truncation removes their funding
The new restoration disposition survives a later removal of its funding. With no intervening ChainLock, observe an input-only spender at height 200, persist its funding F at 100 as ObservedSpent, truncate to 150, and project the returned coin. Then truncate to 90. In pinned key-wallet, ManagedCoreFundsAccount::truncate_above calls apply_abandon, removing F and its restored UTXO; that second truncation names F in txids but has no restored_outpoints. This arm ignores txids and produces an empty changeset, so it neither cancels the pending restoration/new_utxos entry nor removes an already-persisted restored coin. SQLite's apply_restorations and the mobile final restoration passes therefore leave the previously excluded output spendable after reopening, although the engine no longer has it. Classification cannot repair this because missing funding yields Unknown. This newly changes a spent row into a spendable phantom, rather than merely retaining the pre-existing orphaned history covered by #5380. Represent ordered funding-removal invalidations in the canonical Rust persistence operation, covering both buffered restorations and restorations committed in an earlier round. Add the two-truncation sequence with reopened-store assertions.
In `packages/rs-platform-wallet/src/wallet/platform_wallet_traits.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_wallet_traits.rs:280-294: Track finalized-spend retention in key-wallet
(existing thread: https://github.com/dashpay/platform/pull/5259#discussion_r4433815913)
Reading input_details and streaming the capture fixes the ownership inference and temporary allocation, but Platform still predicts key-wallet's record eviction and intercepts both ChainLock promotion and already-ChainLocked arrivals to maintain finalized_spends. The pinned and supplied default revisions of rust-dashcore keep the relevant spent-outpoint query crate-private, so the adapter cannot consume a public finalized-spend result instead. As your reply acknowledges, that capability belongs in key-wallet. Link a rust-dashcore issue or PR for exposing finalized spent-outpoint evidence through ManagedCoreFundsAccount/ManagedWalletInfo or finalization outcomes. The snapshot proposal #1196 does not specify that consumer-facing API, and the supplied discussion leaves this follow-up untracked. This is nonblocking and does not require removing the working adapter here.
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)
The revert leaves Room at v15 with only separate TXO indexes, while this automatic query combines walletId and isSpent predicates with an outpoint cursor and ordering. SQLite can repeatedly filter and sort a wallet's history or scan outpoint entries that fail those predicates; LIMIT bounds returned rows, not the work within each page. Because readUnspentPage runs under callbackExclusion, that work also delays persistence callbacks, and the Rust sixteen-page budget does not bound an individual SQL query. Keeping the migration separate is reasonable, but your latest reply supplies no tracking reference. Link the follow-up for the (walletId, isSpent, outpoint) index and its migration/schema coverage, or include that work here.
The reason will be displayed to describe this comment to others. Learn more.
🔴 Blocking: Invalidate restorations when a later truncation removes their funding
The new restoration disposition survives a later removal of its funding. With no intervening ChainLock, observe an input-only spender at height 200, persist its funding F at 100 as ObservedSpent, truncate to 150, and project the returned coin. Then truncate to 90. In pinned key-wallet, ManagedCoreFundsAccount::truncate_above calls apply_abandon, removing F and its restored UTXO; that second truncation names F in txids but has no restored_outpoints. This arm ignores txids and produces an empty changeset, so it neither cancels the pending restoration/new_utxos entry nor removes an already-persisted restored coin. SQLite's apply_restorations and the mobile final restoration passes therefore leave the previously excluded output spendable after reopening, although the engine no longer has it. Classification cannot repair this because missing funding yields Unknown. This newly changes a spent row into a spendable phantom, rather than merely retaining the pre-existing orphaned history covered by #5380. Represent ordered funding-removal invalidations in the canonical Rust persistence operation, covering both buffered restorations and restorations committed in an earlier round. Add the two-truncation sequence with reopened-store assertions.
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.
This PR grew from a Kotlin port of #4638 into changes across the Rust wallet, the FFI and Swift, which made it hard to review to a finish. #5767 starts over as a Kotlin-only port: JNI bindings and Kotlin code that mirror Swift's reconcile as it stands in v5.1-dev, with the in-scope review fixes from this PR carried over.
The engine-level fixes made here (restored coins after a chain truncation; verdicts only for ChainLocked spenders) apply to Swift as well, so they will follow as their own PRs. This branch stays as the source for them.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
github-actions
Bot
added
waiting-self-review
thepastaclaw
left a comment
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 thereconcile only exists in #4439, which cannot build on
v5.1-devbecause its address-pool hole repair needsrust-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?
wallet_utxos_pageandclassify_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.rules):
its target) and every 30 minutes per wallet.
behind the scan tip; a run for the wallet already in flight. Closing the manager or removing the wallet stops
a run between pages.
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.
only on
KnownUncredited(a mined spender on record), and drops pending-input claims on it. The page is readwith a count of committed persistence rounds; if a round committed before the apply, nothing is written and
the page is classified again.
walletManagerCoreWalletSyncedHeight: a single call toplatform_wallet_core_wallet_state, so the reconcile gates on the engine's watermark as Swift does. Thepersisted copy in Room trails it by a round, which is where the SYNCED transition lands.
rs-unified-sdk-jniregisterson_persist_wallet_changeset_utxo_verdicts_fnand passes the round'sverdicts to
NativePersistenceBridge.onWalletChangesetUtxoVerdictsas flat arrays, before the changesetcallback.
PlatformWalletPersistenceHandlerkeeps them in the round's buffer.upsertUtxoRowwrites anobserved_spent/doomedoutput spent at creation, with no spender link. Any verdict vetoes theredelivery recovery clear (both the flag and the sweep stamp).
uncreditedchanges nothing else. Arolled-back round leaves no verdict behind.
log. Both can follow separately.
Kotlin-specific choices, same behaviour:
open round.
How Has This Been Tested?
On this branch (
v5.1-dev, rust-dashcoree4208c90, no #979):cargo test -p platform-wallet --libcargo test -p platform-wallet-ffi --libcargo test -p rs-unified-sdk-jnicargo clippy … -D warningscargo fmt --check--rerun-tasks)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,CoreTxoReconcileShutdownTestsandCoreTxoReconcilePrivacyTests:separately;
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:
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.
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.
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
Rust only:
platform_wallet::changeset::CoreChangeSetgains a publicrestored_utxosfield. The struct has public fields and is not#[non_exhaustive], so code that builds it by listing every field, or destructures it without.., must addrestored_utxos(an emptyVeckeeps the old behaviour) or use..Default::default()/... Serialized changesets without the field still decode (serde(default)). NoPlatformVersionchange.Kotlin and Swift: none. One additive bridge method with a no-op default, one additional extension slot wired, and
SDKLogValuekeeps its cases.Checklist
🤖 Generated with Claude Code
Summary by CodeRabbit