Repository navigation
fix(upgrade): v12 cutover and sync lifecycle, plus a measured account of what still loses money - #1568
Merged
HashEngineering merged 185 commits intoSep 27, 2026
Conversation
…rocess (MO-995, MO-998) Post-cutover the SDK's L1 (SPV) engine went down on a routine BlockchainServiceImpl teardown and never came back for the rest of the process. Field symptoms on the 12.0.0 QA builds: incoming transactions never arrived (two Coinbase deposits invisible for 2h10m on mainnet), the sync header sat at "syncing", and Buy Credits failed with "pre-broadcast: SPV client not started". platformSyncService.resume() — the only in-process path that re-kicks the SDK engines, and whose own comment says every service (re)start must call it — lived solely inside checkService()'s peergroup-start block, BELOW the `!dashjEngineMayStart` early return. Post-cutover that return always fires, so resume() was unreachable and the engines only ever started from PlatformSyncService.init(), which runs once per process from WalletApplication.finalizeInitialization(). Any teardown that stopped them — release-build shutdown() -> stopSdkEngines(), reached from onTrimMemory's low-memory stopSelf(), the idle detector, or the Android 15 FGS timeout — therefore killed L1 sync permanently. It was a latch, not a transient: the idle detector samples this same engine's progress post-cutover, so a dead engine reads as zero activity and kept tearing the service down once a minute. Debug builds hid it entirely, because shutdown() deliberately keeps the engines warm there. Call resume() from the post-cutover branch of onCreate — beside the SDK-quorum wiring, which already exists to compensate for checkService() never proceeding once the cutover is committed. resume() is idempotent (single-flight bind, idempotent startIfEnabled), so the first service start of a process harmlessly re-kicks what init() started. Scoped to `!dashjEngineMayStart`, so the pre-cutover path keeps its original resume()-at-peergroup-start behavior byte for byte. Regression test: PlatformSyncEngineRestartTest, the mirror of PlatformSyncEngineTeardownTest. The onCreate call site itself is not host-testable, so the tests lock down the contract the fix depends on — resume() brings the engines back after a teardown, repeatedly, including after the full shutdown() path that cancels the sync scope's children. That last case is mutation-verified: swapping shutdown()'s cancelChildren() for syncJob.cancel() fails it and nothing else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…O-995)
Tapping the local-currency option on Enter Amount To Transfer crashed the app:
java.lang.IndexOutOfBoundsException: setSpan (12 ... 7) has end before start
at EnterAmountToTransferFragment.spanAmount(EnterAmountToTransferFragment.kt:185)
at EnterAmountToTransferFragment.formatTransferredAmount(...:172)
Three times in Andrei's 12.0.0 mainnet log (13:30:11, 13:31:47, 13:34:03),
each a foreground process death.
The fiat currency-last branch bounded the small-text/colour span with
`viewModel.inputValue.length`. The fiat branch of `applyNewValue` sets
`fiatBalance = inputValue` only when the input has at most two decimals;
otherwise `fiatBalance` is the 2-dp formatted figure, which is shorter.
Switching DASH -> fiat feeds `formatInput` — an exchange-rate-converted value
with many decimals — through the same path, so `from` (12) overshot `to` (7)
and Spannable.setSpan threw. The currency-first branch above it already used
`fiatBalance` and even guarded the subtraction, so only this branch was wrong.
Use `fiatBalance` as the boundary, matching the text `applyNewValue` actually
returned ("$fiatBalance $symbol").
The deeper hazard is that all three branches take their bounds from separate
mutable ViewModel fields while `applyNewValue` refreshes only the one belonging
to the branch it took (`formattedValue` for DASH, `fiatBalance` for fiat), and
setSpan throws on a bad range rather than ignoring it. So the computation moves
out of the fragment into a pure `amountCurrencySpan()` that returns null for an
empty or out-of-range result: a future field mismatch loses the styling instead
of the screen.
Tests: AmountCurrencySpanTest pins the bounds for each of the three branches
plus the degrade-to-null cases, including the crash's own numbers. Verified by
mutation — dropping the range check fails 3 tests, and reverting the boundary
to another field fails 3.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
QA's walletB upgraded to 12.0.0-sync, ran fine on dashj all morning (fully synced at 08:38), then at 18:20 lost sync entirely: "network is not synced", "setup is incomplete". It ended the 11.5-hour log with ZERO L1 engines and no `L1Shadow phase=` line anywhere. Two independent defects in the UPGRADE cutover seam, both fixed here. GATE 1 — the version-code test now gates the COMMIT, not just the explainer. `commitForUpgradedWalletAsync` committed unconditionally and only consulted `previousVersionCode` afterwards, to decide whether to arm the one-time sync explainer. walletB reached the seam on a SAME-VERSION relaunch (previous code 12000001 on a 12000001 build) — not an upgrade across the cutover boundary at all. The code even logged that its own precondition did not hold and committed regardless. A launch that did not cross the boundary has no business handing over L1; the readiness-gated auto-commit observer owns that decision. Extracted as the pure, host-testable `isPreCutoverUpgrade`. The crossing is LATCHED (`CUTOVER_UPGRADE_BOUNDARY_CROSSED`) because `Configuration.lastVersionCode` is "the version the PREVIOUS LAUNCH ran", not "the version this install upgraded from" — `updateLastVersionCode` overwrites it every startup. So the crossing is visible for exactly one launch, and that is the one launch where GATE 2 cannot yet pass. Without the latch these two gates could never both hold and the seam would never commit again, taking the one-time sync explainer with it. GATE 2 — never hand L1 to an SDK that has never bound on this install. Committing HOLDS the dashj engine, so committing while the bind is broken leaves no engine at all. walletC and walletD (both clean upgrades) show the commit runs 3 and 7 seconds BEFORE the first bind is even attempted — they succeeded on luck, not design. walletB took the identical path with 16 consecutive `KeystoreDeviceLockedException` denials on the lock-bound `org.dashfoundation.wallet.master` alias, and the designed escape hatch (`rollbackForFailedBind`) never fired because it was withheld pending an `ACTION_USER_PRESENT` that never arrived. The seam now requires durable evidence — `SDK_BIND_EVER_SUCCEEDED`, set by `SdkWalletBinder` on the first successful bind. Absent or false reads as never-bound (fail safe, not fail open). Net effect: a healthy upgrade latches on launch 1 and commits on launch 2 (one launch of delay, invisible — dashj is already serving a synced wallet); walletB latches and never commits, staying on dashj instead of being left with no engine. Deliberately NOT a deferred/awaited commit: `BlockchainServiceImpl` resolves its engine gate once at service onCreate and `onCutoverStateChanged` is un-hold-only, so a commit landing mid-launch cannot stop a live dashj peergroup — the "never two live SPV engines" invariant the existing rollback-over-defer comment protects. Declining outright keeps dashj as the single engine for the launch; once a bind succeeds, the seam (or auto-commit) takes it from there. Two pre-existing tests asserted "the commit itself must be unaffected by the notice gate"; that is precisely the behaviour that was wrong, so they now assert no-commit. Seven new cases cover both gates, the latch's two-launch sequence, and the boundary. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…st a new one (MO-995)
Found by running the cutover scenarios on the emulator: after a clean reset,
launch 2 still logged "the SDK wallet bind has never succeeded on this install"
and declined the commit, while the L1 engine was demonstrably running. The
persisted state confirmed it — `cutover_upgrade_boundary_crossed` was there,
`sdk_bind_ever_succeeded` was absent entirely.
Cause: the marker was written inside `sdkService.bindAppWallet(...).also { }`,
which only runs when a NEW SDK wallet is created. Any launch that found the
wallet already bound took the "app wallet already bound" path and never wrote
it — so on those devices the upgrade seam declined forever and the cutover
could never commit. The real-upgrade run passed only because it happened to be
a first-ever bind.
Moved to `noteBindOutcome(failed = false)`, the canonical success signal, whose
contract is already "any pass that leaves boundWalletIdHex set is a success".
Also in this commit:
- CutoverCoordinatorTest fixtures now DERIVE from
`CutoverCoordinator.FIRST_CUTOVER_VERSION_CODE` instead of hardcoding a
release line. The constant moved from 11100000 to 12000000 (the cutover ships
in 12.0.0), which silently turned the hardcoded 11100100 "already cut over"
fixture into a *pre*-cutover value and inverted what two tests asserted.
- scripts/cutover-emulator-test.sh: the emulator harness for the four
scenarios. Three harness bugs were fixed while getting it trustworthy, all of
which produced false FAILs on healthy code:
* `since_mark | grep -q` under `set -o pipefail` — grep -q exits on first
match, tail dies of SIGPIPE, the pipeline reports 141 and a MATCH looks
like a failure. Only bit patterns appearing early in a large window,
which is why later assertions passed and earlier ones did not. Windows
are now written to a file and grepped there, never through a pipe.
* log truncation was unreliable, so a stale line from an earlier run
satisfied a refute. Assertions are now scoped to lines added since a
mark, with a rotation guard.
* `pull_log` fell back to `run-as`, impossible on a release build (not
debuggable), silently leaving an EMPTY file. It now retries with root and
keeps the last good copy rather than clobbering it with nothing.
Plus `show_state`/`require_state` so a contaminated precondition aborts
loudly instead of reporting a meaningless failure.
Emulator results with these fixes, on a real 11.9.1 -> 12.0.0-sync upgrade:
S1 launch 1 declined + latched, no commit PASS
S1 launch 2 committed + explainer armed PASS (real-upgrade run)
S4 trim -> teardown -> engine restarted PASS
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All three produced misleading PASS/FAIL on healthy code: - `wake_unlock` led with `keyevent 26` (power), which TOGGLES: on an already-unlocked device it turned the screen OFF and re-locked it, silently re-creating the Keystore device-locked denial inside scenarios that need a WORKING bind. S1 failed twice for this reason alone. Now wakes with keyevent 224 only, and warns instead of pretending when a keyguard is still up. (`input text` does not reach the keyguard PIN pad either — digits need keyevents 8-11 — so an actually-locked device must be unlocked by hand.) - S3 never called `mark_log`, so `LOG_MARK` stayed 0 and its assertions matched the ENTIRE log history. It reported PASS for in-session recovery purely off lines from an hour earlier. - Added `keep_awake` (svc power stayon + 30min screen_off_timeout): scenarios poll for up to 45s, and an idle screen-off re-locks the device mid-run. Verified results with these fixes, on a real 11.9.1 -> 12.0.0-sync upgrade: S1 launch 1 declines+latches, launch 2 commits+arms explainer 5/5 PASS S2 real Keystore -72 denial: declines, dashj live, not engine-less 5/5 PASS S4 trim -> teardown -> engine restarts PASS S3 BLOCKED: needs a manual unlock between S2 and S3 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The bind-evidence gate lived only in `commitForUpgradedWalletAsync`, which left
the readiness-driven path wide open. Reproduced on the emulator: with every
bind failing on a real `KeystoreDeviceLockedException`, the upgrade seam
correctly declined and `CutoverAutoCommitObserver` committed anyway — FOUR
times, at 16:46:23, 17:57:52, 18:08:33 and 18:13:16:
cutover state DUAL_RUNNING -> READY_OBSERVED on OBSERVE_READINESS (ready=true)
cutover state READY_OBSERVED -> CUT_OVER on COMMIT_CUTOVER (ready=true)
cutover auto-commit: SDK is now L1-primary (dashj held); observer standing down
That reaches walletB's engine-less end state through a different door. The
readiness evaluator has no notion of whether the wallet is bound, so the check
has to sit where the write happens — and there are TWO write sites:
`writeState` (via commitLocked) and `transition` (auto-commit / manual commit).
`refusesCutOverWithoutBindEvidence` is now consulted by both, so the seam, the
fresh-wallet commit and auto-commit all inherit one implementation. Only
CUT_OVER is guarded: ROLLBACK and the wipe reset move AWAY from a committed
state and must never be blocked — that is the escape hatch.
This also closes the clean-install exposure flagged earlier: the fresh-wallet
commit had no bind gate at all, so a new wallet on a denying-Keystore device
would commit, hold dashj, fail to bind, and have nothing but the rollback to
save it. It now stays on dashj instead.
Two existing tests needed their premise stated rather than assumed:
- `commitForFreshWalletSetupAsync_commitsOnTheInjectedScope` is about WHICH
SCOPE runs the commit, so it now supplies the evidence it isn't testing.
- `endToEnd_persistentBindFailure_endsWithDashjAllowed_neverBothHeld` models a
wallet that bound successfully and whose Keystore later started denying, so
the commit is legal there and the rollback is the safety net under test. A
never-bound wallet is refused up front instead.
Three new cases; mutation-verified — removing the `transition()` guard fails
`autoAdvance_refusesToCommit_whenTheSdkBindHasNeverSucceeded` and nothing else.
36 tests in CutoverCoordinatorTest, full :wallet suite green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`input text` does not reach the keyguard PIN pad, so every scripted unlock silently failed and left the device locked — which then denied the master-alias keystore op inside scenarios that need a WORKING bind. S1 failed twice and S3 was unrunnable for this reason. Enter the PIN with digit keyevents instead (KEYCODE_0 is 7, so digit d -> 7+d), after a swipe to raise the PIN pad, and verify the keyguard is actually down afterwards rather than assuming it. Verified on device: lock -> script unlock -> keyguard down. With this, S2 -> S3 runs unattended: S2 locks to force a real Keystore denial, S3 unlocks to test whether recovery happens without an app restart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`noteAppForeground()` was state-only — it reset the retry ladder and left the
actual retry to "the existing triggers". There is exactly one such trigger:
`CutoverUiDataService.awaitBoundWallet()`, the sole caller of `maybeRetry`, and
that loop runs only while the cutover holds dashj with NO bound wallet.
Once the coordinator correctly refuses to commit onto an unbindable SDK
(717d27df9), that state never occurs. So the loop never runs, `maybeRetry` is
never called, the unlock receiver is never armed, and nothing ever retries. The
retry machinery was built to rescue the engine-less state; removing that state
removed its only trigger.
Reproduced on the emulator (scenario S3), all with the device unlocked:
- after a real Keystore denial, unlock + foreground produced ZERO
SdkWalletBinder / SdkBindRetryService lines
- "unlock-heal receiver registered" appeared 0 times in the whole session
- only a full app restart healed it (app wallet already bound…)
which is walletB's shape exactly: it too logged ACTION_USER_PRESENT zero times
across ten hours while Andrei repeatedly opened the app.
So foregrounding now arms the receiver AND drives a pass via the existing
`retryNowInBackground`, which resets the ladder, runs one bind pass, and
consults the rollback. App-foreground is the right trigger because it depends
on neither the cutover state nor a broadcast: the user is looking at the app, so
the device is provably unlocked — the heal condition for a device-locked
keystore denial — and nothing has to survive an OEM's background restrictions
(walletB is a HONOR PTP-N49; MagicOS suppresses exactly that broadcast class).
`noteAppForeground_collapsesTheBackoffWindow` asserted the OLD contract — that
foregrounding runs no pass — so it is renamed and now asserts the new one. Two
new cases pin the gap itself: recovery with no polling trigger at all, and the
receiver being armed by a foreground visit. Mutation-verified — reverting to
state-only fails all three.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…O-995)
`BlockchainServiceImpl` observed `ProcessLifecycleOwner` for foreground/
background transitions. It has never fired once.
`wallet/AndroidManifest.xml` removes `androidx.startup.InitializationProvider`
with `tools:node="remove"` — a deliberate cold-start optimisation (that provider
runs before Application.onCreate on every launch, and removing it also
suppresses WorkManager's default initializer). But that provider is what runs
`lifecycle-process`'s `ProcessLifecycleInitializer`, which is the only thing
wiring `ProcessLifecycleOwner` to activity transitions. Without it the process
owner never leaves INITIALIZED, so onStart/onStop never fire — silently, no
error. Confirmed with aapt2: the provider is absent from the built APK.
Field evidence: across a 27,000-line log with 36 service onCreate()s, "App moved
to foreground" and "App moved to background" appear ZERO times. Consequences,
both pre-existing:
- `isAppInBackground` was frozen at its initial value for every reader
- `SdkBindRetryService.noteAppForeground()` had never once been called, so
8cd47b19b's foreground bind-retry was attached to a dead hook — and the
older state-only version was dead code too
Together with ACTION_USER_PRESENT never arriving on MagicOS, that is why
walletB had zero working recovery triggers across ten hours while Andrei
repeatedly opened the app.
Fix keeps the manifest optimisation and takes the same edges from
`WalletActivityTracker`, which is registered the plain way
(`registerActivityLifecycleCallbacks`, WalletApplication:384), demonstrably
works, and whose `ActivitiesTracker` base already computes the first-started /
last-stopped edges. New `AppForegroundMonitor` exposes them as a StateFlow; the
service collects it. No androidx.startup, no effect on any other library's
initializer.
Not yet device-verified: needs a signed install, then S2 -> S3.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…MO-995) a3dd2904b hung the app-foreground signal off ActivitiesTracker's onStartedAny/onStoppedLast hooks. Those never run: WalletActivityTracker overrides onActivityStarted/onActivityStopped WITHOUT calling super, so the base class's numStarted counter never advances and neither onStartedFirst, onStartedAny nor onStoppedLast ever fires. Verified on device — the new build was confirmed installed (AppForegroundMonitor present in classes3.dex) and "App moved to foreground" still logged zero times. Derive it from visibleActivityCount instead, the counter this class maintains itself and which demonstrably tracks reality (its logState() output is all over the field logs as "visible: N foreground: N"). Note for follow-up: onStoppedLast being dead means `autoLogout.setAppWentBackground(true)` and the zero-minute force-finish broadcast in that method never run either. Not touched here — separate concern, and auto-logout has its own paths. Still not device-verified: needs a signed install, then S2 -> S3. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… out (MO-995) S3's two failing assertions were both wrong in the same way — they looked for log lines that structurally cannot appear in that scenario's window: - "committed in-session" wanted `DUAL_RUNNING -> CUT_OVER` within 45s. But the upgrade seam only runs at process start, so in-session the only route is the readiness-gated auto-commit, which needs MIN_PARITY_STREAK readings at a 10s throttle AND the SDK scan caught up to tip. A missing commit there is correct deferral, not a defect. Moved to a new s3b step that checks the NEXT launch. - "dashj still owns L1" wanted `dashjEngineMayStart=` or `starting peergroup`. Those are logged at service onCreate, and S3 deliberately keeps ONE process alive — that is the whole point of the scenario — so no onCreate occurs. Replaced with `assert_not_committed`, which reads the persisted DataStore state instead of the log. S3 now asserts the actual recovery chain, each link of which was broken until b49ef25b0: the foreground signal fires, it drives a retry, the retry runs a pass, the bind heals, retry pressure clears, and no process restart occurred. Verified on device, all green: S2 5/5 denial -> gates decline -> dashj live -> not engine-less S3 7/7 foreground -> retry -> bind heals IN THE SAME PROCESS S3b 2/2 next launch commits -> SDK L1 engine starts Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…995) The device lock PIN was committed as a literal in the harness. It now comes from the EMULATOR_PIN environment variable with no default, and the unlock helper fails loudly with instructions rather than silently typing an empty PIN. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`onStoppedLast()` has never run. It is reached only from `ActivitiesTracker.onActivityStopped`, and this class overrides onActivityStarted/onActivityStopped WITHOUT calling super, so the base's `numStarted` never advances and none of its hooks fire. Not a recent regression: the anonymous tracker this class replaced (WalletApplication, pre-fae81fefb) had the identical defect, so `autoLogout.setAppWentBackground (true)` and the zero-minute force-finish broadcast have been dead for a long time. Found while tracking down why the app-foreground signal never fired (b49ef25b0 / 2125a73) — same root cause, different victim. Call `onStoppedLast()` explicitly from the branch that does run, next to the AppForegroundMonitor edge. DELIBERATELY NOT FIXED IN THE SAME WAY: `onStartedAny` is equally dead, and it forces a full app restart on the first activity start after any upgrade. Its stated purpose (pushing v6.x installs through the PIN upgrade) is long obsolete, and blanket-adding `super` would switch that on for every 12.x upgrade during the cutover release. Left dormant with a comment saying so, to be enabled on purpose and tested, or deleted along with `myPackageReplaced`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
QA saw "Received DASH -0.0x" after a gift-card purchase. The producer is `BlockchainServiceImpl.notifyCoinsReceived` — the SDK announcer (`CutoverUiDataService`) formats a value it only ever gates positive, so it cannot render a negative amount. Post-cutover every send is authored by the SDK and then committed into the dashj wallet-of-record by `SdkBridgedTransactionFactory` (for the send UI, not for notifications — the BIP70 site at SendCoinsTaskRunner:1075 is the gift-card path and is NOT DEBUG-gated). bitcoinj's `maybeCommitTx` raises `onCoinsReceived` whenever `getValueSentToMe() > 0`, and dashj post-cutover has never synced the funding txs, so the inputs are unconnected: it sees the +change output alone, values the send positive, and delivers it to the received listener. `onCoinsSent` does not fire at all. The guard against that was `correctedNetValue()`, and it was resolved TWICE: once on the wallet-listener thread for the announced amount (correctly negative) and again inside `passFilters` — which runs inside `handler.post`, i.e. on the MAIN thread, where Room throws (no `allowMainThreadQueries`, DatabaseModule:41) and the lookup fails soft to dashj's positive misread. Gate said "received", amount said "-0.0x". Deterministic, not a race, which is why it reproduced. Fix, primary: a transaction this wallet authored is a send however dashj values it. The bridge stamps `purpose = USER_PAYMENT` and `confidence.source = SELF` before the commit, so both are present the instant the listener fires — unlike the tx_display_cache row, which does not exist yet in the window between the bridge commit and the SDK's display-row insert. Fix, secondary: resolve the corrected net ONCE, on the listener thread, and thread it into passFilters, so the gate and the announced value are the same number by construction. Plus a final belt in notifyCoinsReceived refusing any non-positive amount. Pre-cutover this changes nothing: a dashj-authored send is valued negative and already failed the sign test, and a genuine receive is neither self-authored nor negative. Out-of-band receives (direct payment / Bluetooth `receivePending`) still notify — which is why this is a self-authored guard and not a blanket `!dashjEngineMayStart` gate on the listener. NOT FIXED: `handleContactPayments` is reached only from the received path, so a bridged send still feeds `updateFrequentContacts`, and the CrowdNode matchers are still evaluated before the guard (inert — they key on the CrowdNode address). The structural version is one early return at the top of the listener; deliberately left out of this commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…(MO-995) QA saw an ordinary 0.0002 DASH send labelled "CoinJoin Mixing" on BOTH the sending and receiving wallet, while the coinjoin balance was 0 for the whole session — no mixing had occurred. 0.0002 DASH is not a CoinJoin denomination, so amount-matching does not explain it. The label comes from CoinJoinMixingTxSet.tryInclude, which groups a transaction as CoinJoin unless dashj's shape heuristic CoinJoinTransactionType.fromTx returns None or Send — so ANY of CreateDenomination / MakeCollateralInputs / MixingFee / Mixing / CombineDust pulls it in. That heuristic is evaluated against the DASHJ wallet, which post-cutover is held with its balance frozen at the cutover snapshot, while the transaction itself originates from the SDK. A stale wallet is being asked to classify a new transaction, and fromTx depends on wallet context (which inputs are mine, input values, denomination and collateral matching). That is the suspect; nothing currently logs the verdict, so this adds it — bounded to transactions actually being grouped as CoinJoin, not one line per list row. Also fixes a latent crash found while reading the path: `tx.raw as Transaction` was unconditional, but `TxInfo.raw` is `Any?` and is documented as an OPAQUE handle. Post-cutover a TxInfo can be built from the SDK rather than dashj, so a non-dashj payload — or null — threw a ClassCastException out of transaction-list grouping. A payload the heuristic cannot read is not a transaction we can call CoinJoin: exclude it, log it, keep the list alive. Tests: three cases pinning the payload contract; mutation-verified — restoring the unchecked cast fails both crash tests. The misclassification itself cannot be unit-tested (fromTx needs real wallet/UTXO context and there is no WalletEx fixture), which is exactly why the diagnostic exists; the next field report settles which of the five types it returned. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…metadata
QA hit this on a release build:
java.lang.ClassCastException: DaggerWalletApplication_HiltComponents_SingletonC
$FragmentCImpl cannot be cast to UpholdPortalFragment_GeneratedInjector
at Hilt_UpholdPortalFragment.inject
at Hilt_UpholdPortalFragment.onAttach
`integrations/uphold` was the ONLY module in the project setting
`minifyEnabled true` on its own library output. R8 strips the entire
`hilt_aggregated_deps/` package from the library's release artifacts —
measured 5 entries in debug, 0 in release, including
`_org_dash_wallet_integrations_uphold_ui_UpholdPortalFragment_GeneratedInjector`.
That package is how the APP module's `hiltAggregateDeps` task discovers
@androidentrypoint classes in library modules. Without it the generated
component never implements the interface. The interface itself survives (it
lives in a package the existing `-keep class org.dash.wallet.integrations.uphold.**`
rule protects), which is why the failure is a ClassCastException rather than a
missing class.
Measured on the app's generated component, before -> after:
_testNet3Debug uphold=2 total=154 (unchanged)
_testNet3Release uphold=0 total=152 -> uphold=2 total=154
Removing it costs nothing: the app module already sets `minifyEnabled true`
(wallet/build.gradle, release), so these classes are still shrunk and
obfuscated in the final APK — once, at the app level, with the whole program
visible. Minifying a library separately only removes metadata its consumers
still need. `consumerProguardFiles` already ships this module's keep rules to
the app, which is where they belong.
NOT a regression from this branch: `minifyEnabled true` is present on master
too (added in #934), so master release builds carry the same defect. Most
likely nobody had opened the Uphold portal on a master release build. Worth
confirming with QA — if it reproduces there, it has been shipping broken.
Verification note for anyone re-checking this: partial task invocations
mislead. `aar_main_jar` (what the app compiles against) needs the library's own
release rebuild, and the app component is written by
`hiltJavaCompile_<variant>` — which neither `hiltAggregateDeps_<variant>` nor
`ksp_<variant>Kotlin` runs, even with --rerun-tasks. Only a full
`assemble_<variant>` regenerates it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
QA screenshot: "Synced / Block headers 2,532,259 / Block filters — /
Masternode list height 2,532,250 / ChainLock height 2,532,258". Every row held
its value except filters.
`mergeL1SyncDetail` already has a restart-sawtooth guard: the engine reports
all-zero heights through IDLE/CONNECTING for minutes after a process restart,
so percentage, headers, mnlist and chainlock are all backstopped rather than
collapsing to zero. Filters were taken raw:
percentage = if (idleOrConnecting) state?.percentageSync ?: 0 else ...
headerHeight = maxOf(progress.headerHeight, state?.bestChainHeight ?: 0)
mnListHeight = maxOf(progress.mnListHeight, state?.mnlistHeight ?: 0)
chainLockHeight = maxOf(sessionChainLockHeight, state?.chainlockHeight ?: 0)
filterHeight = progress.filterHeight <-- no guard
filterTarget = progress.filterTarget <-- no guard
and `NetworkMonitorActivity.formatHeights` renders "-" exactly when height AND
target are both <= 0. The field log carries the matching state:
`L1Shadow phase=IDLE 0.0% headers 0/0 filters 0/0`.
BlockchainState persists no filter height — dashj has no filter pipeline, so
the row never carries one — so the backstop is the last non-zero pair seen in
this process, held in L1SyncStatusService and passed into the (still pure)
merge function. On a cold start with no reading yet, "-" remains the honest
answer.
Scoped to IDLE/CONNECTING deliberately. Outside that window a filter height
moving BACKWARDS is real: the DashPay coreHeight backfill legitimately rewinds
the scan (observed dropping 1,543,144 -> 1,252,305 in the field), and a blanket
max() would mask it and report a position the engine no longer holds. There is
a test for that case.
Cosmetic only — no sync behaviour changes. Three tests; mutation-verified,
reverting to raw filters fails the idle case and nothing else.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Andrei, MO-995 comment 91138 #2: "After upgrade finished and network was resync (I assume it happened exactly 24 hours later), I received again" — the one-time upgrade sync explainer, whose own copy says "This happens only once, after this update". From the field log (2026-09-02, prod, 11.9.1 -> 12.0.0-sync): 07:31:54 PACKAGE_UPDATED — the upgrade launch 07:31:56 declining to commit the cutover: the SDK wallet bind has never succeeded on this install 07:32:10 cutover auto-commit observer started ... 8h29m of silence, shadow at phase=SYNCED 100.0%, parity "first sustained MATCH streak complete" ... 17:31:54 cutover state DUAL_RUNNING -> CUT_OVER (upgraded-wallet launch) 17:31:54 upgrade cutover: one-time sync explainer armed The notice was already pending at 07:31 and the user acknowledged it; the seam then armed it a second time ten hours later. CUTOVER_UPGRADE_NOTICE_PENDING cannot guard against this: the sheet sets it back to false on acknowledgment, which an arming site cannot tell apart from never-armed. And the arming site is reachable more than once per install — GATE 1 passes on the durable CUTOVER_UPGRADE_BOUNDARY_CROSSED latch, so any rollbackForFailedBind -> re-commit cycle arms it again. So add CUTOVER_UPGRADE_NOTICE_EVER_ARMED, a marker that is never cleared, and route the arming through armUpgradeNoticeOnce(). The commit itself is untouched — only the explainer is suppressed. An unreadable latch suppresses too: a missed explainer is not the user-visible defect. Why the commit lands that late is structural, not a glitch: the bind-evidence gate cannot pass on the upgrade launch (the bind runs after the seam), so the commit always defers to a later process start. Bounding that would mean changing which engine owns L1 mid-launch, which the "never two live SPV engines" invariant forbids. Left as-is; this makes the deferral harmless to the user instead. Also: CutoverAutoCommitObserver is the in-launch path that would have closed the ten-hour gap, and it ran 8.5 hours emitting exactly one line. Both of its non-committing paths returned early in silence, so the log cannot say whether the stability gate never armed or the readiness policy kept refusing. Add a 5-minute-throttled "still waiting: <reason>" line so the next field log answers that. Tests: 3 added to CutoverCoordinatorTest (39 total, all green), each mutation-verified — dropping the latch check, swapping the write order, and inverting the unreadable-latch fallback each fail exactly one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`startIfEnabled`/`stop` tear down and relaunch four long-lived loops —
progress monitor, parity probe, watchdog, wallet-event tap — and nothing
recorded how long the engine was actually down between them.
That cost real time on MO-995. In the 2026-09-02 field log:
10:38:00 BlockchainServiceImpl - idling detected, stopping service
10:38:00 L1Shadow progress monitor cancelled
10:38:00 L1 shadow sync stopped
... 5h28m, no line from anything ...
16:06:27 L1Shadow phase=IDLE 0.0% headers 0/0 filters 0/0
`progress` is a StateFlow fed by the cancelled monitor, so it froze at its
last value and its consumers simply stopped being called.
CutoverAutoCommitObserver was armed and waiting that entire span and
emitted nothing — which is why the cutover fell through to the next
process start, ten hours after the upgrade. Establishing that meant
diffing two timestamps 12,000 log lines apart.
So report the gap on the start that ends it. A stop is only a problem if
nothing restarts, and "nothing restarted" has no log line by construction
— the thing that would log it is the thing that did not run. Measuring on
the NEXT start needs no timer and no watchdog:
L1ShadowLifecycle STOPPED after 3h1m up; all four loops torn down ...
L1ShadowLifecycle RESUMING after 5h28m down (teardown #3 this process)
WARN past LONG_ENGINE_DOWNTIME_MS (10 min) because a routine idle-detector
bounce is seconds to minutes — the three real teardowns in that log were
~26s, ~3min and 5h28m, and a WARN on every bounce is a WARN nobody reads.
Deliberately narrow: instrumentation only, no behaviour change. The
underlying fragility (one watchdog restart per PROCESS, while the shadow
stops and starts several times per process) is left alone — and this
outlives the dashj retirement (MO-992), where these loops stop being a
debug shadow's and become THE engine's, so an unnoticed teardown goes
from a lost readiness signal to a wallet with no L1 at all.
Tests: 2 added, both mutation-verified — dropping the minutes from the
hours format and raising the threshold past the field outage each fail
exactly one. Full :wallet suite green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…O-995) Both from CodeRabbit on PR #1555. 1. The harness printed FAIL and exited 0. `assert_log`, `refute_log` and `assert_not_committed` each end in a `printf`, so the function returns its status — 0 — and an automated run could call a failed cutover scenario a success. Added a FAILURES ledger and an EXIT trap. NOT an `exit 1` at the failing assertion, as suggested: each scenario is a SEQUENCE of assertions and the diagnostic value is in seeing all of them. S3 asserting "bind failed" then "dashj came back" then "state not committed" tells you WHERE the chain broke; aborting at the first FAIL reports only that it broke. Assertions still print and continue; only the exit status changes. 2. CodeRabbit also asked to revert FIRST_CUTOVER_VERSION_CODE from 12000000 back to 11100000. Not doing that — 12000000 is correct, the cutover ships in 12.0.0 (wallet/build.gradle versionName 12.0.0), and reverting would classify every 11.10–11.25 build as already cut over and silently deny those users the one-time sync explainer. But the inconsistency it detected was real, and one instance was a bug: `fake_pre_cutover_previous_launch` hardcoded its own copy of the boundary (`-lt 11100000`). A device last run on 11.10–11.25 — which the APP counts as pre-cutover — was judged post-cutover by the script, so it overwrote a genuine `last_version` with a faked one, defeating the "leave the truth alone" branch the surrounding comment describes. The script now reads the boundary from one variable documented to track the constant. The rest was stale prose: KDoc and comments named "11.10" in eight places after the boundary moved to 12.0. Version numbers are now stated only where the constant is defined — prose that names a release goes stale silently, and this boundary has already moved once. Also dropped an orphaned KDoc block left over from rederiving a test fixture. No behaviour change in the app; the only executable change is the script. Full :wallet suite green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…995) QA on 12000003: "one time sync not started". A brand-new wallet synced on dashj instead of the SDK, so the SDK's fast initial sync never ran. My own bug, from 36792cc ("refuse EVERY path into CUT_OVER without SDK bind evidence"). I put that guard inside commitLocked, which the FRESH-WALLET commit shares with the upgrade seam. On a fresh install there is no bind evidence BY CONSTRUCTION — the commit is what routes the launch, and the first bind pass only runs once platform sync starts, after it. So the guard could never pass there. Field log (2026-09-03, prod 12.0.0-sync/12000003, new wallet) — the bind succeeding three seconds AFTER the refusal is the whole bug in one line: 07:10:02 successfully created new wallet 09:29:06 declining to commit the cutover: the SDK wallet bind has never succeeded on this install 09:29:06 Phase 5d cutover gate: dashjEngineMayStart=true 09:29:09 app wallet bound to new SDK wallet d992760a… The original author had documented exactly this, in rollbackForFailedBind: "the fresh-wallet commit CANNOT wait for the first successful bind, because the commit IS what routes the fresh-wallet launch ... So the commit stays immediate and THIS is the escape hatch." I read that KDoc, then made "every path" literally every path anyway. So commitLocked now takes requireBindEvidence explicitly — true for the upgrade seam, false for fresh-wallet setup. No default: every call site states its intent. A failing bind on a fresh wallet is handled the way it was designed to be, by rollbackForFailedBind (wired at SdkBindRetryService:133), which restores dashj. Deferring instead cannot work: a deferred commit lands mid-launch with the dashj peergroup already up, i.e. two live SPV engines. The guard is UNCHANGED on the paths where it can be satisfied — the upgrade seam (walletB's engine-less HONOR state) and the readiness auto-commit. The refusal log now names which path refused; not knowing that cost time reading this report. Tests: the old freshWalletCommit_refusesWithoutBindEvidence asserted the regression, so it is replaced by a pin on the corrected behaviour, plus a new test holding both halves of the asymmetry together. Mutation-verified in both directions: restoring the guard on the fresh path fails exactly the new pin, and disabling it everywhere fails 6 tests. Full :wallet suite green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
QA reported "crash on openning". It actually fired while the app sat
idle, 12 hours before the wallet was next opened — the exit reason on the
following launch is what they saw the aftermath of:
PROCESS EXIT REASON: CRASH(4) — previous process died 12h 6m ago
The crash, 2026-09-02 18:48:44, prod 12.0.0-sync:
18:48:44 idling detected, stopping service
18:48:44 .onStartCommand(Intent { … (has extras) })
18:48:44 .onDestroy()
18:48:44 onStartCommand waiting for onCreate to complete...
18:48:44 CrashReporter - crashing because of uncaught exception
android.app.RemoteServiceException$ForegroundServiceDidNotStartInTimeException:
Context.startForegroundService() did not then call Service.startForeground()
A start delivered by startForegroundService() arms a system promise: call
startForeground() within a few seconds or the process is killed. That
call sat INSIDE onStartCommand's serviceScope.launch, behind
onCreateCompleted.await() — so it was satisfied only after the service's
entire async init, and not at all if the service was being torn down
instead. Here an AlarmManager PendingIntent.getForegroundService
(WalletApplication:1689, rescheduleService) fired just as the idle
detector was stopping the service, and the two interleaved.
Fixed by calling it synchronously in onStartCommand, before the
coroutine. Safe: Android guarantees onCreate() has RETURNED before
onStartCommand() runs, and Hilt injects notificationService in
super.onCreate() (line 1833), so the notification can be built. The
onCreateCompleted latch is about this service's own async init, which the
FGS deadline does not wait for — the two were conflated.
Note on scope: ea506f9 (mine, earlier in this branch) added SDK
engine-start work to onCreate, which lengthens async init and so widens
this window. The defect predates it, but that change likely made it more
reachable.
The condition is extracted as the pure carriesForegroundStartPromise() so
it is testable at all — the bug was a MISSED call, and that is only
testable if the decision to make it is separable from making it; the
service lifecycle is not reachable from a host-JVM test. Kept narrow so
the fix cannot over-promote: an ordinary startService() start, absent
extras, and a null-intent system redelivery must all stay out of the
foreground. Mutation-verified both ways — promoting unconditionally fails
3 tests, never promoting fails 1.
Not addressed: the underlying stopSelf()/onStartCommand race. Satisfying
the promise makes it survivable, but the idle detector can still stop a
service that a start command has just asked for. stopSelfResult(startId)
is the usual answer and is a behaviour change, so it is left separate.
Full :wallet suite green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…1022) QA on 12000004, both devices: "I still don't get dialog after upgrade, that one time resync is needed." It was never armed at all — not late, never. Two facts combined. The upgrade seam cannot commit on the upgrade launch (GATE 2 wants bind evidence and the bind lands seconds later), so it returned before reaching the arming. The commit then happened on the READINESS auto-commit path — which had no arming logic whatsoever. Field log (2026-09-04, prod 12000004, SM-A536B), the whole story: 17:10:59 declining to commit (upgraded-wallet launch): bind has never succeeded 17:11:02 app wallet bound to new SDK wallet a60ed232… 18:06:02 cutover state DUAL_RUNNING -> READY_OBSERVED on OBSERVE_READINESS 18:06:02 cutover state READY_OBSERVED -> CUT_OVER on COMMIT_CUTOVER 18:06:02 cutover auto-commit: SDK is now L1-primary (dashj held) Committed, and no explainer. So the arming belongs to the COMMIT, not to one particular caller: armUpgradeNoticeIfUpgraded() is now called from both commitLocked and transition, gated on the durable CUTOVER_UPGRADE_BOUNDARY_CROSSED latch (so a fresh install is never told its wallet was upgraded) and on the existing once-ever latch. The fresh-wallet suppression moved in with it, since that path shares commitLocked. That also means my previous commit (210e50c, "don't show the notice twice") was fixing a double-arm on a path that in the normal upgrade flow never fires. The once-ever latch it added is still what keeps this idempotent now that three paths can arm. Same log also answers the question left open in 3e978df's message — why the auto-commit observer never committed. It commits fine; in the earlier report it was starved because the idle detector had torn the shadow down. Here it took 55 minutes of waiting and then worked. Every log line in this area now names the committing path, which is what made this diagnosable at all. Tests: 2 added — the auto-commit path must arm, and must NOT arm on an install that never crossed the boundary. Mutation-verified: removing the arming from transition fails exactly the first. Also made two test mocks faithful — noticeCoordinator's evidence collector was unstubbed (so the readiness path could not be driven through it), and the boundary latch was a constant-false stub where production writes it before committing. Full :wallet suite green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…022) QA on 12000004: "Block filters / ChainLock Height are shown as '-'", after I had already claimed this fixed in 786ebea. That fix had a hole: its backstop is the last non-zero filter pair seen IN THIS PROCESS, which is empty on a cold start — exactly when the Network Monitor gets opened. Field log (2026-09-04, prod 12000004, SM-A536B). App opened 19:13:07, Network Monitor checked immediately: 19:13:14 L1Shadow phase=IDLE 0.0% headers 0/0 filters 0/0 wallet 2533349 19:14:14 L1Shadow phase=SYNCED 100.0% headers 2533366/2533366 filters 2533366/2533366 A full minute of "-" before the engine reconnects. Note `wallet 2533349` on the very line where filters read 0/0. That is the engine's committed wallet cursor, seeded at shadow start from the SDK's DURABLE watermark (L1ShadowSyncService.startIfEnabled → sdkWalletSyncedHeight), so unlike lastKnownFilter* it survives a process restart. It is not a proxy: the committed wallet cursor IS the filter-scan position — that is why blockPipelineLagging measures the scan with it. So it becomes the cold-start floor. My comment in 786ebea asserted "nothing persists a filter height"; that was simply wrong, and the value was already on the progress object being read. The target takes the same floors, so the pair can never render as height > target. Deliberately not the persisted header tip: that would report a target the filter pipeline never received. ChainLock: NOT changed. Andrei's phrasing groups it with filters, but the evidence says it renders — BlockchainStateDataProvider already max-guards chainlockHeight (a 0 never clobbers the row), clearSdkDerivedState does not touch it, and his own earlier screenshot (MO-995 comment 91138) shows ChainLock at 2,532,258 while filters read "—". Changing it would mean inventing a consensus height from the scan cursor, which would be a false claim. Needs his screenshot to confirm before touching. Tests: 1 added for the cold-start floor, asserting the pair invariant too. Mutation-verified. The existing "nothing known yet → - is honest" test still holds: with no cursor either, "-" remains correct. Full :wallet suite green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
QA: Buy Credits failed because the cutover was not committed. It wasn't
committed because the SDK L1 scan could never reach the tip, and it could
never reach the tip because the engine was killed on every backgrounding.
override fun onTrimMemory(level: Int) {
if (level >= TRIM_MEMORY_BACKGROUND) { // 40
log.warn("low memory detected, stopping service")
stopSelf()
TRIM_MEMORY_BACKGROUND does not mean low memory. It means "your process
has gone onto the LRU background list" — the user left the app. And the
signals that DO mean running-low, TRIM_MEMORY_RUNNING_LOW (10) and
RUNNING_CRITICAL (15), are numerically BELOW 40, so they were ignored
entirely. The scale is not ordered by severity, so `>=` cannot express
the policy at all: the test was wrong in both directions and the log line
misreported what it had detected.
Field log (2026-09-06, testnet 12000004, HONOR PTP-N49, 384/512MB heap) —
the ordinary backgrounding sequence, two seconds apart:
10:47:24 onTrimMemory(20) called <- UI hidden
10:47:26 onTrimMemory(40) called <- on the LRU list
10:47:26 low memory detected, stopping service
10:47:26 .onDestroy()
10:47:50 onDestroy() cleanup is taking longer than 5 seconds
10:48:42 low memory detected, stopping service
That wallet had upgraded from 11.9.1-scan and resolved birthHeight to 0
(birth time 2018-08-09 -> checkpoint 3456, minus a 4032 margin, clamped),
so it faced a from-genesis scan of 1,548,486 testnet blocks — about four
hours at the observed rate of 31k filters per five minutes. It never got
four uninterrupted minutes: three engine restarts in the five logged.
scanCaughtUpToTip never held, the observer sat at "still waiting: streak
0/5", and Buy Credits had no SDK-owned L1.
Now stops only on the two levels that mean imminent death:
TRIM_MEMORY_COMPLETE (80, top of the LRU kill list) and
RUNNING_CRITICAL (15, the system is already killing background
processes). MODERATE (60) is deliberately excluded — mid-LRU is not
imminent danger, and a long L1 scan is exactly the workload that has to
ride it out.
Related but distinct from ea506f9 earlier in this branch: that made the
engine RESTART after a teardown, which is why the scan resumes rather
than dying outright. It did not stop the spurious teardown, and for an
hours-long scan the thrash is nearly as costly.
Tests: the predicate is extracted as a pure Int function and every
documented level is pinned, because the numbers are not ordered by
severity and a future `>=` rewrite must fail loudly. Mutation-verified:
restoring `>= TRIM_MEMORY_BACKGROUND` fails 4 of 5, including the
stops-on-critical case. Full :wallet suite green.
Not addressed: the four-hour from-genesis scan itself, and the >5s
onDestroy cleanup that compounds each restart.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
grpc-netty-shaded's Utils.isEpollAvailable() looks up io.grpc.netty.shaded.io.netty.channel.epoll.Epoll.isAvailable() via reflection. R8 kept the class but stripped the method, so the probe threw NoSuchMethodException instead of returning false, and NettyChannelBuilder's static initializer died with ExceptionInInitializerError the first time PlatformHealthProbe constructed a DAPIGrpcMasternode in a release build. Keep the shaded epoll package so the probe returns false on Android and grpc falls back to NIO as it already does in debug builds. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Username creation crash-loops on release builds. 11 crashes on a HONOR
PTP-N49 in one hour, every one identical:
java.lang.ExceptionInInitializerError
at DAPIGrpcMasternode.<init>(DAPIGrpcMasternode.kt:52)
at PlatformHealthProbe.fetchDapiNodeCoreHeight(PlatformHealthService.kt:129)
Caused by: IllegalArgumentException: Class NioSocketChannel does not have
a public non-arg constructor
at NettyChannelBuilder.<clinit>(NettyChannelBuilder.java:84)
Caused by: NoSuchMethodException: ...channel.socket.nio.NioSocketChannel.<init>
NettyChannelBuilder's static initializer wires its transport entirely by
REFLECTION, so R8 sees no reference to the members it needs and strips
them. Each strip becomes an ExceptionInInitializerError that kills the
process the first time anything constructs a DAPIGrpcMasternode. Two were
hit in consecutive release builds:
1. 12000005: Utils.isEpollAvailable() does Class.forName(...epoll.Epoll)
.getDeclaredMethod("isAvailable") — class kept, method stripped, so
the probe threw instead of returning false. Fixed by 7c2aa22.
2. 12000006: with epoll now correctly false, <clinit> falls through to
`new ReflectiveChannelFactory<>(NioSocketChannel.class)` on the NEXT
line, which does getConstructor() — and that no-arg constructor had
been stripped.
Targeted rules are the wrong shape: each one only reveals the next
stripped member, and the set depends on the netty build, the device (epoll
vs NIO) and the grpc version. So keep the package wholesale.
Cost, measured on the _testNet3 release APK: 26,542 methods / 1.9 MB of
dex for io.grpc.netty.shaded, against a 233 MB APK — under 1%, and the app
is already multiDex. Cheaper than a crash loop.
Verified by building the release variant and reading the dex back rather
than by reasoning about R8:
apkanalyzer dex code --class ...NioSocketChannel -> .method public constructor <init>()V
apkanalyzer dex code --class ...epoll.Epoll -> .method public static isAvailable()Z
Both members now survive. No unit tests: R8 does not run for the JVM test
variant, so a release build is the only thing that can check this.
This one crash accounts for all three QA reports: the username-creation
crash, the username-creation FAILURE (CreateUsernameActivity recreated
after each death, so the flow never completes), and MO-973's "lock screen
appears after clicking Continue" — Continue navigates to
RequestUsernameFragment, whose onViewCreated fires checkNetworkHealth(),
and the lock screen is Android relaunching the killed process. Seven
times: activity created -> crash 3-4s later -> WalletApplication.onCreate
in the same second -> show lock screen true -> fingerprint prompt.
NOT the real fix. Netty is present only because the legacy Java
dapi-client uses it for transport; the Kotlin SDK does not. Also reachable
from TopUpRepository.getTransaction, IdentityRepository's credit balance,
and PlatformSyncService.reportNetworkStatus — which is why this is a keep
rule and not just a fix to the health probe. Still to do: catch Throwable
in probe() (a static-initializer Error escapes its `catch (Exception)`
today), and retire the legacy client.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
probe() caught only Exception. The legacy gRPC stack fails with an ERROR: ExceptionInInitializerError out of NettyChannelBuilder.<clinit> when R8 strips a member its static init reaches by reflection. That sailed past this catch, past the caller's bare try/finally (RequestUserNameViewModel.checkNetworkHealth has no catch at all), out of the coroutine, and killed the process — 11 times on one HONOR PTP-N49. The probe feeds ONE advisory warning row on the username screen, and its own KDoc says it must never gate the submit button. It should not have been able to do that under any circumstances. a772bdc keeps R8 from stripping the member. This is the other half: whatever breaks underneath an advisory probe next, it degrades to UNKNOWN instead of crashing. The keep rule buys immunity to a known trigger; this buys survivability of an unknown one. Two details that matter more than the widened catch: - CancellationException is rethrown. A blanket catch(Throwable) that swallowed it would break structured concurrency — the screen closing mid-probe would leave the caller's coroutine looking successful. - A non-Exception Throwable logs at ERROR with the full stack, not just toString(). Degrading silently would trade a loud crash for an invisible one, and it was precisely the stack in a QA report that made this diagnosable. The next stripped member should still be obvious in a log, just not fatal. The KDoc's "the probe never throws" claim is now true, and says what it excludes. Tests: 4 added. The load-bearing one asserts probe() RETURNS at all — under catch(Exception) it does not fail an assertion, it dies with the Error, exactly as the app did. They force Constants.SUPPORTS_PLATFORM true and restore it after: it is a mutable static set from a native-ABI check, so it is false under a JVM test and every assertion would otherwise pass vacuously on probe()'s first line. Mutation-verified both ways: reverting to catch(Exception) fails 2, dropping the cancellation rethrow fails 1. Full :wallet suite green. Still open: the probe makes a legacy-dapi-client gRPC call at all (PlatformHealthService.fetchDapiNodeCoreHeight). The value it wants, ResponseMetadata.core_chain_locked_height, rides free on every platform response but is not exported through the Kotlin SDK's JNI surface — so removing netty from the live path is legacy-client retirement work, not a bug fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…O-972)
Two QA reports on 12000007, two different failures, and neither log could
say what actually went wrong.
MO-973 (SM-A536B, shielded): the identity was created, then DPNS name
registration failed three times with
IllegalStateException: username registration did not complete
(retryable): pre-broadcast identity-key validation failure
That reason is a message-match on the FFI's "Invalid identity data", and
the label is the CONTACT-REQUEST reading of it — a missing
ECDSA_SECP256K1 encryption key. It was reported verbatim for a DPNS
registration, which needs no encryption key. The message is genuinely
overloaded: the invitation amount-cap rejection arrives with the same
prefix ("Invalid identity data: invitation amount ... exceeds the cap"),
which InviteCreationFailureTest has been pinning all along. So the label
named a cause nobody had established.
Worse, the engine's own message never reached the log at all:
RestoreIdentityWorker threw via `error(...)`, which builds an
IllegalStateException WITHOUT a cause, so `result.cause` was dropped on
the floor. BaseWorker does log.error(msg, e), so a cause WOULD have
printed as a "Caused by:" chain — there just wasn't one. The real reason
was unrecoverable from the report.
MO-972 (HONOR PTP-N49, transparent): reported as
signing failure (pre-broadcast): Keystore auth window expired
11:49:16 SendCoinsTaskRunner - authenticate with biometric
11:49:17 transparent identity funding rejected pre-broadcast
One second. The window had not expired — the label asserted a timeout
nobody measured, and sent diagnosis the wrong way. The raw error is
"Generic Error: User not authenticated": the Keystore refused to treat a
fresh biometric as satisfying the identity key's auth gate. On the
Samsung the same flow gets PAST signing and fails later, differently, so
this is device-specific — the same OEM Keystore defect family as the
false-locked master alias, on the auth-gated identity alias that
dashpay/platform#4643 explicitly does not cover (#4060's DEVICE_BOUND
policy is the remedy). Nothing to fix in the wallet beyond not lying
about the cause.
So:
- both reasons now carry the engine's message verbatim;
- the auth reason states the refusal and offers expiry as one
POSSIBILITY rather than a fact;
- RestoreIdentityWorker and CreateIdentityService's invite path throw
with `result.cause` attached, so the "Caused by:" chain reaches the
log.
Checked the coupling before changing the strings:
classifyInviteCreationFailure matches over the reason AND the whole cause
chain, so REJECTED/UNREACHABLE verdicts are unchanged — the cause still
carries "Invalid identity data". Its two test literals are updated to
mirror the new production strings, with that coupling pinned so the next
reason-string edit fails loudly instead of silently reclassifying invite
failures.
Tests: 2 added. One asserts two different "Invalid identity data"
failures no longer read alike (missing-encryption-key vs amount-cap) —
the whole point of the change. One asserts the auth reason does not claim
an expiry as fact while staying recognisable. Both mutation-verified by
restoring the old labels. Full :wallet suite green.
This is diagnosis, not a fix: MO-973's actual cause is still unknown and
the next field report is what will name it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two CodeRabbit findings on #1555, both confirmed against the code. CutoverCoordinator: the cutover boundary crossing is computable on exactly one launch — `Configuration.lastVersionCode` is overwritten at every startup — so when GATE 1's `set(CUTOVER_UPGRADE_BOUNDARY_CROSSED)` threw, the crossing existed only as a local `val` and the explainer was lost permanently. This launch's `armUpgradeNoticeIfUpgraded` reads the STORE, not that value, so it saw `false` and returned; every later launch computed `crossedNow == false` with nothing on disk to recover from. The commit itself still happened, so the symptom is a user who crossed the boundary silently never getting the one-time sync explanation — MO-1022's symptom, reached by a different route. The failure is now held in memory for the launch that observed it and the persist is retried when the cutover commits. That covers the transient case fully; if the store never accepts the key, arming still proceeds from memory, which is all that can be salvaged — the explainer's own flags need the same store, and armUpgradeNoticeOnce already logs when they fail. L1SyncStatusService: the IDLE/CONNECTING filterTarget took every floor the height took EXCEPT the height's own raw value, so an engine snapshot reporting currentHeight > targetHeight rendered inverted (2533400/2533349 — a row reading past 100%). Minor, and the non-idle branch passes such a snapshot through unchanged anyway, but the comment directly above claimed the pair "can never render as height > target" and that was not true. Note CodeRabbit's stated mechanism is wrong: both fields are read off the same `data.filters` snapshot in `toShadowSyncProgress`, so they cannot drift apart — the gap needs an already-inverted snapshot, not a partial update. Comment corrected to say what actually holds. Three regression tests, each verified to fail against the unfixed code: a dropped latch write that recovers, one that never persists, and an inverted engine snapshot. Full wallet suite green (1964 tests). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…a straggler the next start must not overlap
cancel() makes the deferred inactive at once, but a bring-up inside a
native call cannot observe the cancel until that call returns — and
`takeIf { it.isActive }` rejected exactly the straggler a stop() had
retained after its bounded join gave up, so the next start created a second
bring-up alongside it. The straggler is now recognised by "not completed":
a cancelled one is waited for up to the bring-up budget, and if it is still
running, SPV starts without a new bring-up and the straggler stays recorded
until it ends. The stop's join bound is injectable for the test.
Test: nextStart_doesNotOverlapACancelledBringUpStillInsideItsNativeCall —
the fake's bring-up ignores its cancel until a gate opens; caller cancelled,
stop times out, the next start runs no second bring-up and starts SPV, and
once the native call returns a later start runs one fresh bring-up.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…s nothing The identity check in classifyAndAnnounce compares against a value the failure collector itself wrote, so it cannot see a success the other collector delivered while this failure waited on outcomeMutex — the success cleared the blocker, and the late failure republished it and its notification, with the null failure emission after it a no-op. Under the lock the classifier now also consults the binder's live bindRetryPending: false means the bind has since succeeded, and the failure is dropped. Test: staleBindFailure_arrivingAfterTheBindSucceeded_publishesNothing. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ime that issued it The kick waits on the bind on the long-lived syncScope and checked only isCleaningUpNow — teardown in progress, not whether the requesting foreground-service lifetime still exists. For a wallet without a DashPay identity shutdown() skips both identity-gated cancelChildren branches, so the kick survived teardown and, once the cleanup flags cleared, started L1 for a service that was gone. shutdown() now bumps a lifetime token and cancels the kick; the kick re-checks the token after the bind join and starts nothing if the lifetime moved. stopSdkEngines() cancels it too. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… a percent the fixture already holds Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
thepastaclaw
suggested changes
Sep 24, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Contributor
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the findings and all 20 prior identities against head 459a253: 19 prior findings are fixed, and the delayed-alarm foreground-recovery request is withdrawn. Two current findings remain: wallet wipe does not cancel the newly distinct restart alarm, and the locked-device emulator assertion rejects the intended bind deferral. Shell syntax validation passed and the worktree remains unchanged; Android unit tests could not initialize because no usable Java runtime is available, with the installed Homebrew JDK permission-denied.
🔴 1 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); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
normalbygpt-6-astra(effort low) — The diff is large and intricate but changes SDK cutover orchestration, bind retries, sync lifecycle, persistence timing, and diagnostics rather than funds movement, cryptography or key handling, consensus, peer deserialization, or storage migrations; the documented money-loss defects remain unfixed upstream. - 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 high); agentphase2-reviewer,gpt-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/BlockchainServiceImpl.kt`:
- [BLOCKING] wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt:1252-1254: Cancel the separately identified restart alarm during wallet wipe
The restart alarm now uses request code 1, but destroyWalletFiles() still calls cancelScheduledStartBlockchainService(), which cancels only the periodic PendingIntent with request code 0. An interrupted replay can therefore arm this repeating 15-minute alarm and a subsequent wallet wipe leaves it scheduled. Where background foreground-service starts are permitted, it can launch BlockchainServiceImpl after the wallet has been removed: onCreate promotes the service, then the wallet-null branch returns without completing onCreateCompleted or registering the idle receiver. The surviving restart alarm also bypasses the periodic scheduler's inactivity backoff. Preserve the distinct alarm identities, but cancel both during wallet wipe and define when the recovery alarm is retired. Add regression coverage for interrupted replay → restart alarm armed → wallet wipe.
In `scripts/cutover-emulator-test.sh`:
- [SUGGESTION] scripts/cutover-emulator-test.sh:456-457: Accept the locked-device deferral in the S2 assertion
S2 locks the secure screen but still requires the SDK's 'Keystore denied' message. The new first-bind path checks deviceProvablyLocked() before invoking the unlock provider or SDK wallet creation; when the device is provably locked, it logs 'SDK bind deferred: the device is locked' and throws SdkBindDeferredWhileLockedException without making the keystore call. Consequently, this assertion fails for the intended behavior. Accept the deferral message, retaining the denial alternative for devices whose lock state cannot be established.
…ext teardown with nothing to recover Plan §32.9 gave the replay restart alarm its own PendingIntent identity (request code 1) so the periodic scheduler could no longer overwrite it. That also put it out of reach of the periodic scheduler's cancel, and the wallet wipe cancelled only the periodic alarm: a replay interrupted before a wipe left a REPEATING fifteen-minute alarm that went on starting the service against the wiped wallet, bypassing the periodic tier's backoff (review, 2026-09-24). - `replayRestartAlarmIntent` builds the PendingIntent in one place, so arming and cancelling can never disagree about which alarm they mean. - `WalletApplication.scheduleStartBlockchainService(ctx, cancelOnly=true)` — the wipe's phase 4a — now retires both alarms. - `decideOnReplayRestartAlarm` defines the rule for a teardown: armed when it interrupts a replay the service did not choose to abandon; retired when the replay completed, the user wiped/reset, or the SDK bind is blocked; left alone by an instance that never finished initialising, since that alarm may be the only thing that can recover the instance that armed it (§37). - `ReplayRestartAlarmTest` drives the real AlarmManager under Robolectric: interrupted replay → restart alarm armed → wipe leaves nothing armed, the two identities survive each other, and the rule is pinned. - cutover-emulator-test.sh S2 accepts the locked-device deferral, which the first-bind path now takes before the keystore is asked. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
thepastaclaw
suggested changes
Sep 25, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Contributor
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The PR fixes the previously reported lifecycle, replay-state, bind-race, alarm, and UI issues, but a new destructive-cleanup race remains. stopInternal() can return while a cancelled native DashPay bring-up is still executing, after which wallet removal and SPV-directory deletion proceed and a subsequent wallet start can overlap that native operation or skip its own bring-up. This can access or mutate SDK wallet state concurrently with destructive cleanup, so the change is blocking.
🔴 1 blocking
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:
normalbygpt-6-astra(effort low) — This is a large, intricate cross-cutting change to wallet upgrade, SDK binding, blockchain synchronization, service lifecycle, alarms, and UI, but the diff does not itself alter funds movement, coin selection, cryptography, key handling, consensus, peer-facing deserialization, or storage migrations. - 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 67% left, 5h 0% 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 high); agentphase2-reviewer,gpt-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/L1ShadowSyncService.kt`:
- [BLOCKING] wallet/src/de/schildbach/wallet/service/platform/sdk/L1ShadowSyncService.kt:2728-2736: Do not remove the SDK wallet while a cancelled native bring-up is still running
`stopInternal()` waits only `bringUpStopJoinMs` for a cancelled `startWalletSubsystems()` call and deliberately retains `detachedBringUp` when the native call has not returned. However, both destructive callers continue immediately after `stop()`: `recoverByRecreatingWallet()` calls `recreator.removeSdkWallet(walletIdHex)` and `deleteSpvDataDir()`, while `clearForWalletWipe()` removes the SDK wallet and deletes the same directory. If the native bring-up remains inside its non-cancellable call, those operations can run concurrently with code still using the old SDK wallet or its storage. A later wallet start can also encounter the retained cancelled deferred and start SPV without creating a fresh bring-up, while the old native operation is still active. The bounded wait is appropriate for an ordinary service stop, but destructive cleanup must wait for the native call to finish, defer removal until the retained deferred completes, or isolate the cleanup by terminating/restarting the process; it must not proceed as though the SDK wallet were stopped.
…leave its native call before removing the SDK wallet stop() joins a cancelled DashPay bring-up for a bounded time and, if it is still inside its native call, retains it — which is right for an ordinary stop and wrong for the callers that go on to remove the SDK wallet and delete the SPV dataDir that call is still using (review, 2026-09-25). quiesceBringUpForDestruction, under the mutex so no start can create another meanwhile, cancels and waits for the straggler: - recoverByRecreatingWallet and a hard resetShadowState wait up to DESTRUCTIVE_BRING_UP_JOIN_MS (60 s) and REFUSE if it is still running, touching nothing — both are re-runnable. - clearForWalletWipe waits as long as it takes, logging every 30 s: a wipe cannot be refused, and cannot be retried later against a directory the next wallet may by then own. The wipe already runs for minutes and survives process death through its marker. Three tests park a bring-up inside a NonCancellable native call and check that re-creation refuses and then succeeds once the call returns, that the wipe waits past the stop's join bound and removes only after the call returns, and that a hard reset refuses next to a straggler. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
thepastaclaw
suggested changes
Sep 25, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Contributor
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the findings against bd31b08: 22 prior issues are fixed and one is withdrawn. One production notification race and two restart-test fixture defects remain. Source inspection, shell syntax validation and working-tree checks completed; the reported Gradle test results were not independently rerun, and the worktree remains unchanged.
🔴 1 blocking | 🟡 2 suggestion(s)
2 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Await each startup before stopping the next service lifetime
wallet/test/de/schildbach/wallet/service/platform/PlatformSyncEngineRestartTest.kt:192-197
resume() launches the engine kick asynchronously, and stopSdkEngines() now cancels the retained startupKickJob. These back-to-back iterations can therefore cancel earlier kicks before they invoke startIfEnabled(), yet the final assertion requires three invocations. This makes the test scheduling-dependent and rejects the intended cancellation behavior. Await each startup before stopping the next lifetime so this test measures rearmability across completed cycles; cancellation before startup belongs in a separate test.
repeat(3) { cycle ->
service.stopSdkEngines()
service.resume()
coVerify(timeout = kickTimeoutMs, exactly = cycle + 1) { l1ShadowSyncService.startIfEnabled() }
}
source: gpt-6-astra (phase2-reviewer: general)
🟡 Suggestion: Stub successful engine startup in the restart-test fixture
wallet/test/de/schildbach/wallet/service/platform/PlatformSyncEngineRestartTest.kt:61-63
The relaxed L1 mock returns false from startIfEnabled(), which now arms BindHealL1Starter instead of representing successful startup. The fixture also supplies a relaxed sdkWalletBinder, so its bindEstablished is a mocked StateFlow rather than a real flow that waits for an outcome. Its collection can return without emitting, causing first { it } to throw NoSuchElementException outside the starter's try/catch on the service's independent Dispatchers.IO scope. Those uncaught exceptions can escape the test and contaminate subsequent coroutine tests. Explicitly stub startIfEnabled() to return true in this successful-start fixture; tests for declined startup should supply a controlled real bindEstablished flow and cancel their service work during cleanup.
source: gpt-6-astra (phase2-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:
normalbygpt-6-astra(effort low) — The diff is large and intricate but changes SDK cutover orchestration, sync lifecycles, bind retries, persistence timing, and diagnostics rather than directly modifying funds movement, key handling, consensus, network deserialization, or storage migrations; the reported funds-loss defects remain upstream. - 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 20% 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 high); agentphase2-reviewer,gpt-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/SdkBindRetryService.kt`:
- [BLOCKING] wallet/src/de/schildbach/wallet/service/platform/sdk/SdkBindRetryService.kt:400-402: Serialize background notification posting with bind success
noteAppBackground() reads and posts the blocker outside outcomeMutex, while the independent bind-success collector clears it under that mutex. If the background callback captures a blocker before success and posts after onBindEstablished() clears the notification, an obsolete ongoing setup-pending notification remains on an already-bound wallet. Notification construction performs several operations before notify(), so the callback need not suspend for this interleaving to occur. Returning to the foreground does not repair it: noteAppForeground() only clears the notification when _blocker is non-null, but success has already cleared that value. Serialize the background callback's blocker read and notification post with bind outcomes, checking the live pending and background state inside the critical section.
In `wallet/test/de/schildbach/wallet/service/platform/PlatformSyncEngineRestartTest.kt`:
- [SUGGESTION] wallet/test/de/schildbach/wallet/service/platform/PlatformSyncEngineRestartTest.kt:192-197: Await each startup before stopping the next service lifetime
resume() launches the engine kick asynchronously, and stopSdkEngines() now cancels the retained startupKickJob. These back-to-back iterations can therefore cancel earlier kicks before they invoke startIfEnabled(), yet the final assertion requires three invocations. This makes the test scheduling-dependent and rejects the intended cancellation behavior. Await each startup before stopping the next lifetime so this test measures rearmability across completed cycles; cancellation before startup belongs in a separate test.
- [SUGGESTION] wallet/test/de/schildbach/wallet/service/platform/PlatformSyncEngineRestartTest.kt:61-63: Stub successful engine startup in the restart-test fixture
The relaxed L1 mock returns false from startIfEnabled(), which now arms BindHealL1Starter instead of representing successful startup. The fixture also supplies a relaxed sdkWalletBinder, so its bindEstablished is a mocked StateFlow rather than a real flow that waits for an outcome. Its collection can return without emitting, causing first { it } to throw NoSuchElementException outside the starter's try/catch on the service's independent Dispatchers.IO scope. Those uncaught exceptions can escape the test and contaminate subsequent coroutine tests. Explicitly stub startIfEnabled() to return true in this successful-start fixture; tests for declined startup should supply a controlled real bindEstablished flow and cancel their service work during cleanup.
…e mutex, from live state noteAppBackground() read the blocker and posted outside outcomeMutex, while the bind-success collector cleared the blocker and the notice under it. A success landing between the read and the notify() left an "unlock your device" notification on a bound wallet, and noteAppForeground() could not repair it because it clears only while a blocker is recorded (review, 2026-09-25). The callback now takes the mutex on the service scope and posts only if a blocker is still recorded, the binder still reports a retry pending, and the app is still in the background. noteAppForeground()'s clear runs under the same mutex for the mirror race: a failure handler that passed its background check and is parked holding the lock re-checks the foreground and skips, and the clear runs after it. Two tests park each handler inside persistBlocker to pin both interleavings. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
thepastaclaw
suggested changes
Sep 25, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Contributor
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the supplied findings and all 26 prior identities against head 118a2ac: 23 are fixed, two remain valid, and one is withdrawn. The asynchronous foreground notification clear introduces a separate in-scope race, leaving one blocker and two test suggestions. Verification was source-based; the emulator script passed shell syntax validation, but the reviewers' Android compilation and unit-test results were not independently rerun.
🔴 1 blocking | 🟡 2 suggestion(s)
2 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Stub successful engine startup in the restart-test fixture
wallet/test/de/schildbach/wallet/service/platform/PlatformSyncEngineRestartTest.kt:61-63
The fixture leaves startIfEnabled() unstubbed on a relaxed mock, so its Boolean result defaults to false. PlatformSynchronizationService now interprets that result as declined startup and arms BindHealL1Starter, whose bindEstablished flow is also unstubbed here. Consequently, tests that verify a successful restart enter the bind-healing path instead, and invocation-count assertions do not expose that mismatch. Explicitly stub the suspending startIfEnabled() call to return true; keep declined-start and bind-healing behavior in the dedicated heal-starter tests.
source: muse-spark-1.3-contributor (phase1-reviewer: general); gpt-6-astra (phase2-reviewer: general)
🟡 Suggestion: Await each startup before stopping the next service lifetime
wallet/test/de/schildbach/wallet/service/platform/PlatformSyncEngineRestartTest.kt:192-197
Each resume() schedules an asynchronous startup kick, but the loop immediately begins the next stop/restart cycle. Both stopSdkEngines() and the next kick cancel pending startup work, so an earlier iteration can legitimately finish without ever calling startIfEnabled(). The final exactly-three assertion therefore depends on dispatcher timing and can reject the intended cancellation behavior rather than detect a broken restart contract. Await the cumulative start count inside each iteration before stopping that lifetime, so the test actually exercises three completed restarts.
repeat(3) { cycle ->
service.stopSdkEngines()
service.resume()
coVerify(timeout = kickTimeoutMs, exactly = cycle + 1) { l1ShadowSyncService.startIfEnabled() }
}
coVerify(exactly = 3) { l1ShadowSyncService.startIfEnabled() }
source: muse-spark-1.3-contributor (phase1-reviewer: general); gpt-6-astra (phase2-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:
normalbygpt-6-astra(effort low) — Despite its size and intricate concurrency, the diff changes SDK cutover orchestration, binding retries, sync lifecycles, and diagnostics rather than storage migration mechanics, key handling, or funds movement; the documented payment-loss defects remain upstream and unfixed here. - 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 52% 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 high); agentphase2-reviewer,gpt-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/SdkBindRetryService.kt`:
- [BLOCKING] wallet/src/de/schildbach/wallet/service/platform/sdk/SdkBindRetryService.kt:506-509: Recheck foreground state before executing the queued notification clear
The foreground clear now launches on the application scope, which uses Dispatchers.Default, but does not recheck visibility after acquiring outcomeMutex. A foreground callback can enqueue its clear, then a subsequent background callback can acquire the mutex first and post the pending notification. When the older foreground coroutine runs, it clears that valid notification even though the app is backgrounded and binding remains blocked. The mutex serializes notification operations, not the order in which independently dispatched coroutines reach it. Unlike the original success-versus-background race, this interleaving loses the recovery prompt for a still-blocked wallet. Check !appInBackground() inside the critical section before clearing, and add a regression that executes the background post before an earlier queued foreground clear.
In `wallet/test/de/schildbach/wallet/service/platform/PlatformSyncEngineRestartTest.kt`:
- [SUGGESTION] wallet/test/de/schildbach/wallet/service/platform/PlatformSyncEngineRestartTest.kt:61-63: Stub successful engine startup in the restart-test fixture
The fixture leaves startIfEnabled() unstubbed on a relaxed mock, so its Boolean result defaults to false. PlatformSynchronizationService now interprets that result as declined startup and arms BindHealL1Starter, whose bindEstablished flow is also unstubbed here. Consequently, tests that verify a successful restart enter the bind-healing path instead, and invocation-count assertions do not expose that mismatch. Explicitly stub the suspending startIfEnabled() call to return true; keep declined-start and bind-healing behavior in the dedicated heal-starter tests.
- [SUGGESTION] wallet/test/de/schildbach/wallet/service/platform/PlatformSyncEngineRestartTest.kt:192-197: Await each startup before stopping the next service lifetime
Each resume() schedules an asynchronous startup kick, but the loop immediately begins the next stop/restart cycle. Both stopSdkEngines() and the next kick cancel pending startup work, so an earlier iteration can legitimately finish without ever calling startIfEnabled(). The final exactly-three assertion therefore depends on dispatcher timing and can reject the intended cancellation behavior rather than detect a broken restart contract. Await the cumulative start count inside each iteration before stopping that lifetime, so the test actually exercises three completed restarts.
Comment on lines
+506
to
+509
| scope.launch { | ||
| outcomeMutex.withLock { | ||
| if (_blocker.value != null) clearPendingNotice() | ||
| } |
Contributor
There was a problem hiding this comment.
🔴 Blocking: Recheck foreground state before executing the queued notification clear
The foreground clear now launches on the application scope, which uses Dispatchers.Default, but does not recheck visibility after acquiring outcomeMutex. A foreground callback can enqueue its clear, then a subsequent background callback can acquire the mutex first and post the pending notification. When the older foreground coroutine runs, it clears that valid notification even though the app is backgrounded and binding remains blocked. The mutex serializes notification operations, not the order in which independently dispatched coroutines reach it. Unlike the original success-versus-background race, this interleaving loses the recovery prompt for a still-blocked wallet. Check !appInBackground() inside the critical section before clearing, and add a regression that executes the background post before an earlier queued foreground clear.
Suggested change
| scope.launch { | |
| outcomeMutex.withLock { | |
| if (_blocker.value != null) clearPendingNotice() | |
| } | |
| scope.launch { | |
| outcomeMutex.withLock { | |
| if (!appInBackground() && _blocker.value != null) clearPendingNotice() | |
| } | |
| } |
source: gpt-6-astra (phase2-reviewer: general)
Collaborator
Author
There was a problem hiding this comment.
Fixed in 6cd6c09, as suggested: the queued clear now runs only if !appInBackground() && _blocker.value != null holds while the lock is held. Agreed that the mutex only serializes the operations, not the order the two callbacks' coroutines reach it, and that this one is worse than the original race because it loses the prompt for a wallet that is still blocked.
The regression parks a failure handler on the mutex (gate in persistBlocker), flips foreground then background while it is parked so the foreground edge's clear is queued ahead of the background edge's post, and releases: the failure handler posts, the queued clear skips on the live background state, the later post re-posts harmlessly, and noticeClears stays 0 with the blocker still recorded. The existing foregroundWithAPendingBind_clearsTheNotification now flips the monitor before the callback, which is the order the real AppForegroundMonitor produces. 29 in the class.
…under the mutex The mutex serializes the notification operations, not the order independently dispatched coroutines reach it: a clear queued by a foreground edge could run after the app had gone back to the background and a valid notice had been posted for a still-blocked wallet, and removed it — losing the recovery prompt (review, 2026-09-25). The clear now runs only if the app is still in the foreground when the lock is held. A test parks a failure handler on the mutex through a fast foreground/background flip and checks the notice survives. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
HashEngineering
added a commit
that referenced
this pull request
Sep 28, 2026
…nation Two blocking findings and both suggestions from the #1570 review, plus the rebase onto the new base. BLOCKING — an older read still handed its own stale address to its caller. Rejecting the out-of-order PUBLICATION was only half the job: `completeReceiveRead` returned `address ?: cache`, so a read that lost the ordering still answered with its own older result, and `WalletApplication`'s live accessors pass a non-null result straight through to the Receive screen. Every caller is now answered from the cache, which after the gate holds the newest address for the binding; the read's own result survives only as the never-cached residual. The ordering regression now asserts the older CALLER receives the newer address too — it previously discarded that value. Mutation-confirmed. BLOCKING — the internal chain is shared with ordinary payment change. Not fixable here, as the review now also concludes: `next_change_address` returns next-unused without issuing or reserving, from the same allocator the engine's change selection uses, and v42int21 exposes neither a reservation nor a dedicated-pool API. What WAS actionable is that the KDoc claimed a guarantee this does not have. It now documents the residual accurately: a payment's recipient can see their change output and link a later self-transfer to it — cross-operation linkage, visible to a counterparty — and says plainly that what the internal chain buys is avoiding the easier unpaid-QR attack, not isolation. An earlier version of that comment called the collision invisible to payers; that was wrong. SUGGESTION — the SDK read now happens BEFORE the ownership snapshot in `unadvertisedDestinationLive`. Snapshotting first meant a cutover committing just afterwards would skip the SDK and issue a dashj key with no guarantee of differing from the engine's advertised address. The window narrows to the read itself, which gates on the live flag under the service lock. SUGGESTION — the paper-wallet sweep moves to the self-transfer destination. The review is right that "already linked to the paper key" does not answer the objection: the NEW link is from that paper key to whoever holds an unpaid Receive QR. Since the destination now throws post-cutover, the sweep handles preparation failure explicitly and lands in State.FAILED before anything is sent, rather than letting an exception escape a Handler runnable. Rebased with `--onto` from `fix/upgrade-memory-and-sync` to `feat/kotlin-sdk-phase-1`: #1568 SQUASH-merged the old base, so a plain rebase tried to replay the base's own commits. Trees are identical, and the six commits replayed without conflict. Suite: 162 failures on head and 162 on `feat/kotlin-sdk-phase-1` — no new failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Sep 28, 2026
HashEngineering
added a commit
that referenced
this pull request
Sep 28, 2026
…nation Two blocking findings and both suggestions from the #1570 review, plus the rebase onto the new base. BLOCKING — an older read still handed its own stale address to its caller. Rejecting the out-of-order PUBLICATION was only half the job: `completeReceiveRead` returned `address ?: cache`, so a read that lost the ordering still answered with its own older result, and `WalletApplication`'s live accessors pass a non-null result straight through to the Receive screen. Every caller is now answered from the cache, which after the gate holds the newest address for the binding; the read's own result survives only as the never-cached residual. The ordering regression now asserts the older CALLER receives the newer address too — it previously discarded that value. Mutation-confirmed. BLOCKING — the internal chain is shared with ordinary payment change. Not fixable here, as the review now also concludes: `next_change_address` returns next-unused without issuing or reserving, from the same allocator the engine's change selection uses, and v42int21 exposes neither a reservation nor a dedicated-pool API. What WAS actionable is that the KDoc claimed a guarantee this does not have. It now documents the residual accurately: a payment's recipient can see their change output and link a later self-transfer to it — cross-operation linkage, visible to a counterparty — and says plainly that what the internal chain buys is avoiding the easier unpaid-QR attack, not isolation. An earlier version of that comment called the collision invisible to payers; that was wrong. SUGGESTION — the SDK read now happens BEFORE the ownership snapshot in `unadvertisedDestinationLive`. Snapshotting first meant a cutover committing just afterwards would skip the SDK and issue a dashj key with no guarantee of differing from the engine's advertised address. The window narrows to the read itself, which gates on the live flag under the service lock. SUGGESTION — the paper-wallet sweep moves to the self-transfer destination. The review is right that "already linked to the paper key" does not answer the objection: the NEW link is from that paper key to whoever holds an unpaid Receive QR. Since the destination now throws post-cutover, the sweep handles preparation failure explicitly and lands in State.FAILED before anything is sent, rather than letting an exception escape a Handler runnable. Rebased with `--onto` from `fix/upgrade-memory-and-sync` to `feat/kotlin-sdk-phase-1`: #1568 SQUASH-merged the old base, so a plain rebase tried to replay the base's own commits. Trees are identical, and the six commits replayed without conflict. Suite: 162 failures on head and 162 on `feat/kotlin-sdk-phase-1` — no new failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
HashEngineering
added a commit
that referenced
this pull request
Sep 29, 2026
…nation Two blocking findings and both suggestions from the #1570 review, plus the rebase onto the new base. BLOCKING — an older read still handed its own stale address to its caller. Rejecting the out-of-order PUBLICATION was only half the job: `completeReceiveRead` returned `address ?: cache`, so a read that lost the ordering still answered with its own older result, and `WalletApplication`'s live accessors pass a non-null result straight through to the Receive screen. Every caller is now answered from the cache, which after the gate holds the newest address for the binding; the read's own result survives only as the never-cached residual. The ordering regression now asserts the older CALLER receives the newer address too — it previously discarded that value. Mutation-confirmed. BLOCKING — the internal chain is shared with ordinary payment change. Not fixable here, as the review now also concludes: `next_change_address` returns next-unused without issuing or reserving, from the same allocator the engine's change selection uses, and v42int21 exposes neither a reservation nor a dedicated-pool API. What WAS actionable is that the KDoc claimed a guarantee this does not have. It now documents the residual accurately: a payment's recipient can see their change output and link a later self-transfer to it — cross-operation linkage, visible to a counterparty — and says plainly that what the internal chain buys is avoiding the easier unpaid-QR attack, not isolation. An earlier version of that comment called the collision invisible to payers; that was wrong. SUGGESTION — the SDK read now happens BEFORE the ownership snapshot in `unadvertisedDestinationLive`. Snapshotting first meant a cutover committing just afterwards would skip the SDK and issue a dashj key with no guarantee of differing from the engine's advertised address. The window narrows to the read itself, which gates on the live flag under the service lock. SUGGESTION — the paper-wallet sweep moves to the self-transfer destination. The review is right that "already linked to the paper key" does not answer the objection: the NEW link is from that paper key to whoever holds an unpaid Receive QR. Since the destination now throws post-cutover, the sweep handles preparation failure explicitly and lands in State.FAILED before anything is sent, rather than letting an exception escape a Handler runnable. Rebased with `--onto` from `fix/upgrade-memory-and-sync` to `feat/kotlin-sdk-phase-1`: #1568 SQUASH-merged the old base, so a plain rebase tried to replay the base's own commits. Trees are identical, and the six commits replayed without conflict. Suite: 162 failures on head and 162 on `feat/kotlin-sdk-phase-1` — no new failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
HashEngineering
added a commit
that referenced
this pull request
Sep 30, 2026
…nation Two blocking findings and both suggestions from the #1570 review, plus the rebase onto the new base. BLOCKING — an older read still handed its own stale address to its caller. Rejecting the out-of-order PUBLICATION was only half the job: `completeReceiveRead` returned `address ?: cache`, so a read that lost the ordering still answered with its own older result, and `WalletApplication`'s live accessors pass a non-null result straight through to the Receive screen. Every caller is now answered from the cache, which after the gate holds the newest address for the binding; the read's own result survives only as the never-cached residual. The ordering regression now asserts the older CALLER receives the newer address too — it previously discarded that value. Mutation-confirmed. BLOCKING — the internal chain is shared with ordinary payment change. Not fixable here, as the review now also concludes: `next_change_address` returns next-unused without issuing or reserving, from the same allocator the engine's change selection uses, and v42int21 exposes neither a reservation nor a dedicated-pool API. What WAS actionable is that the KDoc claimed a guarantee this does not have. It now documents the residual accurately: a payment's recipient can see their change output and link a later self-transfer to it — cross-operation linkage, visible to a counterparty — and says plainly that what the internal chain buys is avoiding the easier unpaid-QR attack, not isolation. An earlier version of that comment called the collision invisible to payers; that was wrong. SUGGESTION — the SDK read now happens BEFORE the ownership snapshot in `unadvertisedDestinationLive`. Snapshotting first meant a cutover committing just afterwards would skip the SDK and issue a dashj key with no guarantee of differing from the engine's advertised address. The window narrows to the read itself, which gates on the live flag under the service lock. SUGGESTION — the paper-wallet sweep moves to the self-transfer destination. The review is right that "already linked to the paper key" does not answer the objection: the NEW link is from that paper key to whoever holds an unpaid Receive QR. Since the destination now throws post-cutover, the sweep handles preparation failure explicitly and lands in State.FAILED before anything is sent, rather than letting an exception escape a Handler runnable. Rebased with `--onto` from `fix/upgrade-memory-and-sync` to `feat/kotlin-sdk-phase-1`: #1568 SQUASH-merged the old base, so a plain rebase tried to replay the base's own commits. Trees are identical, and the six commits replayed without conflict. Suite: 162 failures on head and 162 on `feat/kotlin-sdk-phase-1` — no new failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
HashEngineering
added a commit
that referenced
this pull request
Oct 1, 2026
…nation Two blocking findings and both suggestions from the #1570 review, plus the rebase onto the new base. BLOCKING — an older read still handed its own stale address to its caller. Rejecting the out-of-order PUBLICATION was only half the job: `completeReceiveRead` returned `address ?: cache`, so a read that lost the ordering still answered with its own older result, and `WalletApplication`'s live accessors pass a non-null result straight through to the Receive screen. Every caller is now answered from the cache, which after the gate holds the newest address for the binding; the read's own result survives only as the never-cached residual. The ordering regression now asserts the older CALLER receives the newer address too — it previously discarded that value. Mutation-confirmed. BLOCKING — the internal chain is shared with ordinary payment change. Not fixable here, as the review now also concludes: `next_change_address` returns next-unused without issuing or reserving, from the same allocator the engine's change selection uses, and v42int21 exposes neither a reservation nor a dedicated-pool API. What WAS actionable is that the KDoc claimed a guarantee this does not have. It now documents the residual accurately: a payment's recipient can see their change output and link a later self-transfer to it — cross-operation linkage, visible to a counterparty — and says plainly that what the internal chain buys is avoiding the easier unpaid-QR attack, not isolation. An earlier version of that comment called the collision invisible to payers; that was wrong. SUGGESTION — the SDK read now happens BEFORE the ownership snapshot in `unadvertisedDestinationLive`. Snapshotting first meant a cutover committing just afterwards would skip the SDK and issue a dashj key with no guarantee of differing from the engine's advertised address. The window narrows to the read itself, which gates on the live flag under the service lock. SUGGESTION — the paper-wallet sweep moves to the self-transfer destination. The review is right that "already linked to the paper key" does not answer the objection: the NEW link is from that paper key to whoever holds an unpaid Receive QR. Since the destination now throws post-cutover, the sweep handles preparation failure explicitly and lands in State.FAILED before anything is sent, rather than letting an exception escape a Handler runnable. Rebased with `--onto` from `fix/upgrade-memory-and-sync` to `feat/kotlin-sdk-phase-1`: #1568 SQUASH-merged the old base, so a plain rebase tried to replay the base's own commits. Trees are identical, and the six commits replayed without conflict. Suite: 162 failures on head and 162 on `feat/kotlin-sdk-phase-1` — no new failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
HashEngineering
added a commit
that referenced
this pull request
Oct 1, 2026
…nation Two blocking findings and both suggestions from the #1570 review, plus the rebase onto the new base. BLOCKING — an older read still handed its own stale address to its caller. Rejecting the out-of-order PUBLICATION was only half the job: `completeReceiveRead` returned `address ?: cache`, so a read that lost the ordering still answered with its own older result, and `WalletApplication`'s live accessors pass a non-null result straight through to the Receive screen. Every caller is now answered from the cache, which after the gate holds the newest address for the binding; the read's own result survives only as the never-cached residual. The ordering regression now asserts the older CALLER receives the newer address too — it previously discarded that value. Mutation-confirmed. BLOCKING — the internal chain is shared with ordinary payment change. Not fixable here, as the review now also concludes: `next_change_address` returns next-unused without issuing or reserving, from the same allocator the engine's change selection uses, and v42int21 exposes neither a reservation nor a dedicated-pool API. What WAS actionable is that the KDoc claimed a guarantee this does not have. It now documents the residual accurately: a payment's recipient can see their change output and link a later self-transfer to it — cross-operation linkage, visible to a counterparty — and says plainly that what the internal chain buys is avoiding the easier unpaid-QR attack, not isolation. An earlier version of that comment called the collision invisible to payers; that was wrong. SUGGESTION — the SDK read now happens BEFORE the ownership snapshot in `unadvertisedDestinationLive`. Snapshotting first meant a cutover committing just afterwards would skip the SDK and issue a dashj key with no guarantee of differing from the engine's advertised address. The window narrows to the read itself, which gates on the live flag under the service lock. SUGGESTION — the paper-wallet sweep moves to the self-transfer destination. The review is right that "already linked to the paper key" does not answer the objection: the NEW link is from that paper key to whoever holds an unpaid Receive QR. Since the destination now throws post-cutover, the sweep handles preparation failure explicitly and lands in State.FAILED before anything is sent, rather than letting an exception escape a Handler runnable. Rebased with `--onto` from `fix/upgrade-memory-and-sync` to `feat/kotlin-sdk-phase-1`: #1568 SQUASH-merged the old base, so a plain rebase tried to replay the base's own commits. Trees are identical, and the six commits replayed without conflict. Suite: 162 failures on head and 162 on `feat/kotlin-sdk-phase-1` — no new failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
HashEngineering
added a commit
that referenced
this pull request
Oct 1, 2026
…nation Two blocking findings and both suggestions from the #1570 review, plus the rebase onto the new base. BLOCKING — an older read still handed its own stale address to its caller. Rejecting the out-of-order PUBLICATION was only half the job: `completeReceiveRead` returned `address ?: cache`, so a read that lost the ordering still answered with its own older result, and `WalletApplication`'s live accessors pass a non-null result straight through to the Receive screen. Every caller is now answered from the cache, which after the gate holds the newest address for the binding; the read's own result survives only as the never-cached residual. The ordering regression now asserts the older CALLER receives the newer address too — it previously discarded that value. Mutation-confirmed. BLOCKING — the internal chain is shared with ordinary payment change. Not fixable here, as the review now also concludes: `next_change_address` returns next-unused without issuing or reserving, from the same allocator the engine's change selection uses, and v42int21 exposes neither a reservation nor a dedicated-pool API. What WAS actionable is that the KDoc claimed a guarantee this does not have. It now documents the residual accurately: a payment's recipient can see their change output and link a later self-transfer to it — cross-operation linkage, visible to a counterparty — and says plainly that what the internal chain buys is avoiding the easier unpaid-QR attack, not isolation. An earlier version of that comment called the collision invisible to payers; that was wrong. SUGGESTION — the SDK read now happens BEFORE the ownership snapshot in `unadvertisedDestinationLive`. Snapshotting first meant a cutover committing just afterwards would skip the SDK and issue a dashj key with no guarantee of differing from the engine's advertised address. The window narrows to the read itself, which gates on the live flag under the service lock. SUGGESTION — the paper-wallet sweep moves to the self-transfer destination. The review is right that "already linked to the paper key" does not answer the objection: the NEW link is from that paper key to whoever holds an unpaid Receive QR. Since the destination now throws post-cutover, the sweep handles preparation failure explicitly and lands in State.FAILED before anything is sent, rather than letting an exception escape a Handler runnable. Rebased with `--onto` from `fix/upgrade-memory-and-sync` to `feat/kotlin-sdk-phase-1`: #1568 SQUASH-merged the old base, so a plain rebase tried to replay the base's own commits. Trees are identical, and the six commits replayed without conflict. Suite: 162 failures on head and 162 on `feat/kotlin-sdk-phase-1` — no new failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
HashEngineering
added a commit
that referenced
this pull request
Oct 2, 2026
…nation Two blocking findings and both suggestions from the #1570 review, plus the rebase onto the new base. BLOCKING — an older read still handed its own stale address to its caller. Rejecting the out-of-order PUBLICATION was only half the job: `completeReceiveRead` returned `address ?: cache`, so a read that lost the ordering still answered with its own older result, and `WalletApplication`'s live accessors pass a non-null result straight through to the Receive screen. Every caller is now answered from the cache, which after the gate holds the newest address for the binding; the read's own result survives only as the never-cached residual. The ordering regression now asserts the older CALLER receives the newer address too — it previously discarded that value. Mutation-confirmed. BLOCKING — the internal chain is shared with ordinary payment change. Not fixable here, as the review now also concludes: `next_change_address` returns next-unused without issuing or reserving, from the same allocator the engine's change selection uses, and v42int21 exposes neither a reservation nor a dedicated-pool API. What WAS actionable is that the KDoc claimed a guarantee this does not have. It now documents the residual accurately: a payment's recipient can see their change output and link a later self-transfer to it — cross-operation linkage, visible to a counterparty — and says plainly that what the internal chain buys is avoiding the easier unpaid-QR attack, not isolation. An earlier version of that comment called the collision invisible to payers; that was wrong. SUGGESTION — the SDK read now happens BEFORE the ownership snapshot in `unadvertisedDestinationLive`. Snapshotting first meant a cutover committing just afterwards would skip the SDK and issue a dashj key with no guarantee of differing from the engine's advertised address. The window narrows to the read itself, which gates on the live flag under the service lock. SUGGESTION — the paper-wallet sweep moves to the self-transfer destination. The review is right that "already linked to the paper key" does not answer the objection: the NEW link is from that paper key to whoever holds an unpaid Receive QR. Since the destination now throws post-cutover, the sweep handles preparation failure explicitly and lands in State.FAILED before anything is sent, rather than letting an exception escape a Handler runnable. Rebased with `--onto` from `fix/upgrade-memory-and-sync` to `feat/kotlin-sdk-phase-1`: #1568 SQUASH-merged the old base, so a plain rebase tried to replay the base's own commits. Trees are identical, and the six commits replayed without conflict. Suite: 162 failures on head and 162 on `feat/kotlin-sdk-phase-1` — no new failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
HashEngineering
added a commit
that referenced
this pull request
Oct 2, 2026
…nation Two blocking findings and both suggestions from the #1570 review, plus the rebase onto the new base. BLOCKING — an older read still handed its own stale address to its caller. Rejecting the out-of-order PUBLICATION was only half the job: `completeReceiveRead` returned `address ?: cache`, so a read that lost the ordering still answered with its own older result, and `WalletApplication`'s live accessors pass a non-null result straight through to the Receive screen. Every caller is now answered from the cache, which after the gate holds the newest address for the binding; the read's own result survives only as the never-cached residual. The ordering regression now asserts the older CALLER receives the newer address too — it previously discarded that value. Mutation-confirmed. BLOCKING — the internal chain is shared with ordinary payment change. Not fixable here, as the review now also concludes: `next_change_address` returns next-unused without issuing or reserving, from the same allocator the engine's change selection uses, and v42int21 exposes neither a reservation nor a dedicated-pool API. What WAS actionable is that the KDoc claimed a guarantee this does not have. It now documents the residual accurately: a payment's recipient can see their change output and link a later self-transfer to it — cross-operation linkage, visible to a counterparty — and says plainly that what the internal chain buys is avoiding the easier unpaid-QR attack, not isolation. An earlier version of that comment called the collision invisible to payers; that was wrong. SUGGESTION — the SDK read now happens BEFORE the ownership snapshot in `unadvertisedDestinationLive`. Snapshotting first meant a cutover committing just afterwards would skip the SDK and issue a dashj key with no guarantee of differing from the engine's advertised address. The window narrows to the read itself, which gates on the live flag under the service lock. SUGGESTION — the paper-wallet sweep moves to the self-transfer destination. The review is right that "already linked to the paper key" does not answer the objection: the NEW link is from that paper key to whoever holds an unpaid Receive QR. Since the destination now throws post-cutover, the sweep handles preparation failure explicitly and lands in State.FAILED before anything is sent, rather than letting an exception escape a Handler runnable. Rebased with `--onto` from `fix/upgrade-memory-and-sync` to `feat/kotlin-sdk-phase-1`: #1568 SQUASH-merged the old base, so a plain rebase tried to replay the base's own commits. Trees are identical, and the six commits replayed without conflict. Suite: 162 failures on head and 162 on `feat/kotlin-sdk-phase-1` — no new failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
HashEngineering
added a commit
that referenced
this pull request
Oct 7, 2026
…held dashj chain (#1570) * fix(receive): serve the receive address from the SDK engine, not the held dashj chain SR-03 / D-003: after Reset Wallet + restore, the Receive screen served an address the chain had already paid. 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, and `freshReceiveAddress()` walked forward from the same index-0 base through the used range instead of skipping it. Correctness defect for the payer, privacy defect for the user. The engine's own pointer is derived from the SPV scan's used-set, so it skips every address the chain has paid. Read it over the FFI (`core_wallet_next_receive_address` -> `ManagedCoreWallet.nextReceiveAddress`), deliberately NOT the SDK's Room `core_addresses` mirror: that mirror is a projection the engine writes behind itself, so it trails every used-marker by a persistence pass. The BIP70 `refund_to` source was reading the mirror and now takes the same FFI call (the upstream Kotlin plumbing gap noted there closed in the v42int21 AAR). Two accessors on CutoverUiDataService, following the existing sdkBalanceOrNull / sdkSpendableUtxoCountOrNull overlay idiom: - sdkReceiveAddressOrNull() - synchronous and non-blocking, serving a cache refreshed on the balance cadence. That cadence is right by construction: the only thing that moves the engine's pointer is a receive landing on the current address, which IS an engine tx event. - sdkReceiveAddressLive{,Blocking}OrNull() - bypasses the cache with a live FFI read, for the callers already off the main thread. A failed engine read HOLDS the last known address rather than dropping to null; null would send the overlay back to the frozen dashj address, which is the defect itself. No l1Synced gate, unlike the UTXO count: holding back has no safe direction here, because the dashj fallback is known-wrong rather than a sound over-estimate. Call sites moved onto the live read: the Receive screen and its QR, the amount-dialog path, the `...OffMain` helpers on both WalletData and the neutral WalletDataProvider (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. Two deliberate narrowings, documented at the call sites: - Post-cutover `fresh` and `current` coincide. 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 handing out already-paid addresses. - Mid-scan the engine can return an address whose payment the scan has not found yet. It converges as the scan advances. WalletAddressFragment also reads the dashj wallet directly, but nothing references that fragment (no layout, no navigation graph), so it is left alone. Tests: six new CutoverUiDataServiceTest cases pin that pre-cutover never touches the SDK, post-cutover serves the engine address, it advances on a tx event, a failed read holds, an unavailable read falls back to dashj, and the live read bypasses the cache. The shielded and WalletDataExt tests now stub the `Live` accessors the helpers call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(receive): correct two stale docs and pin the rollback clear 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 02e741b0c; the upstream CutoverUiDataService changes merged without conflict. Full suite: 152 failures, an identical set to the base at 02e741b0c (pre-existing conscrypt native-lib failures in this environment). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(receive): stamp the address cache with a binding generation 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> * fix(shielded): unshield to the unadvertised chain, not the receive address 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> * fix(shielded): fail closed rather than fall back to the advertised address 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> * fix(receive): revoke ownership eagerly and order address publications 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> * fix(receive): answer every caller from the cache; own the sweep destination Two blocking findings and both suggestions from the #1570 review, plus the rebase onto the new base. BLOCKING — an older read still handed its own stale address to its caller. Rejecting the out-of-order PUBLICATION was only half the job: `completeReceiveRead` returned `address ?: cache`, so a read that lost the ordering still answered with its own older result, and `WalletApplication`'s live accessors pass a non-null result straight through to the Receive screen. Every caller is now answered from the cache, which after the gate holds the newest address for the binding; the read's own result survives only as the never-cached residual. The ordering regression now asserts the older CALLER receives the newer address too — it previously discarded that value. Mutation-confirmed. BLOCKING — the internal chain is shared with ordinary payment change. Not fixable here, as the review now also concludes: `next_change_address` returns next-unused without issuing or reserving, from the same allocator the engine's change selection uses, and v42int21 exposes neither a reservation nor a dedicated-pool API. What WAS actionable is that the KDoc claimed a guarantee this does not have. It now documents the residual accurately: a payment's recipient can see their change output and link a later self-transfer to it — cross-operation linkage, visible to a counterparty — and says plainly that what the internal chain buys is avoiding the easier unpaid-QR attack, not isolation. An earlier version of that comment called the collision invisible to payers; that was wrong. SUGGESTION — the SDK read now happens BEFORE the ownership snapshot in `unadvertisedDestinationLive`. Snapshotting first meant a cutover committing just afterwards would skip the SDK and issue a dashj key with no guarantee of differing from the engine's advertised address. The window narrows to the read itself, which gates on the live flag under the service lock. SUGGESTION — the paper-wallet sweep moves to the self-transfer destination. The review is right that "already linked to the paper key" does not answer the objection: the NEW link is from that paper key to whoever holds an unpaid Receive QR. Since the destination now throws post-cutover, the sweep handles preparation failure explicitly and lands in State.FAILED before anything is sent, rather than letting an exception escape a Handler runnable. Rebased with `--onto` from `fix/upgrade-memory-and-sync` to `feat/kotlin-sdk-phase-1`: #1568 SQUASH-merged the old base, so a plain rebase tried to replay the base's own commits. Trees are identical, and the six commits replayed without conflict. Suite: 162 failures on head and 162 on `feat/kotlin-sdk-phase-1` — no new failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(receive): keep sweep failures retryable; park a real read in the rollback test CodeRabbit findings on #1570 — now reviewing this PR for the first time, since retargeting to feat/kotlin-sdk-phase-1 brought it inside its auto-review scope. - The sweep's destination-read failure set State.FAILED, which disables the sweep button and shows nothing, stranding the user. A transient failure (the engine not bound yet, an FFI error) must stay retryable, so it now restores State.CONFIRM_SWEEP and surfaces the existing common `loading_error` string rather than adding a new translatable resource. - `rollbackRevokesOwnershipEvenWhileThePipelineReadIsParked` never parked a read. `refreshNativeSplit` only reads at pipeline start, on a tx event or on the ticker; the test emitted no event and `pumpUntil` only advances `runCurrent()`, so nothing was in flight when the state flipped. It passed while proving only that the unbind EXISTS. My earlier mutation missed this because it DELETED the unbind outright, which any version of the test catches. The test now emits a tx event and waits for the read to be parked before rolling back, and the mutation is the realistic one — MOVING the unbind back inside `collectLatest`'s action — which it catches. Also corrected an overclaim rather than leaving it: the javadoc on `currentReceiveAddressLive` said falling back to `currentReceiveAddress()` made it "always safe to prefer". It does not. The third finding is real — post-cutover with a cold cache those accessors reach the HELD dashj chain's frozen address, which is SR-03 itself, and the window between the cutover flag going active and `awaitBoundWallet` resolving is not momentary when a bind is retrying. That one is NOT fixed here, deliberately. Failing closed the way `unadvertisedDestinationLive` does needs an explicit "address unavailable" path at ~14 call sites — the Receive screen, the BIP70/URI handlers and every exchange-integration deposit address — most of which call from a bare `lifecycleScope.launch` with no failure handling. Throwing without that work would replace a reused address with an app crash. The gap is documented at the accessor and raised on the PR for a design decision. Suite: 162 failures, identical set — none new. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(receive): fail closed after cutover instead of the frozen dashj address The remaining blocker on #1570, and both reviewers independently reached the same verdict: documenting the hazard was not enough, because the Receive UI, the URI/BIP70 handlers and the integration deposit paths would still advertise the unsafe address. They were right and my earlier "document it" call was wrong. The defect: post-cutover, when the engine had no answer and the cache was cold, `currentReceiveAddressLive()`/`freshReceiveAddressLive()` fell through to the HELD dashj chain — whose pointer is frozen wherever the restore left it, i.e. the already-paid index-0 address. That is SR-03, reintroduced precisely in the initialization and retry window this PR creates. Two parts. 1. REMOVE THE WINDOW. A live read now waits (bounded, 5s) for the SDK wallet to bind when the cutover is active and nothing is bound yet. That covers the dominant case the finding names — before `awaitBoundWallet` resolves — and it is the honest response: the wallet genuinely has no address it can safely advertise until the engine binds, and these accessors are contractually off-main, so waiting costs a spinner rather than a wrong address. Pre-cutover it never waits. 2. FAIL CLOSED for what remains. Post-cutover with still no answer, the accessors throw the new `ReceiveAddressUnavailableException` rather than serving dashj. The second part is why this took the shape it did: sixteen consumers had to gain an unavailable path, and none of them had one. Rather than a throw that trades a reused address for a crash, each now surfaces a retry in the idiom that site already uses — - Receive screen / amount dialog: an error toast, no address advertised; - wallet-URI handler: RESULT_CANCELED to the calling app; - Topper (Buy&Sell, Uphold), Coinbase review, Uphold transfer, Explore faucet: an error toast, flow abandoned before any deposit target is used; - Coinbase conversion: the existing commit-failure state (its surrounding try/catch covered only the sell branch, not the address read); - Coinbase transfer: the existing null-params callback; - Maya DEX: the screen's own `orderErrorRes`; - SwapKit: `ResponseResource.Failure`, the same shape as "wallet not loaded"; - CrowdNode: contained in `resetAddressAndApi` so all four launch entry points are covered at once, reported through the existing `apiError`. A CrowdNode account address is PERSISTED as the account identity, so failing to reset beats resetting onto a frozen address. Test: `liveReadWaitsForTheBindingRatherThanAnsweringEmpty` pins that an unbound wallet makes the read wait instead of answering empty. Mutation-confirmed — dropping the wait fails it. Suite: 161 failures across all six touched modules, no new failures. ktlint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(receive): repair six consumer paths the fail-closed change broke Five blocking findings from thepastaclaw plus three overlapping ones from CodeRabbit. Every one is a consequence of the previous commit: making the live accessors throw introduced a new failure into sixteen call sites, and I wired several of them wrongly. Verified each against the source first. TWO ORDERING BUGS — the address was read AFTER the financial side effect: - `CoinbaseOrderReviewFragment` called `tryBuyDash()` (which submits `placeBuyOrder()` with a fresh UUID) BEFORE `getTransferDashParams()`. An address failure then left a SUCCEEDED purchase with no transfer parameters and no submitted-order state, so pressing confirm again placed a second order with a different UUID. The address is now acquired first, making a failure a no-op. - `CoinbaseConversionPreviewViewModel` read the address after `commitSwapTrade()` had already succeeded, then reported that success as CONVERSION_ERROR and dropped the withdrawal. The address is now acquired before the commit, and only for the non-DASH branch that needs it. The rule both share, and the one I should have applied when writing these: acquire the destination before anything irreversible. TWO CROWDNODE CONTROL-FLOW BUGS: - `resetAddressAndApi()` returned silently on failure while `retrySignup()` went on to call `signUp()`, which dereferences `_accountAddress.value!!` — so it either signed up on the PREVIOUS identity or threw on null. It now returns whether it succeeded, and signup is gated on that. - `recheckState()` reaches `getOrCreateAccountAddress()` when nothing is persisted, and StakingActivity calls it from a bare `lifecycleScope.launch` while building its navigation graph. Uncontained, opening staking crashed. ONE WRONG ERROR CHANNEL: - `TransferDashViewModel` cleared `onBuildTransactionParamsCallback` to null, which does NOT suppress the observer — `SingleLiveEvent` marks a null value pending just the same, and the fragment forwarded that null into a non-null navigation argument. It now emits `onAddressCreationFailedCallback`, the event the fragment already observes for exactly this case. I invented a mechanism where one already existed. ONE MISSED CALL SITE: - The home screen's Topper shortcut (`WalletFragment`), which reaches the address through `ShortcutsViewModel.getTopperUrl()`. My enumeration grepped for the accessors rather than for transitive callers, so it found the other two Topper entry points and not this one. Swept all five transitive helpers afterwards; every caller is now guarded. Suite: 161 failures across the six touched modules, none new. ktlint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(crowdnode): guard signup and linking when no account address exists Blocking finding on #1570, and once again a consequence of my own previous fix: containing the exception in `recheckState()` left `_accountAddress` null, which only MOVED the crash from staking startup to the next user tap. `StakingActivity` installs its navigation graph regardless of that failure, and its error observer only handles `MISSING_PRIMARY`, so `NewAccountFragment` is still reachable — and it enables its action on accepted terms alone. `signUp()` and `linkOnlineAccount()` then force-unwrapped `_accountAddress.value!!`. Both now go through `requireAccountAddress()`, which reports the same `apiError` the startup path does — the same transient condition, the same retry — and returns without acting. `getAccountUrl()` had the third force-unwrap of the same value; it is now nullable, and its single caller declines to open a browser on a "null" URL rather than doing it anyway. Swept the rest: no force-unwrap of the account address remains. The `!!` left in `CrowdNodeApi` (`linkingApiAddress`, `accountDashAddress`) belong to the restore path for an ALREADY-established linked account and are not reachable from this null state. Suite: 161 failures across the six touched modules, none new. ktlint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(crowdnode): report and retry a missing account address, don't fail silently Blocking finding on #1570. My previous guard was fail-closed but SILENT, which is its own defect: linking did nothing at all, signup returned without explanation after the user had already authenticated, and repeated taps could never recover once the engine came up because nothing re-ran initialisation. Two changes, matching the two halves of the finding. REPORT. `StakingActivity.observeCrowdNodeError()` is observed at the ACTIVITY level, so it already covers the new-account screen — `handleCrowdNodeError` just had no branch for this type and fell through its `MISSING_PRIMARY` check. It now shows the error dialog for `ReceiveAddressUnavailableException` too, which is what puts anything at all in front of the user on that screen. RETRY. `requireAccountAddress()` now re-runs `getOrCreateAccountAddress()` when no address is held, instead of latching the outage. The failure is transient — the SDK engine had not bound yet — so each attempt genuinely re-tries and the flow recovers by itself once the engine is up. `signUp()` and `linkOnlineAccount()` move into `viewModelScope.launch` to accommodate the suspend call; both already had fire-and-forget semantics. `getAccountUrl()` keeps its nullable return without a retry: it opens a browser link rather than driving an action, so declining is the whole of the correct behaviour there. Rebased onto e80f26085 (#1571); no conflicts. Suite: 161 failures across the six touched modules, none new. ktlint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(coinbase): bind the deposit address instead of force-unwrapping it CodeRabbit nitpick left over from the 5009da6d9 review (its other comment there, on CrowdNode signup/linking, is already fixed). When I moved the address acquisition ahead of `commitSwapTrade`, the value became nullable — null on the DASH branch, which sells rather than depositing — and the use site force-unwrapped it. It is non-null by construction, guarded by the same `inputCurrency != DASH` condition that populated it, but that invariant is non-local: two branches separated by the trade commit have to agree. Bind it instead, so drift fails the commit rather than throwing on a null address. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(receive): refuse on COMMITTED ownership, not the pipeline flag The post-cutover refusal keyed off `CutoverUiDataService.isCutoverActive()`, which is the SDK pipeline's own flag: it is published only once the gated collector has cleared the wipe fence, bound a wallet and started the pipelines. On startup with a persisted CUT_OVER state it is therefore false for the whole of the initialization window — and in that window a receive or integration request got null from the SDK read, sailed through `requireNoEngineAddressExpected`, and fell through to `currentReceiveAddress()`, which serves the HELD dashj chain's frozen pointer. That is the already-paid address SR-03 is about, reached with the fail-closed change in place. Ownership is now answered from the persisted CUTOVER_STATE instead: the gate collector publishes it as soon as it observes it, and a caller that arrives before the first emission gets a bounded blocking read of the state. If even that cannot be resolved the answer is "owned", because an unknown ownership guessed as "dashj" is exactly how the frozen address gets served, while guessing "engine" costs a retry on a screen. The gate's own fail-open catch is kept for the pipeline (do not start the SDK on a state you could not read) and deliberately not shared with the refusal, which needs the opposite direction. The live read now waits for the binding on committed ownership too, so the ordinary startup window resolves into a short wait rather than a refusal. The decision itself is extracted as `decideLiveReceiveAddress` so the rule the defect broke — post-cutover NEVER reaches the dashj fallback — is pinned without standing up the Hilt graph WalletApplication needs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(receive): revoke the binding before publishing disabled ownership Deactivation published `_cutoverActive = false` and then called `unbindReceiveAddressWallet()`, with the flag update outside `receiveAddressLock`. In between, the two accessors disagreed in the worst possible way: the synchronous overlay serves the cache on the binding GENERATION alone, so it still handed out the retired wallet's address, while a live reader that had already seen the flag fell through to the held dashj chain's frozen one. Both halves of that window advertise an address nothing owns any more — on a rollback, or on a Reset Wallet that deliberately does not restart the process. The two are now one lock-protected transition: binding and cache first, flag last, under `receiveAddressLock`, so every reader that takes the lock sees either the whole old state or the whole new one. The three sites that published the disabled flag (the eager ownership collector, the pipeline gate, and the wipe-fenced/held path) all go through it. Committed ownership is deliberately NOT touched there: it is the persisted gate's answer, and a pipeline that stops while the gate still says CUT_OVER must leave it standing, or the refusal added in the previous commit would fall through to dashj precisely while nothing may answer for the wallet. The regression test samples the cache from inside the transition — an unconfined collector of `cutoverActive` is resumed on the thread that publishes the value — because the window is otherwise a few nanoseconds wide and polling for it would be a coin flip. Also drains the pipeline's first read in `inFlightReadCannotRepublishAWipedWalletsAddress` instead of assuming one `runCurrent()` covers a `withContext(IO)` hop: same expectation, no longer a race that fails under a different test ordering. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(receive): fence EVERY completed read, not only the successful ones `lastPublishedReceiveTicket` advanced only when a read came back with an address, and the completion gate ended in `?: address`, handing the read its own result whenever the cache was empty. On a cold cache that let a superseded read publish: read 1 takes A and parks in the FFI a payment lands on A — the engine's pointer has moved past it read 2 finds the engine unavailable, publishes nothing, leaves the fence read 1 returns and publishes A so an already-paid address reached the Receive screen *after* a newer read had established that the engine was no longer answering. The fence records COMPLETION ORDER, and a failed read completes just as definitively as a successful one, so it now advances for both. A failed read still HOLDS the warm cache rather than clearing it — dropping it is what sends the overlay back to the frozen dashj address — it simply does not get to publish, and a rejected read with nothing cached now answers nothing instead of smuggling its own result past the gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(receive): carry the binding identity with the self-transfer destination `unadvertisedDestinationLive()` read the engine's internal address, which validates its own generation on the way out of the FFI, and then took a SEPARATE `isCutoverActive()` snapshot to decide what to do with it. A Reset Wallet fits between those two reads: wallet A's address is validated, A is wiped, wallet B activates and commits, and B's `true` then ACCEPTS A's address. The funding wallet is chosen independently downstream, so B's own funds get paid to an address only the erased wallet can spend — and the self-transfer callers have no way to notice. The address now travels with the binding GENERATION it was read under (`BoundEngineAddress`), and the caller re-validates it with `receiveBindingStillCurrent()` as the LAST thing it does, after the ownership answer the decision is made with. A bumped generation — or a cutover that has since rolled back — is refused, and post-cutover a refusal is a throw, never a dashj key. Ownership for that decision also comes from the committed state now, for the same reason as the receive accessors. What remains is the window after the final check, which this seam cannot close: the destination would have to be re-validated at spend time, where the funding wallet is actually chosen. That is documented at the call site rather than pretended away. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(coinbase): take the confirm single-flight on the tap, not in the coroutine The confirm button launched a coroutine and only then did anything the user could see. Its first step — reading the deposit destination — blocks: post cutover it waits for the SDK engine to bind and then takes the engine's wallet-manager lock. Nothing is on screen for that whole wait, so repeated taps each started their own coroutine, and every one that got an address went on to place its OWN order under a fresh idempotency UUID (and, on the bank path, to repeat the fiat deposit). Coinbase cannot collapse those: different UUIDs are different orders by definition, so the user is charged twice. The guard is now taken synchronously in the click listener, before anything is launched — the only point at which two taps are ordered against each other — and held through address preparation, the purchase and the handoff to 2FA. It is deliberately NOT released after navigating: the navigation transaction has not executed when the coroutine ends, so the review view is still attached and still clickable, and a queued tap would place the second order. `onResume` re-arms it when the user comes back from 2FA with nothing running. Every retryable path — an unavailable destination, a declined purchase, an unexpected throw — gives it back in a `finally`, so a failure cannot leave the button dead. `buyDash()` asserts it holds the guard. Reaching the deposit + placeBuyOrder pair twice IS the double purchase, so a wiring mistake fails loudly on the review screen instead of billing the user, and the invariant is testable without standing up the fragment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * style: drop a redundant !! and merge two stacked KDoc blocks `markAddressAsTransferInAsync(to!!, ...)` sat two lines below the elvis binding that made `to` non-null, so the assertion claimed a risk that no longer exists and hid the guarantee the binding provides. `requireAccountAddress()` carried two consecutive KDoc blocks. Only the second attaches to the declaration, so the first — the explanation of the startup path that leaves no address — was invisible to generated documentation and to anyone reading the signature. One block, both explanations. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(receive): drain the pipeline's first read instead of racing it Six receive-overlay tests asserted the cache was warm immediately after `service.start(); runCurrent()`. The pipeline's first engine read hops onto a REAL dispatcher (`refreshNativeSplit` wraps it in `withContext(IO)`), so one `runCurrent()` does not guarantee it has landed — the assertion passed on timing, and failed under a different test ordering or a busier JVM. Two of them (`inFlightReadCannotRepublishAWipedWalletsAddress`, `unadvertisedDestinationNeverFallsBackToTheWarmReceiveCache`) were observed flaking here. Same expectation, drained with the existing `pumpUntil` helper: a build that genuinely serves the wrong address still fails, it just no longer depends on winning a race. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(sdk): let nothing escape the bind-heal job `BindHealL1Starter.arm()` launches on a long-lived service scope with no exception handler, and only the `startL1()` call was guarded. The wait itself can throw: `Flow.first { }` raises NoSuchElementException if the bind feed COMPLETES without ever emitting true. The production feed is a StateFlow so this is unreachable today, but the seam takes any Flow — `SdkBindRetryService` already defaults one to `emptyFlow()` — and in the host tests it is reached routinely, where the escaped exception lands on the JVM default handler and kotlinx-coroutines-test then reports it against whatever unrelated test starts next (`UncaughtExceptionsBeforeTest` in `AssetLockKindResolverNegativeCacheTest`, which has nothing to do with it). A background job on a shared scope must not throw into the default handler. The wait and the start are now one guarded body that logs instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(receive): never let a remembered "dashj owns it" authorise the fallback The committed-ownership lookup short-circuited on whatever the gate collector last observed, and that memory was ambiguous in the one direction that costs money. Two ways a remembered `false` lied: - an ERROR. The memory was fed from cutoverUiActive(), whose catch turns a failed read of the persisted state into `false` so the PIPELINE does not start on a state it could not read. The receive refusal needs the opposite failure direction: `false` there means "dashj owns the key chain", which is precisely what authorises WalletApplication to serve the HELD dashj wallet's frozen, already-paid address. - STALENESS. Start on DUAL_RUNNING, observe `false`, the cutover then commits — and until the collector is resumed the lookup still answers `false` for a wallet whose key chain the engine has already taken over. A cold-cache beginReceiveReadAwaitingBinding() then returns null and the caller falls through to the frozen address. That is SR-03 itself, reached through the machinery built to prevent it. So the observation becomes genuinely tri-state — observed true, observed false, UNKNOWN — fed by its own collector over cutoverOwnershipCommitted() (the same key, without the gate's fail-open catch), and a failed read is recorded as UNKNOWN rather than as dashj ownership. The gate keeps its own catch, because it still must not start the SDK on an unreadable state. And the memory may now only answer in the fail-CLOSED direction: a remembered `true` short-circuits (that is the startup window it exists for), while anything else resolves against the persisted state and fails closed if it cannot. The cost is a DataStore read — served from memory after the first one — on a path that is already blocking in the FFI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(receive): order the receive snapshots at the engine read, not before it The ticket fence throws away a read whose ticket is older than the last published one. That is only sound if a higher ticket means a NEWER engine snapshot — and taking the ticket before the read was dispatched did not establish that: the refresh takes ticket 1 and suspends at withContext(Dispatchers.IO) a live read takes ticket 2, reads address A and publishes it the engine advances to B the queued refresh finally reaches the engine and obtains B — under ticket 1, so its own fence rejects it leaving the already-used A in the cache and handing A back to the refresh's caller: the reused address SR-03 is about, served by the machinery built to prevent it. Plain thread scheduling between the ticket and either FFI call inverts them the same way. Entry order into the engine's wallet-manager write lock IS snapshot order, but it is not observable from out here, so the stamp and the snapshot are now taken together under a dedicated lock: reads queue in the order they will read, and the ticket each carries is the position it actually read in. An ordering stamp taken merely "just before" the FFI call would leave the same preemption window, which is why the serialized form was chosen. It costs no real concurrency — the engine already serializes these reads behind its own write lock; what changes is that the queue is now ordered where the fence can see it. The lock is NOT receiveAddressLock (an unbind must never wait on an FFI read) and is strictly outside it. Publication still happens after that lock is released, so the fence is still load-bearing — which is also why parking inside the fake FFI read no longer stages the out-of-order window, and the two tests that pin it now hold their read at an explicit publish barrier instead. Also moves the stranded KDoc that described taking "the binding and a read ticket together" onto beginReceiveRead(), where Kotlin can attach it; it sat above beginReceiveReadAwaitingBinding()'s own block, documenting nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(receive): stop an older deactivation revoking a newer activation The independent deactivation collector exists so a revocation never waits behind a pipeline parked in an uncancellable FFI read. But nothing ordered it against the gated collector that GRANTS ownership, and observing an emitted `false` is not the same step as applying it: the production scope is Dispatchers.Default, so the collector can be descheduled between the two while CUT_OVER commits, the gated collector passes its own fresh-state check, publishes _cutoverActive and binds the pipeline. The stale `false` then landed on that NEW binding and tore it down. Being deactivation-only, the `true` behind it did nothing, and the gated collector had already consumed its own `true` and was parked on the running job — so nothing re-granted. beginReceiveRead() refuses every ticker and tx-event refresh while the flag is false, so live receive and self-transfer reads waited and then failed until the next activation trigger or a process restart. Revoking EARLY is safe, which is what the previous round assumed; revoking LATE is not. Both decisions now run under one ownershipDecisionMutex and each is taken from a persisted-state read made while it is held, so whichever runs second also read second and an observation that has been overtaken can no longer undo what overtook it. The DataStore gives us no version to compare, and a shared stamped feed would have had to couple the two collectors' backpressure — which is exactly the independence the deactivation collector exists for. The lock is held only across that read and the flag write, never across a pipeline run or a blocking engine call, so a parked runPipelines() still cannot delay a revocation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(receive): read the ownership state so a failed read stays a failure The receive refusal was given its own feed last round precisely so an unreadable cutover state could not be recorded as "dashj owns the key chain" — the one answer that authorises serving the held dashj chain's frozen, possibly already-funded address. But the error never reached that feed. DashPayConfig.observe() is served from BaseConfig.data, which catches IOException and emits emptyPreferences(), so an unreadable preferences file arrived as a MISSING key; CutoverState.fromStored(null) maps that to DUAL_RUNNING, and the ownership read published a plain `false`. A cold start with CUT_OVER persisted, an unreadable file, no binding and nothing remembered then had cutoverOwnershipCommittedBlocking() accept it, the live SDK read return null, and WalletApplication.decideLiveReceiveAddress() take its dashj fallback — SR-03 again, below the service boundary this time. BaseConfig gains observePreservingErrors(): the same read without that rescue, so an ABSENT key is still `null` (a real answer) while a FAILED read arrives as the IOException it was. DashPayConfig overrides it the same way observe() is overridden, so the locked production flags keep their policy on this path too. The ownership feed uses it and fails closed on the throw; the pipeline gate keeps its fail-open catch over the same flow, which is the opposite direction it needs. The existing regression threw from a mocked observe(), above the rescue it was meant to be about, so it never covered the real path; it stays for what it does cover and now says so. CutoverOwnershipUnreadableStateTest drives a genuine DataStore CorruptionException through the real DashPayConfig/BaseConfig stack over a real file instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(receive): prove the activation survives, and record what this test cannot see Two additions to `aDeactivationAppliedLateCannotRevokeANewerActivation`. It now asserts the activation survives INTACT — a tx event after the stale `false` must still drive the cache from the engine — rather than only that the ownership flag stayed true. A flag that outlives its pipeline would have passed the original assertions. And it records a limitation I measured rather than assumed. The test is attached to the deactivation collector only POSITIONALLY: `laggingGateConfig` routes by call order and `start()` launches that collector first. I swapped the two `scope.launch` blocks to check, and the test still PASSES — under the fix both collectors re-read the state and both decline, so the stale observation lands on a different path with nothing observable changing. It would quietly stop exercising the scenario instead of going red. That is worth writing down rather than leaving as a trap: this PR has repeatedly shipped tests that passed for the wrong reason. The mutation that matters does still bite — deleting the overtaken-observation guard fails the test — so it covers the defect today; what it cannot do is notice if a future edit decouples it. Fixing that needs the two collectors distinguishable at the config seam, which a mock on a single key cannot provide. Suite: 176 failures, identical by name to the reviewed head — none new. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(receive): stub the ownership read the service actually calls `stopForWalletWipe_waitsForADeferredCoinsReceivedNotifyAlreadyInProgress` arrived with the rebase onto afa91bff2, after the commit that moved the ownership feed from `observe(CUTOVER_STATE)` to `observePreservingErrors(CUTOVER_STATE)` had retargeted the seven stubs that existed then. Its `mockk<DashPayConfig>` is not relaxed, so the unstubbed call raised MockKException inside the collectors, no pipeline ever ran, and the tx never reached the display store — surfacing as `assertTrue(txid in store)`. Only the stub's method name changes. Not one assertion, value or `runCurrent()` is touched: the test keeps its wipe ordering and its timing assumptions exactly as upstream wrote them. This is not the extra-scheduler-turn it looked like. The grant site already did `cutoverUiActive().first()` inside `pipelineMutex` before this PR (same line, same place); serializing it under `ownershipDecisionMutex` added no read, only an uncontended mutex acquire, and `runCurrent()` drains every task at the current virtual time anyway, so an extra immediate resumption could not have broken it. Confirmed by construction: the stub change alone turns the test green, 3/3 with `--rerun-tasks`. Swept for the same mistake elsewhere — this was the last `observe(DashPayConfig.CUTOVER_STATE)` stub on any config fed to CutoverUiDataService. The remaining ones (CutoverTxSeamServiceTest, CutoverCoordinatorTest, SdkBlockchainStateServiceTest, WalletDataAdapterSeamRoutingTest) back services that still call `observe` in production and are correct as they stand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(receive): never let a Receive failure dismiss the payments flow Receive is a ViewPager2 page of PaymentsFragment, built alongside Send (and Internal when the flag is on) -- in the two-tab layout it is even instantiated as the adjacent page while Send is selected. The address request starts in onViewCreated on viewLifecycleOwner.lifecycleScope, which is not cancelled when the user switches tabs, so a slow cold-cache read can fail long after Receive went offscreen. The catch block handled that by popping the back stack. findNavController from a pager child resolves the parent NavController, so that closed the entire Payments destination out from under whatever tab the user was actually using -- for a failure that is transient (the engine may simply not be bound yet) and belongs to a page they cannot see. Handling now stays inside Receive and never navigates. The toast is gated on the view lifecycle having reached RESUMED: ViewPager2 caps an offscreen page at STARTED and lifts only the selected page to RESUMED, so that is exactly the "the user is looking at Receive" test. Offscreen the failure is silent. Either way no address is advertised -- ReceiveInfoView with no address set leaves the address line and the QR code empty, which is the point of SR-03: better nothing than the held dashj chain's frozen, already-paid address. The threshold is extracted as shouldSurfaceAddressFailure and pinned by a plain JUnit test, following tabIdsFor/PaymentsTabsTest. That the handler no longer navigates is not covered: it needs Robolectric, and every Robolectric test in this module currently fails on a missing conscrypt_jni. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(receive): retry the address read when Receive is selected Removing the popBackStack() left the other half undone: a read that failed while Receive was the offscreen pager page set no address and never tried again. ViewPager2 RESUMES the retained page rather than recreating its view, so the selected page stayed blank for good even once the engine had bound. Move the request into PaymentsViewModel as a small state machine (Idle/Loading/Available/Unavailable) and ask again from repeatOnLifecycle(RESUMED). The state is the single-flight gate, taken with an atomic getAndUpdate: only a never-asked or FAILED state starts a read, so a resume during the read's 5s binding wait is a no-op and a loaded address is never re-read on a tab switch. It runs on viewModelScope, not the view's, because pausing must not cancel a read whose blocking wait would hold its IO thread anyway and whose cancellation would let the next resume overlap it. Offscreen behaviour is unchanged: no navigation, and the toast is still gated on shouldSurfaceAddressFailure(), so only a RESUMED page speaks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(receive): revalidate the address instead of latching the first one Once requestReceiveAddress() published Available(A) the gate refused every later read, and nothing else ever wrote the state. The ViewModel outlives both a tab switch and a view recreation, so Receive could show A, be paid on A — moving the engine's next-unused pointer to B — and then re-render the retained A after a configuration change, with the fragment's onViewCreated prefetch and its repeatOnLifecycle(RESUMED) retry both no-ops. That is the address reuse this branch exists to fix (SR-03); before the retry/latch change each recreated view had performed a live read. Only an in-flight read now refuses a new one. Available is startable, so a view recreation or a resume revalidates: the engine's next-unused read is idempotent while the address is unpaid, so nothing changes until a payment lands, and when one does the next render advertises the new address. Single-flight is unchanged — the atomic getAndUpdate CAS still admits one caller, Loading is still the only refused state, and the read still runs on viewModelScope because its binding wait is an uncancellable 5s sleep. Loading now carries the address it is checking so revalidating does not blank an already-rendered QR for those five seconds, and the fragment renders ReceiveAddressState.address rather than switching on the state. A revalidation that fails drops the address and shows nothing: this read exists to find out whether that address is still unused, and an engine that cannot answer cannot vouch for it. That is recoverable — Unavailable is startable and the RESUMED hook asks again — whereas re-advertising a used address is the defect itself. ReceiveInfoView gained clearInfo() so a failure can actually take down a QR that is already on screen; previously the Unavailable branch only toasted and left a stale address visible on a retained view. The quiet-offscreen behaviour is unchanged: no navigation in any branch, and the toast stays behind shouldSurfaceAddressFailure(). Tests cover the regression (A then B across two requests), idempotence while unpaid, the carried address during an in-flight revalidation, fail-closed plus recovery, and single-flight — the last one rewritten to detect a duplicate read from inside the stub, because a trailing verify() raced the duplicate's IO thread and passed with the gate removed. Each piece of the fix was reverted in turn and a named test dies for each. Not covered, as before: anything needing the fragment itself, which is Robolectric and so hits the missing conscrypt_jni native library. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(receive): a held address is not a successful revalidation 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> * fix(coinbase): release the confirm flight only to the attempt that owns 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> * fix(receive): decide the ownership no-op under the ownership mutex 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> * test(cutover): route the cutover-state fixtures by caller, not by subscription order The wipe tests meant…
2 of 3 tasks
HashEngineering
added a commit
that referenced
this pull request
Oct 27, 2026
…flaky tests (#1632) * fix(sdk): keep the L1 funding gate on the height rule The shared spend gate (wallet->shielded and SDK L1 sends) opened on scanCaughtUpToTip, which since 2026-09-21 also accepts the iOS aggregate rule: a filter shortfall of up to 0.3% of chain height (~4,600 blocks on testnet). The pipeline-lag veto cannot see that gap while the wallet cursor is unknown (0), so a spend could open with thousands of blocks unscanned. When the cursor is known, the §34 stall parks it with the filter cursor and the veto closed the gate anyway, so requiring the height rule only closes the unsafe case. The gap is measured against the highest tip the snapshot knows (max(bestKnownTipHeight, filterTarget)): dash-spv restores the header target from the last peer report at engine start, which can sit below the stored header tip the filter target already names. The display predicate is unchanged. Raised by CodeRabbit on #1584. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(sync): release the replay wake lock while the replay cannot progress Post-cutover a replay keeps the blockchain service alive and holds a PARTIAL_WAKE_LOCK with no timeout. While offline (NETWORK impediment) or once the filter-stall watchdog has spent its restarts on a wedge, the replay cannot move, yet the CPU stayed awake for as long as it was stuck. The service still stays up (stopping it re-walks from the durable watermark and loops through the replay restart alarm); only the lock is released. FilterStallWatchdogDecider.standingDown marks the wedge, cleared when the cursor moves, events resume, the scan is no longer behind, or the engine stops. TIME_TICK is not delivered while the CPU sleeps, so the impediment update re-acquires the lock when the replay is due again. Acquire and release run under one guard and re-check the inputs, and onDestroy retires the lock so a late network-callback coroutine cannot re-acquire it after the teardown's release. Raised by CodeRabbit on #1584. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(sync): gate the bind-heal L1 start on the issuing service lifetime kickSdkEngines checked serviceLifetime once, after the bind join. A shutdown landing after startIfEnabled() returned (arm() does not suspend, so cancelling the kick cannot stop it) let arm() run after the shutdown's cancel; that waiter survived the teardown and started L1 on a later bind heal with no foreground service. The kick now arms only while its lifetime is current and passes a stillCurrent predicate that the waiter re-reads after the heal. arm and cancel are serialized and the shared job fields are volatile. Raised by CodeRabbit on #1584. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(sync): fix the race-dependent restart and cutover UI tests PlatformSyncEngineRestartTest.resume_isRearmableAcrossRepeatedStopRestartCycles went stale within #1568: a later commit there made stopSdkEngines() cancel a kick still waiting on its bind, so three back-to-back cycles collapse into one or two starts. Each cycle now waits for its own start, and a new test pins the cancel-on-stop behaviour. Six CutoverUiDataServiceTest cases failed depending on timing: since #1570 refreshNativeSplit reads the receive address via withContext(Dispatchers.IO), a real-thread hop inside every runTest refresh, so runCurrent() returned before the refresh published. The read's dispatcher is now injectable (default IO); the test builder passes the runTest dispatcher, and the two parked-read rollback tests keep the real IO hop they gate on. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs: drop the obsolete correction request from the issues record rust-dashcore#1032 as filed already tells implementers to resolve the key source in wallet_checker.rs from the fully keyed account. Raised by CodeRabbit on #1584. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(sync): re-take the replay wake lock when the filter wedge clears Once the filter-stall watchdog had spent its restarts, the minute tick released the replay wake lock. Only the tick or an impediment change re-acquired it, and ACTION_TIME_TICK is not delivered while the CPU sleeps, so a replay that recovered on its own ran unprotected until an unrelated wakeup. The replay hold moves into ReplayWakeLock. It keeps the guard, the teardown retirement, the re-check before release and the owner-only teardown release. FilterStallWatchdogDecider.standingDown is now backed by a StateFlow, and the blockchain service re-checks the lock the moment it clears. The clear is also event-driven: the progress feed (a filter cursor off the stand-down height) and the wallet-event tap end the stand-down at once, instead of waiting for the watchdog's one-minute delay, which also stalls while asleep. The spend gate's pipeline-lag veto now measures against the same highest-known tip as the height rule. headerTarget alone can sit below the stored tip after a restart. Regression test: ReplayWakeLockTest drives the real decider through exhaustion, release and recovery with no tick or network change. Raised by PastaClaw and CodeRabbit on #1632. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(sync): seed the NETWORK impediment from connectivity at service start The blockchain service's NETWORK impediment was only ever set by its network callback, and a passive registerNetworkCallback reports nothing while no network matches. A service created while already offline therefore read as online, and a replay persisted from the previous session held the no-timeout replay wake lock in airplane mode. onCreate now seeds the impediment from the default network's capabilities (internet + validated, the callback's own rule) before it registers the callback, so every callback update overrides the seed. An unreadable connectivity state answers "usable", the old assumption. Raised by PastaClaw on #1632. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(sync): one definition of the replay wake-lock hold rule The minute tick computed the replay's hold decision inline, while ReplayWakeLock re-read replayWakeLockDue() before releasing. They agreed, but a later edit to one copy alone could leave the tick asking to release while the re-check still read "due", refusing the release on every tick and keeping the no-timeout lock held. The tick now uses replayWakeLockDue() too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(sync): an ended lifetime's kick cannot replace or cancel the current bind-heal waiter A startup kick passes its lifetime check, then shutdown and the next service start can complete before it reaches arm(). arm() cancelled the waiter the new lifetime had armed and installed one that refuses to start, leaving the running service with no L1 engine and nothing waiting to start it. The kick's cancel() on a successful start had the same hole. arm() now checks stillCurrent first, under the monitor, and ignores an obsolete arm; the kick's cancel takes the same check (shutdown's cancel stays unconditional). The waiter still re-checks after the heal. Raised by PastaClaw on #1632. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(sync): seed connectivity after registering the network callback The start-up connectivity read ran before registerNetworkCallback. A network lost in between was never reported (nothing matched any more), so the service read as online and a persisted replay could take the no-timeout wake lock offline. The read now runs after registration. ImpedimentGate keeps it from overwriting the callback: every callback update bumps a counter under one lock, and the seed marks the service offline only if no update has landed since the token taken just before registering. The same lock now serializes every change to the impediment set (the network callbacks on IO, the keystore observer on main), and the callbacks run one at a time in delivery order, so an onLost can no longer be processed before the onAvailable it follows. Raised by CodeRabbit on #1632. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(sync): re-take the replay wake lock even when the observer misses a wedge flip ReplayWakeLock observed the filter wedge as a StateFlow<Boolean>. A collector slow to run can miss intermediate StateFlow values, so clear → re-wedge → clear arrived as the "not wedged" it had already seen, while the minute tick released the lock during the re-wedge. Nothing re-took it, and a screen-off replay could stay asleep. FilterStallWatchdogDecider now counts its true → false transitions (standDownClears, updated with the flag under one lock), and the service observes that count (L1ShadowSyncService.filterScanWedgeClears). A count the observer has not seen always means a clear happened, so it re-checks; reacquireIfDue reads the current state, so a clear already followed by a new wedge acquires nothing. The baseline is read when observation starts, not on the collector's first run. The teardown retirement guard is unchanged; no polling. Regression: ReplayWakeLockTest runs the clear → re-wedge → clear sequence with the collector held back on a test dispatcher and asserts the lock is re-taken once it runs; plus a clear superseded by a new wedge, and a clear between registration and the collector's first run. Raised by Codex review. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(sync): give a restarted engine its own filter-stall window onEngineStopped() cleared the stand-down flag but kept the stopped run's stall timing (lastHeight / lastAdvanceMs). A run restarted at the height the watchdog stood down at was then read as "same cursor, still since long ago" on its first check and stood down again within a minute, so the replay lost its wake lock with none of the stall window spent. Drop the per-run timing on stop (the restart budget stays per process), and synchronize onEngineStopped() with onCheck() so a check still in flight on the cancelled watchdog cannot write the old baseline back. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(sync): default-network impediment, guarded wake-lock rechecks, bind-heal stop generation Three review blockers on #1632: - NETWORK impediment from the default network. The callback was registered for any network with internet + validated, so a validated secondary network cleared the impediment while the default route was unusable, and one matching network losing validation set it while another was up. It is now a default-network callback (DefaultNetworkTracker): offline unless the default network has internet and is validated. A loss of a former default is ignored in either delivery order. - Replay wake lock rechecks under one monitor. reacquireIfDue() read the decision outside the lock's monitor and could only acquire, so a recovery that read "due" just before a network loss kept the no-timeout lock offline until the next tick. tick() and every event now run recheck(), which reads tickHolds || replayDue() and acquires or releases under the monitor, so the loss's own recheck releases. The tick records the holds only it can see (engine start, durable save). - Bind-heal start carries the engine stop generation. The waiter samples it before its lifetime/teardown checks and passes it to startIfEnabled(expectedExternalStopGeneration), so a shutdown that passes the checks and finishes the engine stop before the start keeps L1 down. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(sdk): close the L1 funding gate while the engine waits for peers WAITING_FOR_CONNECTIONS maps to CONNECTING, and after a route loss or a restart that snapshot can keep the previous session's heights, which pass every height rule while the engine cannot see a new block. The gate admitted it, so a wallet->shielded transfer or an SDK L1 send could open with sync offline. CONNECTING now fails closed like IDLE; the display already reads it as offline. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(sdk): close the L1 funding gate while the wallet cursor is unknown The pipeline-lag veto only applied when walletSyncedHeight > 0, so a snapshot with filters at the tip and no cursor evidence opened a spend over a UTXO view nothing proved current. An unknown cursor now closes the gate ("SDK L1 wallet scan cursor is not known yet"). It cannot wedge a synced wallet: the cursor is seeded at engine start from the SDK's durable WalletEntity.syncedHeight, and dash-spv emits SyncHeightAdvanced for every wallet each committed filter batch covered, matches or not. On the emulator a fresh testnet wallet reported a non-zero cursor from its first batch, and its restart was seeded from the durable value. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(sync): ignore capability updates for a network that is not the default DefaultNetworkTracker.onCapabilitiesChanged made whatever network it was told about the current default. A late update for the former default, delivered after its successor's onAvailable, took it back (and could mark the device offline), and that network's later onLost then matched and left the tracker offline with the successor still up. Only onAvailable now changes the default; an update for any other network returns no verdict, and the callback updates the impediment and re-checks only when there is one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(sync): serialize the watchdog's event-driven clears with its stall decisions onFilterProgress() and onWalletEvent() read the stand-down and cleared it outside the monitor onCheck() holds. A callback that read an old stand-down could clear a newer one a check had just decided, and the clear re-took the replay wake lock on a wedged engine until the next check. Both are now synchronized on the decider, so the read and the clear judge the state the last decision left. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(sync): let only a network-callback verdict supersede the offline seed ImpedimentGate counted every network-callback update, including callbacks the default-network tracker answered with no verdict (an onAvailable whose capabilities could not be read, a loss of or update for a network that is not the default). One of those landing during registration skipped the start-up seed, and a service with no usable network read as online. networkCallbackUpdate now takes the verdict its block applies and counts only a non-null one; onUnavailable returns its offline verdict, and the teardown clear goes through update {}. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(sync): read the replay wake lock's other holds live, not from the tick QA on #1632 (D-1632-01): on a network loss during a large replay the replay wake lock was released only at the next minute tick (53 s and 11 s on the emulator). recheck() decided with tickHolds || replayDue(), and tickHolds was the tick's engineStarting || durableLagging, recorded once a minute; the durable save kept it true, so the loss's recheck could not release. ReplayWakeLock now takes an otherHolds supplier and reads it in every recheck. BlockchainServiceImpl computes the same two holds live, the durable lag through DurableHeightTracker.msSinceChange so only the tick's observe() ages it, and the tick records then rechecks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(sdk): an unknown wallet cursor is refused before a deferred build reserves #1616's aDeferredBuildWithNoCommittedCursorRecordsNoHeight built a deferred Maya deposit with walletSyncedHeight = 0 and expected a null reservation height. With this branch's funding gate an unknown cursor is refused first ("SDK L1 wallet scan cursor is not known yet"), so that build is no longer reachable. The test now asserts the refusal and that nothing was reserved; reservationHeightLowerBound()'s null stays as the defence behind the gate. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #1564 — target that branch, not
master. The diff here is the 65 commits on top of it.What this is
Field logs from one mainnet install showed a v12 upgrade that ran out of memory and stopped syncing. Chasing that produced two kinds of work: fixes to the cutover and sync lifecycle, and a measured account of what is actually wrong — including two defects that lose money, both of which turned out to be SDK-side rather than ours.
docs/upgrade-memory-and-sync-plan.mdis the record. §30 is a register over the whole thing; start there.Defects that lose money (neither is fixed here)
b837bcb2…, 29,000 duffs, block 1555216; receiver synced through 1555217 with no record of the destination address at alldashpay/rust-dashcore#1032491fc9f1…, 10,000 duffs, block 1555561; receiver synced through that block, zero rowsdashpay/platform#4740. Field workaround in §29.4Both were reproduced end to end on testnet with the sender's broadcast confirmed independently, so an absence on the receiver is a real miss rather than a failed send.
Code changes
Cutover and bind:
ACTION_USER_PRESENTcomes from SystemUI, so a not-exported registration could never match it. Verified on device: the receiver fired for the first time across three test roundsSync lifecycle (this is the part that matters for the reference install, see §31):
Boot path:
dataSyncFGS fromBOOT_COMPLETEDwith no exemption. Product decision recorded in §25.5: background catch-up after a reboot is not requiredDiagnostics:
ALARM-DIAGlogging to settle whether the periodic alarm path works without a battery-optimisation exemption (§28)What is verified on hardware vs reasoned
§30.3 has the table. Three are proven on device: the package-replacement start, the unlock receiver firing, and the corrected coverage diagnostic. The two boot changes rest on compilation and the documented platform behaviour, since the platform refuses the start either way.
ALARM-DIAGis pending a test on a phone that is not whitelisted.§24 measured a clean install plus restore end to end: 7m06s to tip, 536 MB peak native heap, zero teardowns, zero idle stops, balances identical to pre-wipe.
Known gaps
no available addresses) because the SDK quorum source carries only current validators, and §26 confirms this hits freshly restored wallets too, not just upgrades.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation