fix: shield asset-lock funding from all funds accounts incl. CoinJoin (#4073)

[bfoss765]

Problem

shieldedFundFromAssetLock could only reach the standard BIP44 account when selecting UTXOs for the funding asset lock. Funds held on other derivation accounts — specifically the DIP-9 CoinJoin account, where previously-mixed coins live — counted in the wallet balance but could not be shielded.

Live evidence (S22 / testnet, dashpay/dash-wallet#1507): total balance 1.534 DASH with exact SDK↔dashj parity, yet shielding failed:

Coin selection error: Insufficient funds: available 8999527 (0.09 DASH), required 20000000

The 0.09 is the BIP44-only slice; ~1.44 DASH sat on the CoinJoin path and was invisible to the asset-lock coin selector.

Root cause (file:line)

The shielded pipeline resolves funding as:

• rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs → resolve_funding_with_is_timeout_fallback(...) with AssetLockShieldedAddressTopUp
• rs-platform-wallet/src/wallet/asset_lock/orchestration.rs (AssetLockFunding::FromWalletBalance) → AssetLockManager::create_funded_asset_lock_proof
• rs-platform-wallet/src/wallet/asset_lock/build.rs::build_asset_lock_transaction → delegated to the pinned key-wallet ManagedWalletInfo::build_asset_lock_with_signer.

That pinned builder (key-wallet .../managed_wallet_info/asset_lock_builder.rs:256) funds from a single BIP44 account:

let funds_acc = self.accounts.standard_bip44_accounts.get_mut(&account_index)...;
TransactionBuilder::new()
    .set_funding(funds_acc, &acc)                                   // inputs from ONE account
    .build_signed(signer, |addr| funds_acc.address_derivation_path(&addr))  // single-account resolver

CoinJoin UTXOs live on coinjoin_accounts (ManagedAccountCollection), which is included in the wallet balance (all_funding_accounts()) but not in this selector. The single-account path resolver is the hard blocker: any input outside funds_acc cannot be signed.

Diagnosis: the restriction is in the PINNED crate, not workspace code (rev 1860089eddea27f66e5b3e637d18a73ae138478c). Even the general build_and_sign_transaction_with_signer is single-account (AccountTypePreference + source_index) — multi-account funding does not exist upstream.

What changed (workspace-side, no pin patch, no Kotlin/JNI change)

build_asset_lock_transaction now routes only the shielded funding type (AssetLockShieldedAddressTopUp) through a new AssetLockManager::build_asset_lock_tx_from_all_funding_accounts, which:

• keeps account_index as the primary BIP44 account — its reservation ledger gates concurrent primary-account builds and change flows back to it (set_funding);
• adds the spendable UTXOs of every other funds account (BIP44 + BIP32 + CoinJoin + DashPay) as explicit add_inputs;
• signs with a resolver that spans all funds accounts (an owned Address → DerivationPath map built from each account's address_derivation_path), so mixed inputs are each signed under their own account's path;
• derives the credit-output key from the shielded-topup account exactly as the pinned builder does (peek path → signer pubkey → mark used).

Every other funding type (identity registration/top-up, platform-address funding) stays single-BIP44-account: spending mixed CoinJoin coins into an identity registration would de-anonymize them, a deliberate privacy choice left out of scope.

Typed error (part of #4073): coin-selection shortfalls now map to PlatformWalletError::AssetLockInsufficientFunds { available, required } instead of a stringly-typed message. After this change available reflects the union of all spendable accounts, not just the BIP44 slice. (Threading this typed variant through the FFI/JNI/Kotlin error surface is a small follow-up — the FFI still carries it as to_string() today.)

Interim caveat (superseded by the upstream fix below)

TransactionBuilder::set_funding captures only the primary account's ReservationSet, and that accessor is pub(crate) in key-wallet, so this crate cannot reserve per-account. Inputs selected from non-primary accounts are therefore recorded in the primary account's reservation ledger rather than their own — harmless because shielded funding is single-flighted under shield_guard + the wallet write lock, the app issues no concurrent non-shielded spend on the CoinJoin/BIP32 accounts, and the broadcast tx's processing releases the outpoints from every account's ledger.

Exact minimal upstream key-wallet diff (the clean long-term fix)

The proper fix belongs in key-wallet's build_asset_lock_with_signer so set_funding semantics and per-account reservations are preserved. Sketch:

// managed_wallet_info/asset_lock_builder.rs::build_asset_lock_with_signer
// Instead of one funds_acc:
let mut builder = TransactionBuilder::new()
    .set_fee_rate(FeeRate::new(fee_per_kb))
    .set_current_height(height)
    .set_special_payload(TransactionPayload::AssetLockPayloadType(AssetLockPayload::new(credit_outputs)))
    .set_funding(primary_funds_acc, &acc);          // primary: change + its reservations
for other in self.accounts.all_funding_accounts_mut_except(primary) {
    builder = builder.add_inputs_reserving(other);  // reserve in each account's OWN ledger
}
let path_map = /* Address -> DerivationPath across all funding accounts */;
let (tx, _fee) = builder
    .require_final_inputs()
    .build_signed(signer, move |addr| path_map.get(&addr).cloned())
    .await?;

This needs a small key-wallet addition (a reservation-aware multi-account add_inputs, or making ReservationSet reachable) — out of scope for the pinned-crate no-patch rule here, hence the workspace composition above.

Tests (cargo test -p platform-wallet)

New fixture split_funded_wallet_manager funds BIP44 account 0 and CoinJoin account 0 so neither alone covers the lock:

• shielded_asset_lock_funds_from_bip44_and_coinjoin_union — a 0.15 DASH shielded lock over a 0.09/0.09 split spends inputs from both accounts and every mixed input carries a signature (per-account signing).
• non_shielded_asset_lock_stays_single_bip44_account — an identity-registration lock for the same amount fails (BIP44-only) while the shielded lock succeeds from the union.
• shielded_asset_lock_union_shortfall_is_typed — a lock exceeding the union surfaces AssetLockInsufficientFunds with available reflecting both accounts.

All 424 lib tests pass; clippy clean under default and shielded features. The native library was not rebuilt (coordinator handles the AAR).

cc @QuantumExplorer

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Shielded asset-lock top-ups can now use available funds across supported wallet accounts.
  • Insufficient-funds errors now report the available and required amounts.

• Bug Fixes

  • Improved funding selection for wallets with many CoinJoin funds, reducing delays and preventing builder hangs.
  • Preserved account-specific funding behavior for non-shielded asset locks.

• Documentation

  • Clarified which accounts are used for different asset-lock funding types.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d1f52510-1b6c-4ebf-a6ea-2def8705c2bb

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Shielded asset-lock top-ups now select spendable inputs across BIP44, BIP32, CoinJoin, and DashPay accounts. Structured insufficient-funds errors, multi-account test fixtures, coin-selection regression tests, and funding documentation were added.

Changes

Asset-lock funding

Layer / File(s)	Summary
Asset-lock funding contract packages/rs-platform-wallet/src/error.rs, packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs	Adds AssetLockInsufficientFunds with available and required values and documents primary-account and funding-type selection behavior.
Multi-account wallet fixtures packages/rs-platform-wallet/src/test_support.rs	Adds test wallets funded across BIP44 and CoinJoin accounts, including fixtures with multiple CoinJoin UTXOs.
Shielded multi-account builder and validation packages/rs-platform-wallet/src/wallet/asset_lock/build.rs	Routes shielded top-ups through union funding, uses largest-first selection and derivation-path resolution, maps insufficient-funds errors, and adds coverage for mixed-account spending, account restrictions, precise balances, signing, and selection timeouts.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related issues

• kotlin-sdk: shieldedFundFromAssetLock coin selection only reaches one account (CoinJoin/other-account funds unspendable) #4073 — Enables shielded asset-lock coin selection across CoinJoin and other accounts with typed insufficient-funds reporting.

Possibly related PRs

• dashpay/platform#3753 — Updates the asset-lock building and coin-selection path for shielded top-ups, including unified multi-account selection and structured insufficient-funds errors.

Suggested reviewers: shumkov, lklimek, llbartekll, ZocoLini

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: shielded asset-lock funding now uses all funds accounts, including CoinJoin.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[thepastaclaw]

✅ Review complete (commit 189e068)

[thepastaclaw]

Code Review

The shielded union-funding change is sound and well-tested. The two headline agent findings about the signing resolver's path_map being narrower than the coin-selection candidate set are false positives: the CoinSelector filters candidates through is_spendable (coin_selection.rs:149) — the same predicate that builds path_map from spendable_utxos — so a non-spendable coin can never be selected, and every selectable spendable UTXO's address is in its account pool. Remaining items are a low-severity CoinJoin privacy consideration (mixed inputs co-spent with BIP44 and change routed to a clear BIP44 address) and a minor typed-error gap for the zero-spendable-balance case. No blocking issues.

Source: reviewers claude-general (opus, completed), codex-general (gpt-5.6-sol, failed), claude-security-auditor (opus, completed), codex-security-auditor (gpt-5.6-sol, failed), claude-rust-quality (opus, completed), codex-rust-quality (gpt-5.6-sol, failed), claude-ffi-engineer (opus, completed), codex-ffi-engineer (gpt-5.6-sol, failed); verifier claude (opus).

🟡 1 suggestion(s) | 💬 1 nitpick(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/rs-platform-wallet/src/wallet/asset_lock/build.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:308-317: Shielded union funding co-spends CoinJoin inputs with BIP44 and routes change to a clear BIP44 address, partially de-mixing previously-mixed coins
  The union builder seeds the primary BIP44 account via `set_funding` (which also sets the change address to the primary BIP44 account's `next_change_address`) and appends the spendable UTXOs of every other funds account, including CoinJoin, via `add_inputs`. Two on-chain linkages follow when a previously-mixed CoinJoin coin is selected: (1) the mixed input is co-spent in the same transaction as clear BIP44 inputs, linking them under the common-input-ownership heuristic; and (2) any surplus is returned as change to a fresh clear BIP44 change address, linkable to the CoinJoin input that funded it. Both erode the mixing those coins previously had. This is a deliberate consequence of the feature — the PR keeps non-shielded funding single-account precisely to avoid de-anonymizing mixed coins, and accepts spending them for the shielded path since the locked value lands in the shielded credit output. The clear change leg and cross-account co-spend, however, are not shielded. If preserving CoinJoin privacy on the change/co-spend legs matters, consider routing change from mixed inputs back to a mixed pool and/or preferring CoinJoin-only input sets when they can cover the amount. Not a consensus, crypto, or fund-loss issue — a client-side privacy tradeoff worth a conscious decision.

[thepastaclaw]

🟡 Suggestion: Shielded union funding co-spends CoinJoin inputs with BIP44 and routes change to a clear BIP44 address, partially de-mixing previously-mixed coins

The union builder seeds the primary BIP44 account via set_funding (which also sets the change address to the primary BIP44 account's next_change_address) and appends the spendable UTXOs of every other funds account, including CoinJoin, via add_inputs. Two on-chain linkages follow when a previously-mixed CoinJoin coin is selected: (1) the mixed input is co-spent in the same transaction as clear BIP44 inputs, linking them under the common-input-ownership heuristic; and (2) any surplus is returned as change to a fresh clear BIP44 change address, linkable to the CoinJoin input that funded it. Both erode the mixing those coins previously had. This is a deliberate consequence of the feature — the PR keeps non-shielded funding single-account precisely to avoid de-anonymizing mixed coins, and accepts spending them for the shielded path since the locked value lands in the shielded credit output. The clear change leg and cross-account co-spend, however, are not shielded. If preserving CoinJoin privacy on the change/co-spend legs matters, consider routing change from mixed inputs back to a mixed pool and/or preferring CoinJoin-only input sets when they can cover the amount. Not a consensus, crypto, or fund-loss issue — a client-side privacy tradeoff worth a conscious decision.

_{source: ['claude']}

[thepastaclaw]

Correction: this finding remains valid at 53762359. The latest push changes the coin-selection strategy, tracing, and regression coverage; it does not change the BIP44 change destination or the cross-account co-spend behavior. The prior auto-resolution message was produced by a reconciliation/deduplication error.

[bfoss765]

Intentional — this is the deliberate product decision for this PR (Option B), and I'm leaving the thread open for visibility rather than silently resolving it.

The shielded/asset-lock funding path co-spends the spendable UTXOs of ALL funds accounts — including mixed CoinJoin coins alongside clear BIP44 — and routes change to the primary BIP44 account's next_change_address. We understand and accept the privacy trade-off you've laid out precisely: (1) the common-input-ownership linkage from co-spending a mixed input with clear BIP44 inputs, and (2) the clear-change leg linking surplus back to a fresh BIP44 address. The locked value itself lands in the shielded credit output; the co-spend and clear-change legs are not shielded.

This was chosen over a CoinJoin-only-preferred selection / mixed-change-pool policy to keep funding able to cover the target from the full balance without fragmenting across accounts. Preserving mixing on the change/co-spend legs is a reasonable future enhancement (CoinJoin-only candidate sets when they cover the amount, and/or a mixed-input → mixed-change policy), but it's out of scope here. Not a consensus/crypto/fund-loss issue — a conscious client-side privacy trade-off, made consciously.

[thepastaclaw]

Reconciliation at a5ea9e5: this privacy tradeoff remains present. The union-funded shield path can still co-spend CoinJoin and clear inputs and route change to the primary BIP44 account, but bfoss765 explicitly accepted this as the deliberate Option B product decision and asked to leave the thread open for visibility.

_{Classification: INTENTIONALLY_DEFERRED — not fixed or silently resolved.}

[thepastaclaw]

💬 Nitpick: map_builder_error routes SelectionError::NoUtxosAvailable to the generic string error, missing the typed-error goal for the zero-spendable case

map_builder_error promotes BuilderError::InsufficientFunds and CoinSelection(SelectionError::InsufficientFunds) to the typed AssetLockInsufficientFunds, but the CoinSelector returns SelectionError::NoUtxosAvailable (verified at coin_selection.rs:152) when the candidate pool has zero spendable UTXOs — the most extreme shortfall. That case falls through the other arm to the generic AssetLockTransaction(String), so a shielded funding attempt on a wallet with no spendable coins does not surface the typed error that #4073 introduced, and hosts must string-match to recognize it. Threading a precise required is impossible here (NoUtxosAvailable carries no amounts), so this is a consistency gap rather than a defect; if surfaced as typed with available: 0, callers would at least stay on the structured path. Left as a judgment call for the author.

_{source: ['claude']}

[thepastaclaw]

Resolved in 380645a — map_builder_error routes SelectionError::NoUtxosAvailable to the generic string error, missing the typed-error goal for the zero-spendable case 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.}

[thepastaclaw]

Resolved in a5ea9e5 — map_builder_error routes SelectionError::NoUtxosAvailable to the generic string error, missing the typed-error goal for the zero-spendable case 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]

Follow-up: fixed an on-device hang on the first real multi-account shield (5376235)

On a Galaxy S22 the first multi-account shield (0.2 DASH, BIP44 = 8_999_527 duffs, CoinJoin ≈ 1.44 DASH) hung inside the FFI for 11+ minutes — FundingNative.shieldedFundFromAssetLock RUNNABLE in native, zero Rust logs, nothing broadcast.

Deadlock audit (first, as requested): not a deadlock.

• build_asset_lock_tx_from_all_funding_accounts takes no wallet-manager lock itself; the production signer (MnemonicResolverCoreSigner) derives keys from the mnemonic for any path without touching the wallet lock — and the single-account path already calls it under the same held lock and works.
• The reservation step is a single std::sync::Mutex acquire, never held across an .await.

Root cause: exponential coin selection. The multi-account builder inherited TransactionBuilder's default SelectionStrategy::BranchAndBound, which routes to CoinSelector::find_exact_match — a recursive exact-match subset-sum whose search space is exponential in the number of sub-target UTXOs. The single-BIP44-account path tolerated it (a handful of UTXOs); a DIP-9 CoinJoin account holds many small mixed denominations (0.001 / 0.01 / 0.1 DASH), so feeding the whole set blows up. It is synchronous CPU work with no .await, so the coroutine never yields — exactly the RUNNABLE-in-native / no-logs / no-broadcast signature, and why no tokio timeout could have rescued it.

Fix: pin SelectionStrategy::LargestFirst on the multi-account builder (linear greedy accumulate_coins_with_size, and it also minimizes the input count → fewer Keychain resolver upcalls, smaller tx/fee). Single-account and all other funding types are unchanged.

Observability: added entry/exit + per-stage debug tracing (account enumeration, union totals, select+sign handoff, built-tx txid/fee, credit-key derivation) — this path previously emitted nothing before the post-build "tracked as Built" log, so the stall was unlocalizable.

Regression test: split_funded_wallet_manager_many_coinjoin seeds CoinJoin account 0 with 40 small denominations; the test runs the build on a detached OS thread and fails via recv_timeout if it doesn't return within 30s (a plain tokio::time::timeout can't express this — the blowup never yields). Verified: fails bounded ~30s under BranchAndBound, passes in ~25ms under LargestFirst.

Kotlin/JNI surface: unchanged — no AAR API change beyond the native-lib rebuild.

[thepastaclaw]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

🧹 Nitpick comments (1)

packages/rs-platform-wallet/src/wallet/asset_lock/build.rs (1)

1369-1406: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Strengthen the negative assertion with an error-variant/reason check.

assert!(identity.is_err(), ...) only proves some error occurred, not that it failed specifically at coin selection due to the BIP44-only scope. A regression that fails this call for an unrelated reason (e.g. a wiring bug elsewhere) would still pass this test.

♻️ Proposed tightening

-        assert!(
-            identity.is_err(),
-            "identity registration must NOT reach CoinJoin coins — BIP44 alone \
-             is short of 0.15 DASH, got {identity:?}"
-        );
+        assert!(
+            matches!(identity, Err(PlatformWalletError::AssetLockInsufficientFunds { .. })
+                | Err(PlatformWalletError::AssetLockTransaction(_))),
+            "identity registration must fail at coin selection (BIP44 alone is \
+             short of 0.15 DASH), got {identity:?}"
+        );

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/rs-platform-wallet/src/wallet/asset_lock/build.rs` around lines 1369
- 1406, Strengthen the negative assertion in
non_shielded_asset_lock_stays_single_bip44_account by unwrapping the identity
error and matching the expected coin-selection failure variant and reason
indicating insufficient BIP44-only funds. Assert the error is specifically the
funding/coin-selection failure caused by excluding CoinJoin coins, while
preserving the existing success assertion for shielded funding.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@packages/rs-platform-wallet/src/wallet/asset_lock/build.rs`:
- Around line 1369-1406: Strengthen the negative assertion in
non_shielded_asset_lock_stays_single_bip44_account by unwrapping the identity
error and matching the expected coin-selection failure variant and reason
indicating insufficient BIP44-only funds. Assert the error is specifically the
funding/coin-selection failure caused by excluding CoinJoin coins, while
preserving the existing success assertion for shielded funding.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d6e4470c-ef02-492f-8a92-1a7c80c2d18a

📥 Commits

Reviewing files that changed from the base of the PR and between 4805c54 and 5376235.

📒 Files selected for processing (4)

• packages/rs-platform-wallet/src/error.rs
• packages/rs-platform-wallet/src/test_support.rs
• packages/rs-platform-wallet/src/wallet/asset_lock/build.rs
• packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs

[thepastaclaw]

Code Review

Prior Reconciliation: Both prior findings (prior-privacy-308, prior-no-utxos-846) re-verified against HEAD 5376235 and classified STILL VALID; neither was fixed by the delta. Carried-Forward Prior Findings: (1) shielded union funding co-spends CoinJoin inputs with clear BIP44 inputs and routes change to a clear BIP44 change address, partially de-mixing previously-mixed coins (build.rs:328-352); (2) map_builder_error drops SelectionError::NoUtxosAvailable to the generic string error, missing the typed AssetLockInsufficientFunds goal for the zero-spendable case (build.rs:895-912). New Findings In Latest Delta: none — the 9be2e14..HEAD delta is a tightly-scoped, correct liveness fix (pins LargestFirst over the exponential BranchAndBound default, adds debug tracing that leaks no keys/addresses, and adds a detached-thread hang-regression test). Additional Cumulative Findings: none. No consensus, crypto, proof, or fund-loss defects. Client-side wallet change, off the consensus path.

Source: reviewers claude-general (opus, completed), codex-general (gpt-5.6-sol, failed), claude-security-auditor (opus, completed), codex-security-auditor (gpt-5.6-sol, failed), claude-rust-quality (opus, completed), codex-rust-quality (gpt-5.6-sol, failed), claude-ffi-engineer (opus, completed), codex-ffi-engineer (gpt-5.6-sol, failed); verifier claude (opus).

🟡 1 suggestion(s) | 💬 1 nitpick(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/rs-platform-wallet/src/wallet/asset_lock/build.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:345-352: Shielded union funding co-spends CoinJoin inputs with clear BIP44 and routes change to a clear BIP44 address, partially de-mixing previously-mixed coins
  Carried-forward prior finding (prior-privacy-308), re-verified STILL VALID at HEAD. The union builder seeds the primary BIP44 account via `set_funding(primary_funds, &acc)` — which also fixes the change address to that account's next change address — and appends every other funds account's spendable UTXOs, including previously-mixed CoinJoin coins, via `add_inputs(extra_inputs)`. When a CoinJoin coin is selected, two on-chain linkages follow: (1) the mixed input is co-spent in the same transaction as clear BIP44 inputs, linking them under the common-input-ownership heuristic; and (2) any surplus is returned as change to a fresh clear BIP44 change address, linkable to the CoinJoin input that funded it. A passive chain observer can therefore attribute previously-mixed value to the wallet's clear BIP44 cluster. The latest-delta switch to `SelectionStrategy::LargestFirst` does not mitigate this and arguably sharpens it: LargestFirst grabs the largest candidate first, and CoinJoin denominations are typically large, so even a small shield amount the BIP44 slice alone could cover may pull in — and de-mix — a large CoinJoin coin. This is a deliberate, documented tradeoff (non-shielded funding is intentionally kept single-account, and the shielded value itself lands in the shielded credit output), not a consensus/crypto/fund-loss defect — but the co-spend and clear-change legs are not themselves shielded. If preserving CoinJoin privacy on those legs matters, prefer a CoinJoin-only input set when it can cover the amount and/or route change from mixed inputs back to a mixed pool.

[bfoss765]

Follow-up: fixed a post-broadcast balance bug on multi-account shields (e1593f4)

After the LargestFirst fix, a CoinJoin-funded 0.2 DASH shield broadcast fine, but the SDK balance credited the change yet never debited the spent CoinJoin inputs — unspent count stayed high by the full spent amount for 3+ minutes until an app-side watchdog reset it.

Root cause — the pinned transaction router, not the reservation caveat. key-wallet's TransactionRouter::get_relevant_account_types(TransactionType::AssetLock) returns BIP44 + BIP32 + the identity/asset-lock key accounts but omits CoinJoin, DashpayReceivingFunds, DashpayExternalAccount (transaction_checking/transaction_router/mod.rs:137). check_core_transaction — the only path that marks a UTXO spent, run by dash-spv on both the mempool relay and the confirmation scan — scopes its spend detection (ManagedAccountCollection::check_transaction) to those relevant types, so it never debits inputs from the omitted accounts. Before the multi-account builder an asset lock only ever spent BIP44, so the gap was invisible; now a CoinJoin-funded shield spends CoinJoin UTXOs the wallet keeps counting. The change (a BIP44 output) IS credited, so the balance lands high by exactly the CoinJoin amount spent — matching the on-device numbers precisely.

Real fix — upstream key-wallet (can't bump the pin here). Add CoinJoin + DashPay to the AssetLock relevant types. Ready-to-submit patch, verified to apply cleanly against the pinned rev 1860089eddea27f66e5b3e637d18a73ae138478c (I could not create the fork/PR from this environment — publishing to an external repo is gated — so please cut it from here):

diff --git a/key-wallet/src/transaction_checking/transaction_router/mod.rs b/key-wallet/src/transaction_checking/transaction_router/mod.rs
index b7d74b2..7fc84b4 100644
--- a/key-wallet/src/transaction_checking/transaction_router/mod.rs
+++ b/key-wallet/src/transaction_checking/transaction_router/mod.rs
@@ -135,8 +135,21 @@ impl TransactionRouter {
                 AccountTypeToCheck::CoinJoin,
             ],
             TransactionType::AssetLock => vec![
+                // Fund-bearing accounts an asset lock can spend from. CoinJoin
+                // and the DashPay accounts are load-bearing here: only
+                // `check_core_transaction` marks a UTXO spent, and it scopes
+                // spend detection to these types, so a wallet that funds an
+                // asset lock (e.g. a shielded top-up) from CoinJoin or DashPay
+                // UTXOs would otherwise never have those inputs debited — the
+                // spent UTXOs are then counted in the balance indefinitely. The
+                // change output (a BIP44 address) IS credited, so the balance
+                // ends up high by exactly the CoinJoin/DashPay amount spent
+                // (dashpay/platform#4073, dashpay/dash-wallet#1507).
                 AccountTypeToCheck::StandardBIP44,
                 AccountTypeToCheck::StandardBIP32,
+                AccountTypeToCheck::CoinJoin,
+                AccountTypeToCheck::DashpayReceivingFunds,
+                AccountTypeToCheck::DashpayExternalAccount,
                 AccountTypeToCheck::IdentityRegistration,
                 AccountTypeToCheck::IdentityTopUp,
                 AccountTypeToCheck::IdentityTopUpNotBound,

Workspace interim mitigation (this commit). After a shielded asset-lock broadcast, debit_router_omitted_asset_lock_spends removes the tx's spent CoinJoin/DashPay input UTXOs from the owning account (leaving BIP44/BIP32 to the pinned path) and republishes the aggregate to the lock-free WalletBalance atomics the SDK reads. WalletInfoInterface::update_balance sums every funds account from its current UTXO map, so once those UTXOs are gone every later recompute — dash-spv's mempool/confirmation events included — is correct too; the mitigation is idempotent with, and order-independent of, that async processing, and becomes a no-op once the pin is bumped. Gated to the shielded funding type, so single-account asset locks are untouched.

Test: multi_account_asset_lock_broadcast_debits_router_omitted_spends_immediately first reproduces the router gap (change credited, CoinJoin input left counted → balance jumps up), then applies the mitigation and asserts the aggregate equals previous − inputs + change immediately and the SDK-facing atomics were republished to match.

Kotlin/JNI surface: unchanged.

[bfoss765]

Follow-up: the router fix now ships in the build via vendored rust-dashcore (da97eec)

The broadcast-time mitigation alone was insufficient on-device. After a hard reset cascades into a full rescan, the wallet is rebuilt from scratch and the asset-lock tx is re-processed through check_core_transaction — which on the un-patched pin re-inflates the balance by the spent CoinJoin amount (the rescan re-adds the UTXOs and the router still won't mark them spent). The broadcast mitigation only runs on the broadcast path, never on a rescan, so the reset→rescan→inflate loop can never stably clear and the verification gate can't reopen on any lib lacking the router fix.

This commit carries the upstream router fix in the workspace build now, without waiting for rust-dashcore review:

• Vendored a slim, separately-[workspace]-rooted copy of the pinned rev 1860089e under third_party/rust-dashcore (~11 MB: .git, dash/tests, and crate-level tests/benches/examples stripped — cargo only compiles the lib target of a patched dependency; lib-referenced data like dash-network-seeds/seeds/ is kept), with the router fix applied to key-wallet/.../transaction_router/mod.rs.
• [patch."https://github.com/dashpay/rust-dashcore"] in the workspace Cargo.toml redirects all 12 crates that resolve from that git source to the vendored paths (patching only some would split dashcore into two incompatible instances), and the vendor dir is excluded from the workspace.

Verification:

• cargo tree -p platform-wallet -i key-wallet → resolves from third_party/rust-dashcore/key-wallet (all 12 crates repointed).
• rs-unified-sdk-jni (the libdash_sdk_jni.so crate) cargo check clean → the AAR rebuild will pick up the fix.
• router_fix_debits_coinjoin_asset_lock_spend_on_rescan — the stuck case: check_core_transaction alone (no broadcast mitigation) marks the spent CoinJoin UTXO spent on a block/rescan, so the balance settles to previous − inputs + change. This passes only because the vendored router fix is active.
• broadcast_mitigation_and_router_fix_do_not_double_debit — runs both paths in both orders; balance lands at exactly previous − inputs + change (no double debit). The broadcast mitigation is kept as belt-and-braces and is idempotent with the router fix.

Full platform-wallet suite (427) + clippy green. This [patch]/third_party/ vendoring is temporary — remove it once the fix lands upstream and the rev is bumped past it (the exact upstream diff is in the earlier comment).

Kotlin/JNI surface: unchanged.

[thepastaclaw]

Code Review

PRIOR RECONCILIATION: all 4 required prior findings are STILL VALID at HEAD da97eec. CARRIED-FORWARD PRIOR FINDINGS: prior-privacy-308 (CoinJoin de-mixing), prior-swift-preflight-e159 (Swift UI gate blocks the new funding path), and prior-no-utxos-846 (NoUtxosAvailable error-mapping gap) are unchanged — none of their code was touched by the e1593f4..da97eec delta, confirmed by diff. NEW FINDINGS IN LATEST DELTA: none beyond a refined root-cause analysis of prior-persistence-e159 — the delta's vendored rust-dashcore genuinely fixes TransactionRouter::get_relevant_account_types(AssetLock) to include CoinJoin/DashPay (verified in transaction_router/mod.rs:137-159, matching the PR's stated intent), and in isolation this lets a rescan durably debit a CoinJoin-funded asset-lock spend (proven by the new router_fix_debits_coinjoin_asset_lock_spend_on_rescan test). However, tracing the full production pipeline (broadcast → debit_router_omitted_asset_lock_spends → dash-spv scan → record/changeset → persistence → restart → resume) shows the workspace's own broadcast-time mitigation (build.rs:453-514, invoked synchronously at build.rs:967-969, before wait_for_proof) removes the CoinJoin UTXO from the in-memory account map before any scan can see it. check_transaction_for_match (account_checker.rs:668-696) determines a spend purely via self.utxos.get(&input.previous_output) (line 671); with the entry already gone, no match/record is produced for the CoinJoin account, so derive_spent_utxos (core_bridge.rs:492-502), which walks only record.input_details, never queues that UTXO for deletion. load_from_persistor (manager/load.rs:32-102) reloads the wallet_info snapshot verbatim with no rescan-based reconciliation, and normal SPV resume continues forward from synced_height rather than re-scanning already-confirmed blocks, so the stale row does not self-heal on an ordinary restart. The new broadcast_mitigation_and_router_fix_do_not_double_debit test's Order-A branch (explicitly labeled "production timing") only asserts the in-memory aggregate, masking this gap. This is the same #1507 persistence defect as before, now proven to survive specifically because of the retained mitigation's ordering relative to the genuinely-fixed router — kept as STILL VALID / blocking. ADDITIONAL CUMULATIVE FINDINGS: the vendored ~190K-line rust-dashcore tree was checked for integration soundness — the [patch] table covers all 12 git-sourced crates with matching package names, the router-fix diff is correctly scoped, and no split-dependency or injected-code risk was found; the tree's size is noted only as an internal follow-up, not a review blocker, since the PR documents it as temporary.

Source: orchestrator openai/gpt-5.6-sol (reasoning=high; orchestration-only); reviewers sonnet5-general (claude-sonnet-5, completed), opus-general (claude-opus-4-8, completed), sonnet5-security-auditor (claude-sonnet-5, completed), opus-security-auditor (claude-opus-4-8, completed), sonnet5-ffi-engineer (claude-sonnet-5, completed), opus-ffi-engineer (claude-opus-4-8, completed); verifier sonnet5 (claude-sonnet-5, completed). Experiment: sonnet-primary-opus-quarter-sample-20260710 / sonnet_opus_sample / bucket 0.

🔴 1 blocking | 🟡 2 suggestion(s)

1 additional finding(s) omitted (not in diff).

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/rs-platform-wallet/src/wallet/asset_lock/build.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:453-969: Vendored router fix cannot persist the CoinJoin/DashPay debit because the retained broadcast-time mitigation strips the UTXO before any scan can act on it — #1507 still recurs on restart
  CARRIED-FORWARD prior-persistence-e159, re-verified STILL VALID at HEAD da97eec4 by tracing the full pipeline from first principles. The vendored `TransactionRouter::get_relevant_account_types(TransactionType::AssetLock)` (third_party/rust-dashcore/key-wallet/src/transaction_checking/transaction_router/mod.rs:137-159) now correctly includes `CoinJoin`, `DashpayReceivingFunds`, and `DashpayExternalAccount` — this part of the fix is real, and in isolation (no broadcast mitigation) it does let a scan durably debit a CoinJoin-funded asset-lock spend, as the new `router_fix_debits_coinjoin_asset_lock_spend_on_rescan` test proves.

  But `debit_router_omitted_asset_lock_spends` (build.rs:453-514) was not removed, and it still runs synchronously at step 4b immediately after broadcast (build.rs:967-969) — strictly before `wait_for_proof` (step 5), i.e. before dash-spv's mempool/confirmation scan ever gets to observe the transaction. That method directly `.remove()`s the spent outpoint from the CoinJoin/DashPay account's `utxos` map with no changeset queued (only an in-memory `update_balance()` + atomics republish, build.rs:497-504).

  `ManagedCoreFundsAccount::check_transaction_for_match` (third_party/rust-dashcore/key-wallet/src/transaction_checking/account_checker.rs:668-696) determines a spend purely via `self.utxos.get(&input.previous_output)` (line 671). Once the mitigation has already removed that entry, this returns `None`, `sent` stays 0, no output pays the CoinJoin account either (change goes to BIP44), so `has_addresses` is false and the CoinJoin account produces no match — no `WalletEvent::TransactionDetected`, no `record_transaction` call for that account. `derive_spent_utxos` (packages/rs-platform-wallet/src/changeset/core_bridge.rs:492-502) walks only `record.input_details`, which is therefore empty for this spend, so the persistence layer is never told to delete the CoinJoin UTXO row.

  `load_from_persistor` (packages/rs-platform-wallet/src/manager/load.rs:32-102) rehydrates `PlatformWalletInfo` directly from the persisted `wallet_info` snapshot with no rescan-based reconciliation step, and normal SPV resume continues forward from `synced_height` rather than re-processing already-confirmed blocks — so a normal restart does not naturally re-trigger the router-fixed scan against the already-mined transaction. The result: on the next ordinary app restart, the stale CoinJoin UTXO row reloads, `update_balance()` recounts it, and the wallet balance is inflated by exactly the spent amount again — the same dashpay/dash-wallet#1507 regression this PR's title claims to close.

  The new `broadcast_mitigation_and_router_fix_do_not_double_debit` test's Order-A branch (build.rs:1812-1823, explicitly commented "production timing") only asserts `aggregate_total` — an in-memory recompute that is trivially correct because the mitigation's raw `.remove()` alone makes it so — never that a persisted record/changeset was produced for the CoinJoin account. This passes while the persistence gap remains open, creating false confidence. Fix by either (a) removing `debit_router_omitted_asset_lock_spends` now that the router fix is vendored, forcing a synchronous local `check_core_transaction` call on the just-broadcast tx so the router-fixed, event-emitting path is what performs the debit, or (b) routing the mitigation's removal through the same event/changeset-emitting API the pinned path uses so the deletion is itself durable.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:1812-1823: Double-debit regression test validates only the in-memory balance sum, masking the persistence gap above
  The Order-A branch of `broadcast_mitigation_and_router_fix_do_not_double_debit` (production-timing order: mitigation then scan) asserts `aggregate_total(&manager)` equals the expected post-spend total, which holds trivially because the manual `.utxos.remove()` mutation alone makes `update_balance()` correct — it says nothing about whether the subsequent `check_core_transaction` scan produced a `TransactionRecord`/`WalletEvent` for the CoinJoin account. It did not (see the paired blocking finding), so this test's green result masks the fact that the router-fix + mitigation composition silently drops the CoinJoin account's persisted transaction record in the real production order. Add an assertion on the scan's `result.affected_accounts` / `is_relevant`, or a `coinjoin_has_utxo` check immediately after the scan step (before the mitigation call) in the Order-A branch, to make this gap fail loudly in CI.

In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ShieldedFundFromAssetLockView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ShieldedFundFromAssetLockView.swift:795-845: Shield preflight still enforces single-BIP44 coverage, blocking the cross-account union-funding path from the example UI
  CARRIED-FORWARD prior-swift-preflight-e159, re-verified STILL VALID — no Swift files were touched by this delta. Rust now treats `account_index` only as the primary BIP44 account supplying change/reservations while shielded input selection spans BIP44 + BIP32 + CoinJoin + DashPay. But `coreAccountOptions` still filters to standard BIP44 accounts with positive balance only (line ~795), and `canSubmit` still gates submission on `selectedCoreAccountBalanceDuffs >= amount` (line ~843). A wallet with, e.g., 0.09 DASH BIP44 plus a CoinJoin balance still cannot submit a 0.20 DASH shield from this screen even though Rust can now fund it — the request is blocked client-side before the FFI call is ever made, leaving the new union-funding capability unreachable end-to-end from the example app. Keep BIP44-account selection for the primary/change account, but let the FFI call perform the authoritative union coin-selection check rather than pre-gating on the BIP44 slice alone.

[thepastaclaw]

🔴 Blocking: Vendored router fix cannot persist the CoinJoin/DashPay debit because the retained broadcast-time mitigation strips the UTXO before any scan can act on it — #1507 still recurs on restart

CARRIED-FORWARD prior-persistence-e159, re-verified STILL VALID at HEAD da97eec by tracing the full pipeline from first principles. The vendored TransactionRouter::get_relevant_account_types(TransactionType::AssetLock) (third_party/rust-dashcore/key-wallet/src/transaction_checking/transaction_router/mod.rs:137-159) now correctly includes CoinJoin, DashpayReceivingFunds, and DashpayExternalAccount — this part of the fix is real, and in isolation (no broadcast mitigation) it does let a scan durably debit a CoinJoin-funded asset-lock spend, as the new router_fix_debits_coinjoin_asset_lock_spend_on_rescan test proves.

But debit_router_omitted_asset_lock_spends (build.rs:453-514) was not removed, and it still runs synchronously at step 4b immediately after broadcast (build.rs:967-969) — strictly before wait_for_proof (step 5), i.e. before dash-spv's mempool/confirmation scan ever gets to observe the transaction. That method directly .remove()s the spent outpoint from the CoinJoin/DashPay account's utxos map with no changeset queued (only an in-memory update_balance() + atomics republish, build.rs:497-504).

ManagedCoreFundsAccount::check_transaction_for_match (third_party/rust-dashcore/key-wallet/src/transaction_checking/account_checker.rs:668-696) determines a spend purely via self.utxos.get(&input.previous_output) (line 671). Once the mitigation has already removed that entry, this returns None, sent stays 0, no output pays the CoinJoin account either (change goes to BIP44), so has_addresses is false and the CoinJoin account produces no match — no WalletEvent::TransactionDetected, no record_transaction call for that account. derive_spent_utxos (packages/rs-platform-wallet/src/changeset/core_bridge.rs:492-502) walks only record.input_details, which is therefore empty for this spend, so the persistence layer is never told to delete the CoinJoin UTXO row.

load_from_persistor (packages/rs-platform-wallet/src/manager/load.rs:32-102) rehydrates PlatformWalletInfo directly from the persisted wallet_info snapshot with no rescan-based reconciliation step, and normal SPV resume continues forward from synced_height rather than re-processing already-confirmed blocks — so a normal restart does not naturally re-trigger the router-fixed scan against the already-mined transaction. The result: on the next ordinary app restart, the stale CoinJoin UTXO row reloads, update_balance() recounts it, and the wallet balance is inflated by exactly the spent amount again — the same dashpay/dash-wallet#1507 regression this PR's title claims to close.

The new broadcast_mitigation_and_router_fix_do_not_double_debit test's Order-A branch (build.rs:1812-1823, explicitly commented "production timing") only asserts aggregate_total — an in-memory recompute that is trivially correct because the mitigation's raw .remove() alone makes it so — never that a persisted record/changeset was produced for the CoinJoin account. This passes while the persistence gap remains open, creating false confidence. Fix by either (a) removing debit_router_omitted_asset_lock_spends now that the router fix is vendored, forcing a synchronous local check_core_transaction call on the just-broadcast tx so the router-fixed, event-emitting path is what performs the debit, or (b) routing the mitigation's removal through the same event/changeset-emitting API the pinned path uses so the deletion is itself durable.

_{source: ['sonnet5-general', 'sonnet5-security-auditor', 'opus-security-auditor']}

[bfoss765]

Fixed in a5ea9e5 by taking option (a): removed debit_router_omitted_asset_lock_spends entirely (method + the step-4b call site). You correctly traced the gap — the mitigation's bare in-memory utxos.remove() stripped the spent CoinJoin/DashPay UTXO before dash-spv's scan, so the router-fixed check_core_transaction found no UTXO, emitted no TransactionRecord for that account, and derive_spent_utxos walked an empty input_details → the deletion was never persisted → #1507 re-inflates on restart.

Now that the router fix is vendored (get_relevant_account_types(AssetLock) includes CoinJoin + DashpayReceivingFunds + DashpayExternalAccount), the normal pipeline owns the debit and its persistence: the scan produces a record whose input_details cover the spent outpoints, which flows WalletEvent::TransactionDetected → core_bridge::derive_spent_utxos → the persister, surviving restart. In production the asset-lock flow blocks in wait_for_proof until the tx is IS-locked/ChainLocked, by which point dash-spv has already processed it through that path.

Test evidence (cargo test -p platform-wallet --lib, 429 passed / 0 failed; asset_lock subset 24/24):

• The false-confidence broadcast_mitigation_and_router_fix_do_not_double_debit test (the sibling-thread concern: it only asserted the in-memory aggregate_total) is replaced by router_fix_records_spent_coinjoin_input_for_persistence. That test runs the mitigation-free scan and asserts the emitted record is a CoinJoin record whose input_details[i] resolve — via the same record.transaction.input[detail.index].previous_output lookup derive_spent_utxos uses — to the spent outpoint. That is the exact data the persister deletes by, so it closes the persistence gap directly rather than masking it.
• router_fix_debits_coinjoin_asset_lock_spend_on_rescan (unchanged) continues to prove the in-memory debit + no re-inflation when the scan runs standalone with no mitigation in play.

No queue_asset_lock_changeset fallback was needed: the router fix demonstrably covers the spend through the normal record→changeset→persist pipeline, so the preferred removal was sufficient.

[thepastaclaw]

Resolved in a5ea9e5 — Vendored router fix cannot persist the CoinJoin/DashPay debit because the retained broadcast-time mitigation strips the UTXO before any scan can act on it — #1507 still recurs on restart 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.}

[thepastaclaw]

🟡 Suggestion: Double-debit regression test validates only the in-memory balance sum, masking the persistence gap above

The Order-A branch of broadcast_mitigation_and_router_fix_do_not_double_debit (production-timing order: mitigation then scan) asserts aggregate_total(&manager) equals the expected post-spend total, which holds trivially because the manual .utxos.remove() mutation alone makes update_balance() correct — it says nothing about whether the subsequent check_core_transaction scan produced a TransactionRecord/WalletEvent for the CoinJoin account. It did not (see the paired blocking finding), so this test's green result masks the fact that the router-fix + mitigation composition silently drops the CoinJoin account's persisted transaction record in the real production order. Add an assertion on the scan's result.affected_accounts / is_relevant, or a coinjoin_has_utxo check immediately after the scan step (before the mitigation call) in the Order-A branch, to make this gap fail loudly in CI.

_{source: ['sonnet5-general']}

[bfoss765]

Resolved by a5ea9e5906. The masking test you flagged (broadcast_mitigation_and_router_fix_do_not_double_debit, whose Order-A branch asserted only aggregate_total) was removed along with the broadcast-time debit mitigation itself, and replaced with router_fix_records_spent_coinjoin_input_for_persistence.

The new test asserts exactly the gap you asked to fail loudly, at the persistence-feeding seam rather than the balance sum:

• coinjoin_has_utxo(&manager, &spent_outpoint) is true before the scan;
• it runs the normal-pipeline check_core_transaction (no mitigation) and asserts a CoinJoin TransactionRecord exists whose input_details resolve — through the same record.transaction.input[detail.index].previous_output lookup derive_spent_utxos uses — to the spent CoinJoin outpoint. This is the record whose absence was the chore(release): update changelog and bump version to 0.25.7 #1507 gap: without it the persister is never told to delete the row;
• and coinjoin_has_utxo(...) is false after the scan (the in-memory debit did happen).

What it covers: the scan emits a CoinJoin record carrying the persistable input_details for the spent outpoint — the exact hook that flows WalletEvent::TransactionDetected → core_bridge::derive_spent_utxos → persister. What it does NOT do: drive a literal persister write → restart → reload round-trip asserting the balance stays deflated after restart; it validates the record that feeds persistence, not the on-disk survival end to end. That end-to-end round-trip would be a stronger follow-up, but the silent-drop regression you identified now fails loudly in CI.

[thepastaclaw]

Resolved in a5ea9e5 — Double-debit regression test validates only the in-memory balance sum, masking the persistence gap above 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]

Follow-up: CoinJoin address gap-limit starvation — the mismatch that survived clean re-creation (ce482fd)

Confirmed the working hypothesis. On the router-fixed lib the mismatch survives a clean re-creation + full rescan because of a third, distinct bug: CoinJoin address gap-limit starvation.

Both numbers (task 1):

• key-wallet CoinJoin discovery is dynamic — the pool watches highest_used + DEFAULT_COINJOIN_GAP_LIMIT addresses — and that constant was 30.
• dashj (org.dashj:dashj-core:22.0.3) watches its DeterministicKeyChain lookahead DEFAULT_LOOKAHEAD_SIZE = 100 for the CoinJoin keychain.

Why it explains the evidence exactly: DIP-9 mixing sprays denominations across many sequential addresses. A run of >30 unused CoinJoin addresses stalls the SDK's discovery; dashj (100 lookahead) bridges it. A single mixing tx whose wallet-relevant addresses all sit past the 30-window is skipped entirely — its outputs never become UTXOs (your dashj-only set) and its spent inputs are never debited (your sdk-only set): one skipped block, both directions, matching the 18 missing txs and 23200232 − 2100021 = 21100211.

Fix (task 2): DEFAULT_COINJOIN_GAP_LIMIT 30 → 100 in the vendored key-wallet, matching dashj so the SDK discovers every CoinJoin tx dashj does. It's a Rust-side default (no API change → no Kotlin/JNI change; the gap is already per-pool-configurable via set_gap_limit for any future caller). Cost: a CoinJoin account watches two pools, so ~200 pre-derived/filter-matched addresses per account instead of ~60 — BIP158 matching is a set intersection, so per-block cost is negligible and the derivation is a one-time keychain expansion.

Upstream patch (added to the patch set):

diff --git a/key-wallet/src/gap_limit.rs b/key-wallet/src/gap_limit.rs
--- a/key-wallet/src/gap_limit.rs
+++ b/key-wallet/src/gap_limit.rs
@@ -16,8 +16,31 @@
 /// Standard gap limit for internal (change) addresses
 pub const DEFAULT_INTERNAL_GAP_LIMIT: u32 = 30;
 
-/// Standard gap limit for CoinJoin addresses
-pub const DEFAULT_COINJOIN_GAP_LIMIT: u32 = 30;
+/// Standard gap limit for CoinJoin addresses.
+///
+/// Set to match dashj's `DeterministicKeyChain` lookahead
+/// (`DEFAULT_LOOKAHEAD_SIZE = 100`), which is the effective discovery window
+/// dashj-core (the reference wallet, `org.dashj:dashj-core:22.x`) watches for
+/// CoinJoin keychains. Address discovery here is dynamic — the pool watches
+/// `highest_used + gap_limit` addresses — so this bounds the largest run of
+/// UNUSED CoinJoin addresses the wallet can bridge during a filter/rescan
+/// before discovery stalls.
+///
+/// The previous value of 30 starved heavy mixers: DIP-9 CoinJoin mixing
+/// sprays denominations across many sequential addresses, and a gap wider
+/// than 30 unused addresses caused the SDK to stop discovering — it then
+/// missed both the txs that CREATED far-index UTXOs and the txs that SPENT
+/// nearer ones (one skipped block, both directions), leaving a persistent
+/// balance mismatch vs dashj that survived a clean re-creation + full rescan
+/// (dashpay/platform#4073, dashpay/dash-wallet#1507). 100 restores parity
+/// with dashj's window so the SDK discovers every CoinJoin tx dashj does.
+///
+/// Cost is modest: the CoinJoin account watches two pools (external +
+/// internal), so this pre-derives and filter-matches ~200 CoinJoin addresses
+/// per account instead of ~60. BIP158 filter matching is a set intersection,
+/// so the extra scripts add negligible per-block work; the derivation is a
+/// one-time keychain expansion.
+pub const DEFAULT_COINJOIN_GAP_LIMIT: u32 = 100;
 
 /// Standard gap limit for special purpose keys (identity, provider keys)
 pub const DEFAULT_SPECIAL_GAP_LIMIT: u32 = 5;

The 3000030 gap (task 4): 3000030 = 3 × 1000010 (the 0.01-DASH+fee CoinJoin denomination) = three CoinJoin UTXOs counted in the aggregate WalletCoreBalance.total() (which sums confirmed+unconfirmed+immature+locked) but excluded from a confirmed-only unspent-sum — i.e. the normal total-vs-confirmed bucket delta (in-flight/mempool CoinJoin denoms), not an accounting bug, and orthogonal to the discovery/spend fixes. Worth reconfirming against the device's exact unspent-sum bucket definition once the two large inconsistencies are gone.

Test (task 5): coinjoin_gap_limit_discovers_addresses_beyond_the_old_window — a fresh CoinJoin account pre-generates and filter-recognizes an address at index 50 (beyond the old 30-window); a tx paying it is discovered and its UTXO tracked. Verified it fails on the old gap of 30 and passes at 100. Full platform-wallet suite (428) + clippy green; rs-unified-sdk-jni builds.

Kotlin/JNI surface: unchanged. Existing persisted wallets carry the old gap in their persisted pool state, but the device path is a clean re-creation, which picks up the new default; a migration/ bump for pre-existing wallets can be a follow-up if needed.

[thepastaclaw]

Code Review

PRIOR RECONCILIATION: prior-persistence-e159 and prior-double-debit-test-da97 are OUTDATED because every shielded asset-lock transaction still matches its tracked credit-output account, producing a record whose complete raw input list is reconciled by Swift and Kotlin persistence independently of Rust input_details; confirmed spent rows are then excluded from restart restoration. CARRIED-FORWARD PRIORS: prior-privacy-308, prior-swift-preflight-e159, and prior-no-utxos-846 remain valid and are included below. NEW FINDINGS IN THE LATEST DELTA: none; the CoinJoin gap-limit increase and focused discovery test introduce no additional issue.

Source: orchestrator openai/gpt-5.6-sol (reasoning=high; orchestration-only); reviewers codex-general gpt-5.6-sol completed, sonnet5-general claude-sonnet-5 failed, codex-security-auditor gpt-5.6-sol failed, sonnet5-security-auditor claude-sonnet-5 failed, codex-ffi-engineer gpt-5.6-sol completed, sonnet5-ffi-engineer claude-sonnet-5 failed; verifier codex fallback gpt-5.6-sol completed after sonnet5 claude-sonnet-5 failed. Experiment: sonnet-primary-opus-quarter-sample-20260710 / sonnet_primary / bucket 1.

🟡 2 suggestion(s) | 💬 1 nitpick(s)

1 additional finding(s) omitted (not in diff).

🤖 Prompt for all review comments with AI agents

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

In `packages/rs-platform-wallet/src/wallet/asset_lock/build.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:345-350: Union funding can de-mix CoinJoin inputs through co-spending and clear change
  CARRIED-FORWARD prior-privacy-308 — STILL VALID. The builder combines the primary BIP44 account's candidates with every other funding account's UTXOs, selects across that union using LargestFirst, and retains the primary BIP44 change destination. Selecting a CoinJoin input can therefore link it to clear BIP44 inputs under the common-input-ownership heuristic and link any surplus to a clear BIP44 change address. Prefer a CoinJoin-only candidate set when it can cover the target, or use a privacy-preserving change policy when mixed inputs are selected.

In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ShieldedFundFromAssetLockView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ShieldedFundFromAssetLockView.swift:795-843: Swift preflight blocks cross-account union funding
  CARRIED-FORWARD prior-swift-preflight-e159 — STILL VALID. Rust uses account_index as the primary/change BIP44 account while selecting inputs from the union of BIP44, BIP32, CoinJoin, and DashPay accounts. This view exposes only positive-balance standard BIP44 accounts and requires the selected account's confirmed balance to cover the entire amount. A wallet whose BIP44 slice is insufficient but whose union balance is sufficient is therefore rejected before the FFI call. Keep the BIP44 selection for primary/change routing, but let Rust perform the authoritative funds check.

[thepastaclaw]

🟡 Suggestion: Union funding can de-mix CoinJoin inputs through co-spending and clear change

CARRIED-FORWARD prior-privacy-308 — STILL VALID. The builder combines the primary BIP44 account's candidates with every other funding account's UTXOs, selects across that union using LargestFirst, and retains the primary BIP44 change destination. Selecting a CoinJoin input can therefore link it to clear BIP44 inputs under the common-input-ownership heuristic and link any surplus to a clear BIP44 change address. Prefer a CoinJoin-only candidate set when it can cover the target, or use a privacy-preserving change policy when mixed inputs are selected.

_{source: ['codex-general', 'codex-ffi-engineer']}

[bfoss765]

Same disposition as the paired privacy thread (e598425ec6a7): intentional, and left open for visibility rather than silently resolved.

This is the deliberate Option B for this PR — union funding combines the primary BIP44 account's candidates with every other funds account's UTXOs (including CoinJoin), selects across the union, and retains the primary BIP44 change destination. The de-mixing consequences you describe (co-spend linkage under common-input-ownership; clear-change linkage of surplus) are understood and accepted as a conscious privacy trade-off; the locked value lands in the shielded credit output. A CoinJoin-only-preferred candidate set and/or a privacy-preserving change policy for mixed inputs is a reasonable follow-up but out of scope here.

[thepastaclaw]

Still intentionally deferred at 189e068 — the CoinJoin co-spending and clear-change privacy trade-off remains unchanged. The maintainer previously accepted this as the deliberate Option B design; the finding is retained for visibility and no code change is requested in this review.

[thepastaclaw]

💬 Nitpick: NoUtxosAvailable bypasses the typed insufficient-funds error

CARRIED-FORWARD prior-no-utxos-846 — STILL VALID. LargestFirst returns SelectionError::NoUtxosAvailable when filtering leaves no spendable candidates, but map_builder_error only promotes the two InsufficientFunds variants. The zero-candidate case consequently becomes AssetLockTransaction(String) while partial shortfalls use AssetLockInsufficientFunds. Preserve the requested amount when constructing or mapping this error so callers receive the same typed shortfall contract for an empty candidate set.

_{source: ['codex-general', 'codex-ffi-engineer']}

[bfoss765]

Per @HashEngineering's request, I've moved the two rust-dashcore changes in this vendored tree upstream as proper PRs against dashpay/rust-dashcore:dev:

• fix(key-wallet): cover CoinJoin/DashPay accounts for AssetLock routing rust-dashcore#867 — AssetLock transaction-router account coverage (the balance-inflation fix; includes an end-to-end regression test). This is the undisputed correctness fix.
• feat(key-wallet): widen DEFAULT_COINJOIN_GAP_LIMIT 30 -> 100 for dashj parity (proposal) rust-dashcore#868 — DEFAULT_COINJOIN_GAP_LIMIT 30→100 for dashj lookahead parity, filed as a proposal with the two-sided evidence (scan algorithm is already correct at gap 30; field heavy-mixers have >30 unused runs that only a wider window can bridge). I've invited the June Slack discussion onto that PR to settle the value.

Both port cleanly onto dev (only 3 commits of drift). One note on #867: dev added a fund_bearing_account_types() helper since the pin, so I reused it instead of the inline list from this vendored copy — functionally identical.

Question on the vendored third_party/rust-dashcore copy here: would you prefer we drop it now and have the Kotlin SDK point at a branch/commit carrying #867, or keep it until the next SDK pin bump? Happy to do either — just want to avoid the vendored tree and upstream diverging.

[bfoss765]

prior-no-utxos-846 fixed: map_builder_error now threads the requested target so SelectionError::NoUtxosAvailable maps to AssetLockInsufficientFunds { available: 0, required }, keeping the empty candidate set on the same typed shortfall contract as partial shortfalls (carried InsufficientFunds amounts still take precedence). Unit-tested. The CoinJoin-privacy finding is left for a conscious product decision, and no third_party/gap-constant code was touched.

[thepastaclaw]

Code Review

Source: reviewers codex/general=gpt-5.6-sol(completed_parseable_output_with_nonzero_wrapper_exit), sonnet5/general=claude-sonnet-5(completed); verifier=claude-sonnet-5; orchestrator=openai/gpt-5.6-sol (orchestration-only, not a reviewer/verifier).

Carried-Forward Prior Findings

Two prior suggestions remain unresolved and are carried forward unchanged: (1) union coin selection in build.rs still co-spends CoinJoin inputs with clear BIP44 UTXOs via LargestFirst and routes surplus to a clear BIP44 change address, weakening the privacy of newly-spendable mixed coins; (2) the SwiftExampleApp preflight (ShieldedFundFromAssetLockView.swift) still gates canSubmit on the single selected BIP44 account's balance, rejecting requests that Rust's union selector could actually satisfy. Neither file was touched by this push.

New Findings In Latest Delta

None. This push is a single, well-scoped fix: SelectionError::NoUtxosAvailable now maps to the typed AssetLockInsufficientFunds { available: 0, required: requested } instead of the generic string error, closing out prior nitpick 0412ce118d37. Verified directly against build.rs:1029-1034 — the mapping matches the prior suggestion, threads target_duffs through as required at the call site (build.rs:361), and is covered by a new unit test (no_utxos_available_maps_to_typed_insufficient_funds, build.rs:1073) that checks both the zero-candidate and partial-shortfall cases.

Source (experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_primary, bucket 3): reviewers codex/general=gpt-5.6-sol(completed_parseable_output_with_nonzero_wrapper_exit); sonnet5/general=claude-sonnet-5(completed); verifier=verifier-sonnet5-4074-1783795403=claude-sonnet-5; orchestrator=openai/gpt-5.6-sol reasoning=high (orchestration-only, not a reviewer/verifier).

🟡 2 suggestion(s)

Posting note: review_poster could not post an inline/top-level review cleanly; posting exact-SHA body-only review from the same verifier output.

Verified Findings

suggestion: Union funding can de-mix CoinJoin inputs through co-spending and clear change

packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:317-352

CARRIED-FORWARD (95b0cf72ed51) — STILL VALID. The builder seeds the primary BIP44 account (set_funding(primary_funds, &acc)) and appends every other funding account's spendable UTXOs (add_inputs(extra_inputs)), then selects across that union with SelectionStrategy::LargestFirst. Change still returns to the primary BIP44 account. When LargestFirst picks a CoinJoin input, it can co-spend that mixed coin alongside clear BIP44 inputs — linking them under the common-input-ownership heuristic — and route any surplus to a clear BIP44 change address, undermining the privacy of the previously-mixed coins this PR intentionally makes spendable. Unchanged since the prior review; this push only touched the error-mapping function. Consider preferring a CoinJoin-only candidate set when it alone can cover the target, or routing change from mixed-input selections through a privacy-preserving path.

suggestion: Swift preflight blocks cross-account union funding

packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ShieldedFundFromAssetLockView.swift:795-845

CARRIED-FORWARD (595152091b4b) — STILL VALID. coreAccountOptions filters to positive-balance standard BIP44 accounts only, and canSubmit requires selectedCoreAccountBalanceDuffs >= amount — the single selected BIP44 account's confirmed balance must alone cover the entire request. But Rust's asset-lock builder treats account_index only as the primary/change account while selecting inputs from the union of BIP44, BIP32, CoinJoin, and DashPay accounts. A wallet whose BIP44 slice is insufficient but whose union balance covers the amount is therefore still rejected client-side before the FFI call that would succeed. Keep the BIP44 picker for primary/change routing, but let Rust's union selector perform the authoritative funds check (don't gate canSubmit on selectedCoreAccountBalanceDuffs alone).

🤖 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.

- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:317-352: Union funding can de-mix CoinJoin inputs through co-spending and clear change
  CARRIED-FORWARD (95b0cf72ed51) — STILL VALID. The builder seeds the primary BIP44 account (`set_funding(primary_funds, &acc)`) and appends every other funding account's spendable UTXOs (`add_inputs(extra_inputs)`), then selects across that union with `SelectionStrategy::LargestFirst`. Change still returns to the primary BIP44 account. When `LargestFirst` picks a CoinJoin input, it can co-spend that mixed coin alongside clear BIP44 inputs — linking them under the common-input-ownership heuristic — and route any surplus to a clear BIP44 change address, undermining the privacy of the previously-mixed coins this PR intentionally makes spendable. Unchanged since the prior review; this push only touched the error-mapping function. Consider preferring a CoinJoin-only candidate set when it alone can cover the target, or routing change from mixed-input selections through a privacy-preserving path.
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ShieldedFundFromAssetLockView.swift:795-845: Swift preflight blocks cross-account union funding
  CARRIED-FORWARD (595152091b4b) — STILL VALID. `coreAccountOptions` filters to positive-balance standard BIP44 accounts only, and `canSubmit` requires `selectedCoreAccountBalanceDuffs >= amount` — the single selected BIP44 account's confirmed balance must alone cover the entire request. But Rust's asset-lock builder treats `account_index` only as the primary/change account while selecting inputs from the union of BIP44, BIP32, CoinJoin, and DashPay accounts. A wallet whose BIP44 slice is insufficient but whose union balance covers the amount is therefore still rejected client-side before the FFI call that would succeed. Keep the BIP44 picker for primary/change routing, but let Rust's union selector perform the authoritative funds check (don't gate `canSubmit` on `selectedCoreAccountBalanceDuffs` alone).

[thepastaclaw]

Code Review

Source: reviewers codex/general=gpt-5.6-sol(completed_parseable_output_with_nonzero_wrapper_exit), sonnet5/general=claude-sonnet-5(completed), codex/security-auditor=gpt-5.6-sol(completed), sonnet5/security-auditor=claude-sonnet-5(completed), codex/rust-quality=gpt-5.6-sol(completed), sonnet5/rust-quality=claude-sonnet-5(completed), codex/ffi-engineer=gpt-5.6-sol(completed), sonnet5/ffi-engineer=claude-sonnet-5(completed); verifier=claude-sonnet-5; orchestrator=openai/gpt-5.6-sol (orchestration-only, not a reviewer/verifier).

Carried-Forward Prior Findings

Both prior findings from the 380645a review are STILL VALID — the underlying files are byte-identical to that review (git diff 380645a8a2..HEAD returns 0 lines for both). (1) build_asset_lock_tx_from_all_funding_accounts in rs-platform-wallet/src/wallet/asset_lock/build.rs funds asset-lock transactions from the UNION of all Core funding accounts (BIP44 + BIP32 + CoinJoin + DashPay) using SelectionStrategy::LargestFirst, meaning a CoinJoin-mixed UTXO can be co-spent with clear BIP44 inputs in the same transaction — a common-input-ownership heuristic risk that de-mixes the CoinJoin output. This is now explicitly documented as intentional/union-funding in orchestration.rs's doc comment (lines 187-233), which also acknowledges an interim reservation-ledger gap as safe-by-construction under shield_guard + wallet write-lock single-flighting. (2) The Swift preflight canSubmit gate in ShieldedFundFromAssetLockView.swift (lines ~795-845) only checks selectedCoreAccountBalanceDuffs >= amount against the single selected BIP44 account, which is inconsistent with the backend's union-funding behavior — it can both wrongly disable submission when the union of accounts would cover the amount, and wrongly enable it in edge cases, since it doesn't reflect the real funding source.

New Findings In Latest Delta

The delta between 380645a and HEAD (b43caed) removes the vendored third_party/rust-dashcore directory and switches to a Cargo [patch] on a personal fork (bfoss765/rust-dashcore, rev e8c7335) pinned by SHA, carrying a fix for TransactionRouter::get_relevant_account_types(AssetLock) omitting CoinJoin/DashPay accounts (root-caused in commit e1593f4/380645a8) plus a CoinJoin gap-limit bump (30->100). New findings surfaced in this delta: (1) the [patch] table repoints 12 crates to an unpublished personal-fork branch — acceptable as a documented, SHA-pinned, TEMPORARY stopgap (author's own comment states intent to drop once upstream lands), but worth flagging as a merge-blocking dependency risk if not tracked to resolution. (2) The FFI-facing doc comments in rs-platform-wallet-ffi/src/shielded_send.rs:811 (platform_wallet_manager_shielded_fund_from_asset_lock) and the Swift wrapper PlatformWalletManagerShieldedFunding.swift:110 still describe account_index/fundingAccountIndex as the sole UTXO funding source, now stale relative to the corrected orchestration.rs union-funding doc — this can mislead FFI/Swift consumers about actual funding semantics.

Source (experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_primary, bucket 2): reviewers codex/general=gpt-5.6-sol(completed_parseable_output_with_nonzero_wrapper_exit); sonnet5/general=claude-sonnet-5(completed); codex/security-auditor=gpt-5.6-sol(completed); sonnet5/security-auditor=claude-sonnet-5(completed); codex/rust-quality=gpt-5.6-sol(completed); sonnet5/rust-quality=claude-sonnet-5(completed); codex/ffi-engineer=gpt-5.6-sol(completed); sonnet5/ffi-engineer=claude-sonnet-5(completed); verifier=verifier-sonnet5-4074-1783808063=claude-sonnet-5; orchestrator=openai/gpt-5.6-sol reasoning=high (orchestration-only, not a reviewer/verifier).

🟡 4 suggestion(s)

Posting note: review_poster could not fetch GitHub’s >300-file PR diff (HTTP 406), so this exact-SHA review was posted body-only from the same policy-gated verifier output.

Verified Findings

suggestion: Union funding across all Core accounts can co-spend CoinJoin-mixed UTXOs with clear inputs, degrading CoinJoin privacy

packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:234-352

build_asset_lock_tx_from_all_funding_accounts (packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:234-352) builds the funding input set from the UNION of every spendable Core account — BIP44, BIP32, CoinJoin, and DashPay — then applies SelectionStrategy::LargestFirst across that combined set. Because CoinJoin-mixed UTXOs and regular BIP44 UTXOs can end up selected into the same asset-lock transaction, an observer applying the common-input-ownership heuristic can link the mixed coins back to the user's clear balance, undermining the purpose of the CoinJoin mix. This is a known, intentional tradeoff — orchestration.rs (lines 187-233) documents the union-funding design and an interim reservation-ledger caveat as safe under the current single-flight guard — but the CoinJoin-privacy implication of co-spending in build.rs itself is not called out anywhere near the selection logic, and no opt-out/ordering exists to prefer non-CoinJoin funds first or exclude CoinJoin UTXOs entirely.

suggestion: Swift preflight `canSubmit` check only validates the selected single account balance, not the union-funded total

packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ShieldedFundFromAssetLockView.swift:795-845

ShieldedFundFromAssetLockView.swift computes canSubmit (lines ~795-845) using selectedCoreAccountBalanceDuffs >= amount, where selectedCoreAccountBalanceDuffs reflects only the chosen BIP44 account's confirmed balance. This preflight gate doesn't match the backend's actual funding behavior (union across BIP44/BIP32/CoinJoin/DashPay accounts in build_asset_lock_tx_from_all_funding_accounts), so the UI can present a misleading enabled/disabled state relative to what the transaction builder will actually attempt.

suggestion: Cargo `[patch]` on unpublished personal fork branch for rust-dashcore

Cargo.toml:140-171

Cargo.toml (lines 140-171) adds a [patch."https://github.com/dashpay/rust-dashcore"] table repointing 12 crates (dashcore, dashcore_hashes, dashcore-private, dash-network, dash-network-seeds, dash-spv, dashcore-rpc, dashcore-rpc-json, git-state, key-wallet, key-wallet-ffi, key-wallet-manager) to bfoss765/rust-dashcore rev e8c7335478c046ca0bdf758a68c9a73051aee2ee — a personal fork branch (patch/4074-assetlock-router-1860089e), not an upstream dashpay branch or release. The pin is SHA-locked (safe against silent upstream mutation) and all 12 crates are consistently repointed to the same rev (no split-crate-instance risk), and the author's own comment clearly marks this TEMPORARY pending upstream merge, explaining the root-cause bug (TransactionRouter::get_relevant_account_types(AssetLock) omitting CoinJoin/DashPay, causing balance re-inflation on rescan) and referencing the exact patches applied. The residual risk is availability/bus-factor: the fix lives only on a personal fork branch that could be deleted or rebased, and there's no tracking issue/TODO date to ensure this gets replaced once the fix lands upstream.

suggestion: FFI and Swift wrapper doc comments for asset-lock funding are stale after union-funding change

packages/rs-platform-wallet-ffi/src/shielded_send.rs:811-811

rs-platform-wallet-ffi/src/shielded_send.rs:811 documents account_index on platform_wallet_manager_shielded_fund_from_asset_lock as "selects the BIP44 Core account whose UTXOs fund the asset lock," and the Swift wrapper PlatformWalletManagerShieldedFunding.swift:110 similarly documents fundingAccountIndex as "BIP44 Core account whose UTXOs fund [the lock]." Both comments are now inconsistent with the corrected internal doc in orchestration.rs (lines 187-233), which clarifies that funding is drawn from the union of ALL spendable Core accounts (BIP44 + BIP32 + CoinJoin + DashPay), with account_index only selecting the primary/change-receiving account. FFI/Swift consumers relying on these public-facing doc comments could misunderstand which UTXOs are actually spent.

🤖 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.

- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:234-352: Union funding across all Core accounts can co-spend CoinJoin-mixed UTXOs with clear inputs, degrading CoinJoin privacy
  `build_asset_lock_tx_from_all_funding_accounts` (packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:234-352) builds the funding input set from the UNION of every spendable Core account — BIP44, BIP32, CoinJoin, and DashPay — then applies `SelectionStrategy::LargestFirst` across that combined set. Because CoinJoin-mixed UTXOs and regular BIP44 UTXOs can end up selected into the same asset-lock transaction, an observer applying the common-input-ownership heuristic can link the mixed coins back to the user's clear balance, undermining the purpose of the CoinJoin mix. This is a known, intentional tradeoff — `orchestration.rs` (lines 187-233) documents the union-funding design and an interim reservation-ledger caveat as safe under the current single-flight guard — but the CoinJoin-privacy implication of co-spending in `build.rs` itself is not called out anywhere near the selection logic, and no opt-out/ordering exists to prefer non-CoinJoin funds first or exclude CoinJoin UTXOs entirely.
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ShieldedFundFromAssetLockView.swift:795-845: Swift preflight `canSubmit` check only validates the selected single account balance, not the union-funded total
  `ShieldedFundFromAssetLockView.swift` computes `canSubmit` (lines ~795-845) using `selectedCoreAccountBalanceDuffs >= amount`, where `selectedCoreAccountBalanceDuffs` reflects only the chosen BIP44 account's confirmed balance. This preflight gate doesn't match the backend's actual funding behavior (union across BIP44/BIP32/CoinJoin/DashPay accounts in `build_asset_lock_tx_from_all_funding_accounts`), so the UI can present a misleading enabled/disabled state relative to what the transaction builder will actually attempt.
- [SUGGESTION] Cargo.toml:140-171: Cargo `[patch]` on unpublished personal fork branch for rust-dashcore
  `Cargo.toml` (lines 140-171) adds a `[patch."https://github.com/dashpay/rust-dashcore"]` table repointing 12 crates (dashcore, dashcore_hashes, dashcore-private, dash-network, dash-network-seeds, dash-spv, dashcore-rpc, dashcore-rpc-json, git-state, key-wallet, key-wallet-ffi, key-wallet-manager) to `bfoss765/rust-dashcore` rev `e8c7335478c046ca0bdf758a68c9a73051aee2ee` — a personal fork branch (`patch/4074-assetlock-router-1860089e`), not an upstream `dashpay` branch or release. The pin is SHA-locked (safe against silent upstream mutation) and all 12 crates are consistently repointed to the same rev (no split-crate-instance risk), and the author's own comment clearly marks this TEMPORARY pending upstream merge, explaining the root-cause bug (`TransactionRouter::get_relevant_account_types(AssetLock)` omitting CoinJoin/DashPay, causing balance re-inflation on rescan) and referencing the exact patches applied. The residual risk is availability/bus-factor: the fix lives only on a personal fork branch that could be deleted or rebased, and there's no tracking issue/TODO date to ensure this gets replaced once the fix lands upstream.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/shielded_send.rs:811-811: FFI and Swift wrapper doc comments for asset-lock funding are stale after union-funding change
  `rs-platform-wallet-ffi/src/shielded_send.rs:811` documents `account_index` on `platform_wallet_manager_shielded_fund_from_asset_lock` as "selects the BIP44 Core account whose UTXOs fund the asset lock," and the Swift wrapper `PlatformWalletManagerShieldedFunding.swift:110` similarly documents `fundingAccountIndex` as "BIP44 Core account whose UTXOs fund [the lock]." Both comments are now inconsistent with the corrected internal doc in `orchestration.rs` (lines 187-233), which clarifies that funding is drawn from the union of ALL spendable Core accounts (BIP44 + BIP32 + CoinJoin + DashPay), with `account_index` only selecting the primary/change-receiving account. FFI/Swift consumers relying on these public-facing doc comments could misunderstand which UTXOs are actually spent.

[bfoss765]

Vendoring removed per review feedback (b43caed): the third_party/rust-dashcore tree is gone from the PR — the diff is back to the 6 real files.

The root Cargo.toml [patch] now points the same 12 crates at a git rev instead of the local path: bfoss765/rust-dashcore branch patch/4074-assetlock-router-1860089e @ e8c7335478, which is upstream 1860089e plus exactly the two key-wallet changes this PR depends on (the asset-lock transaction-router account-coverage fix and DEFAULT_COINJOIN_GAP_LIMIT 30 → 100). Verified the cargo-resolved sources are byte-identical to the previously vendored tree, and the full platform-wallet suite (438) + clippy are green against the git patch.

This stays temporary as before: once the router fix lands in dashpay/rust-dashcore and the workspace pin advances past it, the [patch] block gets dropped entirely.

[thepastaclaw]

Code Review

Commit a5ea9e5 removes the interim broadcast-time debit_router_omitted_asset_lock_spends mitigation and its call site, replacing the weak broadcast_mitigation_and_router_fix_do_not_double_debit test with router_fix_records_spent_coinjoin_input_for_persistence. I independently verified this end-to-end: confirmed the exact diff via git diff (single file, build.rs, +73/-149), ran cargo test -p platform-wallet --lib wallet::asset_lock::build::tests myself (11/11 pass, including both router-fix tests) and cargo clippy -p platform-wallet --lib --no-deps (clean), and confirmed no dangling references to the removed symbols remain anywhere in the repo. This closes the prior BLOCKING persistence-gap finding and its paired weak-test finding — both genuinely fixed, not just claimed. I also queried the live PR's review threads directly via gh api graphql rather than trusting any lane's restatement: this confirms author bfoss765 replied on 2026-07-12T19:35:33Z/35Z on both privacy-adjacent threads declaring the CoinJoin union-funding co-spend/de-mixing behavior an intentional 'Option B' product tradeoff, explicitly left open for visibility rather than resolved — reclassifying that carried-forward finding from an unaddressed suggestion to INTENTIONALLY_DEFERRED. The other three carried-forward findings (Swift preflight, Cargo.toml personal-fork patch, stale FFI/Swift docs) have zero-line diffs since b43caed and no review thread addresses any of them — they remain STILL_VALID. One new, independently-confirmed test-coverage gap: the router fix and its persistence rationale explicitly cover CoinJoin, DashpayReceivingFunds, and DashpayExternalAccount, but every test fixture in build.rs (build_coinjoin_shield, coinjoin_has_utxo) exercises only CoinJoin — grep confirms zero DashPay-account test fixtures anywhere in the file. No blocking issues found; this is a clean, well-tested, narrowly-scoped fix.

Source: reviewers Sol (gpt-5.6-sol, high reasoning) and Sonnet (claude-sonnet-5, high effort) across general, security-auditor, rust-quality, and ffi-engineer lanes; final verifier Sonnet (claude-sonnet-5). Orchestrator: openai/gpt-5.6-sol, high reasoning, orchestration-only (not reviewer/verifier). Experiment sonnet-primary-opus-quarter-sample-20260710, sonnet_primary, bucket 1; no Opus lane.

🟡 4 suggestion(s)

2 additional finding(s) omitted (not in diff).

1 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/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ShieldedFundFromAssetLockView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ShieldedFundFromAssetLockView.swift:795-845: [CARRIED-FORWARD, STILL_VALID] Swift preflight `canSubmit` still gates on the single selected BIP44 account, not the backend's union-funded total
  `git diff b43caed9..HEAD` for this file is empty — confirmed independently. `coreAccountOptions` (line 795, `filter { $0.typeTag == 0 && $0.standardTag == 0 && $0.confirmed > 0 }`) still restricts to standard BIP44 accounts only, and `canSubmit`'s Platform-vs-L1 branch still requires `selectedCoreAccountBalanceDuffs >= amount` (line ~842) — the single selected account's balance. Rust's `build_asset_lock_tx_from_all_funding_accounts` treats `account_index` only as the primary/change account and actually funds from the union of BIP44+BIP32+CoinJoin+DashPay. This still rejects the PR's own motivating scenario (BIP44 balance alone insufficient, but the wallet-wide union would cover the request) client-side before the FFI call that would succeed. No GitHub review thread references this file at all — confirmed via the same GraphQL query — so it is genuinely unaddressed, unlike the paired privacy finding.

In `Cargo.toml`:
- [SUGGESTION] Cargo.toml:140-171: [CARRIED-FORWARD, STILL_VALID] Cargo `[patch]` still repoints 12 crates (including the FFI-facing key-wallet-ffi) at an unpublished personal-fork branch
  `git diff b43caed9..HEAD -- Cargo.toml` is empty; the `[patch."https://github.com/dashpay/rust-dashcore"]` table (lines 159-171) still repoints all 12 crates to `bfoss765/rust-dashcore` rev `e8c7335478c046ca0bdf758a68c9a73051aee2ee`. The comment block (lines 140-158) documents this as TEMPORARY with explicit removal instructions once fixes land upstream — this self-documentation predates this delta and isn't a new acknowledgment, so I classify it STILL_VALID rather than INTENTIONALLY_DEFERRED (unlike the privacy finding, no live review thread addresses this specific risk, and 'documented as temporary in a comment' is a weaker signal than a maintainer replying directly to a reviewer-raised thread). Mitigating context multiple lanes independently found via `gh pr view`: upstream tracking PRs dashpay/rust-dashcore#867 (router fix) and #868 (gap-limit bump) exist, both opened 2026-07-11 and still open/unmerged — a real but incomplete path to retiring this patch. The residual bus-factor/availability risk (a personal branch that could be deleted or rebased) is unchanged.

In `packages/rs-platform-wallet-ffi/src/shielded_send.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/shielded_send.rs:811-812: [CARRIED-FORWARD, STILL_VALID] FFI and Swift wrapper doc comments for asset-lock funding are stale after the union-funding change
  `git diff b43caed9..HEAD` for `shielded_send.rs` and the paired Swift wrapper is empty — confirmed independently. Line 811-812 still reads '`account_index` selects the BIP44 Core account whose UTXOs fund the asset lock,' inconsistent with the actual union-funding behavior (BIP44+BIP32+CoinJoin+DashPay) that `orchestration.rs` documents correctly. `PlatformWalletManagerShieldedFunding.swift:110-111` repeats the same stale claim. No review thread addresses this file.

In `packages/rs-platform-wallet/src/wallet/asset_lock/build.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:1621-1817: [NEW IN LATEST DELTA] New router-fix persistence test covers only CoinJoin; DashPay-funded shielded locks have zero test coverage
  The router fix this delta relies on (and the removed mitigation's own doc comment, lines 856-858) explicitly covers three previously-omitted account types: `CoinJoin`, `DashpayReceivingFunds`, and `DashpayExternalAccount`. But `router_fix_records_spent_coinjoin_input_for_persistence` and `router_fix_debits_coinjoin_asset_lock_spend_on_rescan` — the only two tests exercising this fix — both build their fixture exclusively via `build_coinjoin_shield()`/`coinjoin_has_utxo()`. I grepped the full file: zero occurrences of 'Dashpay'/'DashPay' in any test or fixture helper. If the DashPay leg of the vendored router fix has any latent gap (e.g. a different `AccountTypeToCheck` mapping edge case for `DashpayReceivingFunds` vs `DashpayExternalAccount`), the exact same persistence regression this PR closes for CoinJoin (#1507) could recur silently for a DashPay-funded shield with no test to catch it.

[thepastaclaw]

🟡 Suggestion: [CARRIED-FORWARD, STILL_VALID] Cargo [patch] still repoints 12 crates (including the FFI-facing key-wallet-ffi) at an unpublished personal-fork branch

git diff b43caed9..HEAD -- Cargo.toml is empty; the [patch."https://github.com/dashpay/rust-dashcore"] table (lines 159-171) still repoints all 12 crates to bfoss765/rust-dashcore rev e8c7335478c046ca0bdf758a68c9a73051aee2ee. The comment block (lines 140-158) documents this as TEMPORARY with explicit removal instructions once fixes land upstream — this self-documentation predates this delta and isn't a new acknowledgment, so I classify it STILL_VALID rather than INTENTIONALLY_DEFERRED (unlike the privacy finding, no live review thread addresses this specific risk, and 'documented as temporary in a comment' is a weaker signal than a maintainer replying directly to a reviewer-raised thread). Mitigating context multiple lanes independently found via gh pr view: upstream tracking PRs dashpay/rust-dashcore#867 (router fix) and #868 (gap-limit bump) exist, both opened 2026-07-11 and still open/unmerged — a real but incomplete path to retiring this patch. The residual bus-factor/availability risk (a personal branch that could be deleted or rebased) is unchanged.

_{source: ['sonnet5-general', 'sonnet5-security-auditor', 'codex-rust-quality', 'sonnet5-rust-quality', 'codex-ffi-engineer', 'sonnet5-ffi-engineer']}

[bfoss765]

Acknowledged — and to state it directly rather than leaving it to the comment block: the [patch."https://github.com/dashpay/rust-dashcore"] pin to the bfoss765 fork is TEMPORARY and intentional. It exists solely so the Android/Kotlin-SDK integration work is not blocked on upstream merges — it is not a permanent redirection of the dependency.

On the availability/bus-factor risk: the patch is pinned by immutable commit hash (rev = "e8c7335478c046ca0bdf758a68c9a73051aee2ee"), not a branch. A rebase or a branch deletion on the fork cannot silently change what builds — cargo resolves and verifies the exact rev, so at worst the fetch fails loudly rather than pulling different code.

It is retired as soon as the upstream tracking PRs merge, at which point the pin moves back to a dashpay/rust-dashcore rev:

• fix(key-wallet): cover CoinJoin/DashPay accounts for AssetLock routing rust-dashcore#867 — asset-lock router fix (the get_relevant_account_types(AssetLock) widening this PR depends on)
• feat(key-wallet): widen DEFAULT_COINJOIN_GAP_LIMIT 30 -> 100 for dashj parity (proposal) rust-dashcore#868 — gap-limit parity proposal
• fix(dash-spv): re-open committed ranges for gap-limit scripts derived after commit (#846) rust-dashcore#873 — committed-range re-scan fix for ci: dashmate release script fix #846 (already APPROVED by review)

Step-by-step removal instructions are already in the Cargo.toml comment block (lines ~140-158): drop the [patch] table and bump the three key-wallet* git revs to the merged upstream commit. No code change in this delta; this reply is the direct maintainer acknowledgment the finding asked for.

[thepastaclaw]

Still valid at 189e068 — the workspace remains patched to the bfoss765/rust-dashcore revision. Upstream PR #867 has merged, but #868 remains open, so the personal-fork dependency persists; this remains a carried-forward suggestion.

[thepastaclaw]

🟡 Suggestion: [NEW IN LATEST DELTA] New router-fix persistence test covers only CoinJoin; DashPay-funded shielded locks have zero test coverage

The router fix this delta relies on (and the removed mitigation's own doc comment, lines 856-858) explicitly covers three previously-omitted account types: CoinJoin, DashpayReceivingFunds, and DashpayExternalAccount. But router_fix_records_spent_coinjoin_input_for_persistence and router_fix_debits_coinjoin_asset_lock_spend_on_rescan — the only two tests exercising this fix — both build their fixture exclusively via build_coinjoin_shield()/coinjoin_has_utxo(). I grepped the full file: zero occurrences of 'Dashpay'/'DashPay' in any test or fixture helper. If the DashPay leg of the vendored router fix has any latent gap (e.g. a different AccountTypeToCheck mapping edge case for DashpayReceivingFunds vs DashpayExternalAccount), the exact same persistence regression this PR closes for CoinJoin (#1507) could recur silently for a DashPay-funded shield with no test to catch it.

_{source: ['sonnet5-security-auditor', 'codex-rust-quality']}

[bfoss765]

Added DashPay-leg coverage in commit 77561d2 (pushed to fix/kotlin-sdk-assetlock-multi-account).

Both DashPay legs turned out to be fully fundable in the fixture harness, so both are covered (not just one):

New tests in packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:

• router_fix_debits_dashpay_receiving_asset_lock_spend_on_rescan
• router_fix_debits_dashpay_external_asset_lock_spend_on_rescan
• router_fix_records_spent_dashpay_receiving_input_for_persistence
• router_fix_records_spent_dashpay_external_input_for_persistence

These mirror the two CoinJoin tests one-for-one across both DashpayReceivingFunds and DashpayExternalAccount, asserting (a) the check_core_transaction scan emits a DashPay TransactionRecord whose input_details resolve — through the same record.transaction.input[detail.index].previous_output lookup derive_spent_utxos walks — to the spent DashPay outpoint (the persistence-feeding signal), and (b) the in-memory debit / no-reinflation (previous − inputs + change).

Supporting fixture in packages/rs-platform-wallet/src/test_support.rs: split_funded_wallet_manager_dashpay(bip44_duffs, dashpay_duffs, leg) + a DashpayLeg enum. Since WalletAccountCreationOptions::Default does not create DashPay accounts, it provisions one of the requested arm on both the signing Wallet (add_account) and the ManagedWalletInfo (add_managed_account), derives a fresh single-pool receive address, and funds it — the DashPay analogue of the existing BIP44+CoinJoin split fixture. The soft signer derives over the whole seed, so the DashPay input's DIP-15 256-bit path is signed end-to-end. The shield draws entirely from a 2.0-DASH DashPay UTXO over a 0.09-DASH BIP44 slice; the spent DashPay outpoint is resolved against the DashPay account's own UTXO set rather than assumed positionally.

cargo test -p platform-wallet --lib: 433 passed, 0 failed (was 429; +4).

[thepastaclaw]

Resolved in 77561d2 — [NEW IN LATEST DELTA] New router-fix persistence test covers only CoinJoin; DashPay-funded shielded locks have zero test coverage 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.}

[thepastaclaw]

Code Review

The latest delta (a5ea9e5→77561d25d7) is test-only and closes the prior literal DashPay persistence-coverage gap. First-principles verification nevertheless confirms a new cumulative blocker: shielded union funding can select UTXOs from a watch-only DashpayExternalAccount whose private keys belong to the contact, while the production signer derives from the local wallet seed. The new fixture masks this by deriving its external account from the test wallet's own seed. Four prior suggestions remain unchanged: one intentionally deferred and three still valid.

Source (experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_opus_sample, bucket 0): reviewers codex-general=gpt-5.6-sol (Sol reviewer, high reasoning; completed after ACP runtime auth recovery), sonnet5-general=claude-sonnet-5 (completed), opus-general=claude-opus-4-8 (completed); verifier=verifier-sonnet5-4074-1783898302=claude-sonnet-5. Orchestrator=openai/gpt-5.6-sol reasoning=high (orchestration-only; not a reviewer or verifier).

🔴 1 blocking | 🟡 2 suggestion(s)

2 additional finding(s) omitted (not in diff).

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/rs-platform-wallet/src/wallet/asset_lock/build.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:275-291: Union funding includes watch-only DashpayExternalAccount UTXOs the wallet cannot sign for
  This loop iterates `info.core_wallet.accounts.all_funding_accounts()` and unconditionally adds every account's spendable UTXOs to `extra_inputs`, deriving a signing path via `acc.address_derivation_path(&utxo.address)`. The pinned key-wallet fork's `all_funding_accounts()` includes `dashpay_external_accounts` without watch-only filtering, while `spendable_utxos()` checks maturity but not ownership. Production creates `DashpayExternalAccount` in `wallet/identity/network/contacts.rs:506-531` from the contact's decrypted xpub with `is_watch_only: true`, so those private keys belong to the contact. `MnemonicResolverCoreSigner` blindly derives every requested path from the local wallet's own mnemonic master key, producing a different key and therefore an invalid input signature. The new `DashpayLeg::ExternalAccount` fixture masks the defect by calling `wallet.add_account(account_type, None)`, which derives the account from the test wallet's own seed and makes it locally signable unlike production.

In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ShieldedFundFromAssetLockView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ShieldedFundFromAssetLockView.swift:795-845: [CARRIED-FORWARD, STILL_VALID] Swift `canSubmit` gates on the selected BIP44 balance, not the union-funded total
  Unchanged: `canSubmit` still requires `selectedCoreAccountBalanceDuffs >= amount`, reflecting only the chosen BIP44 account, while Rust funds from the union of owned BIP44/BIP32/CoinJoin/DashPay accounts. A request the backend could fund is still rejected before the FFI call.

In `packages/rs-platform-wallet-ffi/src/shielded_send.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/shielded_send.rs:811-812: [CARRIED-FORWARD, STILL_VALID] FFI and Swift docs still describe obsolete single-BIP44 funding
  Unchanged: the public doc still says `account_index` selects the BIP44 account whose UTXOs fund the lock, while the implementation uses it for the primary/change account and draws candidates from a broader union. Callers cannot infer which account classes may be spent.

[thepastaclaw]

🔴 Blocking: Union funding includes watch-only DashpayExternalAccount UTXOs the wallet cannot sign for

This loop iterates info.core_wallet.accounts.all_funding_accounts() and unconditionally adds every account's spendable UTXOs to extra_inputs, deriving a signing path via acc.address_derivation_path(&utxo.address). The pinned key-wallet fork's all_funding_accounts() includes dashpay_external_accounts without watch-only filtering, while spendable_utxos() checks maturity but not ownership. Production creates DashpayExternalAccount in wallet/identity/network/contacts.rs:506-531 from the contact's decrypted xpub with is_watch_only: true, so those private keys belong to the contact. MnemonicResolverCoreSigner blindly derives every requested path from the local wallet's own mnemonic master key, producing a different key and therefore an invalid input signature. The new DashpayLeg::ExternalAccount fixture masks the defect by calling wallet.add_account(account_type, None), which derives the account from the test wallet's own seed and makes it locally signable unlike production.

_{source: ['codex-general']}

[bfoss765]

Fixed in 189e068.

Root cause confirmed. The union loop in build_asset_lock_tx_from_all_funding_accounts added every all_funding_accounts() entry's spendable_utxos() to the builder and derived a path via acc.address_derivation_path(&utxo.address). The fork's all_funding_accounts() includes dashpay_external_accounts; spendable_utxos() checks maturity, not ownership. A DashpayExternalAccount is built in production from the contact's decrypted xpub with is_watch_only: true (contacts.rs register_dashpay_external_account, ~L526), so its coins are the contact's and MnemonicResolverCoreSigner would sign the derived path from the LOCAL master key → wrong key → invalid input signature.

What the watch-only signal turned out to be. There is no usable watch-only flag at the managed layer. ManagedCoreKeysAccount::from_account collapses every account to KeySource::Public(account.account_xpub) regardless of the source Account's is_watch_only, so KeySource::is_watch_only() is uniformly true for all managed funding accounts and cannot discriminate. The canonical ownership signal available here is the account-type variant: ManagedAccountType::DashpayExternalAccount is the sole watch-only funds type (the fork's own doc: "DashPay external (watch-only) account"). The fix skips exactly that variant in the union loop, via acc.managed_account_type():

if matches!(acc.managed_account_type(), ManagedAccountType::DashpayExternalAccount { .. }) {
    continue;
}

DashpayReceivingFunds verified as ours (kept included). register_contact_account (contacts.rs ~L225-231) builds it from our own friendship xpub with is_watch_only: false — the local seed signs it — so its inclusion in asset-lock funding is correct. Only the external arm is watch-only.

Owning the earlier masking. The prior DashPay-external tests were false green: the fixture split_funded_wallet_manager_dashpay built the external account via add_account(_, None), which derives from the test wallet's own seed and makes it locally signable — the opposite of production. The fixture's external arm now builds it watch-only from a foreign seed's xpub via add_account(_, Some(foreign_xpub)) (which stores is_watch_only: true, mirroring contacts.rs); the receiving arm still uses add_account(_, None).

The two router_fix_*_dashpay_external_* tests (which had asserted the external UTXO was spent by an asset lock) are replaced with exclusion tests:

• asset_lock_funding_excludes_watch_only_dashpay_external_utxos — external holds the largest UTXO (0.5 DASH) so a naive LargestFirst over the union would grab it first; asserts it is not selected and stays unspent while signable BIP44 (0.15) covers the 0.1 shield.
• asset_lock_funding_cannot_borrow_watch_only_dashpay_external_utxos — signable BIP44 (0.05) < shield (0.1); asserts the build fails with AssetLockInsufficientFunds rather than borrow the contact's coins and emit an invalid-signature input.

Both fail with the filter disabled (verified: external UTXO gets selected / build wrongly succeeds) and pass with it. The DashPay receiving-funds tests are unchanged and still green.

Audit of other funding/selection paths (step 3). Grepped all all_funding_accounts / spendable_utxos callers in rs-platform-wallet:

• build.rs:280 — the fixed production path.
• build.rs two other all_funding_accounts callers — both #[cfg(test)], building outpoint -> value lookup maps for accounting; they never turn a UTXO into a signed input, so no fix needed.
• platform_wallet_traits.rs get_spendable_utxos — delegates to the fork's balance helper and has no callers in this crate; it is a display/balance surface, not a funding/selection path. (Its fork-level inclusion of external coins in the aggregate balance is a separate display concern, out of scope for this signing-path finding.)

cargo test -p platform-wallet --lib: 433 passed, 0 failed; cargo fmt --check clean.

[thepastaclaw]

Resolved in 189e068 — Union funding includes watch-only DashpayExternalAccount UTXOs the wallet cannot sign for 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.}

[thepastaclaw]

Code Review

The delta from 77561d2 to 189e068 (head 189e068) correctly fixes the prior blocking finding: build.rs now excludes ManagedAccountType::DashpayExternalAccount from the multi-account asset-lock funding union before it can contribute UTXOs, path-resolver entries, or extra inputs, and the DashPay test fixture was upgraded to derive the external account from a genuinely foreign seed's xpub so the two new tests (asset_lock_funding_excludes_watch_only_dashpay_external_utxos, asset_lock_funding_cannot_borrow_watch_only_dashpay_external_utxos) are real adversarial checks rather than a masked shortcut. I independently reproduced all three agents' verification: read build.rs's exclusion logic and the fixture in test_support.rs, ran cargo test -p platform-wallet --lib asset_lock (28/28 pass, including both new tests) and cargo clippy -p platform-wallet --lib --no-deps -- -D warnings (clean) at the exact head SHA. The delta also introduces two minor, non-blocking issues: a functionally-dead 3510-line third_party/rust-dashcore/Cargo.lock committed into a directory that has no Cargo.toml and is excluded from the workspace, and a doc-comment block that was orphaned onto a new private helper (foreign_contact_account_xpub), silently stripping split_funded_wallet_manager_dashpay's original documentation. Four prior suggestion-level findings (CoinJoin co-spend/de-mix, Swift canSubmit gating on BIP44-only balance, the personal-fork Cargo patch, and stale FFI/Swift funding docs) are unchanged in this delta and remain valid on the cumulative diff.

Source (experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_opus_sample, bucket 0): reviewers codex-general=gpt-5.6-sol (Sol reviewer, high reasoning; completed after exact-model selector retry), sonnet5-general=claude-sonnet-5 (high effort; completed), opus-general=claude-opus-4-8 (high effort; completed); verifier=verifier-sonnet5-4074-1783910338=claude-sonnet-5. Orchestrator=openai/gpt-5.6-sol reasoning=high (orchestration-only; not a reviewer or verifier).

🟡 4 suggestion(s) | 💬 1 nitpick(s)

2 additional finding(s) omitted (not in diff).

1 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/rs-platform-wallet/src/test_support.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/test_support.rs:318-361: New helper function inserted between a doc comment and its target orphans split_funded_wallet_manager_dashpay's documentation
  The `///` block starting at line 318 ("Builds a testnet wallet manager whose balance is SPLIT across BIP44 account 0...") was written for `split_funded_wallet_manager_dashpay`, but this commit inserts a new private helper, `foreign_contact_account_xpub`, directly between the comment and that function. Because Rust attaches a doc comment to the very next item, the whole contiguous `///` block (lines 318-349, including the fixture's purpose and router-fix rationale) now documents the small private helper instead, and `split_funded_wallet_manager_dashpay` at line 361 is left with no doc comment at all. I verified this by reading the file directly: line 360 is blank with nothing above `pub(crate) async fn split_funded_wallet_manager_dashpay` on line 361. Move lines 318-340 back above the function (or reduce them to a shorter doc directly above `foreign_contact_account_xpub`) so the fixture keeps its documentation.

In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ShieldedFundFromAssetLockView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ShieldedFundFromAssetLockView.swift:823-843: [CARRIED-FORWARD] Swift canSubmit still gates on the selected BIP44 account's balance, not the union Rust actually funds from
  canSubmit's Core branch still requires `selectedCoreAccountBalanceDuffs >= amount` — the confirmed balance of the one chosen standard BIP44 account. Rust funds shielded top-ups from the union of BIP44, BIP32, CoinJoin, and DashPay-receiving accounts (watch-only DashpayExternalAccount is now correctly excluded, per this PR's fix). A request the backend can fund from CoinJoin or BIP32 balances is still rejected client-side before the FFI call is even attempted, contradicting the point of exposing multi-account funding. This file is untouched by the current delta; I confirmed the gating logic is unchanged at lines 836-843.

In `Cargo.toml`:
- [SUGGESTION] Cargo.toml:140-171: [CARRIED-FORWARD] Workspace still builds 12 crates, including key-wallet-ffi, from a personal rust-dashcore fork
  All 12 rust-dashcore-sourced crates remain patched to `bfoss765/rust-dashcore` rev e8c7335478c046ca0bdf758a68c9a73051aee2ee on a throwaway branch. Upstream PR #867 (asset-lock router fix) has since merged, but PR #868 (CoinJoin gap-limit bump) remains open, so the personal-fork dependency is not yet fully removable. The block is clearly documented as temporary with a removal plan, which is why this stays a suggestion rather than a blocker, but it is provenance/availability risk that should be resolved before a release cut.

In `packages/rs-platform-wallet-ffi/src/shielded_send.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/shielded_send.rs:811-812: [CARRIED-FORWARD] Public FFI doc still describes obsolete single-BIP44 funding
  The doc comment still states `account_index` "selects the BIP44 Core account whose UTXOs fund the asset lock." It now only selects the primary BIP44 account used for change and reservation gating; funding inputs are drawn from the locally-signable union of BIP44, BIP32, CoinJoin, and DashPay-receiving accounts (watch-only DashpayExternalAccount is excluded by this PR's fix). The same stale description appears in the corresponding Swift wrapper and SwiftExampleApp comments. Callers cannot infer which account classes may actually be spent from this doc.

[thepastaclaw]

💬 Nitpick: Orphaned 3510-line Cargo.lock added in a directory the workspace excludes

This commit adds third_party/rust-dashcore/Cargo.lock as a brand-new file, but the directory has no Cargo.toml (the vendored tree was removed in b43caed when the project switched to the git [patch] mechanism) and the root Cargo.toml explicitly lists exclude = ["third_party/rust-dashcore"]. I confirmed the directory contains only this lockfile and that nothing under .github/workflows or scripts references the path. Cargo never reads this file — it's a leftover artifact, most likely from running cargo inside that directory before its Cargo.toml was deleted, picked up by a broad git add when preparing this commit. Delete it; it adds most of this delta's diff size for no functional benefit.

_{source: ['codex-general', 'sonnet5-general']}

[thepastaclaw]

🟡 Suggestion: New helper function inserted between a doc comment and its target orphans split_funded_wallet_manager_dashpay's documentation

The /// block starting at line 318 ("Builds a testnet wallet manager whose balance is SPLIT across BIP44 account 0...") was written for split_funded_wallet_manager_dashpay, but this commit inserts a new private helper, foreign_contact_account_xpub, directly between the comment and that function. Because Rust attaches a doc comment to the very next item, the whole contiguous /// block (lines 318-349, including the fixture's purpose and router-fix rationale) now documents the small private helper instead, and split_funded_wallet_manager_dashpay at line 361 is left with no doc comment at all. I verified this by reading the file directly: line 360 is blank with nothing above pub(crate) async fn split_funded_wallet_manager_dashpay on line 361. Move lines 318-340 back above the function (or reduce them to a shorter doc directly above foreign_contact_account_xpub) so the fixture keeps its documentation.

_{source: ['sonnet5-general']}

[thepastaclaw]

🟡 Suggestion: [CARRIED-FORWARD] Workspace still builds 12 crates, including key-wallet-ffi, from a personal rust-dashcore fork

All 12 rust-dashcore-sourced crates remain patched to bfoss765/rust-dashcore rev e8c7335478c046ca0bdf758a68c9a73051aee2ee on a throwaway branch. Upstream PR #867 (asset-lock router fix) has since merged, but PR #868 (CoinJoin gap-limit bump) remains open, so the personal-fork dependency is not yet fully removable. The block is clearly documented as temporary with a removal plan, which is why this stays a suggestion rather than a blocker, but it is provenance/availability risk that should be resolved before a release cut.

_{source: ['codex-general', 'sonnet5-general', 'opus-general']}