Repository navigation
fix(receive): serve the receive address from the SDK engine, not the held dashj chain - #1570
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 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 change adds live SDK-backed address reads and a cache tied to the active wallet binding. Receive-address consumers use live accessors, and internal-transfer paths use unadvertised destinations. Callers handle unavailable-address failures without continuing the affected operation. ChangesWallet address sourcing
Priority: ⬆️ High Estimated code review effort: 4 (Complex) | ~60 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change now fails closed instead of serving a possibly already-used address when the SDK address is not yet available, and callers show an error or retry. Only a minor documentation cleanup remains, so it is ready to merge. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Address-dependent operations gain safer failure handling, and private transfers are separated from publicly advertised receive addresses. However, selecting private-transfer destinations from a shared address pool can still link them to ordinary payments. Reset and concurrent-operation guarantees remain only partially established. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
|
⛔ Final review complete — 1 blocking finding(s) (commit 1abb621) · triage: critical |
Verified on device — SR-03 fixed, 3/3 vs 3/3Ran on emulator-5560 (AVD Assertion: after restore, the Receive screen must serve an address not already on chain. Procedure (
On-chain status from Same wallet either way — username Regression sweep (HEAD)
Not covered at runtime — please weigh before merge
Full write-up and evidence paths: 🤖 Generated with Claude Code |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
⚠️ DEGRADED — Final validation — Phase 2 only (queue backlog)
⚠️ DEGRADED review. The primary review models were unavailable (gpt-6-astraunavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache), so this review ran on stand-in models:gpt-5.6-luna→muse-spark-1.3-contributor,gpt-5.6-sol→muse-spark-1.3-contributor,gpt-5.6-terra→muse-spark-1.3-contributor,gpt-6-astra→muse-spark-1.3-contributor. Both review phases and the independent verifiers still ran, but on weaker models, with Phase 1 capped athigheffort. Treat the verdict as provisional; a full-strength re-review will run on the next push once the primary models are back.
The fix is correct and well-tested; all three remaining notes check out against the source as minor doc staleness and one untested rollback path. No blocking issues, so this is a COMMENT review.
🟡 1 suggestion(s) | 💬 2 nitpick(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
💬 Nitpick: refundAddressOrNull KDoc still describes the removed Room-pool read
wallet/src/de/schildbach/wallet/service/platform/sdk/SdkL1SendService.kt:1771-1778
Verified: the interface KDoc on SdkL1SendSource.unusedExternalAddress (lines 620-634) was updated to describe the engine FFI read, and the implementation (lines 875-896) reads core.nextReceiveAddress over the FFI. But refundAddressOrNull's KDoc (lines 1771-1778) still says 'the SDK's persisted address pool' with null 'when ... the pool rows are missing', and the call-site comment at SendCoinsTaskRunner.kt:939 repeats the same stale 'persisted address pool' wording. Behavior is correct; only the two comments misdescribe the source and its null conditions (now: wallet unbound, no BIP44 account yet, or read failure).
/**
* The BIP70 `Payment.refund_to` source, post-cutover: the ENGINE's next
* unused external address over the FFI
* ([SdkL1SendSource.unusedExternalAddress]) — no dashj keychain read.
* Contained: null (⇒ the caller omits refund_to, which BIP70 makes
* optional) when the wallet is unbound, has no BIP44 account yet, or
* the read fails.
*/
source: muse-spark-1.3-contributor (phase2-reviewer: general)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: security-auditor); final verifier: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: astra-verifier, role: final-verifier)
- Degraded mode:
gpt-6-astraunavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache (detected by probe, since 2026-09-20T20:45:34Z); stand-insgpt-5.6-luna→muse-spark-1.3-contributor,gpt-5.6-sol→muse-spark-1.3-contributor,gpt-5.6-terra→muse-spark-1.3-contributor,gpt-6-astra→muse-spark-1.3-contributor; Phase 1 effort capped athigh - Triage:
normalbymuse-spark-1.3-contributor(standing in forgpt-6-astra) (effort low) — Large cross-cutting receive-address sourcing change spans 18 files but only switches the address provider without altering consensus, coin selection, signing, cryptography, or migrations. - Phase 1 reviewers: not run (skipped for throughput: 34 PRs queued, above the 10 limit)
- Fresh verifier:
muse-spark-1.3-contributor(standing in forgpt-6-astra) — final-verifier; agentastra-verifier - Phase 2 reviewers:
muse-spark-1.3-contributor(standing in forgpt-6-astra) — general (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — security-auditor (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `wallet/src/de/schildbach/wallet/service/platform/sdk/SdkL1SendService.kt`:
- [NITPICK] wallet/src/de/schildbach/wallet/service/platform/sdk/SdkL1SendService.kt:1771-1778: refundAddressOrNull KDoc still describes the removed Room-pool read
Verified: the interface KDoc on SdkL1SendSource.unusedExternalAddress (lines 620-634) was updated to describe the engine FFI read, and the implementation (lines 875-896) reads core.nextReceiveAddress over the FFI. But refundAddressOrNull's KDoc (lines 1771-1778) still says 'the SDK's persisted address pool' with null 'when ... the pool rows are missing', and the call-site comment at SendCoinsTaskRunner.kt:939 repeats the same stale 'persisted address pool' wording. Behavior is correct; only the two comments misdescribe the source and its null conditions (now: wallet unbound, no BIP44 account yet, or read failure).
In `wallet/src/de/schildbach/wallet/service/platform/sdk/CutoverUiDataService.kt`:
- [NITPICK] wallet/src/de/schildbach/wallet/service/platform/sdk/CutoverUiDataService.kt:2450-2452: Live-read KDoc claims null on unavailable read, but failures hold the cache
Verified: sdkReceiveAddressLiveBlockingOrNull (lines 2470-2479) returns `address ?: _sdkReceiveAddress.value`, so a failed read with a warm cache returns the held address — the deliberate behavior pinned by the postCutover hold-on-failure test. But the sdkReceiveAddressLiveOrNull KDoc (lines 2450-2452) says 'Null when the cutover is not active or the read is unavailable — the caller falls back, it never returns a guessed address.' The second clause is only true when no address was ever published; otherwise the caller gets the held (possibly slightly stale) address, which is the intended outcome but not what the sentence states.
- [SUGGESTION] wallet/src/de/schildbach/wallet/service/platform/sdk/CutoverUiDataService.kt:2599-2610: Rollback clears the receive-address overlay but no test pins it
Verified: the six new tests (CutoverUiDataServiceTest lines 1393-1494) cover pre-cutover passthrough, post-cutover serving, advance-on-tx-event, hold-on-failure, unavailable-from-start, and live-bypass — but nothing drives the cutover flag CUT_OVER to inactive and asserts the overlay clears. If a future change drops either clear (lines 2599-2610 on rollback, or the same clearing in the pipeline-failure catch at lines 2615-2624), the synchronous overlay would keep serving an engine address from an abandoned wallet binding after dashj retakes the key chain. A test asserting sdkReceiveAddressOrNull and the live read both return null after deactivation would close the gap cheaply.
| * BLOCKS on the FFI (it takes the engine's wallet-manager write lock), hence | ||
| * [Dispatchers.IO]. Null when the cutover is not active or the read is | ||
| * unavailable — the caller falls back, it never returns a guessed address. |
There was a problem hiding this comment.
✅ No longer applies at
3862d6c4; see the replies below.
💬 Nitpick: Live-read KDoc claims null on unavailable read, but failures hold the cache
Verified: sdkReceiveAddressLiveBlockingOrNull (lines 2470-2479) returns address ?: _sdkReceiveAddress.value, so a failed read with a warm cache returns the held address — the deliberate behavior pinned by the postCutover hold-on-failure test. But the sdkReceiveAddressLiveOrNull KDoc (lines 2450-2452) says 'Null when the cutover is not active or the read is unavailable — the caller falls back, it never returns a guessed address.' The second clause is only true when no address was ever published; otherwise the caller gets the held (possibly slightly stale) address, which is the intended outcome but not what the sentence states.
| * BLOCKS on the FFI (it takes the engine's wallet-manager write lock), hence | |
| * [Dispatchers.IO]. Null when the cutover is not active or the read is | |
| * unavailable — the caller falls back, it never returns a guessed address. | |
| * BLOCKS on the FFI (it takes the engine's wallet-manager write lock), hence | |
| * [Dispatchers.IO]. Null when the cutover is not active, or when the read is | |
| * unavailable and no address has ever been published — otherwise a failed | |
| * read holds and returns the last known address rather than falling back | |
| * to the frozen dashj pointer. |
source: muse-spark-1.3-contributor (phase2-reviewer: general)
There was a problem hiding this comment.
Resolved (re-reviewed at 9231bf22): Your updated Kotlin live-read KDoc explicitly distinguishes an unavailable read with an empty cache from a failed read that retains a warm cache. It now matches the implementation and the assertions in postCutover_failedEngineReadHoldsTheLastAddress.
There was a problem hiding this comment.
No longer applies (re-reviewed at 3862d6c4): Your revised implementation returns null after failed live revalidation on either a warm or cold cache, preserving the hold only for the synchronous overlay. The current KDoc matches that split, so the earlier contradiction with a live warm-cache hold has been superseded.
| _sdkConfirmedBalance.value = null | ||
| _sdkMaxSendable.value = null | ||
| _sdkSpendableUtxoCount.value = null | ||
| // A rollback puts dashj back in charge of the key chain, | ||
| // so the engine address must stop being served — and the | ||
| // bound id must go with it, or a live read could still | ||
| // answer from the wallet the rollback just abandoned. | ||
| _sdkReceiveAddress.value = null | ||
| activeWalletIdHex = null | ||
| return@collectLatest |
There was a problem hiding this comment.
🟡 Suggestion: Rollback clears the receive-address overlay but no test pins it
Verified: the six new tests (CutoverUiDataServiceTest lines 1393-1494) cover pre-cutover passthrough, post-cutover serving, advance-on-tx-event, hold-on-failure, unavailable-from-start, and live-bypass — but nothing drives the cutover flag CUT_OVER to inactive and asserts the overlay clears. If a future change drops either clear (lines 2599-2610 on rollback, or the same clearing in the pipeline-failure catch at lines 2615-2624), the synchronous overlay would keep serving an engine address from an abandoned wallet binding after dashj retakes the key chain. A test asserting sdkReceiveAddressOrNull and the live read both return null after deactivation would close the gap cheaply.
source: muse-spark-1.3-contributor (phase2-reviewer: general)
There was a problem hiding this comment.
Resolved (re-reviewed at 9231bf22): Your MutableStateFlow-backed fixture now reaches the CUT_OVER → DUAL_RUNNING branch, and rollback_clearsTheReceiveAddressOverlayAndTheBoundWallet asserts that the cached/live receive accessors and balance/UTXO overlays stop serving. This resolves the missing regression coverage; the new finding concerns a separate interleaving where a live read is already in flight when the clear occurs.
Review feedback on #1570. All three findings verified against the source first; none were false positives. - `refundAddressOrNull`'s KDoc and the `SendCoinsTaskRunner` call-site comment still described the Room address-pool read this PR deleted, including its null conditions ("the pool rows are missing"). Both now describe the engine FFI read and its real null cases: wallet unbound, no BIP44 account yet, or the read failed. - `sdkReceiveAddressLiveOrNull`'s KDoc claimed null "when the read is unavailable", which contradicted both the implementation and this PR's own `postCutover_failedEngineReadHoldsTheLastAddress` test: a failed read on a warm cache deliberately returns the last known engine address, because dropping to null would send the caller back to the frozen dashj pointer. The doc now states the hold and keeps the guarantee that actually holds — every value it yields came from the engine, none are guessed. - The rollback clear (CUT_OVER -> DUAL_RUNNING) had no test, and could not have had one: `configWithState` returns a one-shot `flowOf`, so the deactivation branch was unreachable from the suite. Added `configWithMutableState` and a test asserting that after a rollback the cached overlay, the live read, the balance and the UTXO count all stop serving. Confirmed non-vacuous by mutation: deleting `_sdkReceiveAddress.value = null` fails it. Rebased onto 02e741b; the upstream CutoverUiDataService changes merged without conflict. Full suite: 152 failures, an identical set to the base at 02e741b (pre-existing conscrypt native-lib failures in this environment). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1fd56ec to
9231bf2
Compare
Review feedback addressed, rebased, and re-verified on deviceAll three findings from the (degraded-mode) review checked against the source first — none were false positives, all three fixed.
Rebased onto
|
| Build | Commit | Trials | Address served | On chain |
|---|---|---|---|---|
| BASE | 02e741b0c |
3/3 identical | yhAK8CWoeNP9Um9jvqSkLZURQnzenjK3v8 |
txAppearances=2, received 0.11 |
| HEAD | 9231bf227 |
3/3 identical | yLQFsqQqJuayzP3j1usaFpuTL1KjjLyjLv |
txAppearances=0 |
Same addresses as the first run, so neither the upstream commits nor the review fixes moved the result. Both APKs versionCode 12000017, same QA key, emulator-5560 (qa_pr) only. All six trials reached phase=SYNCED 100.0%.
Unit tests on the rebased head: 152 failures, an identical set to a fresh baseline captured on 02e741b0c itself — no new failures. The count rose from 151 because the base gained a failing test, not this branch. ktlint clean.
Still not covered at runtime
Unchanged from before, and worth weighing at merge: pre-cutover fallback (a restore auto-commits to CUT_OVER within seconds, so the dashj path never runs on device), pointer-advance-after-receive (needs funding the displayed address; no usable node), and the other call sites moved onto the live read — unshield destination, paper-wallet sweep, Maya, buy/sell deposit addresses, BIP70 refund_to.
This push should also trigger the full-strength re-review the degraded run promised.
🤖 Generated with Claude Code
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The three prior findings are fixed at the exact requested head. Source inspection confirms one blocking lifecycle race introduced by the receive-address overlay: an outstanding read can republish the previous wallet's address after reset clears it. A separate Java accessor documentation mismatch also remains; this verification did not rerun the reviewers' reported tests.
🔴 1 blocking | 💬 1 nitpick(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
criticalbygpt-6-astra(effort low) — The cross-cutting change introduces lifecycle-sensitive cached and live FFI address selection in CutoverUiDataService.kt and changes actual funds destinations in ShieldedTransferExecutor.kt, SweepWalletFragment.java, and CoinJoinFundsMigrationService.kt, meeting both the intricacy and funds-movement criteria. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— security-auditor (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 1% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `wallet/src/de/schildbach/wallet/service/platform/sdk/CutoverUiDataService.kt`:
- [BLOCKING] wallet/src/de/schildbach/wallet/service/platform/sdk/CutoverUiDataService.kt:2496-2504: Prevent in-flight receive reads from republishing a cleared wallet address
The active-state and wallet-ID checks occur only before the blocking engine read. A caller can capture wallet A's address, be delayed before publication, and resume after the deactivation branch has cleared `_sdkReceiveAddress` and `activeWalletIdHex`. Line 2503 then restores A's address. These live reads run in UI coroutines or Java background handlers outside `runPipelines`, so cancelling the pipeline does not prevent this write. This is a wallet-isolation issue, not just rollback hardening: `WalletApplicationExt.clearDatabasesInner()` resets cutover state during Reset Wallet without restarting the process, while `sdkReceiveAddressOrNull()` serves the cache without checking activation or wallet ownership. The next wallet can therefore receive A's cached address, including when its own live read is unavailable; `WalletApplication` checks the address's network, not ownership by the current seed. Payments to that destination can go to the erased wallet rather than the current wallet. Associate cached values and outstanding reads with a wallet/activation generation, and serialize generation validation plus publication with deactivation and rebinding. A second active-state check alone still permits a check/write race and cannot distinguish successive activations. Add a regression that pauses A's read before publication, deactivates and clears the overlay, then releases the read and activates B with its engine read unavailable.
In `wallet/src/de/schildbach/wallet/WalletApplication.java`:
- [NITPICK] wallet/src/de/schildbach/wallet/WalletApplication.java:2060-2067: Document the Java live accessor's warm-cache fallback
The Javadoc lists an unavailable SDK or failed read as returning null, but this wrapper delegates to `sdkReceiveAddressLiveBlockingOrNull()`, which deliberately returns the last cached engine address when a read is unavailable on a warm cache. A valid cached address passes through `toDashjAddressOrNull()`. Document the cold-cache versus warm-cache distinction here as well, matching the corrected Kotlin accessor documentation.
| fun sdkReceiveAddressLiveBlockingOrNull(): String? { | ||
| if (!_cutoverActive.value) return null | ||
| val walletIdHex = activeWalletIdHex ?: return null | ||
| val address = source.nextReceiveAddressOrNull(walletIdHex) | ||
| // A failed read leaves the last known value in place rather than | ||
| // clearing it: an empty cache sends the synchronous overlay back to the | ||
| // frozen dashj address, which is the defect this exists to fix. | ||
| if (address != null) _sdkReceiveAddress.value = address | ||
| return address ?: _sdkReceiveAddress.value |
There was a problem hiding this comment.
🔴 Blocking: Prevent in-flight receive reads from republishing a cleared wallet address
The active-state and wallet-ID checks occur only before the blocking engine read. A caller can capture wallet A's address, be delayed before publication, and resume after the deactivation branch has cleared _sdkReceiveAddress and activeWalletIdHex. Line 2503 then restores A's address. These live reads run in UI coroutines or Java background handlers outside runPipelines, so cancelling the pipeline does not prevent this write. This is a wallet-isolation issue, not just rollback hardening: WalletApplicationExt.clearDatabasesInner() resets cutover state during Reset Wallet without restarting the process, while sdkReceiveAddressOrNull() serves the cache without checking activation or wallet ownership. The next wallet can therefore receive A's cached address, including when its own live read is unavailable; WalletApplication checks the address's network, not ownership by the current seed. Payments to that destination can go to the erased wallet rather than the current wallet. Associate cached values and outstanding reads with a wallet/activation generation, and serialize generation validation plus publication with deactivation and rebinding. A second active-state check alone still permits a check/write race and cannot distinguish successive activations. Add a regression that pauses A's read before publication, deactivates and clears the overlay, then releases the read and activates B with its engine read unavailable.
source: muse-spark-1.3-contributor (phase1-reviewer: general, security-auditor); gpt-6-astra (phase2-reviewer: general, security-auditor)
There was a problem hiding this comment.
Resolved (re-reviewed at 3c0a76e8): Your generation capture and publication check share the binding-invalidation lock, so a read returning after invalidation cannot restore or return the erased binding's address. The parked-read regression passed on rerun; the first run stopped at its initial cache-population assertion, before exercising the race.
| /** | ||
| * A LIVE engine read of the next unused receive address, bypassing the cache | ||
| * {@link #sdkReceiveAddressOrNull()} serves, or null when the engine has no | ||
| * answer (pre-cutover, rolled back, SDK not up, read failed). | ||
| * | ||
| * <p>BLOCKS on the SDK FFI — it takes the engine's wallet-manager write | ||
| * lock — so both public callers below are contracted off-main. | ||
| */ |
There was a problem hiding this comment.
💬 Nitpick: Document the Java live accessor's warm-cache fallback
The Javadoc lists an unavailable SDK or failed read as returning null, but this wrapper delegates to sdkReceiveAddressLiveBlockingOrNull(), which deliberately returns the last cached engine address when a read is unavailable on a warm cache. A valid cached address passes through toDashjAddressOrNull(). Document the cold-cache versus warm-cache distinction here as well, matching the corrected Kotlin accessor documentation.
| /** | |
| * A LIVE engine read of the next unused receive address, bypassing the cache | |
| * {@link #sdkReceiveAddressOrNull()} serves, or null when the engine has no | |
| * answer (pre-cutover, rolled back, SDK not up, read failed). | |
| * | |
| * <p>BLOCKS on the SDK FFI — it takes the engine's wallet-manager write | |
| * lock — so both public callers below are contracted off-main. | |
| */ | |
| /** | |
| * A LIVE engine read of the next unused receive address. When an active, | |
| * bound SDK read is unavailable, retains the last cached engine address; | |
| * returns null if that cache is empty. Also returns null when the service | |
| * is absent, cutover is inactive, no wallet is bound, or the resulting | |
| * address is invalid for the current network. | |
| * | |
| * <p>BLOCKS on the SDK FFI — it takes the engine's wallet-manager write | |
| * lock — so both public callers below are contracted off-main. | |
| */ |
source: muse-spark-1.3-contributor (phase1-reviewer: general, security-auditor)
There was a problem hiding this comment.
Resolved (re-reviewed at 3c0a76e8): Your revised Java documentation matches the implementation: a cold-cache failure returns null, a warm-cache failure retains the engine address, and a changed binding invalidates the in-flight answer.
There was a problem hiding this comment.
No longer applies (re-reviewed at 373ca4bb): Your later revalidation change removed the live accessor's warm-cache fallback, so documenting that former behavior would now be incorrect. The current Java documentation accurately separates the synchronous overlay's held value from a failed live answer.
BLOCKING review finding on #1570, and it is a real one — verified against the source before changing anything. `sdkReceiveAddressLiveBlockingOrNull` checked the cutover flag and the bound wallet id, then made a BLOCKING FFI read, then published unconditionally. That is check-then-act across a call that cannot be cancelled, so: 1. wallet A's live read captures A's id and blocks in the engine; 2. Reset Wallet wipes A. `WalletApplicationExt.clearDatabasesInner` says in as many words that it does NOT restart the process, and it calls `cutoverCoordinator.resetForWalletWipe()`, so the deactivation branch runs and clears the overlay; 3. the parked read returns and republishes A's address into the cache; 4. wallet B restores. Until B's own engine read lands, `sdkReceiveAddressOrNull` — which checked neither activation nor ownership — serves A's address, and `WalletApplication` validates the address's NETWORK, not which seed owns it, so nothing downstream catches it. The Receive screen then advertises an address only the ERASED wallet can spend. That is worse than the SR-03 defect this PR set out to fix, and the existing `_cutoverActive` gate does not prevent it: re-checking the flag on the way out is still check-then-act, and a flag cannot tell a surviving binding from a different one that activated meanwhile. So the cache now carries the binding GENERATION it was read under. The generation is bumped on every bind and unbind under a lock; a read captures it before the FFI and may publish only if it is still current; the synchronous accessor serves an entry only while its stamp matches. Cross-wallet reuse is unrepresentable rather than merely unlikely. The lock is never held across the FFI call, so a deactivation never waits on the engine. Also removed the public `sdkReceiveAddress` StateFlow: it had no consumers and would have been a fourth, unstamped way to read the cache. Regression: `inFlightReadCannotRepublishAWipedWalletsAddress` parks a read for wallet A inside the FFI, wipes, releases it, then activates B with B's own read unavailable — the only way A's leftover could surface. Confirmed non-vacuous by mutation: deleting the generation check in the live read fails it. The fixture gates by calling thread rather than a one-shot flag, because the balance pipeline reads through the same method — an earlier one-shot version was eaten by the pipeline and passed vacuously. Nitpick also fixed: `WalletApplication.liveSdkReceiveAddressOrNull`'s Javadoc now documents the cold-cache/warm-cache distinction and the changed-binding null. Full suite: 152 failures, an identical set to the previous head — no new failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Blocking finding fixed — it was real, and worse than the bug this PR set out to fixHead is now The defect
B's Receive screen advertises an address only the erased wallet can spend. My existing The fixThe cache now carries the binding generation it was read under, bumped on every bind/unbind under a lock. A read captures it before the FFI and may publish only if it is still current; the synchronous accessor serves an entry only while its stamp matches. Cross-wallet reuse is unrepresentable rather than unlikely. The lock is never held across the FFI call, so deactivation never waits on the engine. I also removed the public The nitpick is fixed too: VerificationUnit — Device — SR-03 unchanged at 3/3 (
On chain: both served addresses 0 appearances; B's own previously funded What the device run does not prove: it shows isolation in the ordinary case; it does not reproduce the race, which needs a read parked in the FFI at the instant of the wipe and is not drivable from the UI. The unit test is the proof for that. Full suite: 152 failures, identical set to the previous head — none new. ktlint clean. One correction worth statingThe wallet-switch check initially reported B serving A's address — an apparent reproduction on the fixed build. It was a fixture failure, not a defect: 🤖 Generated with Claude Code |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified head 3c0a76e: all five prior findings are fixed, but the shared receive/unshield destination introduces a privacy regression, and one CoinJoin KDoc reference remains stale. The 99 focused tests passed on rerun; the first run failed the isolation test's initial cache-population assertion before reaching the parked-read scenario. No tracked files were changed.
🔴 1 blocking | 💬 1 nitpick(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
💬 Nitpick: CoinJoin drain KDoc still names the plain freshReceiveAddress accessor
wallet/src/de/schildbach/wallet/service/platform/sdk/SdkL1SendService.kt:1419-1421
The migration caller now obtains this destination through walletData.freshReceiveAddressLive() on Dispatchers.IO, but the KDoc still names freshReceiveAddress(). Update the reference to preserve the live-read requirement. At this head the plain accessor also has an SDK overlay, but it serves the cache rather than performing the live engine read used by the migration.
* [ownAddressBase58] MUST be an address of THIS wallet's unmixed BIP44
* account (callers derive it via `WalletData.freshReceiveAddressLive()`); the
* only validation possible here is network/format.
source: muse-spark-1.3-contributor (phase1-reviewer: general)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
criticalbygpt-6-astra(effort low) — The diff introduces intricate wallet-generation-aware FFI address caching in CutoverUiDataService.sdkReceiveAddressLiveBlockingOrNull and changes actual funds destinations in ShieldedTransferExecutor and CoinJoinFundsMigrationService, making wallet isolation and address selection critical to funds safety. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— security-auditor (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 21% left, 5h 14% left),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `wallet/src/de/schildbach/wallet/ui/shielded/ShieldedTransferExecutor.kt`:
- [BLOCKING] wallet/src/de/schildbach/wallet/ui/shielded/ShieldedTransferExecutor.kt:605-612: Do not unshield to an address already handed to a payer
Post-cutover, this destination and the Receive screen now resolve to the same engine next-unused address. A counterparty can obtain the user's receive QR, leave it unpaid, and monitor it; a subsequent unshield then pays that disclosed address, linking the withdrawal and its amount to the user. `withdrawToCore()` forwards this address to the withdrawal source without allocating a separate destination. This occurs with successful reads and a fully synced engine, not just during the documented mid-scan window. Previously, the Receive screen's dashj current key was already issued, and the unshield path requested another fresh key. The documented fresh/current narrowing therefore removes destination separation from a privacy-sensitive self-transfer, rather than merely changing invoice allocation. Use an SDK-owned destination reserved separately from externally advertised receive addresses, and add coverage proving that an unpaid receive handout is not reused for unshielding.
In `wallet/src/de/schildbach/wallet/service/platform/sdk/SdkL1SendService.kt`:
- [NITPICK] wallet/src/de/schildbach/wallet/service/platform/sdk/SdkL1SendService.kt:1419-1421: CoinJoin drain KDoc still names the plain freshReceiveAddress accessor
The migration caller now obtains this destination through `walletData.freshReceiveAddressLive()` on `Dispatchers.IO`, but the KDoc still names `freshReceiveAddress()`. Update the reference to preserve the live-read requirement. At this head the plain accessor also has an SDK overlay, but it serves the cache rather than performing the live engine read used by the migration.
| shieldedBalanceService.withdrawToCore( | ||
| debugUnshieldDestinationOverride() | ||
| ?: walletDataProvider.freshReceiveAddressString(), | ||
| // `Live`, not the plain accessor: this whole function runs | ||
| // on [ioDispatcher], and post-cutover the dashj chain is | ||
| // HELD — the plain read would unshield into an address the | ||
| // chain has already paid (SR-03 / D-003). | ||
| ?: walletDataProvider.freshReceiveAddressLive().toBase58(), | ||
| amount |
There was a problem hiding this comment.
🔴 Blocking: Do not unshield to an address already handed to a payer
Post-cutover, this destination and the Receive screen now resolve to the same engine next-unused address. A counterparty can obtain the user's receive QR, leave it unpaid, and monitor it; a subsequent unshield then pays that disclosed address, linking the withdrawal and its amount to the user. withdrawToCore() forwards this address to the withdrawal source without allocating a separate destination. This occurs with successful reads and a fully synced engine, not just during the documented mid-scan window. Previously, the Receive screen's dashj current key was already issued, and the unshield path requested another fresh key. The documented fresh/current narrowing therefore removes destination separation from a privacy-sensitive self-transfer, rather than merely changing invoice allocation. Use an SDK-owned destination reserved separately from externally advertised receive addresses, and add coverage proving that an unpaid receive handout is not reused for unshielding.
source: gpt-6-astra (phase2-reviewer: security-auditor)
There was a problem hiding this comment.
Resolved (re-reviewed at 41b48c28): Your withdrawal executor now obtains its destination through unadvertisedDestinationLive(), not the advertised receive accessors, before invoking withdrawToCore(). The focused executor tests passed, including the assertion that destination preparation failure prevents withdrawal.
…dress BLOCKING review finding on #1570, and it is correct — a privacy regression this PR introduced, not a pre-existing one. Post-cutover `fresh` and `current` receive addresses coincide, because the engine tracks USED, not ISSUED. The unshield destination and the Receive screen therefore resolved to the SAME engine next-unused address. A counterparty handed the user's receive QR who simply never pays it can watch that address; the user's next unshield then pays it, disclosing the withdrawal and its amount — the one thing shielding exists to prevent. Before this PR the two were separate: the Receive screen showed dashj's already-issued current key while the unshield requested a NEW key. The fresh/current collapse removed that separation, and the "deliberate narrowing" note under-weighted the consequence. The engine already has a chain that is never handed out — the BIP-44 internal (change) chain, `core_wallet_next_change_address`. Self-transfers now draw from it via a new `WalletData.unadvertisedDestinationLive()`: - the shielded withdrawal (`ShieldedTransferExecutor`), the finding's subject; - the post-upgrade CoinJoin combine (`CoinJoinFundsMigrationService`), NOT flagged but the identical defect, and its own KDoc already said that reusing a published address would link the mixed coins. Fixed with it rather than left as a known instance of a bug being fixed one line away. Pre-cutover, and whenever the engine has no answer, it falls back to dashj's freshly issued receive key — exactly what these paths used before the overlay, and already distinct from the advertised current key. Deliberately NOT cached: every caller consumes it at spend time, and caching would make two successive self-transfers share a destination for no benefit. Two unshields raced before either confirms can still share one internal address (the FFI exposes no reservation), but that is reuse between two of the user's own transfers, invisible to any payer. Generation-validated like the receive read, so a wipe mid-read cannot send our funds to a wallet that is no longer current. The paper-wallet sweep still draws from the receive chain: it is semantically a receive, and its funds are already linked to the paper key. Flagged for review rather than changed silently. Tests: `unshieldDestinationIsNeverTheAdvertisedReceiveAddress` pins the separation and holds it across repeated UNPAID handouts (the attack), and `unshieldDestinationIsRefusedWhenTheBindingIsGone` pins wallet isolation. Both mutation-confirmed: pointing the accessor back at the receive chain fails them. `ShieldedTransferExecutorTest` additionally verifies the receive accessors are never called on the withdraw path. Also fixed a latent flake this PR introduced: `postCutover_receiveAddressAdvancesOnAnEngineTxEvent` asserted after a bare `runCurrent()` against a refresh that hops to a real dispatcher — it passed by luck. It now pumps until the condition holds, and the hold-on-failure case waits for the read to have been ATTEMPTED instead of passing vacuously. Green 3/3 on repeat runs. Nitpick fixed: the CoinJoin drain KDoc named the plain `freshReceiveAddress()`. Full suite: 152 failures, an identical set to the previous head — none new. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Second blocking finding fixed — and it was a regression this PR introducedHead is now The defectPost-cutover Before this PR the two were separate: Receive showed dashj's already-issued current key while the unshield requested a new key. My fresh/current collapse merged them. The narrowing note under-weighted this. The fixThe engine already has a chain that is never handed out — the BIP-44 internal (change) chain, Deliberately not cached: each caller consumes it at spend time, and caching would make two successive self-transfers share a destination for no benefit. Two unshields raced before either confirms can still share one internal address — the FFI exposes no reservation — but that is reuse between two of the user's own transfers, invisible to any payer. I also fixed the CoinJoin combine, which you did not flag. It is the identical defect, and its own KDoc already said that reusing a published address would link the mixed coins; leaving a known instance of a bug one line from its fix was not defensible. The paper-wallet sweep stays on the receive chain — it is semantically a receive and those funds are already linked to the paper key — flagged here rather than changed silently, in case you disagree. Coverage
Evidence that the internal chain is creditedWallet A's existing shield spend The app displays 0.000007 as that wallet's transparent balance, so it already credits internal-chain outputs. That address has One gap, stated plainlyNo live unshield was executed on device. The end-to-end credit is supported by the chain evidence above plus unit tests, not by an observed withdrawal. That step spends a testnet fee from a shared QA fixture and I am holding it pending an explicit go-ahead. This is a funds-destination change, so please weigh that gap rather than assume it closed. SR-03 itself is unchanged: 3/3 on Also fixed: a latent flake this PR introduced
Nitpick fixed: the CoinJoin drain KDoc named the plain Full suite: 152 failures, identical set to the previous head — none new. ktlint clean. 🤖 Generated with Claude Code |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Preliminary review — Phase 1 blocker gate
Verified the findings against head c0f5bfb. The internal-chain destination fixes the normal self-transfer path, but its failure fallback still permits the previously reported advertised-address privacy leak. The other six prior findings are addressed; one new documentation nit remains.
Validated blockers were found by the Phase-1 review and confirmed by a fresh verifier. Phase 2 is deferred until a fresh same-head revalidation clears the blocker gate.
🔴 1 blocking | 💬 1 nitpick(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-gate-verifier, role: verifier)
- Triage:
criticalbygpt-6-astra(effort low) — The diff combines intricate wallet-binding and receive-address cache lifecycle logic in CutoverUiDataService.kt with changes to actual funds destinations in ShieldedTransferExecutor.kt, CoinJoinFundsMigrationService.kt, and SdkL1SendService.kt, meeting both the complexity and funds-movement criteria. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— security-auditor (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 12% left, 5h 51% left),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6-astra— verifier; agentastra-gate-verifier - Phase 2 reviewers: not run (deferred by blocker gate)
🤖 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 `wallet/src/de/schildbach/wallet/WalletApplication.java`:
- [BLOCKING] wallet/src/de/schildbach/wallet/WalletApplication.java:2138-2146: Do not unshield to an address already handed to a payer
The internal-chain read restores separation on success, but the fallback still pays self-transfers to the advertised address. The production nextChangeAddressOrNull() returns null on an unavailable wallet manager, an empty result, or a caught FFI failure. If the receive cache is warm, unadvertisedDestinationLive() then calls the overlaid freshReceiveAddress(), which returns that cached engine receive address—not a freshly issued dashj key as the Javadoc promises. ShieldedTransferExecutor and CoinJoinFundsMigrationService both consume this result. A counterparty holding an unpaid receive QR can therefore observe the withdrawal or combine and its amount when the change-address read fails. Keep the fallback out of the advertised receive overlay; fail the operation before broadcast when a safe post-cutover destination cannot be obtained, and test the warm-receive-cache/change-read-null case through the WalletApplication accessor.
In `wallet/src/de/schildbach/wallet/service/platform/sdk/CutoverUiDataService.kt`:
- [NITPICK] wallet/src/de/schildbach/wallet/service/platform/sdk/CutoverUiDataService.kt:2573-2584: Restore the blocking receive accessor's KDoc and update its callers
The new internal-chain accessor was inserted between sdkReceiveAddressLiveBlockingOrNull() and its KDoc. The receive-read documentation now precedes a second KDoc for the internal-chain method, leaving the receive method without its attached documentation. Move the receive-read KDoc directly above sdkReceiveAddressLiveBlockingOrNull(). Also remove the unshield destination from the caller lists in sdkReceiveAddressLiveOrNull() and WalletApplication.currentReceiveAddressLive(), since unshield now uses unadvertisedDestinationLive().
| public Address unadvertisedDestinationLive() { | ||
| if (cutoverUiDataService != null) { | ||
| final Address unadvertised = toDashjAddressOrNull( | ||
| cutoverUiDataService.sdkUnadvertisedAddressLiveBlockingOrNull()); | ||
| if (unadvertised != null) { | ||
| return unadvertised; | ||
| } | ||
| } | ||
| return freshReceiveAddress(); |
There was a problem hiding this comment.
🔴 Blocking: Do not unshield to an address already handed to a payer
The internal-chain read restores separation on success, but the fallback still pays self-transfers to the advertised address. The production nextChangeAddressOrNull() returns null on an unavailable wallet manager, an empty result, or a caught FFI failure. If the receive cache is warm, unadvertisedDestinationLive() then calls the overlaid freshReceiveAddress(), which returns that cached engine receive address—not a freshly issued dashj key as the Javadoc promises. ShieldedTransferExecutor and CoinJoinFundsMigrationService both consume this result. A counterparty holding an unpaid receive QR can therefore observe the withdrawal or combine and its amount when the change-address read fails. Keep the fallback out of the advertised receive overlay; fail the operation before broadcast when a safe post-cutover destination cannot be obtained, and test the warm-receive-cache/change-read-null case through the WalletApplication accessor.
source: muse-spark-1.3-contributor (phase1-reviewer: general, security-auditor)
There was a problem hiding this comment.
Resolved (re-reviewed at 41b48c28): Your WalletApplication accessor now throws when the post-cutover internal address is unavailable or invalid, closing the warm-cache receive fallback. Its pre-cutover branch calls wallet.freshReceiveAddress() directly rather than the overlaid application accessor.
| /** | ||
| * [sdkReceiveAddressLiveOrNull] without the coroutine — the whole read is | ||
| * synchronous (the FFI call needs no suspension), so the Java call sites | ||
| * that are already on a background thread | ||
| * ([de.schildbach.wallet.WalletApplication.currentReceiveAddressLive] and | ||
| * [de.schildbach.wallet.WalletApplication.freshReceiveAddressLive], which | ||
| * the paper-wallet sweep drives off its background handler) can use it | ||
| * directly instead of bridging into `runBlocking`. | ||
| * | ||
| * BLOCKS on the engine's wallet-manager write lock — off-main only. | ||
| */ | ||
| /** |
There was a problem hiding this comment.
💬 Nitpick: Restore the blocking receive accessor's KDoc and update its callers
The new internal-chain accessor was inserted between sdkReceiveAddressLiveBlockingOrNull() and its KDoc. The receive-read documentation now precedes a second KDoc for the internal-chain method, leaving the receive method without its attached documentation. Move the receive-read KDoc directly above sdkReceiveAddressLiveBlockingOrNull(). Also remove the unshield destination from the caller lists in sdkReceiveAddressLiveOrNull() and WalletApplication.currentReceiveAddressLive(), since unshield now uses unadvertisedDestinationLive().
source: muse-spark-1.3-contributor (phase1-reviewer: general, security-auditor)
There was a problem hiding this comment.
Resolved (re-reviewed at 41b48c28): Your blocking-accessor KDoc is now attached directly to sdkReceiveAddressLiveBlockingOrNull(), and the live-receive caller documentation correctly directs unshield and CoinJoin self-transfers to the separate unadvertised accessor.
…dress BLOCKING review finding on #1570. Correct, and a defect in my own previous fix. `unadvertisedDestinationLive()` fell back to `freshReceiveAddress()` when the engine's change read returned null — but that accessor is OVERLAID by this very PR, so on a warm cache it returns the cached engine RECEIVE address: the advertised one. The fallback therefore reintroduced the exact leak the internal chain was added to prevent, and did so precisely when the engine read was already failing. The Javadoc claiming "dashj's freshly issued receive key" was simply false post-cutover; I wrote it believing the plain accessor was still dashj's, having overlaid it myself two commits earlier. Post-cutover the method now THROWS instead. There is no safe fallback on that side, and failing is cheap where leaking is not: both callers (`ShieldedTransferExecutor`, `CoinJoinFundsMigrationService`) already catch strictly BEFORE broadcast and report not-sent/not-attempted, so a failure costs a retry while a leak is permanent and invisible to the user. Pre-cutover it now reads `wallet.freshReceiveAddress()` directly rather than through the overlaid accessor, which is what it always meant to do. Tests: `unadvertisedDestinationNeverFallsBackToTheWarmReceiveCache` sets up the dangerous shape exactly — warm receive cache, change read unavailable — and asserts the accessor refuses. Mutation-confirmed: letting the read borrow `cachedReceiveAddress` fails it. `freshAddressFailure_isNotSent_notAmbiguous` now also asserts nothing was broadcast and that no advertised accessor was consulted, which is what makes failing closed safe. Nitpick fixed: inserting the internal-chain method had orphaned `sdkReceiveAddressLiveBlockingOrNull`'s KDoc above the wrong function; it is reattached, and the unshield destination is removed from the caller lists on `sdkReceiveAddressLiveOrNull` and `WalletApplication.currentReceiveAddressLive`. Full suite: 152 failures, an identical set to the previous head — none new. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fixed — and this one was a defect in my own previous fixHead is now
The fixPost-cutover it now throws instead of falling back. There is no safe fallback on that side, and the asymmetry is decisive: both callers already catch strictly before broadcast and report not-sent / not-attempted, so failing costs a retry, whereas leaking is permanent and invisible to the user. Pre-cutover it now reads Coverage
If you'd still prefer an assertion driven through the NitpickInserting the internal-chain method had orphaned Full suite: 152 failures, identical set to the previous head — none new. ktlint clean. Still open, unchanged: no live unshield has been executed on device, so the end-to-end credit into the internal chain rests on chain evidence plus unit tests, not an observed withdrawal. Flagging again rather than letting it fade. 🤖 Generated with Claude Code |
Review feedback on #1570. All three findings verified against the source first; none were false positives. - `refundAddressOrNull`'s KDoc and the `SendCoinsTaskRunner` call-site comment still described the Room address-pool read this PR deleted, including its null conditions ("the pool rows are missing"). Both now describe the engine FFI read and its real null cases: wallet unbound, no BIP44 account yet, or the read failed. - `sdkReceiveAddressLiveOrNull`'s KDoc claimed null "when the read is unavailable", which contradicted both the implementation and this PR's own `postCutover_failedEngineReadHoldsTheLastAddress` test: a failed read on a warm cache deliberately returns the last known engine address, because dropping to null would send the caller back to the frozen dashj pointer. The doc now states the hold and keeps the guarantee that actually holds — every value it yields came from the engine, none are guessed. - The rollback clear (CUT_OVER -> DUAL_RUNNING) had no test, and could not have had one: `configWithState` returns a one-shot `flowOf`, so the deactivation branch was unreachable from the suite. Added `configWithMutableState` and a test asserting that after a rollback the cached overlay, the live read, the balance and the UTXO count all stop serving. Confirmed non-vacuous by mutation: deleting `_sdkReceiveAddress.value = null` fails it. Rebased onto 02e741b; the upstream CutoverUiDataService changes merged without conflict. Full suite: 152 failures, an identical set to the base at 02e741b (pre-existing conscrypt native-lib failures in this environment). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
BLOCKING review finding on #1570, and it is a real one — verified against the source before changing anything. `sdkReceiveAddressLiveBlockingOrNull` checked the cutover flag and the bound wallet id, then made a BLOCKING FFI read, then published unconditionally. That is check-then-act across a call that cannot be cancelled, so: 1. wallet A's live read captures A's id and blocks in the engine; 2. Reset Wallet wipes A. `WalletApplicationExt.clearDatabasesInner` says in as many words that it does NOT restart the process, and it calls `cutoverCoordinator.resetForWalletWipe()`, so the deactivation branch runs and clears the overlay; 3. the parked read returns and republishes A's address into the cache; 4. wallet B restores. Until B's own engine read lands, `sdkReceiveAddressOrNull` — which checked neither activation nor ownership — serves A's address, and `WalletApplication` validates the address's NETWORK, not which seed owns it, so nothing downstream catches it. The Receive screen then advertises an address only the ERASED wallet can spend. That is worse than the SR-03 defect this PR set out to fix, and the existing `_cutoverActive` gate does not prevent it: re-checking the flag on the way out is still check-then-act, and a flag cannot tell a surviving binding from a different one that activated meanwhile. So the cache now carries the binding GENERATION it was read under. The generation is bumped on every bind and unbind under a lock; a read captures it before the FFI and may publish only if it is still current; the synchronous accessor serves an entry only while its stamp matches. Cross-wallet reuse is unrepresentable rather than merely unlikely. The lock is never held across the FFI call, so a deactivation never waits on the engine. Also removed the public `sdkReceiveAddress` StateFlow: it had no consumers and would have been a fourth, unstamped way to read the cache. Regression: `inFlightReadCannotRepublishAWipedWalletsAddress` parks a read for wallet A inside the FFI, wipes, releases it, then activates B with B's own read unavailable — the only way A's leftover could surface. Confirmed non-vacuous by mutation: deleting the generation check in the live read fails it. The fixture gates by calling thread rather than a one-shot flag, because the balance pipeline reads through the same method — an earlier one-shot version was eaten by the pipeline and passed vacuously. Nitpick also fixed: `WalletApplication.liveSdkReceiveAddressOrNull`'s Javadoc now documents the cold-cache/warm-cache distinction and the changed-binding null. Full suite: 152 failures, an identical set to the previous head — no new failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…dress BLOCKING review finding on #1570, and it is correct — a privacy regression this PR introduced, not a pre-existing one. Post-cutover `fresh` and `current` receive addresses coincide, because the engine tracks USED, not ISSUED. The unshield destination and the Receive screen therefore resolved to the SAME engine next-unused address. A counterparty handed the user's receive QR who simply never pays it can watch that address; the user's next unshield then pays it, disclosing the withdrawal and its amount — the one thing shielding exists to prevent. Before this PR the two were separate: the Receive screen showed dashj's already-issued current key while the unshield requested a NEW key. The fresh/current collapse removed that separation, and the "deliberate narrowing" note under-weighted the consequence. The engine already has a chain that is never handed out — the BIP-44 internal (change) chain, `core_wallet_next_change_address`. Self-transfers now draw from it via a new `WalletData.unadvertisedDestinationLive()`: - the shielded withdrawal (`ShieldedTransferExecutor`), the finding's subject; - the post-upgrade CoinJoin combine (`CoinJoinFundsMigrationService`), NOT flagged but the identical defect, and its own KDoc already said that reusing a published address would link the mixed coins. Fixed with it rather than left as a known instance of a bug being fixed one line away. Pre-cutover, and whenever the engine has no answer, it falls back to dashj's freshly issued receive key — exactly what these paths used before the overlay, and already distinct from the advertised current key. Deliberately NOT cached: every caller consumes it at spend time, and caching would make two successive self-transfers share a destination for no benefit. Two unshields raced before either confirms can still share one internal address (the FFI exposes no reservation), but that is reuse between two of the user's own transfers, invisible to any payer. Generation-validated like the receive read, so a wipe mid-read cannot send our funds to a wallet that is no longer current. The paper-wallet sweep still draws from the receive chain: it is semantically a receive, and its funds are already linked to the paper key. Flagged for review rather than changed silently. Tests: `unshieldDestinationIsNeverTheAdvertisedReceiveAddress` pins the separation and holds it across repeated UNPAID handouts (the attack), and `unshieldDestinationIsRefusedWhenTheBindingIsGone` pins wallet isolation. Both mutation-confirmed: pointing the accessor back at the receive chain fails them. `ShieldedTransferExecutorTest` additionally verifies the receive accessors are never called on the withdraw path. Also fixed a latent flake this PR introduced: `postCutover_receiveAddressAdvancesOnAnEngineTxEvent` asserted after a bare `runCurrent()` against a refresh that hops to a real dispatcher — it passed by luck. It now pumps until the condition holds, and the hold-on-failure case waits for the read to have been ATTEMPTED instead of passing vacuously. Green 3/3 on repeat runs. Nitpick fixed: the CoinJoin drain KDoc named the plain `freshReceiveAddress()`. Full suite: 152 failures, an identical set to the previous head — none new. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…dress BLOCKING review finding on #1570. Correct, and a defect in my own previous fix. `unadvertisedDestinationLive()` fell back to `freshReceiveAddress()` when the engine's change read returned null — but that accessor is OVERLAID by this very PR, so on a warm cache it returns the cached engine RECEIVE address: the advertised one. The fallback therefore reintroduced the exact leak the internal chain was added to prevent, and did so precisely when the engine read was already failing. The Javadoc claiming "dashj's freshly issued receive key" was simply false post-cutover; I wrote it believing the plain accessor was still dashj's, having overlaid it myself two commits earlier. Post-cutover the method now THROWS instead. There is no safe fallback on that side, and failing is cheap where leaking is not: both callers (`ShieldedTransferExecutor`, `CoinJoinFundsMigrationService`) already catch strictly BEFORE broadcast and report not-sent/not-attempted, so a failure costs a retry while a leak is permanent and invisible to the user. Pre-cutover it now reads `wallet.freshReceiveAddress()` directly rather than through the overlaid accessor, which is what it always meant to do. Tests: `unadvertisedDestinationNeverFallsBackToTheWarmReceiveCache` sets up the dangerous shape exactly — warm receive cache, change read unavailable — and asserts the accessor refuses. Mutation-confirmed: letting the read borrow `cachedReceiveAddress` fails it. `freshAddressFailure_isNotSent_notAmbiguous` now also asserts nothing was broadcast and that no advertised accessor was consulted, which is what makes failing closed safe. Nitpick fixed: inserting the internal-chain method had orphaned `sdkReceiveAddressLiveBlockingOrNull`'s KDoc above the wrong function; it is reattached, and the unshield destination is removed from the caller lists on `sdkReceiveAddressLiveOrNull` and `WalletApplication.currentReceiveAddressLive`. Full suite: 152 failures, an identical set to the previous head — none new. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
c790597 to
41b48c2
Compare
Rebased onto the current base; SR-03 still 3/3. Plus a scope note on run 8.Head is now
On the QA sign-off — what it covers, and what it doesn'tqa-run8 confirms SR-03 on device with a third independent wallet (throwaway T3, However, the run-8 integration build ( So the sign-off is a statement about SR-03 and about the code two commits in. It is not yet evidence about the funds-destination change in What I would want before merge
The automated review lane had failed on infra ( Still no human review at any point. 🤖 Generated with Claude Code |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
At 41b48c2, the nine previously tracked findings are fixed, but three in-scope blockers remain: out-of-order address-cache publication, delayed binding invalidation during rollback, and unshield destinations sharing the ordinary payment-change allocator without reservation. All 132 focused existing tests passed; temporary deterministic probes reproduced both concurrency defects, and inspection of the bundled SDK confirmed the shared, unreserved address allocation. Temporary source changes were removed, the working tree is clean, and git diff --check passed; a live shield-to-unshield cycle remains unverified.
🔴 3 blocking | 🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
criticalbygpt-6-astra(effort low) — The diff combines intricate wallet-binding and address-cache lifecycle logic in CutoverUiDataService.kt with changes to actual funds destinations, including ShieldedTransferExecutor’s unshield withdrawal and CoinJoinFundsMigrationService’s migration output, meeting both the complexity and funds-movement criteria. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— security-auditor (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 13% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `wallet/src/de/schildbach/wallet/service/platform/sdk/CutoverUiDataService.kt`:
- [BLOCKING] wallet/src/de/schildbach/wallet/service/platform/sdk/CutoverUiDataService.kt:2610-2617: Reserve the unshield destination against ordinary payment change
The internal address is not exclusive to this withdrawal. In the bundled v42int21 SDK, this FFI accessor and TransactionBuilder.fund_from both call ManagedCoreFundsAccount.next_change_address, which returns AddressPool.next_unused without reserving the address. An unshield can therefore select C, remain in progress or await its Core output, and an ordinary payment from existing transparent funds can also select C for change. The transfer continues in application scope after its progress dialog is dismissed, and ordinary L1 payments are not gated on it. A merchant can identify the customer's change output and then observe the withdrawal paying that same address, exposing the withdrawal amount even though the purchase did not spend withdrawn funds. This is not only the documented collision between two self-transfers; moving this destination from a freshly issued external key to the shared internal allocator introduces cross-operation linkage. Allocate and durably reserve the destination so competing change builders skip it, or use a separately issued destination pool. Add a regression that pauses a withdrawal after destination selection and verifies an ordinary payment uses different change.
- [BLOCKING] wallet/src/de/schildbach/wallet/service/platform/sdk/CutoverUiDataService.kt:2799-2804: Invalidate the address binding before waiting for pipeline cancellation
This unbind executes inside collectLatest's replacement action, after collectLatest cancels and joins the previous runPipelines invocation. refreshNativeSplit performs a blocking FFI read in a child of that invocation, so a parked read prevents the DUAL_RUNNING action from reaching either this unbind or the _cutoverActive update. The cache remains valid under its old generation and live reads remain authorized for the retired binding throughout that interval. A deterministic probe that parked the pipeline read, emitted DUAL_RUNNING, and queried the accessors reproduced both cached and live old-binding results; invalidation completed only after releasing the read. The existing in-flight regression parks an independent caller, so it does not exercise this dependency. Invalidate ownership before awaiting cancellation, with binding/publication guards that prevent canceled work from restoring it, or add an explicit synchronous wallet-wipe invalidation hook. Cover rollback with a blocked pipeline read.
- [BLOCKING] wallet/src/de/schildbach/wallet/service/platform/sdk/CutoverUiDataService.kt:3094-3098: Prevent older engine reads from overwriting a newer cached address
The generation check distinguishes wallet bindings but does not order concurrent reads within one binding. A refresh can obtain address A and pause before publication; after the engine advances, a live read can publish B; the older refresh then overwrites B with A because both generations match. The synchronous accessors, and subsequent failed live reads, consequently advertise the already-used address again until another successful refresh. A temporary deterministic overlapping-read probe reproduced exactly this regression: the live accessor published B, then the released older refresh replaced the cache with A. The native wallet-manager lock does not protect the later Kotlin cache publication. Serialize reads through publication across both refreshNativeSplit and sdkReceiveAddressLiveBlockingOrNull, or introduce ordering that prevents obsolete results from replacing newer ones, without holding the invalidation lock across FFI calls. Add an overlapping-read regression.
In `wallet/test/de/schildbach/wallet/service/platform/sdk/CutoverUiDataServiceTest.kt`:
- [SUGGESTION] wallet/test/de/schildbach/wallet/service/platform/sdk/CutoverUiDataServiceTest.kt:1666-1669: Exercise the fail-closed decision through WalletApplication
This assertion verifies that CutoverUiDataService returns null when the change-address read is unavailable, but the previous fallback defect was in WalletApplication, which converted that null into an advertised receive address. The executor regression separately mocks unadvertisedDestinationLive() to throw, so neither test executes the application's null-to-exception decision. Reintroducing a freshReceiveAddress() fallback in WalletApplication would recreate the leak while both tests still passed. Add a test through the real application accessor with cutover active, a warm receive cache, and an unavailable change address, asserting that it throws without consulting either receive fallback. Robolectric is already a wallet test dependency.
| val (generation, walletIdHex) = bound | ||
| val address = source.nextChangeAddressOrNull(walletIdHex) | ||
| return synchronized(receiveAddressLock) { | ||
| // The wallet was wiped or rolled back while we were blocked: this | ||
| // address belongs to a wallet that is no longer current, and paying | ||
| // our own funds to it would send them somewhere the CURRENT wallet | ||
| // cannot spend. Answer nothing; the caller falls back to dashj. | ||
| if (generation != receiveAddressGeneration) null else address |
There was a problem hiding this comment.
🔴 Blocking: Reserve the unshield destination against ordinary payment change
The internal address is not exclusive to this withdrawal. In the bundled v42int21 SDK, this FFI accessor and TransactionBuilder.fund_from both call ManagedCoreFundsAccount.next_change_address, which returns AddressPool.next_unused without reserving the address. An unshield can therefore select C, remain in progress or await its Core output, and an ordinary payment from existing transparent funds can also select C for change. The transfer continues in application scope after its progress dialog is dismissed, and ordinary L1 payments are not gated on it. A merchant can identify the customer's change output and then observe the withdrawal paying that same address, exposing the withdrawal amount even though the purchase did not spend withdrawn funds. This is not only the documented collision between two self-transfers; moving this destination from a freshly issued external key to the shared internal allocator introduces cross-operation linkage. Allocate and durably reserve the destination so competing change builders skip it, or use a separately issued destination pool. Add a regression that pauses a withdrawal after destination selection and verifies an ordinary payment uses different change.
source: gpt-6-astra (phase2-reviewer: security-auditor)
| // A rollback puts dashj back in charge of the key chain, | ||
| // so the engine address must stop being served — and the | ||
| // bound id must go with it, or a live read could still | ||
| // answer from the wallet the rollback just abandoned. | ||
| unbindReceiveAddressWallet() | ||
| return@collectLatest |
There was a problem hiding this comment.
🔴 Blocking: Invalidate the address binding before waiting for pipeline cancellation
This unbind executes inside collectLatest's replacement action, after collectLatest cancels and joins the previous runPipelines invocation. refreshNativeSplit performs a blocking FFI read in a child of that invocation, so a parked read prevents the DUAL_RUNNING action from reaching either this unbind or the _cutoverActive update. The cache remains valid under its old generation and live reads remain authorized for the retired binding throughout that interval. A deterministic probe that parked the pipeline read, emitted DUAL_RUNNING, and queried the accessors reproduced both cached and live old-binding results; invalidation completed only after releasing the read. The existing in-flight regression parks an independent caller, so it does not exercise this dependency. Invalidate ownership before awaiting cancellation, with binding/publication guards that prevent canceled work from restoring it, or add an explicit synchronous wallet-wipe invalidation hook. Cover rollback with a blocked pipeline read.
source: gpt-6-astra (phase2-reviewer: general)
There was a problem hiding this comment.
Resolved (re-reviewed at 32d58473): Your ownership collector now updates activation and unbinds independently of the collectLatest pipeline's cancellation and join. A blocked pipeline read therefore no longer prevents that collector from revoking the binding.
| when { | ||
| generation != receiveAddressGeneration -> false | ||
| nextReceive != null -> { | ||
| cachedReceiveAddress = ReceiveAddressSnapshot(generation, nextReceive) | ||
| true |
There was a problem hiding this comment.
🔴 Blocking: Prevent older engine reads from overwriting a newer cached address
The generation check distinguishes wallet bindings but does not order concurrent reads within one binding. A refresh can obtain address A and pause before publication; after the engine advances, a live read can publish B; the older refresh then overwrites B with A because both generations match. The synchronous accessors, and subsequent failed live reads, consequently advertise the already-used address again until another successful refresh. A temporary deterministic overlapping-read probe reproduced exactly this regression: the live accessor published B, then the released older refresh replaced the cache with A. The native wallet-manager lock does not protect the later Kotlin cache publication. Serialize reads through publication across both refreshNativeSplit and sdkReceiveAddressLiveBlockingOrNull, or introduce ordering that prevents obsolete results from replacing newer ones, without holding the invalidation lock across FFI calls. Add an overlapping-read regression.
source: gpt-6-astra (phase2-reviewer: general)
There was a problem hiding this comment.
Resolved (re-reviewed at 32d58473): Your shared begin/complete ticket gate now prevents a lower-ticket result from overwriting a newer cached address, and the regression checks that cache invariant. The direct return of the rejected address is a distinct remaining defect, recorded separately rather than reopening this cache-publication finding.
| assertNull( | ||
| "an unavailable change read must not borrow the advertised address", | ||
| service.sdkUnadvertisedAddressLiveBlockingOrNull() | ||
| ) |
There was a problem hiding this comment.
🟡 Suggestion: Exercise the fail-closed decision through WalletApplication
This assertion verifies that CutoverUiDataService returns null when the change-address read is unavailable, but the previous fallback defect was in WalletApplication, which converted that null into an advertised receive address. The executor regression separately mocks unadvertisedDestinationLive() to throw, so neither test executes the application's null-to-exception decision. Reintroducing a freshReceiveAddress() fallback in WalletApplication would recreate the leak while both tests still passed. Add a test through the real application accessor with cutover active, a warm receive cache, and an unavailable change address, asserting that it throws without consulting either receive fallback. Robolectric is already a wallet test dependency.
source: gpt-6-astra (phase2-reviewer: general)
There was a problem hiding this comment.
Resolved (re-reviewed at 11ef19c4): decideUnadvertisedDestination() is extracted and covered by UnadvertisedDestinationDecisionTest, pinning the fail-closed decision without requiring the full Hilt graph.
Two of three blockers fixed. The third is not fixable in-app — detail below.Head is now Fixed — #2, ownership revoked too lateThe unbind ran inside Ownership now lives in its own collector, which depends on nothing that can be parked. New regression parks the pipeline's read specifically. Mutation-confirmed. Fixed — #3, unordered publication within a bindingGeneration separates wallets, not two reads of the same wallet. Every read now takes a ticket before it blocks and may publish only if no later-started read already has. Worth flagging: that regression was vacuous as first written. The fake read its address field after the gate, so the parked read returned the new value and the test passed with the ordering check deleted. The fake now captures its answer before parking. I only caught it because I mutation-test every one of these. Not fixed — #1, the shared unreserved change allocatorYour analysis is correct, including the part I got wrong: I had documented the collision as "between two of the user's own transfers, invisible to any payer". It isn't — the change output of an ordinary payment is visible to that payee, so the linkage is real and cross-operation. But neither remedy you propose exists in the bundled SDK. I checked:
That leaves three options, none of them clean:
I read option 1 as the narrowest residual of the three, which is why I have left it in place rather than pick a worse one to clear the gate. The real fix is an upstream address-reservation or dedicated-pool API in rs-platform-wallet — I can file that if you want it tracked. This is a judgement call above my pay grade on this PR, so I am flagging it rather than deciding it: merge with the residual documented, or hold #1570 until the SDK gains the API. On the suggestionDriving the decision through the real Suite162 failures vs 161 on the previous head. The one difference, Still unverified, unchanged: no live shield→unshield cycle on any device — which your review also noted independently. 🤖 Generated with Claude Code |
Two of the three blocking findings on #1570. Both verified against the source; the reviewer reproduced each with a deterministic probe and so did I. 1. OWNERSHIP WAS REVOKED TOO LATE. The unbind ran inside `collectLatest`'s replacement action, and `collectLatest` cancels AND JOINS the previous `runPipelines` before running it. `refreshNativeSplit` blocks in an uncancellable FFI read, so a parked read delayed the replacement — and with it the unbind and the `_cutoverActive` update — for as long as it stayed parked. Throughout that window the retired binding's address was still cached under a matching generation and still served by live reads. The earlier in-flight regression parks an INDEPENDENT caller, so it never exercised this dependency. Ownership now lives in its own collector, which depends on nothing that can be parked, so a rollback or an in-place wipe revokes it at once. 2. PUBLICATIONS WERE UNORDERED WITHIN A BINDING. The generation separates wallets; it says nothing about two reads of the SAME wallet. A refresh could take address A, pause in the FFI, a live read publish the engine's newer B, and the released refresh overwrite B with A — putting an already-used address back on the Receive screen until the next successful refresh, which is the very defect this PR exists to fix. Every read now takes a ticket before it blocks and may publish only if no later-started read has published first. `beginReceiveRead`/`completeReceiveRead` are the single pair both the live accessor and the refresh go through; the lock is never held across an FFI call. Also addresses the review's suggestion, though not the way it proposed. Driving the decision through the real `WalletApplication` accessor would mean standing up a 24-field Hilt graph under Robolectric; instead the decision is extracted as `decideUnadvertisedDestination` and tested directly. The invariant pinned is the one that matters: the post-cutover arm never reaches the dashj supplier, because a fallback there is exactly how the leak shipped once. Mutation-confirmed — reinstating the fallback fails it. This does not stop a future caller passing a receive-address supplier in; only a full application-level test would, and that trade-off is stated rather than hidden. The two new concurrency regressions are mutation-confirmed too. Note the ordering one was VACUOUS on first writing: the fake read its address field after the gate, so the parked read returned the new value and the test passed with the ordering check deleted. The fake now captures its answer before parking. Suite: 162 failures vs 161 on the previous head. The one difference, `AssetLockKindResolverNegativeCacheTest#nullTransactionKind_isNotPinned_soLateUnshieldStillResolves`, passes 3/3 in isolation and this branch touches no asset-lock files. The third blocking finding — the internal chain being shared, unreserved, with ordinary payment change — is NOT addressed here; see the PR discussion. Neither remedy it proposes exists in the bundled SDK. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Preliminary review — Phase 1 blocker gate
At the exact reviewed head, the generation checks and independent ownership collector address the earlier wallet-isolation defects, and ticket ordering protects the receive cache. However, an older read can still return its stale address directly to its caller, and the unreserved internal-chain destination retains a cross-operation privacy exposure. Validation was by source and test inspection; no build, test execution, or device verification was performed.
Validated blockers were found by the Phase-1 review and confirmed by a fresh verifier. Phase 2 is deferred until a fresh same-head revalidation clears the blocker gate.
🔴 2 blocking | 🟡 2 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-gate-verifier, role: verifier)
- Triage:
criticalbygpt-6-astra(effort low) — The diff combines intricate SDK address-cache ownership and publication ordering with direct changes to funds-movement destinations, notably WalletApplication.unadvertisedDestinationLive() and its shielded-withdrawal and CoinJoin-migration callers, including fail-closed behavior and internal-versus-advertised address selection. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— security-auditor (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 13% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6-astra— verifier; agentastra-gate-verifier - Phase 2 reviewers: not run (deferred by blocker gate)
🤖 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 `wallet/src/de/schildbach/wallet/service/platform/sdk/CutoverUiDataService.kt`:
- [BLOCKING] wallet/src/de/schildbach/wallet/service/platform/sdk/CutoverUiDataService.kt:2505-2511: Return the newer cached address when a receive read loses ticket ordering
The ticket comparison prevents an older read from overwriting the cache, but `return address ?: cachedReceiveAddress...` still returns that rejected address directly to its caller. If one live read captures A, a subsequent read publishes B after the engine advances, and the first read then completes, the first caller receives A despite the cache already holding B. WalletApplication's live accessors return this non-null result unchanged, so the Receive screen or an integration can advertise the stale address. The existing ordering regression reproduces the necessary interleaving but discards `olderResult.get()`. Return the current generation's cached address after applying the publication gate, and assert that the older caller receives B as well.
- [BLOCKING] wallet/src/de/schildbach/wallet/service/platform/sdk/CutoverUiDataService.kt:2638-2646: Reserve the unshield destination against ordinary payment change
(existing thread: https://github.com/dashpay/dash-wallet/pull/1570#discussion_r4110572828)
The destination wrapper calls `core.nextChangeAddress(accountIndex)` and returns the next-unused internal address without issuing or reserving it. Selecting the internal chain separates withdrawals from Receive QR handouts, but does not isolate them from ordinary-payment change. While an internal address C remains unused from the engine's perspective, an ordinary payment and an unshield or CoinJoin combine can select C; that payment's recipient can observe its change output and link the other transfer to it. This is cross-operation linkage, not merely reuse between private self-transfers. Keeping the internal chain avoids the easier unpaid-QR attack, and the lack of an exposed reservation or dedicated-pool API explains why the proposed alternatives cannot simply be implemented here. It does not establish the privacy guarantee claimed by the current KDoc. Resolving the exposure requires an SDK-supported allocation mechanism that competing change builders honor, or an explicit maintainer decision to accept this residual; document the residual accurately in either case.
In `wallet/src/de/schildbach/wallet/WalletApplication.java`:
- [SUGGESTION] wallet/src/de/schildbach/wallet/WalletApplication.java:2167-2179: Recheck cutover ownership before selecting the dashj destination
The initial `isCutoverActive()` snapshot controls both whether the SDK is consulted and whether the dashj supplier is allowed. If ownership changes to CUT_OVER after that snapshot, this invocation still skips the SDK and issues a key from the held dashj chain. Unlike the internal-chain path, that key is not guaranteed to differ from the engine's advertised external address. Attempt the generation-validated SDK read before taking the cutover snapshot used for the fallback decision, and cover activation during destination lookup. This narrows the avoidable stale-snapshot window; a strict guarantee across arbitrary ownership changes would require coordination with the transition itself.
In `wallet/src/de/schildbach/wallet/ui/payments/SweepWalletFragment.java`:
- [SUGGESTION] wallet/src/de/schildbach/wallet/ui/payments/SweepWalletFragment.java:621-627: Paper-wallet sweeps now share the advertised receive address
Post-cutover, `freshReceiveAddressLive()` returns the same next-unused external address as the Receive screen rather than issuing a separate key. A counterparty holding an unpaid Receive QR can therefore observe a later paper-wallet sweep to that address and associate its amount and paper-wallet inputs with the recipient. The stated reason for leaving this path unchanged—that the funds are already linked to the paper key—does not eliminate the newly introduced link from that paper key to the identity behind the QR. This is less sensitive than an unshield, but it is a privacy regression from the previous fresh-key behavior. Use the self-transfer destination policy for sweeps, with preparation failures handled before sending, or explicitly document acceptance of this additional linkage.
| private fun completeReceiveRead(generation: Long, ticket: Long, address: String?): String? { | ||
| if (generation != receiveAddressGeneration) return null | ||
| if (address != null && ticket > lastPublishedReceiveTicket) { | ||
| cachedReceiveAddress = ReceiveAddressSnapshot(generation, address) | ||
| lastPublishedReceiveTicket = ticket | ||
| } | ||
| return address ?: cachedReceiveAddress?.takeIf { it.generation == generation }?.address |
There was a problem hiding this comment.
🔴 Blocking: Return the newer cached address when a receive read loses ticket ordering
The ticket comparison prevents an older read from overwriting the cache, but return address ?: cachedReceiveAddress... still returns that rejected address directly to its caller. If one live read captures A, a subsequent read publishes B after the engine advances, and the first read then completes, the first caller receives A despite the cache already holding B. WalletApplication's live accessors return this non-null result unchanged, so the Receive screen or an integration can advertise the stale address. The existing ordering regression reproduces the necessary interleaving but discards olderResult.get(). Return the current generation's cached address after applying the publication gate, and assert that the older caller receives B as well.
| private fun completeReceiveRead(generation: Long, ticket: Long, address: String?): String? { | |
| if (generation != receiveAddressGeneration) return null | |
| if (address != null && ticket > lastPublishedReceiveTicket) { | |
| cachedReceiveAddress = ReceiveAddressSnapshot(generation, address) | |
| lastPublishedReceiveTicket = ticket | |
| } | |
| return address ?: cachedReceiveAddress?.takeIf { it.generation == generation }?.address | |
| private fun completeReceiveRead(generation: Long, ticket: Long, address: String?): String? { | |
| if (generation != receiveAddressGeneration) return null | |
| if (address != null && ticket > lastPublishedReceiveTicket) { | |
| cachedReceiveAddress = ReceiveAddressSnapshot(generation, address) | |
| lastPublishedReceiveTicket = ticket | |
| } | |
| return cachedReceiveAddress?.takeIf { it.generation == generation }?.address | |
| } |
source: muse-spark-1.3-contributor (phase1-reviewer: general, security-auditor); security-auditor
There was a problem hiding this comment.
Resolved (re-reviewed at 11ef19c4): completeReceiveRead() answers every caller from the current cache, and the regression verifies that an older read receives the newer cached address rather than its stale result.
| public Address unadvertisedDestinationLive() { | ||
| final boolean cutoverActive = | ||
| cutoverUiDataService != null && cutoverUiDataService.isCutoverActive(); | ||
| final Address unadvertised = cutoverActive | ||
| ? toDashjAddressOrNull(cutoverUiDataService.sdkUnadvertisedAddressLiveBlockingOrNull()) | ||
| : null; | ||
| return decideUnadvertisedDestination(cutoverActive, unadvertised, () -> { | ||
| // NOT freshReceiveAddress(): that accessor is overlaid and would | ||
| // serve the cached engine RECEIVE address. Go straight to dashj. | ||
| org.bitcoinj.core.Context.propagate(Constants.CONTEXT); | ||
| return wallet.freshReceiveAddress(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Recheck cutover ownership before selecting the dashj destination
The initial isCutoverActive() snapshot controls both whether the SDK is consulted and whether the dashj supplier is allowed. If ownership changes to CUT_OVER after that snapshot, this invocation still skips the SDK and issues a key from the held dashj chain. Unlike the internal-chain path, that key is not guaranteed to differ from the engine's advertised external address. Attempt the generation-validated SDK read before taking the cutover snapshot used for the fallback decision, and cover activation during destination lookup. This narrows the avoidable stale-snapshot window; a strict guarantee across arbitrary ownership changes would require coordination with the transition itself.
source: muse-spark-1.3-contributor (phase1-reviewer: general, security-auditor)
There was a problem hiding this comment.
Resolved (re-reviewed at 11ef19c4): The SDK destination read now occurs before the ownership snapshot, removing the previously identified path where a cutover could commit before a dashj destination was selected.
| // Through the application, not the dashj wallet directly, and through | ||
| // the LIVE accessor: post-cutover the dashj chain is HELD and its | ||
| // pointer frozen, so sweeping the paper wallet straight into it would | ||
| // land the funds on an address the chain has already paid | ||
| // (SR-03 / D-003). Already on the background handler, as the live | ||
| // read requires. | ||
| final Address receivingAddress = application.freshReceiveAddressLive(); |
There was a problem hiding this comment.
🟡 Suggestion: Paper-wallet sweeps now share the advertised receive address
Post-cutover, freshReceiveAddressLive() returns the same next-unused external address as the Receive screen rather than issuing a separate key. A counterparty holding an unpaid Receive QR can therefore observe a later paper-wallet sweep to that address and associate its amount and paper-wallet inputs with the recipient. The stated reason for leaving this path unchanged—that the funds are already linked to the paper key—does not eliminate the newly introduced link from that paper key to the identity behind the QR. This is less sensitive than an unshield, but it is a privacy regression from the previous fresh-key behavior. Use the self-transfer destination policy for sweeps, with preparation failures handled before sending, or explicitly document acceptance of this additional linkage.
source: security-auditor
There was a problem hiding this comment.
Resolved (re-reviewed at 11ef19c4): The sweep now selects unadvertisedDestinationLive() and restores CONFIRM_SWEEP with an error when preparation fails, so it no longer sends to the advertised receive address.
Review feedback on #1570. All three findings verified against the source first; none were false positives. - `refundAddressOrNull`'s KDoc and the `SendCoinsTaskRunner` call-site comment still described the Room address-pool read this PR deleted, including its null conditions ("the pool rows are missing"). Both now describe the engine FFI read and its real null cases: wallet unbound, no BIP44 account yet, or the read failed. - `sdkReceiveAddressLiveOrNull`'s KDoc claimed null "when the read is unavailable", which contradicted both the implementation and this PR's own `postCutover_failedEngineReadHoldsTheLastAddress` test: a failed read on a warm cache deliberately returns the last known engine address, because dropping to null would send the caller back to the frozen dashj pointer. The doc now states the hold and keeps the guarantee that actually holds — every value it yields came from the engine, none are guessed. - The rollback clear (CUT_OVER -> DUAL_RUNNING) had no test, and could not have had one: `configWithState` returns a one-shot `flowOf`, so the deactivation branch was unreachable from the suite. Added `configWithMutableState` and a test asserting that after a rollback the cached overlay, the live read, the balance and the UTXO count all stop serving. Confirmed non-vacuous by mutation: deleting `_sdkReceiveAddress.value = null` fails it. Rebased onto 02e741b; the upstream CutoverUiDataService changes merged without conflict. Full suite: 152 failures, an identical set to the base at 02e741b (pre-existing conscrypt native-lib failures in this environment). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
BLOCKING review finding on #1570, and it is a real one — verified against the source before changing anything. `sdkReceiveAddressLiveBlockingOrNull` checked the cutover flag and the bound wallet id, then made a BLOCKING FFI read, then published unconditionally. That is check-then-act across a call that cannot be cancelled, so: 1. wallet A's live read captures A's id and blocks in the engine; 2. Reset Wallet wipes A. `WalletApplicationExt.clearDatabasesInner` says in as many words that it does NOT restart the process, and it calls `cutoverCoordinator.resetForWalletWipe()`, so the deactivation branch runs and clears the overlay; 3. the parked read returns and republishes A's address into the cache; 4. wallet B restores. Until B's own engine read lands, `sdkReceiveAddressOrNull` — which checked neither activation nor ownership — serves A's address, and `WalletApplication` validates the address's NETWORK, not which seed owns it, so nothing downstream catches it. The Receive screen then advertises an address only the ERASED wallet can spend. That is worse than the SR-03 defect this PR set out to fix, and the existing `_cutoverActive` gate does not prevent it: re-checking the flag on the way out is still check-then-act, and a flag cannot tell a surviving binding from a different one that activated meanwhile. So the cache now carries the binding GENERATION it was read under. The generation is bumped on every bind and unbind under a lock; a read captures it before the FFI and may publish only if it is still current; the synchronous accessor serves an entry only while its stamp matches. Cross-wallet reuse is unrepresentable rather than merely unlikely. The lock is never held across the FFI call, so a deactivation never waits on the engine. Also removed the public `sdkReceiveAddress` StateFlow: it had no consumers and would have been a fourth, unstamped way to read the cache. Regression: `inFlightReadCannotRepublishAWipedWalletsAddress` parks a read for wallet A inside the FFI, wipes, releases it, then activates B with B's own read unavailable — the only way A's leftover could surface. Confirmed non-vacuous by mutation: deleting the generation check in the live read fails it. The fixture gates by calling thread rather than a one-shot flag, because the balance pipeline reads through the same method — an earlier one-shot version was eaten by the pipeline and passed vacuously. Nitpick also fixed: `WalletApplication.liveSdkReceiveAddressOrNull`'s Javadoc now documents the cold-cache/warm-cache distinction and the changed-binding null. Full suite: 152 failures, an identical set to the previous head — no new failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two blocking review findings on the SR-03 receive path. 1. completeReceiveRead answered a FAILED live read out of the warm cache, so a live caller got a non-null address that WalletApplication.decideLiveReceiveAddress returns unchanged. The fail-closed handling added last round — PaymentsViewModel dropping the address rather than advertising a stale one — was therefore unreachable in production: cache A, take a payment on A, let both the event refresh and the next live lookup fail, and the paid QR stayed on screen. The cache is still HELD on failure (the synchronous overlay's only alternative is the held dashj chain's frozen pointer), but whether the newest ACCEPTED read actually reached the engine is now tracked beside it and returned with it. Live callers answer nothing when it did not, and reach ReceiveAddressUnavailableException. The verdict is written under the SAME ticket fence as the address, so an older completion arriving after a newer failed read cannot reinstate a success — which would have re-authorized exactly the address the fence just rejected. 2. The eager deactivation collector deduplicated on the Boolean it was DELIVERED, not on the decision it APPLIED after the authoritative reread. An initial `false` delivered after CUT_OVER commits leaves the dedupe remembering `false` while ownership rightly stands; if the intervening `true` is conflated away, the rollback's `false` is suppressed as a repeat and nothing revokes eagerly. The gated collector sees it but must join a pipeline parked in an uncancellable FFI read, and until then the retired generation and its cached address stay authorized. Revocation is now driven by the ownership this service has APPLIED (the flag and the binding, read together), which cannot drift from itself — and which skips the reread for every repeated `false` that could not change anything, the only thing the dedupe bought. Tests: a live read refused from the hold while the overlay keeps it; an older completion after a newer failed read; the same refusal carried through WalletApplication.decideLiveReceiveAddress to the throw; an overtaken `false`, a missed `true` and another `false` with the pipeline read genuinely parked; and the repeated-`false` reread economy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ns it The confirm single-flight was acquired in the retained view model but released from fragment-instance state, so ownership did not survive the fragment being recreated. Rotate while an attempt is parked in the destination read — post cutover an uncancellable wait for the engine to bind — and cancellation cannot finish that attempt until the read returns, while the new fragment, which has no confirm of its own, clears the shared guard and lets the user confirm again. When the cancelled attempt finally unwinds, its `finally` released a flight it no longer owned: the live confirm, which had already prepared its destination, then failed buyDash()'s check and showed a purchase error. Ownership now stays in the view model as an attempt id. tryBeginConfirm() issues one, and every release names the attempt it releases, so a stale attempt's cleanup is a no-op. The completed-navigation latch, which keeps the button guarded between the purchase and the navigation transaction actually executing, is now separate state: onResume drops that latch and only that latch, and an attempt that is still running keeps its flight. The destroyed fragment hands its own attempt back in onDestroy, so the recreated screen can confirm without waiting out the blocking read. buyDash() checks that the caller still owns the flight rather than that somebody holds it. Address-before-order and the unavailable-address retry are unchanged. CoinbaseBuyDashConfirmGuardTest models the fragment's three call sites and covers the recreation race with a real handshake: the stale attempt unwinds inside the live attempt's window and the live attempt still buys. A real fragment would need Robolectric, which cannot start here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
All three blockers fixed — head
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 2 only (Phase 1 failed)
Static verification at a1f5ae7 confirms 38 prior findings are fixed, with SDK address reservation remaining a separate follow-up and end-to-end self-transfer validation still outstanding. One new blocking race remains: the applied-ownership filter can discard a rollback while a grant is pending under the ownership mutex. The supplied exact-head CI snapshot reports successful check and build jobs; no local builds, tests, or device operations were performed.
🔴 1 blocking | 🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate diff changes funds-movement destinations directly in CoinJoinFundsMigrationService and ShieldedTransferExecutor, alongside cross-cutting SDK address selection, caching, and failure handling. - Phase 1 reviewers: failed (failed on every model; its output was dropped)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer - Model comparison: every Phase-2 reviewer also ran on
gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `wallet/src/de/schildbach/wallet/service/platform/sdk/CutoverUiDataService.kt`:
- [BLOCKING] wallet/src/de/schildbach/wallet/service/platform/sdk/CutoverUiDataService.kt:3391-3395: Check applied ownership under the ownership decision mutex
The no-op filter runs outside ownershipDecisionMutex, so it can miss a grant that already holds that mutex. Starting with ownership unapplied, grantReceiveOwnershipIfStillCommitted() can read CUT_OVER and be preempted before publishing _cutoverActive. If rollback commits during that interval, this independent collector observes false, sees receiveOwnershipApplied() return false, and discards the rollback instead of waiting for the pending grant. The grant then publishes true and can start and bind its pipeline before the gated collector processes its own rollback. If a pipeline read enters the uncancellable FFI, collectLatest must join that read before its deactivation action runs, while the independent collector has already discarded the emission needed for eager revocation. The production Dispatchers.Default scope permits this interleaving. Move the applied-state check inside ownershipDecisionMutex in the revocation helper, and invoke that helper for every false observation. Keep its authoritative ownership reread, and add a regression staging rollback between the grant's authoritative read and flag publication.
In `wallet/src/de/schildbach/wallet/ui/shielded/ShieldedTransferExecutor.kt`:
- [SUGGESTION] wallet/src/de/schildbach/wallet/ui/shielded/ShieldedTransferExecutor.kt:605-617: Verify internal-chain self-transfer destinations end to end
(existing thread: https://github.com/dashpay/dash-wallet/pull/1570#discussion_r4171723484)
The withdrawal now targets the SDK internal/change chain, but ShieldedTransferExecutorTest mocks both unadvertisedDestinationLive() and withdrawToCore(), and the service tests use synthetic change addresses. These tests verify routing and refusal before broadcast, not that an actual withdrawal is recognized, credited once, and spendable after restart. The recorded historical change output demonstrates ordinary internal-chain recognition but does not exercise this withdrawal route or the changed paper-wallet sweep. Keep the queued integration validation explicit: an authorized shield/unshield cycle and paper-wallet sweep should check destination ownership, credit recognition, and subsequent spendability after restart. Successful CI and the earlier SR-03 device runs do not close this funds-destination validation gap.
Out-of-scope follow-up suggestions (2)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Reserve self-transfer destinations against ordinary SDK change — The production destination source calls core.nextChangeAddress(accountIndex) without an application-level reservation. As documented in the current accessor, this internal allocator is shared with ordinary payment change, so concurrent preparation can reuse a destination visible to a payment recipient. Avoiding advertised receive addresses fixes the unpaid-QR exposure, but does not establish cross-operation isolation; addressing that residual requires allocation coordination shared by SDK transaction builders.
- Follow-up: Track an upstream SDK address-reservation or dedicated self-transfer allocation API honored by competing change builders.
- Validate destination ownership against the funding wallet at spend time — WalletApplication revalidates the destination generation before returning it, but ShieldedTransferExecutor and CoinJoinFundsMigrationService subsequently pass a bare address to spending services that resolve the funding wallet separately. A wallet replacement after the accessor's final check remains outside that check's protection. This is a transaction-boundary coordination follow-up, distinct from the now-fixed generation gap inside the accessor.
- Follow-up: Track carrying wallet identity and generation with self-transfer destinations and validating them against the funding wallet during transaction preparation.
| cutoverUiActive() | ||
| .collect { active -> | ||
| if (!active && receiveOwnershipApplied()) { | ||
| revokeReceiveOwnershipIfStillDashjOwned() | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Check applied ownership under the ownership decision mutex
The no-op filter runs outside ownershipDecisionMutex, so it can miss a grant that already holds that mutex. Starting with ownership unapplied, grantReceiveOwnershipIfStillCommitted() can read CUT_OVER and be preempted before publishing _cutoverActive. If rollback commits during that interval, this independent collector observes false, sees receiveOwnershipApplied() return false, and discards the rollback instead of waiting for the pending grant. The grant then publishes true and can start and bind its pipeline before the gated collector processes its own rollback. If a pipeline read enters the uncancellable FFI, collectLatest must join that read before its deactivation action runs, while the independent collector has already discarded the emission needed for eager revocation. The production Dispatchers.Default scope permits this interleaving. Move the applied-state check inside ownershipDecisionMutex in the revocation helper, and invoke that helper for every false observation. Keep its authoritative ownership reread, and add a regression staging rollback between the grant's authoritative read and flag publication.
source: gpt-6.1-sol (phase2-reviewer: general)
There was a problem hiding this comment.
Resolved (re-reviewed at 373ca4bb): Your final change passes every false observation into the helper and checks applied ownership only after taking ownershipDecisionMutex. The handshake regression holds the grant before publication and verifies that the eager rollback waits and then revokes, rather than discarding the observation.
The eager deactivation collector filtered its own emissions on
`receiveOwnershipApplied()`, read OUTSIDE `ownershipDecisionMutex`. That
asks whether there is anything to revoke of a decision that may be
half-made. `grantReceiveOwnershipIfStillCommitted` takes the mutex, reads
CUT_OVER and can be descheduled before it publishes `_cutoverActive` —
the service scope is Dispatchers.Default, so the window is real. Starting
from unapplied ownership, a rollback committing inside it read "nothing
applied", so the collector discarded the emission instead of waiting for
the grant it would have had to undo. The grant then published and bound,
and nothing eager was left: the gated collector sees that rollback too,
but `collectLatest` cancels AND JOINS, and a pipeline parked in an
uncancellable FFI read holds the join — which is the entire reason the
independent collector exists. Until that read returned, the retired
generation and its cached address stayed authorized.
Every `false` now reaches `revokeReceiveOwnershipIfStillDashjOwned`, and
both filters live inside the mutex, the cheap one first: nothing applied
→ return, no authoritative read owed (what the deleted
`distinctUntilChanged` used to buy, without the dedupe state that could
diverge from the applied decision); applied but the state has moved on →
the observation was overtaken. A grant in flight is now WAITED for rather
than raced, and the decision is made against ownership that is actually
applied. The authoritative reread, the ticket/generation fences, the
`ReceiveReadAnswer` revalidation and `observePreservingErrors` are
unchanged.
aRollbackObservedWhileTheGrantIsMidDecisionStillRevokesEagerly stages the
interleaving with handshakes rather than timing: the grant is held inside
its authoritative read, so it provably owns the mutex and provably has
not published; the rollback is emitted from a `flow {}` whose `emit` runs
the collector body synchronously, so one `runCurrent()` is enough to have
made the collector's decision whichever shape the code has; and the gated
collector's own feed never delivers the rollback, so only the eager path
can produce the revocation asserted on. The grant's publication is proved
by the pipeline's bind call, a monotone counter, not by a flag read that
races the revocation.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fixed — head
|
| # | Mutation | Result |
|---|---|---|
| M1 | Full revert (check back outside, in the collector) | died — new test |
| M2 | Keep the in-mutex check but re-add the collector pre-filter | died — new test |
| M3 | Drop only the in-mutex applied check | died — repeatedDashjEmissionsDoNotReReadTheCutoverState and the new test's zero-point-reads precondition |
| M4 | Drop the authoritative reread guard | died — aDeactivationAppliedLateCannotRevokeANewerActivation, rollbackRevokesOwnershipAfterAnOvertakenFalseAndAMissedTrue |
M2 is the one that specifically pins "every false observation must reach the helper"; M3 confirms repeatedDashjEmissionsDoNotReReadTheCutoverState is still a live detector rather than passing trivially; M4 confirms the previous round's protections are intact.
On the three vacuity traps this PR has hit: no verify(exactly=N) anywhere in the new test (counts are boundCalls and an AtomicInteger read only after a handshake has established the state); no channel at all, so nothing can hand a value to a waiting receiver; and the assertion-reachability trap is ruled out empirically — M1 and M2 revert the production change and the test goes red by name, so the asserted outcome is reachable only through the changed branch. One assertion on sdkReceiveAddressLiveOrNull() was deliberately dropped because beginReceiveReadAwaitingBinding would have spent its whole binding wait and measured that rather than the revocation; a comment records why.
Verification
:wallet:compile_testNet3DebugKotlin— BUILD SUCCESSFUL.- Full suite, 2353 tests, 176 failures: 175 conscrypt +
PlatformSyncEngineRestartTest#resume_isRearmableAcrossRepeatedStopRestartCycles(pre-existing, fix(upgrade): v12 cutover and sync lifecycle, plus a measured account of what still loses money #1568). Zero new by name. CutoverUiDataServiceTestrun 3× with--rerun-tasks— green all three, covering every timing-sensitive test in the class.- ktlint clean. Noting again that ktlint's source sets cover
common/,integrations/andfeatures/only —wallet/is not in scope, so neither changed file is actually linted by it.
Known limitations, restated rather than quietly carried
- Positional coupling at the config seam. Both the existing
laggingGateConfigtests and this new one identify collectors byobservePreservingErrorscall order (0 = the deactivation collector, whichstart()launches first). In the new test the coupling fails loudly — the zero-point-reads precondition and the grant-held assertion both depend on the routing being right — but reordering thescope.launchblocks instart()would change what it exercises. Fixing it needs the two collectors distinguishable at the config seam, which a single-key mock cannot do. - The new test models "the gated collector has not been delivered the rollback yet" rather than "delivered but parked on a join". That is the stronger isolation for this assertion and realistic for a conflated DataStore feed; the parked-FFI join stays covered by
rollbackRevokesOwnershipEvenWhileThePipelineReadIsParkedandrollbackRevokesOwnershipAfterAnOvertakenFalseAndAMissedTrue, both still passing. - Pre-existing and untouched: a pipeline that re-binds via
bindReceiveAddressWalletafter an eager revocation leavesactiveWalletIdHexnon-null with_cutoverActivefalse, soreceiveOwnershipApplied()reads true for a subsequentfalse. Harmless — the helper would simply unbind again — and outside this finding.
The suggestion
Unchanged and acknowledged: the end-to-end self-transfer validation is a device matter that no host-JVM change can close. The sharpened criteria are noted — destination ownership, credit recognition, and spendability after restart, across a shield/unshield cycle and a paper-wallet sweep. Pending maintainer go-ahead for an authorized testnet run; procedure in artifacts/pr1570-run19-inclusion.md.
🤖 Generated with Claude Code
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static verification of the complete afa91bf..373ca4b range and all 41 prior findings found no remaining in-scope blocking defect. Two non-blocking coverage suggestions remain: verify the internal-chain self-transfer destinations through the real SDK, and restore delayed pipeline-gate scenarios weakened by the shared test fixture. The supplied exact-head CI snapshot reports successful build and check jobs; no local builds or tests were run.
🟡 2 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate diff changes transaction destination and key-chain selection in WalletApplication.unadvertisedDestinationLive(), ShieldedTransferExecutor’s unshield withdrawal, and CoinJoinFundsMigrationService, alongside cross-cutting receive-address ownership and concurrency logic. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer - Model comparison: every Phase-2 reviewer also ran on
gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `wallet/test/de/schildbach/wallet/service/platform/sdk/CutoverUiDataServiceTest.kt`:
- [SUGGESTION] wallet/test/de/schildbach/wallet/service/platform/sdk/CutoverUiDataServiceTest.kt:2995-2999: Preserve the delayed pipeline-gate scenarios in the wipe tests
laggingGateConfig() gives gateFeed only to the first subscription, which start() now assigns to the independent deactivation collector; the pipeline gate instead subscribes to persisted. This correctly stages delayed deactivation, but the existing wipe tests still use the helper to model an undelivered pipeline-gate reset. In stopForWalletWipe_conflatedReset_restartsForNextWallet_withoutTheWipedWalletsPendingWalk(), persisted changes to DUAL_RUNNING, the scheduler runs, and persisted later changes to CUT_OVER, so the pipeline gate receives both transitions despite the test's stated continuously active gate. The in-flight-walk test similarly no longer withholds the pipeline gate's reset. Separate the delayed-deactivation and delayed-pipeline-gate fixtures so these tests retain coverage of stopGeneration-driven restart when the pipeline gate misses the reset.
In `wallet/src/de/schildbach/wallet/ui/shielded/ShieldedTransferExecutor.kt`:
- [SUGGESTION] wallet/src/de/schildbach/wallet/ui/shielded/ShieldedTransferExecutor.kt:605-617: Verify internal-chain self-transfer destinations end to end
(existing thread: https://github.com/dashpay/dash-wallet/pull/1570#discussion_r4171723484)
The withdrawal now pays an engine-derived internal-chain address, but ShieldedTransferExecutorTest substitutes the destination and mocks withdrawToCore(), while CutoverUiDataServiceTest supplies fixed receive/change strings. These tests establish routing and refusal before broadcast, not native derivation, output discovery, crediting, or spendability. The existing change-output balance evidence supports internal-chain recognition, but does not exercise a withdrawal to the newly selected destination, and the queued integration run is not a completed result. Record an authorized testnet run on the final head, or add a native-backed integration test, confirming that the destination differs from the advertised receive address and that the resulting credit remains available and spendable after restart. Include equivalent checks for the redirected sweep and CoinJoin consolidation destinations.
Out-of-scope follow-up suggestions (2)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Coordinate self-transfer destinations with ordinary payment change — The internal-chain accessor calls nextChangeAddress(0) without reserving its result. Under the documented next-unused contract, a competing ordinary payment can select the same unused address for change, allowing that payment's recipient to correlate a self-transfer with its change output. The accessor avoids the direct unpaid-Receive-QR linkage but does not provide allocation isolation; closing this residual requires an SDK-wide allocation contract beyond this receive-source correction.
- Follow-up: Track a separate SDK reservation or dedicated self-transfer allocation mechanism honored by competing transaction builders, with concurrent payment/self-transfer coverage.
- Bind self-transfer destinations to the funding wallet at spend time — WalletApplication rechecks the destination generation before returning, but returns a bare Address. ShieldedBalanceServiceImpl.runShieldedWrite() and SdkL1SendService.drainCoinJoinAccountTo() select the funding wallet afterward, leaving a destination-versus-funding-wallet replacement window. This address-then-spend lifetime gap also exists in the previous orchestration and needs a separate transaction-boundary change, not another accessor snapshot.
- Follow-up: Track a wallet-bound destination API that validates ownership against the funding wallet under spend/wipe coordination before submitting funds.
| return mockk { | ||
| every { observe(DashPayConfig.CUTOVER_STATE) } answers { | ||
| every { observePreservingErrors(DashPayConfig.CUTOVER_STATE) } answers { | ||
| if (observeCalls++ == 0) gateFeed else persisted | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Preserve the delayed pipeline-gate scenarios in the wipe tests
laggingGateConfig() gives gateFeed only to the first subscription, which start() now assigns to the independent deactivation collector; the pipeline gate instead subscribes to persisted. This correctly stages delayed deactivation, but the existing wipe tests still use the helper to model an undelivered pipeline-gate reset. In stopForWalletWipe_conflatedReset_restartsForNextWallet_withoutTheWipedWalletsPendingWalk(), persisted changes to DUAL_RUNNING, the scheduler runs, and persisted later changes to CUT_OVER, so the pipeline gate receives both transitions despite the test's stated continuously active gate. The in-flight-walk test similarly no longer withholds the pipeline gate's reset. Separate the delayed-deactivation and delayed-pipeline-gate fixtures so these tests retain coverage of stopGeneration-driven restart when the pipeline gate misses the reset.
source: gpt-6.1-sol (phase2-reviewer: general)
There was a problem hiding this comment.
Resolved (re-reviewed at 66facee5): Your delayedPipelineGateConfig now routes by the named pipeline-gate seam and asserts all three subscription identities. Both wipe scenarios keep the gate CUT_OVER throughout, so their restart assertions can no longer be satisfied by a reset transition delivered to the wrong collector.
…scription order The wipe tests meant to withhold the pipeline gate's reset so the restart under test could only come from `stopForWalletWipe`'s generation bump. They routed positionally — "the first `observePreservingErrors` call" — and when the independent deactivation collector was launched in FRONT of the gated one they quietly started staging that collector instead. The pipeline gate was left on `persisted`, which does change across the wipe, so the gate restarted the pipeline and the scenario stopped being staged at all. The tests went on passing. Measured: with `stopGeneration` removed from the gate's `combine`, `stopForWalletWipe_conflatedReset_restartsForNextWallet_withoutTheWipedWalletsPendingWalk`, `stopForWalletWipe_inFlightWalkCannotWriteAfterStop_andPipelineRestartsForNextWallet` and `stopForWalletWipe_reconcileRequestedMidWipe_isNotReplayedForTheNextWallet` all PASSED at 373ca4b. They now all fail. Fixtures split and made non-positional: - `laggingGateConfig` is gone. `delayedDeactivationConfig` stages a late deactivation; `delayedPipelineGateConfig` stages a reset the pipeline gate never sees. The distinction is now visible at the call site. - `RoutedCutoverConfig` routes by WHICH production seam is subscribing, read from the frames below the mock, so reordering the `scope.launch` blocks in `start()` can no longer re-point a fixture. `assertWired()` fails loudly if any of the three long-lived subscriptions did not arrive as itself. - The three other hand-rolled positional mocks (the two rollback tests and `repeatedDashjEmissionsDoNotReReadTheCutoverState`) use the same router, and the KNOWN LIMITATION note on `aDeactivationAppliedLateCannotRevokeANewerActivation` is resolved rather than restated. Production: the three subscriptions get names — `cutoverDeactivationFeed`, `cutoverOwnershipFeed`, `cutoverPipelineGateFeed` — which is what makes them distinguishable at the config seam. No behaviour change; each is the same flow the launch built inline. `stopForWalletWipe_inFlightWalkCannotWriteAfterStop_andPipelineRestartsForNextWallet` no longer delivers the gate's reset late: its gate feed stays CUT_OVER for the whole test, so the restart it asserts on can only be the stop's re-evaluation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fixed — head
|
| test | at 373ca4bbb + mutation |
after this commit + mutation |
|---|---|---|
stopForWalletWipe_conflatedReset_restartsForNextWallet_withoutTheWipedWalletsPendingWalk |
PASSED (vacuous) | FAILED |
stopForWalletWipe_inFlightWalkCannotWriteAfterStop_andPipelineRestartsForNextWallet |
PASSED (vacuous) | FAILED |
stopForWalletWipe_reconcileRequestedMidWipe_isNotReplayedForTheNextWallet |
PASSED (vacuous) | FAILED |
stopForWalletWipe_failedReset_staysStoppedUntilResume_thenRestarts |
FAILED | FAILED |
stopForWalletWipe_sdkClearAlsoFailed_waitsForTheNextWalletsBind |
FAILED | FAILED |
2 of 5 → 5 of 5. The path was never entirely uncovered — the two failedReset/sdkClearAlsoFailed tests still caught it — but all three conflated-reset scenarios were vacuous. Both named wipe tests now fail by name on assertTrue("the stop's re-evaluation must restart the pipeline — the gate never saw the reset, so nothing else can", service.isCutoverActive()).
A third test had the same regression and is not in the finding: stopForWalletWipe_reconcileRequestedMidWipe_isNotReplayedForTheNextWallet. Same cause, fixed by the same change.
Routing is now non-positional
laggingGateConfig is deleted, replaced by two fixtures named for what they stage: delayedDeactivationConfig(...) and delayedPipelineGateConfig(...).
The mock classifies each subscription by which production seam is on the stack below it, not by call order. Three other hand-rolled positional mocks were migrated to the same router, and no observeCalls counter remains in the file. assertWired() is the loud half: each of the three long-lived subscriptions must arrive exactly once as itself, so a renamed seam, a collector that stops subscribing, or a fourth subscription fails the test instead of silently re-routing. It earned its keep during development by catching cutoverOwnershipCommittedBlocking being mis-classified as the ownership feed.
The KNOWN LIMITATION block on aDeactivationAppliedLateCannotRevokeANewerActivation is now resolved and removed, rather than restated.
One further correction: stopForWalletWipe_inFlightWalkCannotWriteAfterStop_… was delivering the gate's reset late, which gave the gate a distinct transition to restart on — that is why it still passed under the mutation even after the routing was fixed. Its gate feed now stays CUT_OVER for the whole test, so the restart it asserts on can only be the stop's re-evaluation.
The production change, and why it was necessary
Three one-line named seams:
private fun cutoverDeactivationFeed(): Flow<Boolean> = cutoverUiActive()
private fun cutoverOwnershipFeed(): Flow<Boolean> = cutoverOwnershipCommitted()
private fun cutoverPipelineGateFeed(): Flow<Boolean> = cutoverUiActive().distinctUntilChanged()No behaviour change — each is byte-equivalent to the flow its launch built inline (distinctUntilChanged() simply moved into the gate's seam). A test-only fix was preferred and attempted first; it is genuinely not possible here. All three subscriptions resolve to dashPayConfig.observePreservingErrors(CUTOVER_STATE) — same method, same key, same argument. The only other discriminator is the synthetic lambda class name (CutoverUiDataService$start$1 vs $start$3), which is itself source-order-derived, i.e. still positional, just spelled differently. Naming the seams is what makes the routing survive a reordering of the scope.launch blocks. The KDoc records this so a future refactor knows not to inline them away.
Verification
:wallet:compile_testNet3DebugKotlin— BUILD SUCCESSFUL.- Full suite, 2353 tests, 176 failures: 175 conscrypt +
PlatformSyncEngineRestartTest#resume_isRearmableAcrossRepeatedStopRestartCycles(pre-existing, fix(upgrade): v12 cutover and sync lifecycle, plus a measured account of what still loses money #1568). Zero new by name. CutoverUiDataServiceTest— 124 tests, 0 failures, 3× with--rerun-tasks, no flakiness.- All six tests required to keep passing do, and four now carry a loud wiring precondition they did not have before.
- ktlint clean (its source sets do not cover
wallet/). - The mutation used for the proof was reverted and the production file verified byte-for-byte against a copy taken before any edit.
Note on assertWired()
It requires each long-lived subscription exactly once. If a future change legitimately adds a second subscription through one of those seams, it will fail — deliberately. That should be reviewed, not auto-accommodated.
The remaining suggestion
The end-to-end self-transfer validation is unchanged and still open. Stated plainly: it cannot be closed by any host-JVM change, and a queued plan is not a result. It needs an authorized testnet run on the final head covering destination ownership, credit recognition and spendability after restart, across the shield/unshield cycle, the redirected sweep and CoinJoin consolidation. Awaiting maintainer go-ahead; procedure is in artifacts/pr1570-run19-inclusion.md.
🤖 Generated with Claude Code
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 2 only (Phase 1 failed)
Static verification at the exact head confirms the receive-address ownership, generation, ordering, and financial-side-effect fixes, but the new unavailable-address handlers can still crash after fragment cancellation. One blocking lifecycle finding and the existing end-to-end self-transfer verification suggestion remain. The supplied CI snapshot at 2026-10-03T08:55:11Z reports successful check and build jobs; no local builds or tests were run.
🔴 1 blocking | 🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The diff is large and intricate and directly changes funds destination selection in CoinJoinFundsMigrationService.migrateMixedFunds and ShieldedTransferExecutor, alongside cross-cutting receive-address ownership, caching, and failure handling. - Phase 1 reviewers: failed (failed on every model; its output was dropped)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `integrations/coinbase/src/main/java/org/dash/wallet/integrations/coinbase/ui/CoinbaseOrderReviewFragment.kt`:
- [BLOCKING] integrations/coinbase/src/main/java/org/dash/wallet/integrations/coinbase/ui/CoinbaseOrderReviewFragment.kt:126-131: Check lifecycle cancellation before displaying an address-read failure
Rotate or leave this screen while getTransferDashParams() is waiting for an unbound SDK wallet. The fragment's lifecycleScope is cancelled, but the underlying Thread.sleep/FFI work continues. When that work subsequently fails, ReceiveAddressUnavailableException can still reach this catch: withContext's cancellation check discards successful results, but does not replace an exception produced by its body. The handler then calls requireContext() after the fragment has detached, throwing an uncaught IllegalStateException instead of presenting a retry. ReceiveFragment and the Topper entry-point fragments contain the same newly added failure-handler pattern. Check coroutine cancellation before performing error UI, and use a still-attached, appropriate lifecycle owner/context. Add coverage for cancellation followed by a failed address read; the current confirmation tests model cancellation cleanup without exercising the fragment's requireContext() call.
In `wallet/src/de/schildbach/wallet/ui/shielded/ShieldedTransferExecutor.kt`:
- [SUGGESTION] wallet/src/de/schildbach/wallet/ui/shielded/ShieldedTransferExecutor.kt:605-617: Verify internal-chain self-transfer destinations end to end
(existing thread: https://github.com/dashpay/dash-wallet/pull/1570#discussion_r4171723484)
The normal withdrawal now obtains its destination through unadvertisedDestinationLive(), which selects the SDK's internal chain. ShieldedTransferExecutorTest verifies the accessor wiring and pre-broadcast refusal, but mocks withdrawToCore, so it does not establish that a real withdrawal to this destination is detected, credited, and subsequently spendable. The ordinary change-output evidence demonstrates internal-chain ownership, not completion of this withdrawal route. Your plan to exercise this in the next integration run leaves that verification pending, and the sweep and CoinJoin combine now use the same destination primitive. Run a testnet shield-to-unshield cycle that checks the actual output, credited transparent balance, and subsequent spend; exercise sweep/combine separately to cover their transaction construction.
Out-of-scope follow-up suggestions (2)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Reserve self-transfer destinations independently of ordinary payment change — sdkUnadvertisedAddressLiveBlockingWithBinding() obtains nextChangeAddress without reserving it. An ordinary payment and a self-transfer can therefore select the same unused internal address, allowing the payment recipient to correlate a self-transfer with the visible change output. The current code documents this residual, and the discussion explicitly defers the allocation contract to upstream SDK support rather than this receive-address fix.
- Follow-up: Track an upstream durable address-reservation or dedicated destination-pool API that competing change builders honor, then adopt it in a separately authorized wallet change.
- Bind self-transfer destinations to the funding wallet at spend time — unadvertisedDestinationLive() validates the destination generation before returning a plain Address, but withdrawToCore() and drainCoinJoinAccountTo() select their funding wallet later. A wallet replacement after the accessor's final check can still pair the previous wallet's destination with the new wallet's funds. The accessor-local race is fixed; transaction-level destination/funding association is the explicitly deferred orchestration follow-up.
- Follow-up: Track a binding-aware spend contract that carries destination wallet identity through preparation and validates it against the funding wallet under spend synchronization.
| } catch (ex: ReceiveAddressUnavailableException) { | ||
| // Depositing to the held dashj chain's frozen address is the | ||
| // SR-03 defect; nothing has been bought yet, so this is safe | ||
| // to retry. | ||
| Toast.makeText( | ||
| requireContext(), |
There was a problem hiding this comment.
🔴 Blocking: Check lifecycle cancellation before displaying an address-read failure
Rotate or leave this screen while getTransferDashParams() is waiting for an unbound SDK wallet. The fragment's lifecycleScope is cancelled, but the underlying Thread.sleep/FFI work continues. When that work subsequently fails, ReceiveAddressUnavailableException can still reach this catch: withContext's cancellation check discards successful results, but does not replace an exception produced by its body. The handler then calls requireContext() after the fragment has detached, throwing an uncaught IllegalStateException instead of presenting a retry. ReceiveFragment and the Topper entry-point fragments contain the same newly added failure-handler pattern. Check coroutine cancellation before performing error UI, and use a still-attached, appropriate lifecycle owner/context. Add coverage for cancellation followed by a failed address read; the current confirmation tests model cancellation cleanup without exercising the fragment's requireContext() call.
source: gpt-6.1-sol (phase2-reviewer: general)
There was a problem hiding this comment.
Resolved (re-reviewed at f4374a46): Your Coinbase failure handler now calls toastIfStillAttached(), which checks coroutine activity and uses nullable context rather than requireContext(). AttachedUiTest also explicitly stages an exceptional withContext completion after cancellation, so the guard addresses the reported late-failure path.
The address reads this PR made fail closed park on an uncancellable Thread.sleep for up to BINDING_WAIT_MS (5s) waiting for the SDK engine to bind, so seconds can pass between a screen going away and the read unwinding. Cancelling a withContext discards a SUCCESSFUL result and resumes the caller with CancellationException, but an exception thrown by its body is delivered as itself -- DispatchedTask.run consults the job only when the resumed result carries no exception. The failure handlers therefore ran on dead screens, where requireContext() threw IllegalStateException instead of offering the retry. runIfStillAttached (common/ui/AttachedUi.kt) is the one guard all seven UI handlers now go through, via Fragment/Activity toastIfStillAttached. Both of its checks are load-bearing and neither implies the other: the cancellation check catches a view-scoped coroutine cancelled while the fragment itself is still attached on the back stack (ReceiveFragment, ExploreTestNetFragment), and the nullable context catches a fragment-scoped coroutine resuming after onDetach. It takes Fragment.getContext() rather than requireContext(), and deliberately not the application context: these are "try again" prompts that belong to the screen the user is on, so a user who has left should not be interrupted. CrowdNodeViewModel's three apiError writes take the same cancellation check for a different reason: CrowdNodeApi.apiError is an app-scoped singleton and only a dialog actually shown clears it, so a read abandoned with the staking screen latched a failure that greeted the NEXT staking session before anything had been read. PaymentsReceiveFragment is unchanged and keeps its RESUMED gating: its handler is reached from a StateFlow collect, which calls collectorJob.ensureActive() before each emit on the same main thread that cancels it, so it cannot deliver after cancellation. Tests. AttachedUiTest pins the library premise directly -- aCancelledWithContextStillDeliversItsBodysException parks a read inside withContext(IO), cancels the job, only then throws, and asserts the catch still runs -- and covers all three arms of the guard. CoinbaseBuyDashConfirmGuardTest drives the same sequence through the modelled review screen and counts handler ENTRIES separately from prompts SHOWN, so "showed nothing" cannot pass by the handler never running at all. CrowdNodeViewModelTest covers the reported and the abandoned read as a pair. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fixed — head
|
| Fixed | Why it was unsafe |
|---|---|
CoinbaseOrderReviewFragment:126 |
requireContext() after onDestroy → crash. The named finding. |
ReceiveFragment:97 |
Worse than crash-only — view scope dies on navigating forward while the fragment stays attached on the back stack, so getContext() is still non-null. Only the cancellation check suppresses a toast over the next screen. |
ExploreTestNetFragment:62 |
Same view-scope shape. |
WalletFragment:466 (Topper), BuyAndSellIntegrationsFragment:76, UpholdPortalFragment:118 |
requireContext() after detach. |
UpholdTransferActivity:147 |
No crash, but a finished activity toasted over whatever the user moved to. |
CrowdNodeViewModel ×3 |
See below — the worst one. |
The CrowdNode one is more serious than the reported crash. crowdNodeApi.apiError is a singleton MutableStateFlow that outlives the ViewModel, and clearError() only runs where a dialog was actually shown. The non-cancelled path self-clears; the cancelled path latches. StakingActivity calls recheckState() from onCreate, so backing out of staking during the 5 s park left an error that greeted the next staking session with a dialog before anything had been read. Now routed through a single reportAddressFailure(ex) with a cancellation check.
Safe, with reasons — the notable one is PaymentsReceiveFragment.showReceiveAddress, where the reviewer's suspicion does not apply: the handler is reached from viewModel.receiveAddress.collect(...), and StateFlowImpl.collect calls collectorJob?.ensureActive() immediately before each emit; both that and the viewLifecycleOwner cancellation run on the main thread, so they cannot interleave. Left unchanged, with shouldSurfaceAddressFailure, the RESUMED gate and the quiet-offscreen behaviour intact. Also safe: WalletUriHandlerActivity (no UI, setResult + finish() already outside the try), StakingActivity.handleCrowdNodeError (a LiveData observer removed at DESTROYED), three ViewModels writing only to scope-owned state, two SwapKit paths returning ResponseResource.Failure, and the two background executors.
On requirement 4 (CancellationException rethrow): no change was needed and none was made. All 18 production handlers catch ReceiveAddressUnavailableException, which extends IllegalStateException, so none can swallow a cancellation. The only broad catches added already rethrow it.
Shape
One shared helper rather than 7 copies — the call sites were byte-identical already, and the reasoning is too long to repeat 7× without drifting.
suspend fun <T : Any> runIfStillAttached(host: T?, block: (T) -> Unit): Boolean {
if (!currentCoroutineContext().isActive) return false
val attached = host ?: return false
block(attached); return true
}Both checks, because neither implies the other. Cancellation alone misses the detached-fragment case (lifecycleScope cancels at onDestroy, context nulls later at onDetach); the null-context check alone misses the view-scoped case (ReceiveFragment, ExploreTestNetFragment) where the fragment is still attached on the back stack.
isActive rather than ensureActive(): every handler ends at the UI it guards, so raising out of a catch would replace plain control flow with a throw for no gain; the two sites with work afterwards already return@launch, and Coinbase's finally { releaseConfirm(attemptId) } runs either way. Not the application context — these are "no address yet, try again" prompts owned by the screen that asked; surviving it would be noise. Ordering fixes and the a1f5ae763 attempt-id semantics are untouched.
Mutation matrix — 6 mutations, all died
| # | Mutation | Named test that failed |
|---|---|---|
| M1 | drop the isActive check |
aFailureArrivingAfterCancellationSurfacesNothing + anAttemptCancelledInTheDestinationReadSurfacesNothingWhenItFails |
| M2 | host ?: return false → checkNotNull(host) |
aDetachedHostSurfacesNothing |
| M3 | never invoke block |
anAttachedHostOnALiveCoroutineSurfacesTheFailure + aRetryableFailureGivesTheFlightBackAndTheNextTapBuys |
| M4 | drop the check in CrowdNodeViewModel.reportAddressFailure |
recheckState_cancelledBeforeTheAddressReadFails_reportsNothing |
| M5 | reportAddressFailure → no-op |
recheckState_addressUnavailable_reportsTheError |
| M6 | premise: parked body returns instead of throwing | aCancelledWithContextStillDeliversItsBodysException |
On the three vacuity traps: no verify on counters anywhere (counts are AtomicIntegers incremented inside the stubbed read, asserted only after join()); no Channel(CONFLATED) — two-way CountDownLatch handshakes, and the cancelled-attempt test asserts the attempt is still parked before releasing it; and the masking-layer trap is why unavailableAddressHandlers exists alongside unavailableAddressRetries — "surfaced nothing" would be satisfied for free if the exception never reached the catch, so both tests assert the handler ran before asserting it showed nothing.
Verification
:wallet:compile_testNet3DebugKotlin— BUILD SUCCESSFUL.- wallet 2353 tests / 176 failed, every JUnit XML parsed by name: 175 conscrypt +
PlatformSyncEngineRestartTest#resume_isRearmableAcrossRepeatedStopRestartCycles. Zero new. common 49, coinbase 21, crowdnode 34, uphold 11, maya 84, exploredash 24 — all 0 failures, freshness confirmed by XML mtime. - ktlint reformatted only import order in touched files. Running exploredash's tests rewrote two binary
.dbfixtures; reverted, not in the commit.
Open items
- The
Toast.makeText+context-vs-requireContext()substitution itself is not unit-covered — the Robolectric wall. The decision is:runIfStillAttachedis the real production function under test and each wrapper is one expression. Not claimed as more than that. - Pre-existing, flagged not fixed:
StakingActivity.handleCrowdNodeErrorcanAdaptiveDialog.show()afteronSaveInstanceStateon API 28+. Not a cancellation bug, predates this PR, and identical for theMISSING_PRIMARYbranch beside it — left alone rather than widen scope. Say if you want it. - Product assumption, stated: a toast for an abandoned address read is dropped, not re-delivered or deferred to the recreated screen. If the prompt should survive (e.g. a recreated Coinbase review re-prompting), that is ViewModel-held state, not a context swap — a different change.
The suggestion
Unchanged and fairly put: a plan is not a result. The end-to-end self-transfer verification needs an authorized testnet run on the final head — real withdrawal to the engine-derived internal-chain destination, output discovery, credited transparent balance, and a subsequent spend after restart, with the sweep and CoinJoin combine exercised separately. Pending maintainer go-ahead.
🤖 Generated with Claude Code
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
At f4374a4, the receive-address ordering, binding isolation, ownership refusal, and previously reported consumer failures are addressed. One remaining lifecycle defect allows a newly handled Topper failure to surface after forward navigation; end-to-end validation of the internal-chain self-transfer destinations also remains outstanding. This was a static review with no builds or tests executed locally; the supplied exact-head CI snapshot reports successful build and check jobs.
🔴 1 blocking | 🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate cross-module change directly alters funds-movement destinations in ShieldedTransferExecutor.withdrawToCore and CoinJoinFundsMigrationService, replacing dashj-derived addresses with live SDK internal-chain addresses while adding ownership, caching, and failure-handling logic. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `wallet/src/de/schildbach/wallet/ui/main/WalletFragment.kt`:
- [BLOCKING] wallet/src/de/schildbach/wallet/ui/main/WalletFragment.kt:462: Scope the Topper address request to the originating view
The Topper request runs in the fragment's lifecycleScope, which remains active when forward navigation destroys its view but retains the fragment on the back stack. Tap Topper while the SDK address read is waiting, then tap Send: the Send navigation explicitly retains walletFragment with setPopUpTo(R.id.walletFragment, false). If the address read subsequently fails, toastIfStillAttached() finds an active coroutine and a non-null fragment context, so the newly added Home/Topper error appears over Payments instead of being suppressed. Launch this click-owned request in viewLifecycleOwner.lifecycleScope so destruction of the originating view makes the existing cancellation guard effective. Apply the same correction to BuyAndSellIntegrationsFragment's Topper launch, which can remain on the back stack after navigating to another integration.
In `wallet/src/de/schildbach/wallet/ui/shielded/ShieldedTransferExecutor.kt`:
- [SUGGESTION] wallet/src/de/schildbach/wallet/ui/shielded/ShieldedTransferExecutor.kt:605-617: Verify internal-chain self-transfer destinations end to end
(existing thread: https://github.com/dashpay/dash-wallet/pull/1570#discussion_r4171723484)
The current tests establish destination routing and pre-broadcast refusal, but not receipt and spendability through the real SDK. ShieldedTransferExecutorTest supplies a mocked unadvertisedDestinationLive() address and mocks withdrawToCore() success, while the service separation tests use fake receive/change strings. The supplied on-chain change-output evidence supports internal-chain ownership, but does not execute this redirected withdrawal path. The queued integration run remains the appropriate place to close the gap: record a shield-to-unshield cycle on the current implementation, verify that the destination differs from the advertised Receive address, and verify that the output is credited and subsequently spendable. Exercise the paper-wallet sweep destination as well, since it now uses the same seam.
Out-of-scope follow-up suggestions (2)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Coordinate self-transfer destinations with ordinary change allocation — The service selects nextChangeAddress(0) without reserving it against ordinary transaction change selection. An ordinary payment and a self-transfer can therefore select the same unused internal address, allowing that payment's recipient to correlate the self-transfer through the visible change output. The implementation documents this concrete residual; closing it requires an SDK allocation contract shared with competing transaction builders and is outside this accessor-level fix.
- Follow-up: Track a separate upstream SDK change for durable address reservation or a dedicated self-transfer pool honored by ordinary change builders.
- Validate destination ownership against the funding wallet at spend time — WalletApplication revalidates the destination generation before returning, but returns only an Address. ShieldedBalanceServiceImpl and SdkL1SendService select their funding wallet afterward, so a wallet wipe and replacement after the final accessor check can separate the destination's owning wallet from the funding wallet. This operation-wide race is not closed by the read-time generation check and remains a separate spend/wipe coordination follow-up.
- Follow-up: Carry destination wallet identity and generation through transaction preparation, then validate them against the funding wallet under spend/wipe coordination before submission.
The Topper entry points launch the receive-address read in the FRAGMENT's `lifecycleScope`, which outlives the view. Home, Buy & Sell and the Uphold portal all stay on the back stack behind the screens they navigate to (Home's Send shortcut pops up to `walletFragment` with inclusive=false), so a read still parked in `CutoverUiDataService`'s uncancellable binding wait resumes into its `catch` with the coroutine still active AND the fragment still attached. Both halves of `toastIfStillAttached` therefore pass and the "loading error" toast lands over whatever the user moved to. Launch from `viewLifecycleOwner.lifecycleScope` instead, so destroying the originating view cancels the request and the existing guard suppresses the toast — the guard is right, the scope under it was not. A successful read is dropped the same way, which is also what we want: opening a Topper custom tab out of nowhere over Payments is worse than doing nothing. `ReceiveFragment`, `ExploreTestNetFragment` and `PaymentsReceiveFragment` were already view-scoped. The activity sites (`UpholdTransferActivity`, `WalletUriHandlerActivity`) have no view scope, and the Coinbase confirm and `PaymentsViewModel` reads deliberately outlive the view; all are left alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fixed — head
|
| Site | Why |
|---|---|
WalletFragment.kt:462 |
Click-owned Topper read, fragment scope, retained by Send's popUpTo(inclusive = false) |
BuyAndSellIntegrationsFragment.kt:71 |
Same, retained when another integration is picked |
UpholdPortalFragment.kt:113 |
Found by the audit. Same Topper launch, same guard added by this PR, same fragment scope; the portal stays on the back stack behind UpholdTransferActivity, the custom tab and showErrorAlert. Identical defect. |
The diff is three one-word changes — lifecycleScope → viewLifecycleOwner.lifecycleScope — plus a comment at each site. The toastIfStillAttached guards are untouched; this is what makes their cancellation half mean anything at these sites. On the Compose case: in BuyAndSellIntegrationsFragment the launch sits in a setContent click lambda declared inside onCreateView, and viewLifecycleOwner is read inside the lambda at click time when getView() is non-null, so it cannot throw the "View's LifecycleOwner when getView() is null" ISE.
Audit of every launch site in afa91bff2..HEAD
Correctly scoped, left alone, with reasons:
- Activity sites have no finer scope:
UpholdTransferActivity:142uses theActivity.toastIfStillAttachedoverload (isFinishing/isDestroyed), which is the activity-shaped equivalent;WalletUriHandlerActivity:187must complete for the calling app (setResult+finish()), so view-scoping would be meaningless. - Deliberately outliving the view, confirmed not "corrected":
CoinbaseOrderReviewFragment:107must stay fragment-scoped — its single-flight bookkeeping is keyed to the fragment (onDestroyhands the attempt back,onResumedistinguishes finished from running viaconfirmJob?.isActive,releaseConfirmis written for a gone view), and since the purchase submits under an idempotency UUID, cancelling at view destruction would widen the window in which a bought-but-unlatched attempt exists.PaymentsViewModel.requestReceiveAddressand the four otherviewModelScopesites are likewise deliberate.
Flagged, not fixed — SweepWalletFragment.java:620-650. Its destination read runs on backgroundHandler and posts back guarded only by isAdded(), which is the attachment half only, so in principle a late failure could toast from a back-stacked fragment. Not changed, for two reasons: it is not a lifecycleScope site so it is outside this fix, and its failure arm does setState(State.CONFIRM_SWEEP) — state-machine restoration that must run even with the view gone, or the fragment returns stuck in PREPARATION with a dead sweep button. Gating it on the view would be a behaviour regression. Sweep also has no forward navigation out of preparation, so the exposure is theoretical. Closing it properly needs a view-presence check that does not skip setState — say if you want that.
Testability — none at the JVM level, stated plainly
There is nothing honest to extract. The decision is literally "which LifecycleOwner owns this scope"; a shouldUseViewScope() helper would assert its own definition. Real coverage needs a FragmentScenario that starts the request, destroys only the view while keeping the fragment on the back stack, and asserts no toast — i.e. Robolectric, which fails at class-load here with UnsatisfiedLinkError: no conscrypt_jni in java.library.path (the cause of all 175 known failures). No stub was written, no tests added, so there is no mutation matrix to report. AttachedUiTest already covers the guard given a cancelled scope; it is the scope delivery that is untested, and that is exactly what this commit changes.
Manual check that would catch a regression — all three screens, post-cutover with the engine unbound so the read parks ~5 s: tap Topper, then immediately navigate forward (Home → Send; Buy & Sell → Uphold or Coinbase; Uphold portal → Transfer). Expected: nothing on the destination screen. Before this commit a "loading error" toast appears there about five seconds later. Second check: back out to the originating screen before the read unwinds — also suppressed, since the view was destroyed either way.
Verification
:wallet:compile_testNet3DebugKotlin+:integrations:uphold:compileDebugKotlin— BUILD SUCCESSFUL.- wallet 2353 tests / 176 failed: 175 conscrypt +
PlatformSyncEngineRestartTest#resume_isRearmableAcrossRepeatedStopRestartCycles(pre-existing, fix(upgrade): v12 cutover and sync lifecycle, plus a measured account of what still loses money #1568). Zero new by name. :integrations:uphold:testDebugUnitTest— 11 tests, 0 failures.- ktlint clean, no files rewritten. exploredash tests not run, so its binary
.dbfixtures are untouched; the commit contains exactly three source files.
The suggestion
Agreed, and I will stop describing a plan as if it were evidence. The end-to-end verification needs an authorized testnet run on this head: a shield-to-unshield cycle confirming the destination differs from the advertised Receive address and that the output is credited and subsequently spendable, with the paper-wallet sweep exercised separately since it uses the same seam. That run is also the only thing that would catch the class of lifecycle defect in this round and the last — the JVM suite structurally cannot reach it, as the two "no coverage" notes above make explicit.
🤖 Generated with Claude Code
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the complete diff at 6c850d2 and revalidated all 44 prior findings. The receive-read ordering, fail-closed handling, wallet-generation checks, and lifecycle fixes address the earlier defects, but the new unreserved internal-chain destination still permits linkage between ordinary-payment change and self-transfers. The supplied exact-head CI snapshot reports successful check and build jobs; this static review ran no builds or tests, and end-to-end unshield and sweep validation remains outstanding.
🔴 1 blocking | 🟡 1 suggestion(s)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate diff changes funds-movement destination selection in CoinJoinFundsMigrationService and ShieldedTransferExecutor, replacing fresh receive addresses with live engine internal/change addresses while adding cross-cutting wallet ownership and address-cache coordination. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `wallet/src/de/schildbach/wallet/service/platform/sdk/CutoverUiDataService.kt`:
- [BLOCKING] wallet/src/de/schildbach/wallet/service/platform/sdk/CutoverUiDataService.kt:3146-3147: Reserve the unshield destination against ordinary payment change
(existing thread: https://github.com/dashpay/dash-wallet/pull/1570#discussion_r4110572828)
This accessor returns the next unused internal address without claiming it for the self-transfer. The source delegates directly to core.nextChangeAddress(accountIndex), and the documented allocator contract at lines 3104–3118 explicitly says ordinary payment builders select from the same pool. WalletApplication returns that address, then ShieldedTransferExecutor passes it to withdrawToCore without any allocation coordination. A withdrawal can therefore capture C, an ordinary payment can subsequently select C for change while the withdrawal is being prepared, and both transactions can pay C. The ordinary payment's recipient can identify its change output and associate the shielded withdrawal and amount with the paying customer. Binding-generation validation does not prevent this same-wallet collision.
This is caused by the PR: the base withdrawal requested a newly issued external dashj key, whereas this change selects an unreserved address from the ordinary-payment internal pool. Separating self-transfers from the advertised QR fixes one linkage path but introduces this different counterparty-visible path. The documented SDK limitation explains why an accessor-only reservation fix is unavailable; it does not make the regression pre-existing or enforce a privacy boundary. Use a durable allocation mechanism honored by competing change builders or a separately issued destination pool. Until a safe allocation is available, refuse the affected self-transfer rather than accepting the shared pointer.
In `wallet/src/de/schildbach/wallet/ui/shielded/ShieldedTransferExecutor.kt`:
- [SUGGESTION] wallet/src/de/schildbach/wallet/ui/shielded/ShieldedTransferExecutor.kt:604-617: Verify internal-chain self-transfer destinations end to end
(existing thread: https://github.com/dashpay/dash-wallet/pull/1570#discussion_r4171723484)
Unshield, paper-wallet sweep, and CoinJoin combine now use the internal-chain destination. The tests verify accessor routing and refusal before submission, but the service fixture supplies a fabricated change address and the executor tests mock withdrawToCore. They do not exercise ManagedCoreWallet derivation, the resulting transaction output, SDK discovery, or subsequent spending. The supplied device evidence establishes ordinary change-output crediting and Receive correctness, but explicitly leaves these changed self-transfer paths unexecuted; a queued integration run is not execution evidence. Add an instrumented regression or attach a targeted testnet shield→unshield cycle and sweep run that verifies separation from the advertised receive address, credited balance/UTXOs, spendability after restart, and a destination-unavailable attempt that remains retryable without broadcasting.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Validate prepared destinations against the funding wallet at spend time — WalletApplication now rechecks the destination generation after resolving ownership, but downstream operations still receive a bare address and independently resolve the funding wallet. A wallet replacement after that final accessor check can pair the previous wallet's destination with the replacement wallet's funds. The base already prepared an address before independently selecting the funding wallet, so this operation-wide identity boundary is a concrete funds-safety follow-up rather than a newly introduced receive-accessor defect.
- Follow-up: Track a separate transaction-lifecycle change carrying wallet identity through destination preparation and validating it against the funding wallet before signing or broadcast.
Comments only on `sdkUnadvertisedAddressLiveBlockingOrNull`. No behaviour
change; `git diff` touches KDoc lines exclusively. Three corrections and one
addition, each checked against the pinned artifacts rather than taken on faith.
1. Wrong SDK version. The block said "no reservation or dedicated-pool API is
exposed at v42int21". `dashSdkVersion` in the root `build.gradle:17` is
`0.1.0-v42int27-SNAPSHOT`. Now names the gradle property and flags the
literal as re-readable rather than freezing another number into prose.
2. Overclaimed "no API exposed". True of the public Kotlin surface, misleading
otherwise, so the block now qualifies it twice:
- A change address can be PINNED, just not RESERVED.
`CoreTransactionBuilder.setChangeAddress$sdk_release(String)` is in the
pinned AAR's `classes.jar` (verified with javap), bound through
`WalletManagerNative.coreTxBuilderSetChangeAddress` to the exported FFI
symbol `core_wallet_tx_builder_set_change_address` (verified in the
dynamic symbol table of `jni/arm64-v8a/libdash_sdk_jni.so`), and
`CoreSendAllNative` is the in-tree precedent for reaching an
SDK-`internal` `$sdk_release` member from Java. It names a change output
on ONE build and does not take the address out of the pool, so a
concurrent builder can still draw it. Whether a pinned address wins over
the pool draw is a Rust detail NOT verified here and is written up as
such.
- The reservation primitive upstream (`AddressState::{Available, Reserved,
Used}`, `next_unused_and_reserve`, `release_reservation`,
`sweep_expired_reservations`) is recorded as REPORTED, from a Rust
revision that is not necessarily the one this AAR was built from, with an
explicit instruction to re-check before quoting. What the comment does
assert is what was verified here: none of it is exported from the pinned
`.so`. The whole address-pool FFI surface is read-only
(`address_pool_get_*`, `managed_*_get_*_address_pool`) plus
`core_wallet_next_change_address` / `core_wallet_next_receive_address` —
no `*_and_reserve`, no release, no sweep, and no internal-chain
counterpart of the receive-side trio. Also notes that the
`releaseReservation` the SDK does expose reserves UTXOs, not addresses,
since the name invites exactly that confusion.
3. Wrong rationale for not caching. "Caching would also make two successive
self-transfers share one destination for no benefit" does not hold: not
caching does not avoid that either, because nothing is reserved and both
transfers draw the same next-unused address regardless. 4f593cb carries
both halves — the same claim in its "Deliberately NOT cached" paragraph and
the correct fact one sentence later ("Two unshields raced before either
confirms can still share one internal address (the FFI exposes no
reservation)"). The KDoc now says the reachable thing: there is no screen to
keep warm, so a cache buys nothing, and not caching buys nothing either.
Also added the real exposure window, which the old text implied was build-time.
The engine marks an internal address used on network OBSERVATION of the
spending transaction, so for the unshield the window is up to ~10 minutes: the
withdrawal runs the platform withdrawal queue (`ProvingOverlay`'s parameter
KDoc in `ShieldedTransferScreen.kt:916-919` records the up-to-10-minute wait
for this direction) and the proving overlay is dismissable (`:904-911` — Hide
keeps the spend on the app scope and hands the wallet back), so sending an
ordinary payment mid-withdrawal is expected, and an ordinary payment is the
builder that can draw the same address. The sweep and CoinJoin-combine callers
broadcast in seconds. This makes the residual read larger than before, which is
the point.
The "## KNOWN RESIDUAL — this is NOT an isolation guarantee" framing and its
cross-operation-linkage paragraph are unchanged; they were already right.
Verified: `:wallet:compile_testNet3DebugKotlin` BUILD SUCCESSFUL. No test run —
nothing executable changed. `./gradlew ktlintFormat` clean and nothing
reformatted (`:wallet` does not apply ktlint; the task does not exist there).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The preceding commit hedged the key-wallet reservation primitive as
"reported" because the Rust sources could not be found locally. They are
present, under ~/.cargo/git/checkouts, and the claims check out at
rust-dashcore@f3dc260: AddressState::{Available, Reserved{at}, Used},
AddressPool::next_unused_and_reserve / release_reservation /
sweep_expired_reservations, and next_unused gating on is_available() so
that it already skips Reserved. The receive-side trio is on
ManagedCoreFundsAccount; there is no change-side counterpart.
That last point is why the upstream ask is small: the selector needs no
change, only an internal-pool equivalent of methods that already exist.
The revision-mismatch caveat stays — f3dc260 is not proven to be what the
pinned v42int27 AAR was built from — but "could not verify" was written
on a false premise and overstated the doubt.
Also records dashpay/platform#5276 and why this codebase accepts the
residual rather than refusing the self-transfer: the alternatives are the
advertised QR chain, or the held dashj chain's already-paid address, which
is SR-03 itself.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contesting this blocker — head
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
At head 3862d6c, the receive-path generation and ordering fences, fail-closed handling, and lifecycle fixes address the prior correctness defects. One in-scope privacy blocker remains in the shared, unreserved internal-address allocator; native end-to-end destination validation and one stale KDoc reference remain non-blocking items. This was static verification only; the supplied exact-head CI snapshot reports successful check and build jobs.
🔴 1 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate diff directly changes funds destination selection in CoinJoinFundsMigrationService and ShieldedTransferExecutor, replacing dashj receive addresses with live SDK internal-chain addresses for mixed-funds migration and unshielding. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `wallet/src/de/schildbach/wallet/service/platform/sdk/CutoverUiDataService.kt`:
- [BLOCKING] wallet/src/de/schildbach/wallet/service/platform/sdk/CutoverUiDataService.kt:3213-3214: Reserve the unshield destination against ordinary payment change
(existing thread: https://github.com/dashpay/dash-wallet/pull/1570#discussion_r4110572828)
This lookup obtains the next internal address without reserving it. ShieldedTransferExecutor fixes that address as the withdrawal destination, while ordinary payments in SdkL1SendService use sendToAddresses() with default BIP44 change selection. Until the engine observes a transaction using that address, an ordinary payment can therefore select the withdrawal destination for its change. The merchant receiving that payment can identify the change output and associate a subsequent withdrawal to it with the paying user, disclosing the withdrawal and its amount. The withdrawal overlay can be hidden while the operation continues, and its documented queue can last up to ten minutes, so this is a supported overlapping flow rather than a tiny scheduling window.
The base comparison does not remove this PR's causal contribution. At afa91bff2, the withdrawal called dashj's freshReceiveAddressString(), which issues external-chain keys; ordinary SDK payment change uses the internal chain. A held restore can reuse paid external keys, but that does not make every newly issued external key already paid: issuance can advance beyond the funded external indices. Moving those destinations onto the shared internal allocator adds merchant-change linkage in those cases even though it improves the restored-wallet receive defect.
Do not globally disable withdrawals: the resulting funds-availability problem is valid. Preserve them by allocating destinations that competing change builders cannot select, or by coordinating ordinary-change construction against outstanding self-transfer destinations until they are observed. The documented residual and upstream tracking accurately describe the limitation but do not prevent this newly introduced disclosure; pinning one builder's change address alone does not reserve it against the others.
- [NITPICK] wallet/src/de/schildbach/wallet/service/platform/sdk/CutoverUiDataService.kt:3245-3248: Restore the blocking receive accessor's KDoc and update its callers
(existing thread: https://github.com/dashpay/dash-wallet/pull/1570#discussion_r4108292164)
The KDoc is attached to the correct accessor, but it still says the paper-wallet sweep drives currentReceiveAddressLive()/freshReceiveAddressLive(). SweepWalletFragment.handleSweep() now calls unadvertisedDestinationLive(), which reads the separate internal/change-address path. Remove the obsolete sweep reference so the documentation does not direct readers of a self-transfer destination flow to the advertised receive accessor.
In `wallet/src/de/schildbach/wallet/ui/shielded/ShieldedTransferExecutor.kt`:
- [SUGGESTION] wallet/src/de/schildbach/wallet/ui/shielded/ShieldedTransferExecutor.kt:605-617: Verify internal-chain self-transfer destinations end to end
(existing thread: https://github.com/dashpay/dash-wallet/pull/1570#discussion_r4171723484)
ShieldedTransferExecutorTest supplies a mocked internal destination and returns SdkWriteResult.Broadcast(Unit) from a mocked withdrawToCore(); the address-service tests likewise supply fake receive/change addresses. These verify address routing and refusal before submission, but do not exercise the native SDK's crediting, persistence, and subsequent spending of the changed withdrawal destination. Existing evidence that an ordinary internal change output is credited supports ownership, but does not establish this withdrawal path end to end. Keep the queued device check as a non-blocking verification item: execute a shield→unshield cycle using the actual destination, confirm the output is credited and spendable after restart, and verify that a temporarily unavailable destination can recover without submitting a withdrawal to an advertised address.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Bind self-transfer destinations to the funding wallet at spend time — WalletApplication now validates the destination's generation after consulting ownership, closing the reviewed accessor window. Callers then pass an Address or String to a spending service that independently selects its funding wallet, so a wallet replacement after that final check can still mismatch the destination and funding wallet. This separate-operation contract existed at the base and requires coordinated spend/wipe changes rather than another accessor snapshot check.
- Follow-up: Track a separate issue to carry wallet identity through destination preparation and validate it when selecting the funding wallet, with wallet-replacement regression coverage.
SweepWalletFragment.handleSweep() moved to unadvertisedDestinationLive() (the internal/change path), but several comments still described the old wiring: - CutoverUiDataService.sdkReceiveAddressLiveBlockingOrNull said the paper-wallet sweep drives the two live receive accessors. It does not; that sent a reader of a self-transfer destination flow to the ADVERTISED accessor. Replaced with what the accessor is for, and a pointer to the unadvertised one. - WalletApplication.unadvertisedDestinationLive and WalletData's interface doc both said "Both callers" catch before broadcast. There are three now (ShieldedTransferExecutor, CoinJoinFundsMigrationService, SweepWalletFragment) and all three do. The invariant is now stated as the requirement on every caller, with the current set as an aside. - The "NOT the unshield or CoinJoin-combine destinations" asides on the two advertised-read accessors, and the self-transfer examples on CutoverUiSource.nextChangeAddressOrNull, were enumerations that go stale on every new caller. They now describe the category (a self-transfer destination) instead of listing who asks for one. Comments only: stripping comments from all three files leaves byte-identical code, and every changed line is a KDoc body line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nitpick fixed — head
|
| this PR | dashwallet-ios | |
|---|---|---|
| Self-transfers draw from | internal / change | external / receive |
| Collides with | the SDK's own change selection | every advertised address |
| Observable by | a payment counterparty, by inference | Coinbase, Uphold, CrowdNode, Topper, SwapKit — directly |
iOS sites: unshield ShieldedTransferCoordinator.swift:919, :1223; CoinJoin consolidation WalletSendService.swift:440; identity withdrawal IdentityWithdrawViewModel.swift:138 — all through SwiftDashSDKReceiveAddressReader.receiveAddress() → nextReceiveAddress(accountIndex: 0), the same function behind PaymentsLandingViewModel.swift:452 (Receive QR), CBAccount.swift:111 (Coinbase), CrowdNodeModel.swift:148 (CrowdNode account identity), and nine others.
The sharpest case there: consolidating freshly de-anonymised CoinJoin funds pays whatever address the Receive QR is currently showing.
This matters here because the receive chain is the only alternative destination this PR could have chosen, and it is demonstrably the worse one — not as a prediction, but as shipping behaviour in a sibling app. Both chains are unreserved; next_receive_address and next_change_address both land on AddressPool::next_unused, which leaves state untouched.
What I am not doing, and why
Your remaining two remedies are "allocate destinations competing builders cannot select" and "coordinate ordinary-change construction against outstanding self-transfer destinations".
The first needs an FFI that does not exist, and carving out an index range would be worse than the disclosure: discovery is gap-limit bounded (defaults 30/30), so an output past highest_used + gap_limit is never derived, BIP158 cannot match it, and rescan cannot recover it — measured in-tree at 0.449 DASH invisible across change indices 828-1630.
The second means the app taking over sendToAddresses — coin selection, UTXO reservation, fee, error classification — on the money path, and the only avoidance control the SDK exposes (changeToFirstInput) sends change to an address the transaction already spent from, which the recipient identifies as change trivially. That trades this leak for another one.
So the fix stays upstream: dashpay/platform#5276, now widened to ask for reserving variants on both chains, since one change fixes this PR's collision and iOS's. The primitive already exists (AddressPool::next_unused_and_reserve, release_reservation, sweep_expired_reservations, and the receive-side trio on ManagedCoreFundsAccount) and next_unused already skips Reserved, so it needs no cooperation from competing builders — only the change-chain wrapper and the FFI bridge.
If a maintainer weighs the residual differently, I'll implement whatever they decide. It is a product call, not a review-thread one.
Suggestion
Unchanged and accepted as non-blocking. Still no device verification of any self-transfer destination.
🤖 Generated with Claude Code
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
At the exact head, 42 of the 44 prior findings are fixed, including the receive-address fail-closed behavior, ownership and ordering races, purchase preparation, and lifecycle handling. The shared, unreserved change destination remains an in-scope privacy blocker; end-to-end verification of the changed self-transfer destinations remains a non-blocking recommendation. The supplied exact-head CI snapshot reports successful check and build jobs; this verification was static, with no local builds or tests run.
🔴 1 blocking | 🟡 1 suggestion(s)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate diff changes funds destinations in CoinJoinFundsMigrationService.kt, ShieldedTransferExecutor.kt and SweepWalletFragment.java, alongside synchronized wallet/key-chain ownership and engine-backed address selection in CutoverUiDataService.kt. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `wallet/src/de/schildbach/wallet/service/platform/sdk/CutoverUiDataService.kt`:
- [BLOCKING] wallet/src/de/schildbach/wallet/service/platform/sdk/CutoverUiDataService.kt:3212-3213: Reserve the unshield destination against ordinary payment change
(existing thread: https://github.com/dashpay/dash-wallet/pull/1570#discussion_r4110572828)
This reads the next available internal address without making it unavailable to ordinary payment builders. The production source calls core.nextChangeAddress(accountIndex), while DashSdkL1SendSource.sendToAddress() uses automatic BIP44 change with no exclusion for this destination. I independently checked the cited rust-dashcore revision f3dc260: ManagedCoreFundsAccount.next_change_address() delegates to AddressPool.next_unused(), which returns an available entry without changing its state; TransactionBuilder.fund_from() obtains change through that same method. Bumping the account's monitoring revision does not reserve the address.
A user can queue a withdrawal to C, hide the proving overlay, and pay a merchant before the withdrawal is observed on L1. The merchant payment can use C as change, allowing the merchant to associate the later withdrawal output and its amount with that customer. ShieldedTransferScreen explicitly permits hiding the overlay while the operation continues and documents a withdrawal wait of up to ten minutes. UTXO reservations do not isolate this destination: the shielded withdrawal and the merchant payment need not spend the same inputs.
The base called dashj's fresh external-address accessor; once issuance had advanced past funded external indices, those destinations were unused and separate from internal change. This PR therefore introduces linkage in a previously clean case even though it improves restored-wallet behavior. The iOS receive-chain exposure, corrected documentation, and upstream issue do not exclude C from this Android allocator. Do not resolve this by permanently disabling withdrawals; provide a reservation or competing-change exclusion mechanism that lasts through network observation. The supplied discussion contains no explicit maintainer acceptance of this newly introduced privacy tradeoff.
In `wallet/src/de/schildbach/wallet/ui/shielded/ShieldedTransferExecutor.kt`:
- [SUGGESTION] wallet/src/de/schildbach/wallet/ui/shielded/ShieldedTransferExecutor.kt:605-617: Verify internal-chain self-transfer destinations end to end
(existing thread: https://github.com/dashpay/dash-wallet/pull/1570#discussion_r4171723484)
The withdrawal now pays an engine internal-chain destination, and paper-wallet sweeps and CoinJoin combines use the same destination seam. The tests establish accessor selection, binding validation, and refusal before broadcast, but ShieldedTransferExecutorTest mocks withdrawToCore(), so it does not demonstrate that an actual withdrawal is discovered, credited, spendable, and recovered after phrase restoration. The cited historical change output supports general internal-chain recognition, not execution of these changed self-transfer paths, and the supplied device evidence explicitly leaves unshield and sweep untested.
Use a disposable testnet wallet to run a shield-to-unshield cycle and a paper-wallet sweep on the pinned build, verify the resulting outputs can be spent, and restore the phrase to confirm recovery. This closes the funds-destination verification gap without spending the shared QA fixture.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Bind self-transfer destinations to the funding wallet at spend time — The final accessor-level generation check is present, but callers still pass a bare address to operations that select the funding wallet separately. An in-process wallet reset after the accessor returns can therefore separate destination ownership from funding-wallet ownership. The base already separated these decisions, so transaction-wide wallet binding is a concrete funds-safety follow-up rather than a blocker introduced by this PR.
- Follow-up: Track a separate change carrying wallet identity with the destination and validating it against the funding wallet before signing or withdrawal submission.
…ured latency
The `## How long the collision window actually is` KDoc on
`sdkUnadvertisedAddressLiveBlockingOrNull` gave the window as "up to ~10
MINUTES, not seconds", sourced from the app's own user-facing copy
("could take up to 10 minutes" / "Up to 10 minutes to spend"). That is a
promise to the user, not a measurement, and reading it as the window's
actual length overstates the residual.
A disposable-wallet device run on this head measured it: unshield
submitted 16:37:58Z, L1 output visible to dashd 16:39:29Z — 91 seconds of
platform withdrawal-queue latency, first confirmation at 2 min 13 s.
The section now carries both numbers with their provenance and their
weight: the 91 s as a single observation on one emulator against testnet
(explicitly not a distribution or a guaranteed latency), and ~10 minutes
as the app's stated upper bound. The reasoning the section exists for is
unchanged — the window is bounded by network observation of the spending
transaction, not by build time, and the proving overlay can be hidden
while it runs, so a concurrent ordinary payment remains a supported flow
rather than a tiny scheduling race. Only the magnitude moved.
Comment-only: stripping all comments from the file at HEAD and at this
commit yields byte-identical code.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Device verification done — head
|
| Assertion | Result |
|---|---|
| Unshield destination ≠ advertised address | PASS — internal 1/1 vs step-2 external 0/0 and the at-withdrawal external 0/1 |
| Destination is on the internal chain | PASS — positively established, not inferred |
| Discovered and credited | PASS — 1.765497 → 2.197597; history row "Unshielded +0.4321" |
| Spendable | PASS — the onward spend was sized to force it: 1.95 when the only other UTXO was 1.7654974, so input[0] had to be the unshield output. Confirmed in the raw tx; dashd received 1.95000000 |
| Paper-wallet sweep ×3 | PASS — destinations 1/3, 1/5, 1/6, all internal; the sweep spend's sole input was the swept output |
| Recovery after phrase restore | PASS — balance 0.959786, matching dashd's 0.95978561 exactly; phase=SYNCED 100.0% in ~2.5 min |
| Spendable after restore | PASS — one 0.90 send consumed all four recovered UTXOs |
| Destination-unavailable path | NOT INDUCED — no coverage claimed (below) |
Whole-run chain usage: the external chain was touched exactly twice — 0/0 (the inbound funding) and 0/1 (the advertised address, still unpaid and unchanged across every self-transfer and across the restore). All eight self-transfer destinations and change outputs landed on internal 1/0 … 1/7, monotonic, no reuse, no collision with the advertised address.
Asset Unlock was type 9 as expected. debugUnshieldDestinationOverride() is DEBUG-gated and inert in this release build — no QA debug override active line in the log — so every destination came from production code.
Correcting a figure I had wrong, which cut against my own argument
The unshield queue was measured at 91 seconds: submitted 16:37:58Z → L1 output visible to dashd 16:39:29Z → confirmed 16:40:11Z, end to end 2 min 13 s.
I had been citing "~10 minutes" as the window's length, including in arguing that this residual was worse than you had described. That figure is the app's own user-facing upper bound — the ProvingOverlay KDoc and the confirm sheet's "Up to 10 minutes to spend" — not a measurement of what the queue does. One sample is not a distribution and the bound still stands, but quoting it as typical overstated the exposure. 1abb62121 corrects the KDoc to carry both numbers with their provenance and their weight, and the same correction went into our own defect record.
The argument the comment exists to make is unchanged at either magnitude: the window closes on network observation of the spending transaction, not at build time, and the overlay's Hide button leaves the spend running — so a concurrent ordinary payment remains an expected flow rather than a tiny scheduling race.
What was not verified — stated, not papered over
- The destination-unavailable path could not be induced. Three attempts, all failed for understood reasons:
trim-memory COMPLETEstops the service but it restarts within ~2 s;RUNNING_CRITICALforegrounded is refused by AMS;am stopservice BlockchainServiceImpldoes stop it, butSweepWalletActivityrestarts it before Sweep can be tapped. The only genuinely reachable window is the ~5 sBINDING_WAIT_MSafter a cold process start, which neither flow reaches because both need PIN entry first. The not-broadcast behaviour remains unverified at runtime. - The collision itself is not device-verified. This run was sequential by construction; it demonstrates the destinations are correct, not that a concurrent ordinary payment cannot draw the same address. That residual stands as documented, tracked at kotlin-sdk: bridge the change-address reservation primitive to FFI (self-transfer destinations collide with ordinary payment change) platform#5276.
CoinJoinFundsMigrationService, the third caller ofunadvertisedDestinationLive(), was not exercised.- The unit suite was not run on this host — Robolectric fails at class load with
no conscrypt_jni, as throughout this PR. No claim is made about it from this run.
Environment notes
The first build failed on uploadCrashlyticsMappingFile_testNet3Release → HTTP 503, a network error with no APK emitted; rebuilt with that task excluded. The in-app sweep offers only QR scanning and the emulator's virtual-scene camera could not be driven to it, so the sweep was exercised through the app's own exported ImportSharedImageActivity (ACTION_SEND image/*), which routes the decoded key into the same SweepWalletFragment under test — a real user path, not a test hook.
Full evidence, timings and per-step logs: artifacts/pr1570-disposable-wallet-verification.md.
On the blocker
Unchanged, and still the one item outstanding. The residual is real, documented in code with its provenance, and tracked upstream with the fix specified. What you identified as missing — explicit maintainer acceptance — is the right thing to be missing, and it is not something I can supply on a maintainer's behalf.
🤖 Generated with Claude Code
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Revalidated all 44 prior findings against head 1abb621: 43 are addressed, while the unreserved self-transfer destination still introduces merchant-change linkage. The receive-read fences, fail-closed decisions, retry handling, and view-scoped Topper requests are present; the supplied device report establishes internal-chain credit, spendability, and recovery but does not establish allocation isolation. The exact-head CI snapshot reports successful check and build jobs; this verification was static, with no builds or tests run.
🔴 1 blocking
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate diff directly changes funds destination selection in ShieldedTransferExecutor.executeTransfer and CoinJoinFundsMigrationService, alongside engine-backed address derivation and wallet-ownership/cache handling in CutoverUiDataService. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `wallet/src/de/schildbach/wallet/service/platform/sdk/CutoverUiDataService.kt`:
- [BLOCKING] wallet/src/de/schildbach/wallet/service/platform/sdk/CutoverUiDataService.kt:3226-3227: Reserve the unshield destination against ordinary payment change
(existing thread: https://github.com/dashpay/dash-wallet/pull/1570#discussion_r4110572828)
This fixes the withdrawal destination through `nextChangeAddressOrNull()` without reserving it or coordinating with ordinary payment change selection. `ShieldedTransferExecutor` passes that address C into `withdrawToCore`, while `SdkL1SendService` independently calls `wallet.sendToAddresses()` with default BIP44 change handling. A user can hide the proving overlay and make a merchant payment before the queued withdrawal reaches L1. If that payment produces change at C, the merchant can identify the other output as change and associate the later withdrawal to C, including its amount, with the customer.
This requires overlapping operations, not simultaneous engine calls: once C is embedded in the withdrawal, observing the merchant payment first cannot update that destination. Binding-generation checks, receive-read serialization, and UTXO reservations do not exclude C from the address allocator. The measured 91-second queue is sufficient for the supported Hide-and-send flow; the finding does not assume the ten-minute UI bound is typical latency.
The disposable-wallet results establish that these outputs belong to the wallet and remain spendable after recovery. Correct balances and sequentially advancing internal indices cannot establish privacy isolation, because a colliding output would still belong to the same wallet and be credited correctly. This is also an introduced regression for active wallets whose base dashj issuance had advanced beyond funded external indices: those newly issued external destinations could not collide with internal payment change. Fixing restored-wallet address reuse does not remove that previously clean case.
Use destination allocation or reservation honored by competing change builders, or coordinate those builders across the pending withdrawal. Tracking dashpay/platform#5276 and documenting the limitation provide no executable protection, and the supplied discussion does not establish a maintainer acceptance of this tradeoff. Permanently disabling unshielding or reverting to advertised receive addresses is not the requested remedy.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Bind self-transfer destinations to the funding wallet at spend time — The accessor now validates the destination generation after resolving ownership, but returns an Address without that identity. ShieldedBalanceServiceImpl.runShieldedWrite subsequently selects readyWalletIdHex, and the CoinJoin drain resolves its funding wallet downstream. A wipe and rebind between these steps can pair an old destination with a new funding wallet. This is a concrete funds-isolation follow-up, but the separate destination and funding-wallet selection already existed at the PR base; it does not invalidate the repaired accessor-boundary check.
- Follow-up: Track a separate issue to carry wallet identity with prepared self-transfer destinations and validate it against the funding wallet during construction or submission.
|
Maintainer acceptance of the unreserved self-transfer destination. The finding is correct and I'm accepting it as a known residual for this PR. Recording that explicitly, since the review has rightly pointed out that nothing in the thread so far does. What I'm accepting. Post-cutover, unshield, paper-wallet sweep and CoinJoin combine take their destination from Why I'm shipping it anyway. The alternatives are worse, and I've had them costed rather than assumed:
The status quo this PR replaces is not neutral either: it sent these destinations to addresses the chain had already paid, which is the defect the PR exists to fix. What happens next. The real fix is upstream and specified: dashpay/platform#5276. The primitive already exists in key-wallet ( When that lands I'll wire it up here and drop the residual. Until then it's documented in What is still unverified, so it's on the record rather than implied: the collision itself is not device-verified — the disposable-wallet run was sequential and shows only that the destinations are correct, credited, spendable and recoverable. The destination-unavailable path could not be induced at runtime, and Thanks for the persistence on this one — the base-behaviour correction and the push for real device evidence both improved the change. |
Fixes SR-03 / D-003 — reproduced in both QA runs.
The defect
After Reset Wallet + restore from the phrase, the Receive screen served
yhAK8CWoeNP9Um9jvqSkLZURQnzenjK3v8again — the address that had already received the 0.11 funding before the reset.Post-cutover the dashj wallet is held. It never sees a block, so its receive key chain's pointer stays wherever the restore left it: index 0 on a fresh restore.
currentReceiveAddress()returned that, andfreshReceiveAddress()walked forward from the same index-0 base through the used range instead of skipping it. That is a correctness defect for the payer and an address-reuse privacy defect for the user.The fix
The engine's own pointer is derived from the SPV scan's used-set, so it skips every address the chain has already paid. This reads it over the FFI —
core_wallet_next_receive_address→ManagedCoreWallet.nextReceiveAddress(0).Deliberately not the SDK's Room
core_addressesmirror. That mirror is a projection the engine writes behind itself (onPersistAccountAddressPoolEntry), so it trails every used-marker by a persistence pass; the engine's own used-set is the only thing that answers "which address has nobody paid yet" at the instant the Receive screen asks. The BIP70refund_tosource was reading the mirror and now takes the same FFI call — the upstream Kotlin plumbing gap documented there closed in the v42int21 AAR (iOS had bound the same FFI all along).Two accessors on
CutoverUiDataService, following the existingsdkBalanceOrNull/sdkSpendableUtxoCountOrNulloverlay idiom:sdkReceiveAddressOrNull()sdkReceiveAddressLive{,Blocking}OrNull()Two decisions worth reviewing:
l1Syncedgate, unlike the UTXO count. Holding back has no safe direction here: the dashj fallback post-cutover is a known-wrong address, not a sound over-estimate.Call sites moved onto the live read
The Receive screen and its QR, the amount-dialog path, the
…OffMainhelpers on bothWalletDataand the neutralWalletDataProvider(so the Coinbase, Uphold, CrowdNode, ExploreDash and Maya deposit addresses follow), Maya's swap source/refund address, the shielded unshield destination, the paper-wallet sweep, and the mixed-funds CoinJoin migration destination.WalletApplication's plain synchronous accessors are overlaid too, so call sites not touched individually still get the engine's answer rather than the frozen one.Two deliberate narrowings
Both documented at the call sites rather than buried here:
freshandcurrentcoincide. The engine tracks used, not issued, and the FFI exposes no index-addressable call, so there is no per-invoice handout. Still strictly better than what it replaces, which was handing out already-paid addresses. It also stops forcing dashj's synchronous full-wallet save on that path (~1.2 s at 215 DashPay friend chains), since no dashj key is issued.WalletAddressFragmentalso reads the dashj wallet directly, but nothing references that fragment — no layout, no navigation graph — so it is left alone.Verification
:wallet:assemble_testNet3Debugsucceeds;ktlintCheckclean.CutoverUiDataServiceTestcases pin the contract: pre-cutover never touches the SDK, post-cutover serves the engine address, it advances on a tx event, a failed read holds the last value, an unavailable read falls back to dashj, and the live read bypasses the cache.wallet+common+maya): 2076 tests, 151 failures — a byte-identical failure set to the pre-change baseline captured on a clean tree. All 151 areUnsatisfiedLinkError: no conscrypt_jni, an environment issue on this machine, not a regression. No new failures.Not yet re-run against the live SR-03 repro on the emulator — worth doing before merge.
🤖 Generated with Claude Code
Summary by CodeRabbit