feat(platform-wallet): sweep + recover CoinJoin mixed coins for the DashSync→SDK migration#3817
feat(platform-wallet): sweep + recover CoinJoin mixed coins for the DashSync→SDK migration#3817llbartekll wants to merge 12 commits into
Conversation
Support recovering DashSync-era CoinJoin "mixed coins" (BIP44 purpose 4') after migration to SwiftDashSDK, which the standard send path cannot reach. - sweep_coinjoin_to_address: build an all-input, single-output tx that empties the CoinJoin account into one destination (no change). Bypasses TransactionBuilder's LargestFirst selection (which can drop small UTXOs) by assembling inputs manually and signing via the Signer. - set_coinjoin_gap_limit: widen the CoinJoin pool's gap limit and pre-generate addresses (maintain_gap_limit) so SPV actually watches the wider window, then bump_monitor_revision. Setting the limit alone is not enough — only materialized addresses are watched. In-memory only (not persisted), so a later load reverts to the default gap. - FFI: core_wallet_sweep_coinjoin, core_wallet_set_coinjoin_gap_limit. - swift-sdk: ManagedCoreWallet.sweepCoinJoinAccount / setCoinJoinGapLimit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A heavy mixer can hold thousands of small mixed-coin UTXOs. The sweep packed all of them into one transaction, which past ~675 inputs exceeds the standard relay-size limit (MAX_STANDARD_TX_SIZE = 100 000 B) and is silently unrelayable — the funds never move. sweep_coinjoin_to_address now partitions the UTXO snapshot into balanced chunks of <= 500 inputs (~74 KB/tx) and returns Vec<Transaction>. Each chunk spends a disjoint slice of the snapshot, so the transactions have no inter-dependency and may broadcast in any order. Broadcast tolerates partial failure: the chunks that did broadcast are returned (the caller refreshes balance and may re-run to sweep the remainder); an error is returned only if none broadcast at all. - rs-platform-wallet: chunk the sweep; sweep_chunk_size helper + unit test. - rs-platform-wallet-ffi: core_wallet_sweep_coinjoin returns N wire-order txids (out_txids = count*32 bytes + out_count) instead of one tx blob — the app only needs the ids. Freed via the existing core_wallet_free_tx_bytes. - swift-sdk: sweepCoinJoinAccount returns [Data] of wire-order txids. Rebuild the xcframework (build_ios.sh) to pick up the new FFI ABI. cargo check both crates + the chunking unit test pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dashpay/rust-dashcore#804 ("fix(key-wallet): correct CoinJoin discovery") is merged into dev. Switch the 8 workspace rust-dashcore crates from the prior pinned rev (eb889af) to dev's HEAD 7ff6b246 — the #804 merge commit — so the CoinJoin sweep/recovery stack builds against the merged fix instead of the pre-merge PR branch. cargo check -p platform-wallet -p platform-wallet-ffi passes; Cargo.lock updated (same 0.43.0 versions, source rev only). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
DashSync CoinJoin uses two chains — external (.../0/i, receive/denominations) and internal (.../1/i, mixing change) — but the SDK's CoinJoin account models only the external pool. A migrated wallet's internal-chain mixed coins are imported as spendable UTXOs (they count in the balance) yet have no derivation path in the account, so signing a sweep that includes them failed with "no derivation path for input address" — blocking the whole sweep (one unresolved input fails its chunk). sweep_coinjoin_to_address now resolves signing paths across BOTH chains: it derives external and internal addresses from the CoinJoin account xpub (non-hardened, public-key-only) into a combined address->path map covering every input, and signs from it. Errors clearly if any input can't be resolved on either chain within COINJOIN_SWEEP_MAX_INDEX, rather than failing mid-sign. Self-contained — no longer relies on the account's external pool being pre-widened. Verified on a real migrated testnet wallet: 12.746 DASH (incl. internal-chain coins) swept to spendable. + 2 unit tests (external + internal resolution, and the unresolvable-input error). Rebuild the xcframework to pick this up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
✅ Review complete (commit 8760508) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR adds a CoinJoin recovery + chunked sweep flow that is well-scoped and faithfully mirrors key-wallet's fee/sizing conventions. One blocking issue: the new set_coinjoin_gap_limit public API does not reject gap_limit == 0, which underflows self.gap_limit - 1 inside maintain_gap_limit (debug panic / release wrap to u32::MAX → effectively unbounded address generation under the wallet write lock). A few non-blocking defensive-coding improvements in the sweep path as well.
🔴 1 blocking | 🟡 2 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/core/coinjoin_recovery.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/core/coinjoin_recovery.rs:96-98: Reject zero CoinJoin gap limit before maintaining the pool
`set_coinjoin_gap_limit` takes `gap_limit: u32` from the Swift FFI and writes it directly into `pool.gap_limit` (only clamped on the upper end via `MAX_GAP_LIMIT`). If `gap_limit == 0`, `AddressPool::maintain_gap_limit` evaluates `self.gap_limit - 1` when `highest_used` is `None` (verified in the pinned key-wallet at `address_pool.rs:888-891`). That panics in debug builds and wraps to `u32::MAX` in release, causing the recovery helper to try generating ~4 billion addresses while holding the wallet write lock. Since this is a brand-new public API (FFI does not validate either), reject the zero case at the boundary before touching the pool.
In `packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/broadcast.rs:451-457: Remove address from `needed` only after `path_map.insert` actually succeeds
`coinjoin_sweep_path_map` does `if needed.remove(addr) { if let Some(info) = pool.address_info(addr) { path_map.insert(...) } }`. The `remove` happens unconditionally on a match, but the `path_map.insert` is gated on `address_info` returning `Some`. The resolver's documented contract is "errors if any input can't be resolved, rather than failing mid-sign" — but if `address_info` ever returns `None` for an address `generate_addresses` just produced (an invariant violation in `AddressPool`), the address is silently dropped from `needed`, `needed.is_empty()` becomes true so the defensive check at line 462 does not fire, and `signer.sign_tx` later fails with the very "no derivation path for input address" error the resolver was added to prevent. Today `generate_address_at_index` inserts into both `addresses` and `address_index` atomically, so this can't trigger — but inverting the gating preserves the "removed from needed ⇒ inserted into path_map" invariant by construction, so a future address-pool refactor cannot silently break this code path.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/broadcast.rs:370-388: Partial-broadcast failures are swallowed silently when at least one chunk succeeds
When some chunks broadcast and others fail, the function returns `Ok(broadcast)` with only the successful subset; `last_err` and the count of failures are dropped. The docstring (lines 194–198) calls this out as intentional — the caller is expected to refresh balance and re-run — but the result type currently carries no signal that the sweep was partial. The Swift caller's `sweepCoinJoinAccount` therefore can't surface "swept K of N chunks" telemetry or warn the user before claiming success. At minimum, log dropped errors via `tracing::warn!` inside the loop so the failures are observable in logs; longer term, consider returning a richer outcome type so the FFI consumer can distinguish full vs partial success.
…nvariants PR #3817 review feedback: - [blocking] set_coinjoin_gap_limit: reject gap_limit == 0 before touching the pool. key-wallet's maintain_gap_limit computes `gap_limit - 1` when no address has been used, underflowing at 0 (debug panic / release wrap to u32::MAX → ~4B address generations under the write lock). Callers only pass 400, but this is a public + FFI API, so validate at the boundary. - coinjoin_sweep_path_map: drop an address from `needed` only after its path is recorded, so "unresolved ⇒ error" holds by construction even if a future AddressPool refactor desyncs generate_addresses / address_info. - sweep chunk: promote the "signer consumed every UTXO" debug_assert_eq! to a runtime error — it guards the single-output `total_input - fee` amount, a fund-correctness invariant that must hold in release too. - broadcast loop: log each dropped chunk error via tracing::warn! so a partial sweep is observable (still tolerated; caller re-runs for the remainder). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements CoinJoin sweep-to-address and address-pool gap-limit recovery across the Rust wallet core, FFI bindings, and Swift SDK, enabling users to consolidate scattered CoinJoin UTXOs into a single destination address and temporarily widen address detection windows for recovery scenarios. ChangesCoinJoin Sweep and Gap-Limit Recovery
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/rs-platform-wallet/src/wallet/core/broadcast.rs (1)
214-367: 🏗️ Heavy liftRelease the wallet write lock before awaiting chunk signing.
Line 215 acquires
wallet_manager.write(), and the loop at Lines 349-352 keeps that writer held across everysign_tx(...).await. By Line 287 you already own the UTXO snapshot andpath_map, so the remaining async signing work no longer needs mutable wallet state; keeping the lock here serializes unrelated wallet operations behind resolver/keychain latency.🤖 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/core/broadcast.rs` around lines 214 - 367, The wallet write lock taken by let mut wm = self.wallet_manager.write().await is held across async signer.sign_tx(...).await calls; release it before the chunk signing loop by ending the wm borrow scope (or calling drop(wm)) immediately after you've extracted account_xpub/account_type/network, managed_account, utxos and computed path_map (i.e., right after the path_map variable is created) so the for chunk in utxos.chunks(...) loop and signer.sign_tx awaits run without the wallet_manager write lock held.
🤖 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.
Inline comments:
In `@packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- Around line 309-310: This file fails rustfmt; reformat the Rust source by
running `cargo fmt --all` (or `rustfmt`) so the expression using
fee_rate.calculate_fee(BASE_SIZE_1_OUTPUT_NO_CHANGE + input_count * INPUT_SIZE)
and other nearby hunks (e.g., around the calculate_fee call and constants
BASE_SIZE_1_OUTPUT_NO_CHANGE / INPUT_SIZE) conform to project style; simply run
the formatter and commit the updated layout so CI rustfmt checks pass.
- Around line 464-482: The loop in broadcast.rs always calls
pool.generate_addresses(BATCH, ...) which can overshoot max_index; change it to
compute remaining = max_index.saturating_sub(generated) and call
pool.generate_addresses(remaining.min(BATCH), key_source, true) (or the
equivalent usize/min) so the final batch is clamped to the remaining search
window, and then increment generated by the actual batch size returned/used
rather than unconditionally by BATCH; update references to BATCH in that loop
accordingly (e.g., the variables generated, max_index, and the call to
generate_addresses).
In `@packages/rs-platform-wallet/src/wallet/core/coinjoin_recovery.rs`:
- Around line 46-51: Run rustfmt across the repo (cargo fmt --all) to fix
formatting in coinjoin_recovery.rs; specifically reformat the block that
acquires wm via self.wallet_manager.write().await and the call to
wm.get_wallet_and_info_mut(&self.wallet_id) which constructs
PlatformWalletError::WalletNotFound, so that the wm, wallet_manager,
get_wallet_and_info_mut, self.wallet_id and PlatformWalletError::WalletNotFound
symbols adhere to rustfmt style and CI passes.
---
Nitpick comments:
In `@packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- Around line 214-367: The wallet write lock taken by let mut wm =
self.wallet_manager.write().await is held across async signer.sign_tx(...).await
calls; release it before the chunk signing loop by ending the wm borrow scope
(or calling drop(wm)) immediately after you've extracted
account_xpub/account_type/network, managed_account, utxos and computed path_map
(i.e., right after the path_map variable is created) so the for chunk in
utxos.chunks(...) loop and signer.sign_tx awaits run without the wallet_manager
write lock held.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7447e1d-e8ba-4c9e-a0c5-eb5fdc12c991
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.tomlpackages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rspackages/rs-platform-wallet/src/wallet/core/broadcast.rspackages/rs-platform-wallet/src/wallet/core/coinjoin_recovery.rspackages/rs-platform-wallet/src/wallet/core/mod.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
All four prior findings are fixed at head c65905f. The latest commit cleanly applies the suggested fixes (zero gap-limit guard, insert-then-remove ordering in path_map, tracing::warn on per-chunk broadcast failures, runtime input-count invariant). One new in-scope blocking issue surfaces in the cumulative review: the new sweep FFI entry point core_wallet_sweep_coinjoin parses the caller-supplied destination with assume_checked() and never matches it against the wallet's network, so a wrong-network address would silently drain the entire CoinJoin account to a script on the wrong chain.
🔴 1 blocking
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs:168-188: Validate the sweep destination against the wallet's network
`core_wallet_sweep_coinjoin` parses `dest_address` with `dashcore::Address::from_str(...).assume_checked()` and then passes the resulting `Address` straight into `sweep_coinjoin_to_address`, which builds and broadcasts a transaction that drains every spendable CoinJoin UTXO to that script. The wallet's network is read a few lines below (line 175) but is never used to verify the destination. A mainnet `CoreWallet` will therefore accept a testnet/devnet/regtest address (or vice versa), and the resulting all-input → single-output sweep will be broadcast on the wallet's network with the wrong-network script — the entire CoinJoin account becomes unrecoverable. This is especially severe because this entry point is the all-funds sweep used by the DashSync migration, and the address comes from arbitrary Swift caller input. Parse as `Address<NetworkUnchecked>` and `require_network(wallet.network())` inside the storage closure so the failure surfaces as a `PlatformWalletFFIResult` error before any tx is built.
PR #3817 round-2 review: - [blocking] core_wallet_sweep_coinjoin (FFI): validate the caller-supplied destination against the wallet's network. It was parsed with assume_checked() and used to sweep every CoinJoin UTXO with no network check — a wrong-network address would drain the account to a script unspendable on this chain. Now parses NetworkUnchecked and require_network(wallet.network()) inside the storage closure, erroring before any tx is built. Mirrors the withdrawal FFI. (Not reachable from the app, which always passes its own current-network receive address, but this is a public fund-moving FFI boundary.) - cargo fmt on the three touched files to satisfy CI rustfmt. Two other round-2 suggestions were declined with on-thread reasons: the resolver batch-overshoot (20000 is an exact multiple of 500 — no overshoot) and releasing the wallet write lock across the sign .await (tokio async RwLock, broadcast is already unlocked, body does no wallet mutation — a throughput nit not worth the borrow-ordering risk on a rare one-time sweep). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Re the CodeRabbit nitpick about releasing the wallet write lock across the sign
Tracking it as a possible future optimization if this path is ever reused in a concurrent/server context. |
CI clippy: - drop a redundant .clone() on AccountType (it is Copy) in sweep_coinjoin_to_address - use RangeInclusive::contains instead of a manual `n >= 1 && n <= MAX` in the sweep_chunk_size test assertion (plus the rustfmt the change required). FFI crate is clippy-clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
llbartekll
changed the title
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta (f52644b) is the focused FFI network-validation fix plus rustfmt. The prior blocking finding is FIXED at packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs:168-185 — dest_unchecked.require_network(network) runs inside the storage closure before any tx is built. Cumulative review surfaces no remaining in-scope blockers. Two suggestion-level findings about the sweep path are worth surfacing: (1) the sweep fee constants are not an upper bound for ≥253-input chunks, and (2) the public Rust sweep_coinjoin_to_address still trusts its dest argument's network. One nitpick on broadcast loop error preservation. The codex-general 'materialize internal CoinJoin chain' concern is downgraded to a suggestion because the SDK's CoinJoin account explicitly models a single (external) pool and the recovery scan is documented as a one-time post-migration helper.
🟡 3 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/core/broadcast.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/broadcast.rs:289-310: Sweep fee constants are not a true upper bound for large chunks
`BASE_SIZE_1_OUTPUT_NO_CHANGE = 8 + 1 + 1 + 34` hard-codes the input-count CompactSize as 1 byte and `INPUT_SIZE = 148` is the typical, not maximum, P2PKH input size. Chunks are explicitly allowed up to MAX_INPUTS_PER_SWEEP, and when `input_count >= 253` the CompactSize input-count grows to 3 bytes; compressed P2PKH ECDSA inputs can serialize to 149 bytes at the larger DER signature size. Because the sweep subtracts the fee *before* signing and never recomputes from the signed transaction, the computed fee can land below 1 duff/byte on large chunks — exactly the heavy-CoinJoin case this PR targets. Use upper-bound constants (or compute the varint length from `input_count`) so the fee is always ≥ policy minimum and the all-funds invariant holds.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/broadcast.rs:198-210: Enforce destination network on the Rust sweep API itself
The wrong-network destination check now lives in the FFI wrapper, but `CoreWallet::sweep_coinjoin_to_address` accepts any `NetworkChecked` `dashcore::Address` and proceeds to drain every spendable CoinJoin UTXO to it. Any non-FFI Rust caller (other consumers of `rs-platform-wallet`, future tests, future wrappers) can pass a mainnet-checked address to a testnet wallet and the sweep will succeed with funds locked to an unspendable script on this chain. Move the `dest.network() == self.network()` check into the sweep method itself so the fund-safety invariant is owned by the API that performs the sweep, not by a single caller.
In `packages/rs-platform-wallet/src/wallet/core/coinjoin_recovery.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/coinjoin_recovery.rs:79-108: Recovery widens only the external CoinJoin pool, not the internal change branch
`set_coinjoin_gap_limit` widens the first pool returned by `address_pools_mut()`. The same PR adds dual-chain signing in `coinjoin_sweep_path_map` because DashSync puts mixed change on the internal `/1/i` branch, yet that branch is not materialized by this recovery API — SPV will not watch scripts that have never been pushed into a pool. If migrated wallets are expected to discover new mixed coins on the internal chain via this scan, the recovery is incomplete; if the assumption is that internal-chain coins are only ever imported via an out-of-band UTXO import, that contract is worth documenting at the top of this module so future callers do not silently miss funds.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta a0ead20 is a clippy-only cleanup in broadcast.rs (drop a redundant clone on a Copy field, range-contains in a test) — no new latest-delta findings. Re-validation of the four prior findings against current head shows all four are STILL_VALID and carried forward: sweep fee constants under-bound large chunks, the Rust sweep API still defers the destination-network invariant to the FFI wrapper, recovery widens only the external CoinJoin pool while internal-chain mixed change is unmaterialized for SPV, and the all-fail broadcast path returns only the final chunk's error.
🟡 1 suggestion(s) | 💬 1 nitpick(s)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/core/coinjoin_recovery.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/coinjoin_recovery.rs:79-108: Recovery widens only the external CoinJoin pool while sweep expects both chains
`set_coinjoin_gap_limit` calls `address_pools_mut().into_iter().next()` and widens that single pool. Yet `coinjoin_sweep_path_map` (broadcast.rs:271-287) explicitly resolves signing paths across BOTH external `/0/` and internal `/1/` chains, with a comment that DashSync CoinJoin puts mixing change on the internal chain. SPV only watches scripts that have been materialized into a pool's script index, so a recovery scan that widens only the external pool will not discover deep internal-chain mixed coins; the dual-chain signing fix only helps with internal-chain UTXOs that arrive through some other import path. Migrated users running `setCoinJoinGapLimit` (Swift wrapper presents this as the migration recovery scan) can silently miss change-chain funds. Either materialize the internal CoinJoin branch alongside the external pool, or rename/document the API to make the external-only scope explicit and require an alternate import path for internal-chain coins.
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "df456b74b6d921a30f3ad26200cb7ff3cec275b5910883aa6fc77dda40ae4751"
)Xcode manual integration:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3817 +/- ##
============================================
+ Coverage 70.73% 71.20% +0.47%
============================================
Files 20 20
Lines 2788 2837 +49
============================================
+ Hits 1972 2020 +48
- Misses 816 817 +1
🚀 New features to boost your workflow:
|
Pulls in the merged BIP-39 mnemonic recover-flow work (#3842): swift-sdk wordList / ffiMnemonicValue + the rust-dashcore pin to the merged #806 core, so the iOS coinjoin-recovery-debug branch can build against the SwiftDashSDK mnemonic API. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> # Conflicts: # Cargo.lock # Cargo.toml
…ep API Round-3 review follow-ups on the CoinJoin sweep (all non-blocking review suggestions; verified with clippy + tests): - Sweep fee is now a true upper bound. FeeRate::normal() is exactly 1 duff/byte (the relay minimum, no headroom), so the size estimate must not undershoot the serialized tx or a chunk pays below the minimum and is rejected. Size the input-count CompactSize per chunk (1 B < 253 inputs, 3 B for 253..=500) and use the 149 B max compressed-P2PKH input instead of the 148 B typical. Reachable for a heavy mixer whose chunk holds 253-500 inputs; over-paying a few duffs on an all-funds sweep is harmless. - sweep_coinjoin_to_address now rejects a wrong-network destination itself (dest.as_unchecked().is_valid_for_network), not only at the FFI boundary, so the irreversible all-funds sweep is fund-safe for any Rust caller. - Partial-broadcast all-fail now returns the FIRST chunk error (last_err.get_or_insert) instead of the last - the likely root cause. - Documented coinjoin_recovery's external-only scope: internal-chain mixed coins arrive as imported UTXOs, so external-pool widening suffices for discovery while the sweep still signs both chains. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-forward prior findings: all four prior findings are resolved at 43ee9af — fee bound is now a true upper bound (149 B/input + dynamic varint), the wrong-network sweep check is enforced inside the Rust API (not just FFI), the broadcast loop preserves the FIRST chunk error via get_or_insert, and the external-only recovery widening is documented as correct given that internal-chain mixed coins are imported as UTXOs (the sweep path_map still re-derives both chains for signing). New findings in the latest delta: codex-flagged null/network issues in core_wallet_send_to_addresses are about pre-existing code that this PR does not modify and are dropped as out of scope. Only a single in-scope doc nitpick remains on core_wallet_free_tx_bytes whose contract now covers both serialized-tx and txid buffers.
💬 1 nitpick(s)
…xid buffer core_wallet_free_tx_bytes is the free-contract for both core_wallet_send_to_addresses (serialized tx bytes) and core_wallet_sweep_coinjoin (the count*32 txid buffer), but its doc only mentioned the former. Pure docs — no behavioral change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-forward prior findings: none — prior-1 (core_wallet_free_tx_bytes doc) is FIXED at packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs:250-254, where the doc now explicitly enumerates both the send_to_addresses tx-bytes buffer and the sweep_coinjoin count*32 txid buffer. New findings in the latest delta (43ee9af..41d5e8a is docs-only): none introduced by the delta itself. Cumulative review against the current head surfaced one concrete edge case in the hand-built sweep tx assembly (dust-output bound) and a few low-value suggestions; no blocking issues, so APPROVE with a single suggestion comment.
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/broadcast.rs:340-346: Sweep chunk can build a sub-dust output and silently fall back to partial success
The hand-built sweep tx bypasses `TransactionBuilder` and only guards `total_input <= fee`. If a chunk's `total_input - fee` lands above zero but below the destination script's dust threshold (≈546 duffs for P2PKH), the chunk produces a TxOut that relay policy will reject. Because broadcast loops over chunks and tolerates partial failure (line 408-432), the API can still return `Ok(broadcast)` while a dust chunk is silently dropped and those UTXOs remain unswept — exactly the failure mode the all-input chunking is designed to prevent. CoinJoin denominations are normally well above the bound, but DashSync-imported sub-denomination change can ride in on the internal chain and land alone in a one-input final chunk, which is precisely the path the new dual-chain resolver enables. Validate `output_amount >= dest.script_pubkey().dust_value()` before building the TxOut and return a typed build error, or fold the dust remainder into a sibling chunk so the sweep is all-or-nothing per UTXO.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-forward prior findings: none — prior-1 (core_wallet_free_tx_bytes doc) is FIXED at packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs:250-254, where the doc now explicitly enumerates both the send_to_addresses tx-bytes buffer and the sweep_coinjoin count*32 txid buffer. New findings in the latest delta (43ee9af..41d5e8a is docs-only): none introduced by the delta itself. Cumulative review against the current head surfaced one concrete edge case in the hand-built sweep tx assembly (dust-output bound) and a few low-value suggestions; no blocking issues, so APPROVE with a single suggestion comment.
Reviewed commit: 41d5e8a
Fresh response to a new review request. The PR head is unchanged since the exact-SHA automated review, so this records the same verified result without duplicating inline threads.
Code Review
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/broadcast.rs:340-346: Sweep chunk can build a sub-dust output and silently fall back to partial success
The hand-built sweep tx bypasses `TransactionBuilder` and only guards `total_input <= fee`. If a chunk's `total_input - fee` lands above zero but below the destination script's dust threshold (≈546 duffs for P2PKH), the chunk produces a TxOut that relay policy will reject. Because broadcast loops over chunks and tolerates partial failure (line 408-432), the API can still return `Ok(broadcast)` while a dust chunk is silently dropped and those UTXOs remain unswept — exactly the failure mode the all-input chunking is designed to prevent. CoinJoin denominations are normally well above the bound, but DashSync-imported sub-denomination change can ride in on the internal chain and land alone in a one-input final chunk, which is precisely the path the new dual-chain resolver enables. Validate `output_amount >= dest.script_pubkey().dust_value()` before building the TxOut and return a typed build error, or fold the dust remainder into a sibling chunk so the sweep is all-or-nothing per UTXO.
| }) | ||
| .collect(); | ||
| let unsigned = Transaction { | ||
| version: 3, |
There was a problem hiding this comment.
Isn't a better idea to use the TransactionBuilder, doing the needed modifications or does it need to much code to adapt to this use case??
Anyway, this code should live in rust-dashcore, platform-wallet core.rs module is just anwrapper around it (in theory)
There was a problem hiding this comment.
Agreed on both counts — moved upstream in dashpay/rust-dashcore#819.
- TransactionBuilder: the sweep now goes through it via a new
TransactionBuilder::sweep_to(dest)drain mode (consume every added input into one no-change outputtotal − fee, no coin selection). That's exactly why the original couldn't just reusebuild_signed— itsLargestFirstselection re-picks a covering subset and would leave small UTXOs behind. The drain mode also gets the dust-output guard for free. - Lives in rust-dashcore: the chunking + dual-chain (
/0/+/1/) path resolution now live inkey-walletasManagedCoreFundsAccount::build_coinjoin_sweep_txs. This method here is now a thin wrapper: resolve the account under the wallet lock, delegate, broadcast.
| @@ -0,0 +1,128 @@ | |||
| //! One-time CoinJoin recovery: widen a CoinJoin account's address gap limit | |||
There was a problem hiding this comment.
This logic should be in rust-dashcore, and exposed directly though ffi to sdk consumers
There was a problem hiding this comment.
Done — the gap-limit widening + address materialization now live in key-wallet as ManagedCoreFundsAccount::set_coinjoin_gap_limit (dashpay/rust-dashcore#819); this file is now a thin wrapper that just resolves the account and delegates.
On the FFI point: for this pass it's still exposed through platform-wallet-ffi (unchanged) rather than key-wallet-ffi directly — I kept the iOS Core FFI + Swift SDK rewiring out of scope here so the change stays focused. `.
…wallet Addresses review feedback (@ZocoLini): the CoinJoin sweep/recovery logic belongs in rust-dashcore, and the sweep should use TransactionBuilder instead of hand-rolling transaction assembly. The logic now lives upstream (dashpay/rust-dashcore#819); this bumps the pin to that commit and reduces the platform side to thin wrappers. - broadcast.rs: sweep_coinjoin_to_address resolves the account under the wallet lock, delegates build+sign to ManagedCoreFundsAccount::build_coinjoin_sweep_txs, then broadcasts (partial-failure tolerance unchanged). -458 lines. - coinjoin_recovery.rs: set_coinjoin_gap_limit delegates to ManagedCoreFundsAccount::set_coinjoin_gap_limit. -91 lines. - Cargo.toml/Cargo.lock: bump rust-dashcore pin 5c0113e7 -> e406d1cb (adds the upstream CoinJoin sweep/recovery + TransactionBuilder::sweep_to drain mode). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
llbartekll
force-pushed
the
feature/coinjoin-sweep-and-recovery
branch
2 times, most recently
from
8760508 to
9bf6a1f
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta refactors CoinJoin sweep + gap-limit recovery into thin wrappers around upstream key-wallet helpers. The prior dust/partial-success finding is resolved: transaction construction (including dust validation) is now delegated to key_wallet::ManagedCoreFundsAccount::build_coinjoin_sweep_txs, and per-chunk broadcast failures emit tracing::warn! and propagate the first error if no chunk succeeds. No in-scope blocking issues remain; one readability nit on variable naming.
💬 1 nitpick(s)
| let mut last_err: Option<PlatformWalletError> = None; | ||
| for tx in signed_txs { | ||
| match self.broadcast_transaction(&tx).await { | ||
| Ok(_) => broadcast.push(tx), | ||
| Err(e) => { | ||
| // Partial failure is tolerated (caller re-runs to sweep the | ||
| // remainder), but never silent: log each dropped chunk error. | ||
| tracing::warn!( | ||
| "CoinJoin sweep: a chunk failed to broadcast, continuing \ | ||
| with remaining chunks (caller can re-run): {}", | ||
| e | ||
| ); | ||
| // Keep the FIRST failure (usually the root cause); the later | ||
| // chunk errors are already surfaced via the warn! above. | ||
| last_err.get_or_insert(e); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: last_err actually holds the first error — rename to match the comment
The variable is named last_err but is populated via last_err.get_or_insert(e) (line 241), which only writes when the slot is None. The inline comment at lines 239–240 correctly states 'Keep the FIRST failure (usually the root cause)'. The implementation matches the comment, but the name contradicts it. The common Rust pattern with last_err is last_err = Some(e) overwriting on each iteration; a reader scanning the variable name will draw the opposite conclusion to what the code does. Rename to first_err so the name agrees with get_or_insert and the inline comment.
source: ['claude']
3 participants
What
Adds platform-wallet support for recovering and offloading CoinJoin ("mixed") coins for wallets migrating from DashSync (legacy SPV) to the SwiftDashSDK Core wallet. Post-migration CoinJoin is being dropped, so this lets users find their scattered mixed coins and sweep them back into spendable BIP44 balance.
Four commits:
sweep_coinjoin_to_address(empties the CoinJoin account to a destination address) +set_coinjoin_gap_limitfor a one-time wide recovery scan that finds deep mixed coins scattered across large address holes (DashSync used a gap of 400 vs the SDK default of 30).Vec<Transaction>, so a heavy mixer whose UTXOs exceed a single relayable tx still sweeps (broadcast with partial-failure tolerance).…/0/i) and internal (…/1/i, mixing change) chains; the SDK's CoinJoin account is external-only. The sweep now builds a dual-chainaddress → DerivationPathresolver from the CoinJoin account xpub so internal-chain UTXOs sign correctly (otherwise a single internal input blocks the whole all-or-nothing chunk).Also includes the FFI (
core_wallet/broadcast.rs) and swift-sdk wrapper (ManagedCoreWallet.swift) surface for the above.Commit 3 bumps the workspace rust-dashcore pin from
eb889af1→7ff6b246(8 crates).7ff6b246iseb889af1+ 5 commits and includes the merged dashpay/rust-dashcore#804, which the internal-chain signing (commit 4) depends on. This is a forward bump, not a revert.Verification
cargo build -p platform-wallet— clean (branch rebased onto currentv3.1-dev).cargo test -p platform-wallet— dual-chain resolver unit tests pass (resolves_external_and_internal_chain_addresses,errors_when_an_input_address_is_unresolvable).DashSDKFFI.xcframeworkrebuilt; the SwiftExampleApp and the downstream dashwallet-ios app both build against it (iOS simulator).Merge order
The dashwallet-ios app branch links the SwiftDashSDK built from this branch, so this PR should merge into
v3.1-devbefore the app-side PR.🤖 Generated with Claude Code
Summary by CodeRabbit