fix(kotlin-sdk): reconcile the TXO store against the engine and repair restored address pools - #4439
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
|
⛔ Blockers found — Opus deferred (commit f0f632e) |
HashEngineering
changed the title
Temporary pin to the head of dashpay/rust-dashcore#<PR> (late-knowledge corrections: record re-emission, durable pending-sweep, address-pool repair, born-spent attribution, spent-outpoints accessor) so this branch builds in CI. Re-pin to the dev merge commit once that PR lands. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…engine's UTXO inventory The Room txos mirror is write-behind with no feedback loop: a changeset that fails to deliver an owned output leaves a permanent hole, and the engine is REBUILT from the mirror on restart (buildUtxoRestoreData), so the hole graduates to a fund-loss on the next launch. Observed on job-flower as the 106.43 -> 86.33 restart drop: the rescan nondeterministically drops the change outputs of sends funded from CoinJoin-account outputs (all three known restores dropped the three Aug 5 change outputs; one of three also dropped the Aug 15 one). - walletManagerAllUtxosJson (JNI): the engine's full per-account UTXO inventory as JSON — account_balances sweep to enumerate accounts, platform_wallet_account_utxos per account, address derived from the script; per-account faults reported in-band so one bad account cannot mask the others' repair. - PlatformWalletManager.reconcileTxoStore / handler.reconcileTxos: insert-only diff of that inventory against Room — never flips spend state, never deletes (the mirror may legitimately be ahead on live spends and carries watch-only contact outputs). 100-conf gate because the snapshot cannot carry isCoinbase/isInstantLocked; fresher holes age into the next sweep. - netAmount repair: a record born blind to its own change output persisted netAmount short by exactly that value (verified: 6cef55ab stored -10.00010000 vs true -0.11000227); credit it back when the transaction row pre-exists with real bytes. - onWalletChangesetUtxoAdded body extracted to upsertUtxoRow so the callback and the reconciler share one insert discipline (stub tx FK row + pending-input drain). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…core changeset A gap-limit rescan correction (rust-dashcore fix/key-wallet-rescan-changeset) arrives as a BlockProcessed *updated* record whose output roles flipped from Sent to Received/Change. Deriving new/spent UTXOs from inserted records only delivered the corrected row but left the store's TXO hole in place — the reload fund-loss shape. Ordinary re-confirmations re-emit the same UTXOs, which the persisters absorb idempotently (upsertUtxoRow preserves spend linkage; spend-first outputs are flipped by the deferred-input drain). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tore restore_core_address_pools ingested the persisted address rows as-is: a mirror that dropped rows (observed in the field: BIP44-change indices 875..=890 absent between surviving rows) produced an in-memory pool with holes, and the row-derived highest_generated suppressed the gap-limit re-derivation that would have filled them. Outputs paying the missing addresses were permanently unrecognizable — the reason a blockchain rescan could not recover funds a fresh seed-restore could (the rescan rebuilds pools from the store; a fresh restore derives them from the seed). The loader now resolves each pool's key source from the signing wallet (built from the persisted account xpubs a few lines earlier) and calls AddressPool::ensure_contiguous_to after row ingestion: every missing index up to the persisted watermark is re-derived, existing rows and used flags untouched. Unresolvable key sources and hardened pools skip the repair and restore exactly as before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ngine disagrees with Review feedback on the reconcile (Layer 1): the insert-only pass misses other producers of the same store-divergence class. The engine inventory export now carries both halves (unspent UTXOs + spent outpoints, via a new platform_wallet_account_spent_outpoints FFI), and the reconcile adds a reverse pass classifying every store row: - store-unspent, engine-spent: the row lost its spend update (dashpay#4425) — flipped to spent in place. The only mutation in the reverse pass; worst-case error hides a coin the next reconcile re-inserts, and four upstream layers now keep the post-SYNCED engine trustworthy. - store-unspent, engine-unknown: swept/abandoned residue (pre-rust-dashcore#971 stores) — LOG-ONLY, counted and named, never removed. Removal by reconciliation is the one direction where a bug destroys user-visible data. - store-spent, engine-unspent: lost release event, or a live spend racing the engine's map — indistinguishable at reconcile time, and un-marking a coin mid-payment would let the wallet double-spend it. LOG-ONLY. - Watch-only DIP-15 contact rows are excluded up front: the engine's accounts never report them, so their absence is expected, not divergence. Five new handler tests pin the flip, the never-remove, the never-unmark, the young-coin consistency case, and the contact-row exclusion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
590ee97 to
ac7c9b2
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The reconciliation and restore repair target real persistence divergence, but four correctness defects can durably mark unconfirmed coins spent, repeatedly report or credit an insertion that never occurred, corrupt an already-correct transaction amount, and leave DashPay receiving pools sparse. Three additional in-scope issues weaken contact-row classification, make the periodic inventory scan quadratic in account count, and bypass the canonical outpoint conversion.
Source: Codex reviewer lanes codex-general, codex-rust-quality, and codex-ffi-engineer (exact backend model IDs were not supplied in the evidence); final verifier backend grok-4.5; orchestration-only openclaw-agent/cliproxy/gpt-5.6-sol is not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 4 blocking | 🟡 3 suggestion(s)
🤖 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:1345-1352: Do not persist unconfirmed inputs as spent
The exact pinned key-wallet revision inserts every input of a recorded transaction into `spent_outpoints`, regardless of whether its context is mempool or in-block. Treating membership in that set as a confirmed spend contradicts this handler's established rule at lines 928-934 and 3073-3080: mempool-linked inputs remain unspent in Room so they can be restored and reclassified after restart. A reconciliation while a payment is unconfirmed therefore makes the coin durably spent; if the transaction is later abandoned and its release update is lost or interrupted, the deliberate never-unmark policy prevents every later reconciliation from recovering it. Export spender context/finality with each outpoint, or only flip rows for spends proven confirmed by another authoritative source.
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt:1288-1301: Only account for TXOs that were actually inserted
`upsertUtxoRow` returns early when the parent transaction is still marked `isGloballySwept`, but it returns `Unit`, so this caller cannot distinguish that refusal from a successful insert. The reconciler then increments `inserted`, adds to `insertedDuffs`, and may update `netAmount` even though the TXO remains absent. A missed reinstatement record can leave exactly this stale tombstone while the engine authoritatively holds the output; every periodic pass then repeats the false heal and can repeatedly add the same amount. Make the helper report whether it materialized the row and perform all counters and amount repair only after a successful insert, or explicitly reconcile the stale swept state first.
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt:1298-1301: Do not infer netAmount from TXO presence
A missing `txos` projection does not prove that the independently persisted transaction record omitted the output from `netAmount`. For example, a corrective transaction callback can already store the engine's recomputed net amount while delivery of the corresponding UTXO projection is omitted, or a TXO can disappear later without changing its parent transaction. In either case this unconditional delta overstates the transaction, and the newly inserted row makes the corruption permanent because later passes become no-ops. The inventory does not include an authoritative expected net amount, so repair must compare against one or recompute the amount from authoritative ownership data rather than infer it from row absence.
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt:1321-1340: Resolve contact ownership through coreAddressId
The exclusion only checks `row.accountId`, but production changeset writes leave that field null and route TXO ownership through `coreAddressId -> core_addresses.accountId`, as documented by `buildUtxoRestoreData` at lines 3065-3069. The regression test manually fills `accountId`, so it does not represent production contact rows. Resolve the effective account through `coreAddressId` when the direct FK is null in both reverse-pass loops; otherwise normal DIP-15 contact outputs are repeatedly reported as engine-unknown and defeat the intended regression-tripwire signal.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:4085-4113: Repair DashPay pools with their concrete account xpub
Both DashPay variants reach this resolver, but the exact pinned implementation of `Wallet::key_source_for_account_type` explicitly returns `NoKeySource` for `DashpayReceivingFunds` and `DashpayExternalAccount`. The guard at line 4131 therefore always skips their hole repair. This is especially harmful for `DashpayReceivingFunds`: it is wallet-owned and funds-bearing, and its concrete account already carries the xpub needed to reconstruct missing addresses. Resolve the full `AccountType` through `wallet.accounts.account_of_type(account_type)` and use that account's xpub as `KeySource::Public`, retaining the existing helper as the fallback for special account types such as the BLS provider account. Add a restore test with a sparse DashPay receiving pool and a real signing wallet; the current test passes `None` and cannot exercise this path.
In `packages/rs-unified-sdk-jni/src/wallet_manager.rs`:
- [SUGGESTION] packages/rs-unified-sdk-jni/src/wallet_manager.rs:3205-3207: Snapshot all account inventories in one pass
The JNI method enumerates all N accounts and then invokes separate per-account UTXO and spent-outpoint FFI accessors. Each accessor reacquires `wallet_manager.blocking_read()`, allocates `all_accounts()`, and linearly searches that N-element collection, making one full reconciliation O(N²) with 2N+1 lock acquisitions. DashPay creates accounts per contact, so this periodic scan scales with wallet history and runs both at sync completion and every 30 minutes. Add a manager-level snapshot that gathers account identity, UTXOs, and spent outpoints under one read lock, then expose it through one FFI operation.
In `packages/rs-platform-wallet-ffi/src/manager_diagnostics.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/manager_diagnostics.rs:655-661: Use the canonical OutPointFFI conversion
`OutPointFFI` already implements `From<&dashcore::OutPoint>` and documents that implementation as the single authority for preserving the persistence join key and txid byte order. The new export manually duplicates the conversion. It is equivalent today but can silently diverge if the canonical representation changes, so route this export through the existing conversion boundary.
| key in engineSpentKeys -> { | ||
| // Lost spend update (#4425): the engine knows this | ||
| // coin was spent; the row missed the flip. Flip in | ||
| // place — spendingTxid stays as-is (usually null; | ||
| // the spender's row, if it ever arrives, relinks | ||
| // via the deferred-input drain). | ||
| database.txoDao().upsert(row.copy(isSpent = true)) | ||
| flippedSpent++ |
There was a problem hiding this comment.
🔴 Blocking: Do not persist unconfirmed inputs as spent
The exact pinned key-wallet revision inserts every input of a recorded transaction into spent_outpoints, regardless of whether its context is mempool or in-block. Treating membership in that set as a confirmed spend contradicts this handler's established rule at lines 928-934 and 3073-3080: mempool-linked inputs remain unspent in Room so they can be restored and reclassified after restart. A reconciliation while a payment is unconfirmed therefore makes the coin durably spent; if the transaction is later abandoned and its release update is lost or interrupted, the deliberate never-unmark policy prevents every later reconciliation from recovering it. Export spender context/finality with each outpoint, or only flip rows for spends proven confirmed by another authoritative source.
source: ['codex']
There was a problem hiding this comment.
Resolved in 2195b18 — Do not persist unconfirmed inputs as spent no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| upsertUtxoRow( | ||
| database, walletId, txid, vout, amount, address, scriptPubKey, | ||
| height, | ||
| isCoinbase = false, | ||
| isConfirmed = true, | ||
| isInstantLocked = false, | ||
| isLocked = isLocked, | ||
| ) | ||
| inserted++ | ||
| insertedDuffs += amount | ||
| if (priorTx != null && priorTx.transactionData.isNotEmpty()) { | ||
| if (database.transactionDao().addToNetAmount(txid, amount) > 0) { | ||
| netAmountRepairs++ | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Only account for TXOs that were actually inserted
upsertUtxoRow returns early when the parent transaction is still marked isGloballySwept, but it returns Unit, so this caller cannot distinguish that refusal from a successful insert. The reconciler then increments inserted, adds to insertedDuffs, and may update netAmount even though the TXO remains absent. A missed reinstatement record can leave exactly this stale tombstone while the engine authoritatively holds the output; every periodic pass then repeats the false heal and can repeatedly add the same amount. Make the helper report whether it materialized the row and perform all counters and amount repair only after a successful insert, or explicitly reconcile the stale swept state first.
source: ['codex']
There was a problem hiding this comment.
Resolved in 2195b18 — Only account for TXOs that were actually inserted no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| if (priorTx != null && priorTx.transactionData.isNotEmpty()) { | ||
| if (database.transactionDao().addToNetAmount(txid, amount) > 0) { | ||
| netAmountRepairs++ | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Do not infer netAmount from TXO presence
A missing txos projection does not prove that the independently persisted transaction record omitted the output from netAmount. For example, a corrective transaction callback can already store the engine's recomputed net amount while delivery of the corresponding UTXO projection is omitted, or a TXO can disappear later without changing its parent transaction. In either case this unconditional delta overstates the transaction, and the newly inserted row makes the corruption permanent because later passes become no-ops. The inventory does not include an authoritative expected net amount, so repair must compare against one or recompute the amount from authoritative ownership data rather than infer it from row absence.
source: ['codex']
There was a problem hiding this comment.
Resolved in 2195b18 — Do not infer netAmount from TXO presence no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| let key_source = signing_wallet | ||
| .and_then(|wallet| { | ||
| key_wallet::transaction_checking::transaction_router::AccountTypeToCheck::try_from( | ||
| &*managed_type, | ||
| ) | ||
| .ok() | ||
| .map(|check_type| { | ||
| let account_index = match &account_type { | ||
| AccountType::Standard { | ||
| index, .. | ||
| } | ||
| | AccountType::CoinJoin { | ||
| index, | ||
| } | ||
| | AccountType::DashpayReceivingFunds { | ||
| index, .. | ||
| } | ||
| | AccountType::DashpayExternalAccount { | ||
| index, .. | ||
| } => Some(*index), | ||
| AccountType::IdentityTopUp { | ||
| registration_index, | ||
| } => Some(*registration_index), | ||
| _ => None, | ||
| }; | ||
| wallet.key_source_for_account_type(&check_type, account_index) | ||
| }) | ||
| }) | ||
| .unwrap_or(key_wallet::KeySource::NoKeySource); |
There was a problem hiding this comment.
🔴 Blocking: Repair DashPay pools with their concrete account xpub
Both DashPay variants reach this resolver, but the exact pinned implementation of Wallet::key_source_for_account_type explicitly returns NoKeySource for DashpayReceivingFunds and DashpayExternalAccount. The guard at line 4131 therefore always skips their hole repair. This is especially harmful for DashpayReceivingFunds: it is wallet-owned and funds-bearing, and its concrete account already carries the xpub needed to reconstruct missing addresses. Resolve the full AccountType through wallet.accounts.account_of_type(account_type) and use that account's xpub as KeySource::Public, retaining the existing helper as the fallback for special account types such as the BLS provider account. Add a restore test with a sparse DashPay receiving pool and a real signing wallet; the current test passes None and cannot exercise this path.
source: ['codex']
There was a problem hiding this comment.
Resolved in 2195b18 — Repair DashPay pools with their concrete account xpub no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| for (row in storeRows) { | ||
| if (!row.isSpent) continue | ||
| if (row.accountId != null && row.accountId in foreignAccountIds) continue | ||
| val key = "${row.txid?.toHex() ?: continue}:${row.vout}" | ||
| if (key in engineUnspentKeys) { | ||
| stuckSpent++ | ||
| stuckSpentDuffs += row.amount | ||
| Log.w( | ||
| TAG, | ||
| "txos reconcile: store row spent but engine lists it " + | ||
| "unspent outpoint=$key amount=${row.amount} — LOG-ONLY " + | ||
| "(lost release, or a live spend racing the engine)", | ||
| ) | ||
| } | ||
| } | ||
| val storeUnspent = storeRows.filter { !it.isSpent } | ||
| for (row in storeUnspent) { | ||
| if (row.accountId != null && row.accountId in foreignAccountIds) { | ||
| skippedForeign++ | ||
| continue |
There was a problem hiding this comment.
🟡 Suggestion: Resolve contact ownership through coreAddressId
The exclusion only checks row.accountId, but production changeset writes leave that field null and route TXO ownership through coreAddressId -> core_addresses.accountId, as documented by buildUtxoRestoreData at lines 3065-3069. The regression test manually fills accountId, so it does not represent production contact rows. Resolve the effective account through coreAddressId when the direct FK is null in both reverse-pass loops; otherwise normal DIP-15 contact outputs are repeatedly reported as engine-unknown and defeat the intended regression-tripwire signal.
source: ['codex']
There was a problem hiding this comment.
Resolved in 2195b18 — Resolve contact ownership through coreAddressId no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| if !entries.is_null() && count > 0 { | ||
| let accounts = unsafe { std::slice::from_raw_parts(entries, count) }; | ||
| for acc in accounts { |
There was a problem hiding this comment.
🟡 Suggestion: Snapshot all account inventories in one pass
The JNI method enumerates all N accounts and then invokes separate per-account UTXO and spent-outpoint FFI accessors. Each accessor reacquires wallet_manager.blocking_read(), allocates all_accounts(), and linearly searches that N-element collection, making one full reconciliation O(N²) with 2N+1 lock acquisitions. DashPay creates accounts per contact, so this periodic scan scales with wallet history and runs both at sync completion and every 30 minutes. Add a manager-level snapshot that gathers account identity, UTXOs, and spent outpoints under one read lock, then expose it through one FFI operation.
source: ['codex']
| let entries: Vec<OutPointFFI> = rows | ||
| .into_iter() | ||
| .map(|op| OutPointFFI { | ||
| txid: txid_to_array(&op.txid), | ||
| vout: op.vout, | ||
| }) | ||
| .collect(); |
There was a problem hiding this comment.
🟡 Suggestion: Use the canonical OutPointFFI conversion
OutPointFFI already implements From<&dashcore::OutPoint> and documents that implementation as the single authority for preserving the persistence join key and txid byte order. The new export manually duplicates the conversion. It is equivalent today but can silently diverge if the canonical representation changes, so route this export through the existing conversion boundary.
| let entries: Vec<OutPointFFI> = rows | |
| .into_iter() | |
| .map(|op| OutPointFFI { | |
| txid: txid_to_array(&op.txid), | |
| vout: op.vout, | |
| }) | |
| .collect(); | |
| let entries: Vec<OutPointFFI> = rows.iter().map(OutPointFFI::from).collect(); |
source: ['codex']
There was a problem hiding this comment.
Resolved in 2195b18 — Use the canonical OutPointFFI conversion no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
|
@bfoss765 — this is the store-reconcile series from the job-flower investigation, retargeted off the integration branch per review: it now stacks on #4406 (whose swept-tombstone semantics the shared 🤖 Generated with Claude Code |
|
Heads-up on the base, since this is stacked on Two changes are landing on that branch that touch files you also touch, so it is worth knowing before you build further on them. Already landed ( Coming next, and this one does overlap you: the swept-tombstone lifetime rule is being reworked. The current version stamps a tombstone with the height at which the sweep was observed and collects it after a fixed margin; that is unsound for an InstantSend-locked winner that stays unmined, so it is being replaced with the winner's actual mined height, carried on the event by dashpay/rust-dashcore#975. Concretely, on this branch that will change:
It also needs a repin once #975 merges. Room schema v13 ( No action needed from you — the base moves under you and your PR keeps rebasing — but if you are about to write anything in the tombstone or |
…ot prove thepastaclaw review round on dashpay#4439, all four blockers: - The spent-flip is demoted to LOG-ONLY (wouldFlipSpent): the engine's spent set records every input of every recorded transaction including MEMPOOL spends, with no context — persisting the flip would settle an unconfirmed spend, contradicting this handler's own in-block gating. Re-arm as a mutation only when the engine exports spends with context. - upsertUtxoRow reports whether it wrote: a globally-swept-parent refusal is now visible to the reconcile (skippedSwept), which no longer counts phantom heals nor flags netAmounts for rows that were never inserted. - The netAmount repair is demoted to LOG-ONLY (netAmountSuspects): a corrective record callback can land while its TXO delivery races this sweep, and blind addition double-credits. The event pipeline owns net correctness; the reconcile reports the suspicion. - The restore-time pool repair announces every pool it cannot repair (DashPay contact pools have no public key source by design and re-derive through DashPay sync; hardened pools cannot be publicly derived). Plus the review suggestions: contact-row exclusion now resolves ownership through coreAddressId -> core_addresses.accountId (production rows leave txos.accountId null, so the accountId-only check was ineffective), the neither-inventory log names the finalized-drop ambiguity, and the JNI spent-outpoint export uses the canonical OutPointFFI conversion. The reconcile is now fully observe-and-heal-forward: its only mutation is inserting provably-owned engine UTXOs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Review round addressed in 2195b18 — all four blockers plus the suggestions:
Suggestions: contact-row exclusion now resolves ownership through 117 handler tests green (all reconcile semantics re-pinned to log-only behavior), FFI 277 green. @romchornyi thanks for the base heads-up — will take the tombstone-lifetime rework and the #975 repin as a merge round here when it lands, and expect the v13 schema renumber. 🤖 Generated with Claude Code |
…68e652) Carries the wallet-scope born-spent attribution, the rescan reseed fix, and the repair-span bound into this branch's engine. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Device validation of this branch exactly as pushed (f0f632e + the rust-dashcore#979 pin): full uninstall → seed restore → rescan → kill+relaunch on the CoinJoin-heavy testnet wallet. Reconcile reported One composition note for reviewers: transaction-history net sums only converge with #4438 landed too. This branch heals coins and balances on its own, but multi-account history rows persist one account's slice until the record fold (#4438) is in — observed directly: the same wallet's history sum converges on a base that includes the fold and doesn't here. The two PRs are independent to merge but complementary in effect. Also observed, expected but worth documenting: contact-payment coins land only after a DashPay backfill pass that needs an unlocked wallet session (deferred contact-crypto builds gate it) — a freshly restored wallet that is never unlocked shows a balance short by its incoming contact payments until first unlock. 🤖 Generated with Claude Code |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The latest review round correctly demotes ambiguous spend and net-amount changes to logging, counts only successful inserts, resolves contact ownership through core addresses, and uses the canonical outpoint conversion. Three blockers remain: restored DashPay pools still skip repair despite carrying usable account xpubs, healed TXOs can lose their exported account ownership and disappear again on restart, and the periodic JNI inventory fully materializes an unbounded chain-inflatable snapshot in a mobile process. Source: Codex general, security-auditor, and FFI-engineer reviewer lanes (exact backend model IDs were not supplied); final verifier: Claude Agent SDK (exact backend model ID was not supplied); openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking | 🟡 1 suggestion(s)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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:1299-1306: Preserve exported account ownership on healed TXOs
The native inventory identifies the account that owns each UTXO, but this insertion ignores those tags. `upsertUtxoRow` preserves an existing `accountId` and sets `coreAddressId` only when the corresponding `core_addresses` row already exists. If persistence lost both the TXO and its address row—the two divergence classes this PR is intended to repair—the reconciled row is inserted with neither ownership link. On the next launch, `buildUtxoRestoreData` can resolve ownership only through `txo.accountId` or the address relationship, so it skips the healed output and recreates the visible fund loss. Emit the complete account tuple already present in `AccountBalanceEntryFFI` (`typeTag`, `standardTag`, index, registration index, key class, and DashPay identity IDs), resolve the Room account during reconciliation, and persist its ID on the healed TXO even when the address projection is absent.
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt:1327-1330: Interpolate values in the healed-TXO diagnostic
Each dollar sign is escaped with `${'$'}`, so the diagnostic emits literal placeholders such as `${txid.toHex()}:$vout`, `$amount`, and `${priorTx.netAmount}`. That removes the outpoint and values needed to investigate the nonzero reconciliation heal this warning is intended to identify.
In `packages/rs-unified-sdk-jni/src/wallet_manager.rs`:
- [BLOCKING] packages/rs-unified-sdk-jni/src/wallet_manager.rs:3202-3348: Do not materialize an unbounded chain-controlled inventory
This periodic operation stores one formatted `String` per UTXO and spent outpoint, duplicates them through `join`, builds another complete JSON string with `format!`, copies that value across JNI, and then Kotlin parses it into a full JSON DOM and constructs additional full-inventory hash sets. Inventory cardinality is unbounded and remotely inflatable because anyone who knows a watched address can repeatedly send dust outputs to it. On a memory-constrained mobile process, an attacker-inflated wallet can therefore make every SYNCED transition and 30-minute reconciliation allocate several simultaneous copies of the inventory, causing repeated allocation failure or process termination. Expose a bounded, cursor-based snapshot or stream/iterate records through a native callback or compact binary representation so neither Rust nor Kotlin must hold the complete serialized inventory at once.
- [SUGGESTION] packages/rs-unified-sdk-jni/src/wallet_manager.rs:3205-3207: Snapshot all account inventories in one pass
(existing thread: https://github.com/dashpay/platform/pull/4439#discussion_r3826530759)
The JNI method first enumerates all N accounts and then invokes separate per-account UTXO and spent-outpoint accessors. Each accessor reacquires `wallet_manager.blocking_read()`, rebuilds `all_accounts()`, and linearly searches that N-element collection. One reconciliation therefore performs O(N²) account traversal and 2N+1 lock acquisitions. DashPay adds accounts per contact, and this path runs at every SYNCED transition and every 30 minutes. Add a manager-level inventory operation that gathers each account's identity, UTXOs, and spent outpoints under one read lock before exposing the snapshot through FFI.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:4085-4113: Repair DashPay pools with their concrete account xpub
(existing thread: https://github.com/dashpay/platform/pull/4439#discussion_r3826530747)
The resolver still routes both DashPay variants through `Wallet::key_source_for_account_type`, which the pinned key-wallet revision explicitly maps to `NoKeySource`. The subsequent `repairable` guard therefore skips every DashPay pool. This is not a cryptographic limitation: `build_wallet_start_state` reconstructs these ECDSA accounts with their persisted `account_xpub`, `AccountCollection::account_of_type` supports both full DashPay account variants, and the normal DashPay registration paths construct the same `Absent` address pools from `KeySource::Public(account.account_xpub)`. A sparse `DashpayReceivingFunds` pool consequently remains unable to recognize payments to omitted indices after cold restore until an unlocked contact-sync happens, so the restore-time repair promised by this PR is incomplete. Resolve the concrete full `AccountType` through `wallet.accounts.account_of_type(account_type)` and use its xpub as the public key source, retaining the existing helper as a fallback for special key accounts. Add a sparse DashPay receiving-pool restore test using a real wallet; the current test passes `None` and cannot exercise derivation.
| val wrote = upsertUtxoRow( | ||
| database, walletId, txid, vout, amount, address, scriptPubKey, | ||
| height, | ||
| isCoinbase = false, | ||
| isConfirmed = true, | ||
| isInstantLocked = false, | ||
| isLocked = isLocked, | ||
| ) |
There was a problem hiding this comment.
🔴 Blocking: Preserve exported account ownership on healed TXOs
The native inventory identifies the account that owns each UTXO, but this insertion ignores those tags. upsertUtxoRow preserves an existing accountId and sets coreAddressId only when the corresponding core_addresses row already exists. If persistence lost both the TXO and its address row—the two divergence classes this PR is intended to repair—the reconciled row is inserted with neither ownership link. On the next launch, buildUtxoRestoreData can resolve ownership only through txo.accountId or the address relationship, so it skips the healed output and recreates the visible fund loss. Emit the complete account tuple already present in AccountBalanceEntryFFI (typeTag, standardTag, index, registration index, key class, and DashPay identity IDs), resolve the Room account during reconciliation, and persist its ID on the healed TXO even when the address projection is absent.
source: ['codex']
| let mut rows: Vec<String> = Vec::new(); | ||
| let mut spent_rows: Vec<String> = Vec::new(); | ||
| let mut errors: Vec<String> = Vec::new(); | ||
| if !entries.is_null() && count > 0 { | ||
| let accounts = unsafe { std::slice::from_raw_parts(entries, count) }; | ||
| for acc in accounts { | ||
| let spec = platform_wallet_ffi::AccountSpecFFI { | ||
| type_tag: acc.type_tag as u8, | ||
| standard_tag: acc.standard_tag as u8, | ||
| index: acc.index, | ||
| registration_index: acc.registration_index, | ||
| key_class: acc.key_class, | ||
| user_identity_id: acc.user_identity_id, | ||
| friend_identity_id: acc.friend_identity_id, | ||
| account_xpub_bytes: ptr::null(), | ||
| account_xpub_bytes_len: 0, | ||
| }; | ||
| let mut utxos: *const platform_wallet_ffi::AccountUtxoEntryFFI = ptr::null(); | ||
| let mut utxo_count: usize = 0; | ||
| let res = unsafe { | ||
| platform_wallet_ffi::platform_wallet_account_utxos( | ||
| manager_handle as Handle, | ||
| wid.as_ptr(), | ||
| &spec, | ||
| &mut utxos, | ||
| &mut utxo_count, | ||
| ) | ||
| }; | ||
| if let Some(msg) = pwffi_error_message(res) { | ||
| errors.push(format!( | ||
| "{{\"typeTag\":{},\"index\":{},\"message\":{}}}", | ||
| acc.type_tag as u8, | ||
| acc.index, | ||
| json_escape(&msg), | ||
| )); | ||
| continue; | ||
| } | ||
| if utxos.is_null() || utxo_count == 0 { | ||
| continue; | ||
| } | ||
| let items = unsafe { std::slice::from_raw_parts(utxos, utxo_count) }; | ||
| for u in items { | ||
| let script: &[u8] = if u.script_pubkey.is_null() || u.script_pubkey_len == 0 { | ||
| &[] | ||
| } else { | ||
| unsafe { | ||
| std::slice::from_raw_parts(u.script_pubkey, u.script_pubkey_len) | ||
| } | ||
| }; | ||
| let script_buf = dashcore::ScriptBuf::from(script.to_vec()); | ||
| let address = dashcore::Address::from_script(&script_buf, net) | ||
| .map(|a| a.to_string()) | ||
| .unwrap_or_default(); | ||
| rows.push(format!( | ||
| "{{\"typeTag\":{},\"standardTag\":{},\"index\":{},\ | ||
| \"txid\":\"{}\",\"vout\":{},\"amount\":{},\ | ||
| \"address\":{},\"scriptHex\":\"{}\",\ | ||
| \"height\":{},\"isLocked\":{}}}", | ||
| acc.type_tag as u8, | ||
| acc.standard_tag as u8, | ||
| acc.index, | ||
| hex_lower(&u.outpoint_txid), | ||
| u.outpoint_vout, | ||
| u.value_duffs, | ||
| json_escape(&address), | ||
| hex_lower(script), | ||
| u.height, | ||
| u.is_locked, | ||
| )); | ||
| } | ||
| unsafe { | ||
| platform_wallet_ffi::platform_wallet_account_utxos_free( | ||
| utxos as *mut platform_wallet_ffi::AccountUtxoEntryFFI, | ||
| utxo_count, | ||
| ) | ||
| }; | ||
| } | ||
| // Second inventory half: the engine's spent outpoints, so the | ||
| // reconcile can classify a store row still marked unspent — | ||
| // present here means the row lost its spend update | ||
| // (dashpay/platform#4425, flip it); present in neither | ||
| // inventory means swept/abandoned residue | ||
| // (pre-rust-dashcore#971 stores, log-only). Soft-fail like the | ||
| // UTXO loop: one bad account must not mask the rest. | ||
| for acc in accounts { | ||
| let spec = platform_wallet_ffi::AccountSpecFFI { | ||
| type_tag: acc.type_tag as u8, | ||
| standard_tag: acc.standard_tag as u8, | ||
| index: acc.index, | ||
| registration_index: acc.registration_index, | ||
| key_class: acc.key_class, | ||
| user_identity_id: acc.user_identity_id, | ||
| friend_identity_id: acc.friend_identity_id, | ||
| account_xpub_bytes: ptr::null(), | ||
| account_xpub_bytes_len: 0, | ||
| }; | ||
| let mut outpoints: *const platform_wallet_ffi::OutPointFFI = ptr::null(); | ||
| let mut spent_count: usize = 0; | ||
| let res = unsafe { | ||
| platform_wallet_ffi::platform_wallet_account_spent_outpoints( | ||
| manager_handle as Handle, | ||
| wid.as_ptr(), | ||
| &spec, | ||
| &mut outpoints, | ||
| &mut spent_count, | ||
| ) | ||
| }; | ||
| if let Some(msg) = pwffi_error_message(res) { | ||
| errors.push(format!( | ||
| "{{\"typeTag\":{},\"index\":{},\"message\":{}}}", | ||
| acc.type_tag as u8, | ||
| acc.index, | ||
| json_escape(&msg), | ||
| )); | ||
| continue; | ||
| } | ||
| if outpoints.is_null() || spent_count == 0 { | ||
| continue; | ||
| } | ||
| let items = unsafe { std::slice::from_raw_parts(outpoints, spent_count) }; | ||
| for op in items { | ||
| spent_rows.push(format!( | ||
| "{{\"txid\":\"{}\",\"vout\":{}}}", | ||
| hex_lower(&op.txid), | ||
| op.vout, | ||
| )); | ||
| } | ||
| unsafe { | ||
| platform_wallet_ffi::platform_wallet_account_spent_outpoints_free( | ||
| outpoints as *mut platform_wallet_ffi::OutPointFFI, | ||
| spent_count, | ||
| ) | ||
| }; | ||
| } | ||
| } | ||
| unsafe { | ||
| platform_wallet_ffi::platform_wallet_manager_free_account_balances( | ||
| entries as *mut platform_wallet_ffi::AccountBalanceEntryFFI, | ||
| count, | ||
| ) | ||
| }; | ||
| let json = format!( | ||
| "{{\"utxos\":[{}],\"spent\":[{}],\"errors\":[{}]}}", | ||
| rows.join(","), | ||
| spent_rows.join(","), | ||
| errors.join(","), | ||
| ); |
There was a problem hiding this comment.
🔴 Blocking: Do not materialize an unbounded chain-controlled inventory
This periodic operation stores one formatted String per UTXO and spent outpoint, duplicates them through join, builds another complete JSON string with format!, copies that value across JNI, and then Kotlin parses it into a full JSON DOM and constructs additional full-inventory hash sets. Inventory cardinality is unbounded and remotely inflatable because anyone who knows a watched address can repeatedly send dust outputs to it. On a memory-constrained mobile process, an attacker-inflated wallet can therefore make every SYNCED transition and 30-minute reconciliation allocate several simultaneous copies of the inventory, causing repeated allocation failure or process termination. Expose a bounded, cursor-based snapshot or stream/iterate records through a native callback or compact binary representation so neither Rust nor Kotlin must hold the complete serialized inventory at once.
source: ['codex']
| "txos reconcile: healed TXO ${'$'}{txid.toHex()}:${'$'}vout " + | ||
| "(${'$'}amount duffs) has a pre-existing record whose " + | ||
| "netAmount may be short by that amount — LOG-ONLY, " + | ||
| "storedNet=${'$'}{priorTx.netAmount}", |
There was a problem hiding this comment.
🟡 Suggestion: Interpolate values in the healed-TXO diagnostic
Each dollar sign is escaped with ${'$'}, so the diagnostic emits literal placeholders such as ${txid.toHex()}:$vout, $amount, and ${priorTx.netAmount}. That removes the outpoint and values needed to investigate the nonzero reconciliation heal this warning is intended to identify.
| "txos reconcile: healed TXO ${'$'}{txid.toHex()}:${'$'}vout " + | |
| "(${'$'}amount duffs) has a pre-existing record whose " + | |
| "netAmount may be short by that amount — LOG-ONLY, " + | |
| "storedNet=${'$'}{priorTx.netAmount}", | |
| "txos reconcile: healed TXO ${txid.toHex()}:$vout " + | |
| "($amount duffs) has a pre-existing record whose " + | |
| "netAmount may be short by that amount — LOG-ONLY, " + | |
| "storedNet=${priorTx.netAmount}", |
source: ['codex']
3 participants
Issue being fixed
The Room store (
txos/transactions/core_addresses) diverged from the engine under rescan, interruption, and delivery loss — and because the engine is rebuilt FROM the store at launch (buildUtxoRestoreData), divergence graduated into visible fund loss on relaunch (testnet field case: 106.43 → 86.33 after restart; root causes fixed engine-side in dashpay/rust-dashcore#979).Stacked on #4406 (this branch's base): the reconcile's shared TXO-insert path absorbs #4406's swept-tombstone semantics, so both writers honor the same rules. Depends on dashpay/rust-dashcore#979: the first commit pins rust-dashcore to that PR's head so CI builds; re-pin to the
devmerge commit once it lands.What was done
reconcileTxoStore— post-sync audit of the store against the engine's full inventory (newwalletManagerAllUtxosJsonJNI export): heals missing TXOs (insert-only, 100-conf gate), repairs the netAmounts those holes falsified, logs every action. Runs on the SPV SYNCED transition and every 30 min. Nonzero heals after fix: same block core chain lock height #979 = regression tripwire.restore_core_address_poolsresolves each pool's key source from the signing wallet and re-derives indices missing from the persisted rows (holes observed in the field made funds rescan-proof invisible; this also closes the rescan-vs-fresh-restore divergence).platform_wallet_account_spent_outpoints). Store rows the engine proves spent are flipped; rows the engine has never seen are LOGGED, never deleted; spent rows the engine disputes are LOGGED, never un-marked (a live spend racing the engine is indistinguishable from lost-release residue, and un-marking could double-spend). Watch-only DIP-15 contact rows are excluded from classification.derive_new_utxosoverupdatedrecords is included for consistency, with a reviewer note thatfrom_changesetre-derives from records and ignorescs.new_utxos— that field may be vestigial and worth a follow-up decision.How this was tested
Merge order: rust-dashcore#979 → re-pin here → #4406 → this.
🤖 Generated with Claude Code