From b994bb6a1634484efa0d929c30134c52e3335366 Mon Sep 17 00:00:00 2001 From: HashEngineering Date: Fri, 28 Aug 2026 10:58:32 -0700 Subject: [PATCH 001/181] fix: restart the SDK L1 engine on every service start, not once per process (MO-995, MO-998) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../wallet/service/BlockchainServiceImpl.kt | 26 +++ .../platform/PlatformSyncEngineRestartTest.kt | 183 ++++++++++++++++++ 2 files changed, 209 insertions(+) create mode 100644 wallet/test/de/schildbach/wallet/service/platform/PlatformSyncEngineRestartTest.kt diff --git a/wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt b/wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt index cc80a185bc..24c7966a54 100644 --- a/wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt +++ b/wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt @@ -2148,6 +2148,32 @@ class BlockchainServiceImpl : LifecycleService(), BlockchainService { }.onFailure { log.warn("failed to wire SDK-sourced quorums for Platform", it) } + + // MO-995 / MO-998: restart the SDK's L1 (SPV) engine on EVERY + // service (re)start, not just the first one of the process. + // + // platformSyncService.resume() — the only in-process path that + // re-kicks the SDK engines — used to live solely in + // 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 came up from PlatformSyncService.init() (once per process, + // from WalletApplication). Any service teardown that stops 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 for the rest + // of the process: no incoming transactions, the sync UI stuck at + // "syncing", and sends/top-ups failing with "SPV client not + // started". Worse, the idle detector samples this same engine's + // progress post-cutover, so a dead engine reads as idle and keeps + // tearing the service down — a latch, not a transient. + // + // resume() is idempotent (single-flight bind + idempotent + // startIfEnabled), so the first service start of a process + // harmlessly re-kicks what init() already started. Scoped to the + // post-cutover branch: the pre-cutover path keeps its original + // resume()-at-peergroup-start behavior, unchanged. + platformSyncService.resume() } // FIX 2: keep the DASHJ_SYNC_DIAGNOSTIC toggle effective on a LIVE diff --git a/wallet/test/de/schildbach/wallet/service/platform/PlatformSyncEngineRestartTest.kt b/wallet/test/de/schildbach/wallet/service/platform/PlatformSyncEngineRestartTest.kt new file mode 100644 index 0000000000..452cc15b9f --- /dev/null +++ b/wallet/test/de/schildbach/wallet/service/platform/PlatformSyncEngineRestartTest.kt @@ -0,0 +1,183 @@ +/* + * Copyright 2026 Dash Core Group. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package de.schildbach.wallet.service.platform + +import de.schildbach.wallet.service.platform.sdk.L1ShadowSyncService +import de.schildbach.wallet.service.platform.sdk.ShieldedBalanceService +import io.mockk.coVerify +import io.mockk.every +import io.mockk.mockk +import kotlinx.coroutines.runBlocking +import org.junit.Test + +/** + * Host-JVM regression tests for the Kotlin-SDK engine RESTART contract on + * [PlatformSynchronizationService] — the mirror image of + * [PlatformSyncEngineTeardownTest]. + * + * MO-995 / MO-998 (v12.0.0 QA, both networks): the SDK's L1 (SPV) engine + * went down on a routine blockchain-service teardown and never came back + * for the rest of the process. Symptoms: incoming transactions never + * arrived (two Coinbase deposits lost for 2h10m on mainnet), the header + * sat at "syncing", and Buy Credits failed with "pre-broadcast: SPV client + * not started". + * + * The call-site half of that bug was in `BlockchainServiceImpl`: + * [PlatformSyncService.resume] lived only inside `checkService()`'s + * peergroup-start block, BELOW the `!dashjEngineMayStart` early return, so + * post-cutover it was unreachable and the engines only ever started from + * [PlatformSyncService.init] (once per process, from `WalletApplication`). + * That is fixed by calling [PlatformSyncService.resume] from the + * post-cutover branch of `BlockchainServiceImpl.onCreate` — a line in an + * Android service, not reachable from a host-JVM test. + * + * What IS testable, and what the fix now depends on, is the contract + * underneath it: **resume() must be able to bring the engines back up + * after a teardown has taken them down, any number of times.** That is not + * a given — [PlatformSyncService.shutdown] cancels the sync scope's + * children, and `stopSync()` does the same on a wallet reset. Cancelling + * the scope's `SupervisorJob` itself instead (an easy, invisible change: + * `syncJob.cancel()` / `syncScope.cancel()` in place of + * `cancelChildren()`) would leave every later `resume()` a silent no-op + * and reinstate exactly the MO-995 latch one layer down, with the + * BlockchainServiceImpl fix still in place and looking correct. + */ +class PlatformSyncEngineRestartTest { + + private val l1ShadowSyncService = mockk(relaxed = true) + private val shieldedBalanceService = mockk(relaxed = true) + private val identityRepository = mockk(relaxed = true) + + /** The engine kick is asynchronous (`syncScope.launch { bindJob.join(); … }`). */ + private val kickTimeoutMs = 10_000L + + private fun service() = PlatformSynchronizationService( + platform = mockk(relaxed = true), + platformRepo = mockk(relaxed = true), + analytics = mockk(relaxed = true), + config = mockk(relaxed = true), + walletApplication = mockk(relaxed = true), + transactionMetadataProvider = mockk(relaxed = true), + transactionMetadataChangeCacheDao = mockk(relaxed = true), + transactionMetadataDocumentDao = mockk(relaxed = true), + blockchainIdentityDataDao = mockk(relaxed = true), + dashPayProfileDao = mockk(relaxed = true), + dashPayContactRequestDao = mockk(relaxed = true), + dashPayConfig = mockk(relaxed = true), + giftCardDao = mockk(relaxed = true), + invitationsDao = mockk(relaxed = true), + usernameRequestDao = mockk(relaxed = true), + usernameVoteDao = mockk(relaxed = true), + identityConfig = mockk(relaxed = true), + topUpRepository = mockk(relaxed = true), + identityRepository = identityRepository, + walletDataProvider = mockk(relaxed = true), + walletSeam = mockk(relaxed = true), + sdkProfileQueries = mockk(relaxed = true), + sdkUsernameQueries = mockk(relaxed = true), + sdkIdentityVerifyQueries = mockk(relaxed = true), + sdkWalletBinder = mockk(relaxed = true), + nonInteractiveWalletUnlock = mockk(relaxed = true), + l1ShadowSyncService = l1ShadowSyncService, + shieldedBalanceService = shieldedBalanceService, + cutoverUiDataService = mockk(relaxed = true), + sdkBlockchainStateService = mockk(relaxed = true), + cutoverTxSeamService = mockk(relaxed = true), + cutoverAutoCommitObserver = mockk(relaxed = true), + shieldedTransferExecutor = mockk(relaxed = true), + contactRequestNotificationService = mockk(relaxed = true), + dashPaySyncStatus = de.schildbach.wallet.service.DashPaySyncStatus() + ) + + @Test + fun resume_startsTheL1Engine() = runBlocking { + service().resume() + + coVerify(timeout = kickTimeoutMs, exactly = 1) { l1ShadowSyncService.startIfEnabled() } + Unit + } + + /** + * The MO-995 / MO-998 sequence, at this layer: the engines are stopped + * by a service teardown, then a later service start calls resume(). + * `stopSdkEngines()` is used rather than `shutdown()` so the assertion + * holds on debug variants too (shutdown() deliberately skips the stops + * there — see [PlatformSyncEngineTeardownTest]). + */ + @Test + fun resume_afterAnEngineStop_bringsTheL1EngineBackUp() = runBlocking { + val service = service() + + service.stopSdkEngines() + coVerify(exactly = 1) { l1ShadowSyncService.stop() } + + service.resume() + + coVerify(timeout = kickTimeoutMs, exactly = 1) { l1ShadowSyncService.startIfEnabled() } + Unit + } + + /** + * A teardown that ran the full [PlatformSyncService.shutdown] path — the + * one that cancels the sync scope's children — must not disarm resume(). + * This is the assertion that fails if the scope's job is ever cancelled + * outright instead of its children. + * + * The setup matters: shutdown()'s cancel block is gated on + * `platformSyncJob != null && identityRepository.hasBlockchainIdentity`, + * so an identity and an armed ticker are both required or shutdown() + * skips the cancellation entirely and the test proves nothing. Verified + * by mutation: replacing shutdown()'s + * `syncScope.coroutineContext.cancelChildren(…)` with + * `syncJob.cancel(…)` fails this test and only this test. + */ + @Test + fun resume_afterFullShutdown_stillStartsTheL1Engine() = runBlocking { + every { identityRepository.hasBlockchainIdentity } returns true + val service = service() + + // Arms platformSyncJob in syncScope, so shutdown() below takes the + // branch that cancels the scope's children. + service.initSync(runFirstUpdateBlocking = false) + service.shutdown() + service.resume() + + coVerify(timeout = kickTimeoutMs, exactly = 1) { l1ShadowSyncService.startIfEnabled() } + Unit + } + + /** + * Every service restart in a process gets its own kick. A single-shot + * guard added to the kick path (or a scope that survives only the first + * teardown) reinstates the latch after the second teardown, which is + * precisely how MO-995 presented: the first service restart of the + * process looked fine, later ones silently had no SPV. + */ + @Test + fun resume_isRearmableAcrossRepeatedStopRestartCycles() = runBlocking { + val service = service() + + repeat(3) { + service.stopSdkEngines() + service.resume() + } + + coVerify(timeout = kickTimeoutMs, exactly = 3) { l1ShadowSyncService.startIfEnabled() } + coVerify(exactly = 3) { l1ShadowSyncService.stop() } + Unit + } +} From ce0bc1551f2f8387907b58e7bc4e13ae80d06f1e Mon Sep 17 00:00:00 2001 From: HashEngineering Date: Fri, 28 Aug 2026 11:30:47 -0700 Subject: [PATCH 002/181] fix: crash tapping the fiat option on the Coinbase transfer screen (MO-995) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../ui/EnterAmountToTransferFragment.kt | 65 +++++++- .../coinbase/AmountCurrencySpanTest.kt | 146 ++++++++++++++++++ 2 files changed, 204 insertions(+), 7 deletions(-) create mode 100644 integrations/coinbase/src/test/java/org/dash/wallet/integrations/coinbase/AmountCurrencySpanTest.kt diff --git a/integrations/coinbase/src/main/java/org/dash/wallet/integrations/coinbase/ui/EnterAmountToTransferFragment.kt b/integrations/coinbase/src/main/java/org/dash/wallet/integrations/coinbase/ui/EnterAmountToTransferFragment.kt index 8e9affaf9c..0be881b076 100644 --- a/integrations/coinbase/src/main/java/org/dash/wallet/integrations/coinbase/ui/EnterAmountToTransferFragment.kt +++ b/integrations/coinbase/src/main/java/org/dash/wallet/integrations/coinbase/ui/EnterAmountToTransferFragment.kt @@ -58,11 +58,13 @@ import androidx.compose.ui.unit.dp import org.dash.wallet.common.ui.components.DashWalletTheme import org.dash.wallet.common.ui.components.LocalDashColors import org.dash.wallet.common.ui.segmented_picker.SegmentedOption +import org.slf4j.LoggerFactory @AndroidEntryPoint class EnterAmountToTransferFragment : Fragment(R.layout.enter_amount_to_transfer_fragment) { companion object { + private val log = LoggerFactory.getLogger(EnterAmountToTransferFragment::class.java) private const val DECIMAL_SEPARATOR = '.' fun newInstance() = EnterAmountToTransferFragment() } @@ -162,15 +164,18 @@ class EnterAmountToTransferFragment : Fragment(R.layout.enter_amount_to_transfer private fun formatTransferredAmount(value: String) { val text = viewModel.applyNewValue(value, pickedCurrencyOption) + val span = amountCurrencySpan( + text = text, + isDashSelected = pickedCurrencyIndex == 0, + formattedValue = viewModel.formattedValue, + fiatBalance = viewModel.fiatBalance, + isCurrencyFirst = viewModel.fiatAmount?.isCurrencyFirst() == true + ) val spannableString = SpannableString(text).apply { - if (pickedCurrencyIndex == 0) { - spanAmount(this, viewModel.formattedValue.length, text.length) + if (span != null) { + spanAmount(this, span.from, span.to) } else { - if (viewModel.fiatAmount?.isCurrencyFirst() == true && text.length - viewModel.fiatBalance.length > 0) { - spanAmount(this, 0, text.length - viewModel.fiatBalance.length) - } else { - spanAmount(this, viewModel.inputValue.length, text.length) - } + log.warn("no valid currency span for amount text of length {}", text.length) } } @@ -286,3 +291,49 @@ class EnterAmountToTransferFragment : Fragment(R.layout.enter_amount_to_transfer } } } + +/** The slice of the amount text that renders in the smaller currency style. */ +internal data class AmountCurrencySpan(val from: Int, val to: Int) + +/** + * Which slice of [text] is the currency label (rendered smaller) rather than the + * amount itself, or `null` when there is nothing valid to style. + * + * Pure and host-testable on purpose. The three branches take their bounds from + * separate mutable ViewModel fields, and `applyNewValue` refreshes only the one + * belonging to the branch it took — `formattedValue` for DASH, `fiatBalance` for + * fiat. Mixing a field from the other branch produces bounds that do not describe + * [text] at all, and `Spannable.setSpan` throws on those rather than ignoring them. + * + * MO-995: the fiat currency-last branch used the raw `inputValue` length. The fiat + * branch of `applyNewValue` sets `fiatBalance = inputValue` only when the input has + * at most 2 decimals; otherwise `fiatBalance` is the 2-dp formatted figure, which is + * SHORTER. Tapping USD converts the DASH amount through `formatInput`, yielding many + * decimals — so `from` (12) overshot `to` (7) and setSpan crashed the screen. + */ +internal fun amountCurrencySpan( + text: String, + isDashSelected: Boolean, + formattedValue: String, + fiatBalance: String, + isCurrencyFirst: Boolean +): AmountCurrencySpan? { + val from: Int + val to: Int + if (isDashSelected) { + // text == "$formattedValue $DASH" — the trailing code. + from = formattedValue.length + to = text.length + } else if (isCurrencyFirst && text.length - fiatBalance.length > 0) { + // text == "$symbol $fiatBalance" — the leading symbol. + from = 0 + to = text.length - fiatBalance.length + } else { + // text == "$fiatBalance $symbol" — the trailing symbol. + from = fiatBalance.length + to = text.length + } + // A stale or mismatched field must degrade to an unstyled amount, never crash. + if (from < 0 || to > text.length || from >= to) return null + return AmountCurrencySpan(from, to) +} diff --git a/integrations/coinbase/src/test/java/org/dash/wallet/integrations/coinbase/AmountCurrencySpanTest.kt b/integrations/coinbase/src/test/java/org/dash/wallet/integrations/coinbase/AmountCurrencySpanTest.kt new file mode 100644 index 0000000000..64757e771a --- /dev/null +++ b/integrations/coinbase/src/test/java/org/dash/wallet/integrations/coinbase/AmountCurrencySpanTest.kt @@ -0,0 +1,146 @@ +/* + * Copyright 2026 Dash Core Group. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.dash.wallet.integrations.coinbase + +import org.dash.wallet.integrations.coinbase.ui.AmountCurrencySpan +import org.dash.wallet.integrations.coinbase.ui.amountCurrencySpan +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Test + +/** + * MO-995: tapping the fiat option on the Coinbase transfer screen crashed with + * `IndexOutOfBoundsException: setSpan (12 ... 7) has end before start`. + * + * The span bounds are computed from separate mutable ViewModel fields and handed + * straight to `Spannable.setSpan`, which throws on a reversed or out-of-bounds + * range. These tests pin the bounds for each branch and the degrade-to-null + * behaviour that keeps a field mismatch off the crash path. + */ +class AmountCurrencySpanTest { + + @Test + fun dashSelected_spansTheTrailingCurrencyCode() { + // applyNewValue returns "$formattedValue $monetaryCode". + val span = amountCurrencySpan( + text = "1.5 DASH", + isDashSelected = true, + formattedValue = "1.5", + fiatBalance = "irrelevant on this branch", + isCurrencyFirst = false + ) + + assertEquals(AmountCurrencySpan(3, 8), span) + } + + @Test + fun fiatCurrencyFirst_spansTheLeadingSymbol() { + // applyNewValue returns "$symbol $fiatBalance". + val span = amountCurrencySpan( + text = "$ 4.52", + isDashSelected = false, + formattedValue = "stale", + fiatBalance = "4.52", + isCurrencyFirst = true + ) + + assertEquals(AmountCurrencySpan(0, 2), span) + } + + @Test + fun fiatCurrencyLast_spansTheTrailingSymbol() { + // applyNewValue returns "$fiatBalance $symbol". + val span = amountCurrencySpan( + text = "4.52 Kč", + isDashSelected = false, + formattedValue = "stale", + fiatBalance = "4.52", + isCurrencyFirst = false + ) + + assertEquals(AmountCurrencySpan(4, 7), span) + } + + /** + * The regression itself, with the field lengths from the crash log. + * + * Switching DASH -> USD converts the amount via `formatInput`, so `inputValue` + * becomes a many-decimal figure (12 chars) while `fiatBalance` is the 2-dp + * formatted one. Using `inputValue.length` gave from=12 against a 7-char text. + * The span must be bounded by `fiatBalance` instead, and must stay in range. + */ + @Test + fun fiatCurrencyLast_manyDecimalInput_staysInRange() { + val text = "4.52 Kč" // length 7, as in the crash + val span = amountCurrencySpan( + text = text, + isDashSelected = false, + formattedValue = "stale", + fiatBalance = "4.52", + isCurrencyFirst = false + ) + + requireNotNull(span) + assertEquals(4, span.from) + assertEquals(7, span.to) + // The exact invariant Spannable.setSpan enforces. + assert(span.from in 0..span.to && span.to <= text.length) + } + + @Test + fun reversedBounds_degradeToNullInsteadOfCrashing() { + // fiatBalance longer than the whole text — the shape that used to throw. + val span = amountCurrencySpan( + text = "4.52 Kč", + isDashSelected = false, + formattedValue = "stale", + fiatBalance = "123456789.12", + isCurrencyFirst = false + ) + + assertNull(span) + } + + @Test + fun emptyRange_degradesToNull() { + // Nothing to style: the amount fills the whole text. + val span = amountCurrencySpan( + text = "1.5", + isDashSelected = true, + formattedValue = "1.5", + fiatBalance = "", + isCurrencyFirst = false + ) + + assertNull(span) + } + + @Test + fun currencyFirstWithNoRoomForASymbol_fallsBackAndDegradesToNull() { + // text.length - fiatBalance.length == 0 sends this down the currency-last + // branch, where from == to; that must be null, not a zero-width span. + val span = amountCurrencySpan( + text = "4.52", + isDashSelected = false, + formattedValue = "stale", + fiatBalance = "4.52", + isCurrencyFirst = true + ) + + assertNull(span) + } +} From 5cbffa89709097edd7d4e69ebd36f1899ba44157 Mon Sep 17 00:00:00 2001 From: HashEngineering Date: Mon, 31 Aug 2026 08:35:30 -0700 Subject: [PATCH 003/181] fix: never commit the cutover onto an SDK that cannot own L1 (MO-995) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../platform/sdk/CutoverCoordinator.kt | 96 ++++++++++-- .../service/platform/sdk/SdkWalletBinder.kt | 11 ++ .../wallet/ui/dashpay/utils/DashPayConfig.kt | 42 ++++++ .../platform/sdk/CutoverCoordinatorTest.kt | 139 +++++++++++++++++- 4 files changed, 275 insertions(+), 13 deletions(-) diff --git a/wallet/src/de/schildbach/wallet/service/platform/sdk/CutoverCoordinator.kt b/wallet/src/de/schildbach/wallet/service/platform/sdk/CutoverCoordinator.kt index 5225ae4b82..2a5d3fe968 100644 --- a/wallet/src/de/schildbach/wallet/service/platform/sdk/CutoverCoordinator.kt +++ b/wallet/src/de/schildbach/wallet/service/platform/sdk/CutoverCoordinator.kt @@ -273,6 +273,70 @@ class CutoverCoordinator @Inject constructor( */ fun commitForUpgradedWalletAsync(previousVersionCode: Int) { scope.launch { + // MO-995 GATE 1 — this must be a REAL upgrade across the cutover + // boundary. The same `previousVersionCode` test below used to gate + // only the explainer, while the commit itself ran unconditionally. + // walletB reached here on a SAME-VERSION relaunch (previous code + // 12000001 == this build): the seam committed, dashj was held, the + // SDK bind then failed on the keystore, and the wallet was left + // with no L1 engine at all. If this launch did not cross the + // boundary, the upgrade seam has no business committing — the + // readiness-gated auto-commit observer owns that decision. + // `previousVersionCode` is the version the PREVIOUS LAUNCH ran, and + // `Configuration.updateLastVersionCode` overwrites it every startup + // — so the boundary crossing is visible for exactly one launch, and + // that is the one launch on which GATE 2 below cannot yet be + // satisfied (the bind runs after this seam). Latch it durably so a + // later launch with a working bind can still commit. + val crossedNow = isPreCutoverUpgrade(previousVersionCode) + if (crossedNow) { + runCatching { + dashPayConfig.set(DashPayConfig.CUTOVER_UPGRADE_BOUNDARY_CROSSED, true) + }.onFailure { + if (it is CancellationException) throw it + log.warn("failed to latch the cutover boundary crossing", it) + } + } + val crossedEver = crossedNow || runCatching { + dashPayConfig.get(DashPayConfig.CUTOVER_UPGRADE_BOUNDARY_CROSSED) == true + }.getOrDefault(false) + if (!crossedEver) { + log.info( + "upgrade seam declining to commit the cutover: previous version code {} is " + + "not a pre-{} upgrade (0 = fresh install, >= {} = already on 11.10+) and " + + "no boundary crossing was ever latched — leaving the state alone for the " + + "readiness-gated auto-commit", + previousVersionCode, FIRST_CUTOVER_VERSION_CODE, FIRST_CUTOVER_VERSION_CODE + ) + return@launch + } + + // MO-995 GATE 2 — never hand L1 to an SDK that has never proved it + // can bind on this install. Committing holds the dashj engine, so + // committing while the bind is broken leaves the wallet with NO L1 + // engine: no sync, no incoming transactions, "setup is incomplete". + // walletC/D show the commit today runs 3-7 seconds BEFORE the first + // bind is even attempted, so success there was luck, not design. + // + // 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. Declining outright + // keeps dashj as the single engine for this launch; once a bind + // succeeds, the auto-commit observer earns CUT_OVER through the + // normal readiness policy. + val bindEverSucceeded = + runCatching { dashPayConfig.get(DashPayConfig.SDK_BIND_EVER_SUCCEEDED) == true } + .getOrDefault(false) + if (!bindEverSucceeded) { + log.warn( + "upgrade seam declining to commit the cutover: the SDK wallet bind has never " + + "succeeded on this install, so handing L1 to the SDK would hold dashj and " + + "leave no L1 engine — staying on dashj until a bind succeeds" + ) + return@launch + } + val (_, justCutOver) = mutex.withLock { commitLocked("upgraded-wallet launch") } if (!justCutOver) return@launch if (freshWalletSetupThisLaunch) { @@ -283,15 +347,10 @@ class CutoverCoordinator @Inject constructor( ) return@launch } - if (previousVersionCode <= 0 || previousVersionCode >= FIRST_CUTOVER_VERSION_CODE) { - log.info( - "upgrade seam committed the cutover, but the previous version code {} is not " + - "a pre-11.10 upgrade (0 = fresh install, >= {} = already on 11.10+) — " + - "suppressing the one-time UPGRADE sync explainer", - previousVersionCode, FIRST_CUTOVER_VERSION_CODE - ) - return@launch - } + // The version-code condition that used to gate ONLY the explainer + // here is now GATE 1 above and gates the commit itself, so reaching + // this line already means a genuine pre-cutover upgrade. Arming is + // unconditional from here. runCatching { dashPayConfig.set(DashPayConfig.CUTOVER_UPGRADE_NOTICE_PENDING, true) } .onSuccess { log.info("upgrade cutover: one-time sync explainer armed") } .onFailure { @@ -470,3 +529,22 @@ class CutoverCoordinator @Inject constructor( private val READY_VERDICT = CutoverVerdict(emptySet()) } } + +/** + * Whether [previousVersionCode] — the versionCode recorded by the launch + * BEFORE this one — means this launch genuinely crossed the cutover boundary, + * i.e. the app was last run on a pre-11.10 build. + * + * `0` means the app was never run before (fresh install), and anything at or + * above [CutoverCoordinator.FIRST_CUTOVER_VERSION_CODE] means the previous + * launch was already on 11.10+ — including a relaunch of the SAME build, + * which is what walletB hit (previous code 12000001 on a 12000001 build). + * Neither is an upgrade across the boundary, so neither may drive the + * UPGRADE seam's commit. + * + * Pure and host-testable: this is the predicate that decides whether the + * upgrade seam is allowed to hand L1 to the SDK at all. + */ +internal fun isPreCutoverUpgrade(previousVersionCode: Int): Boolean = + previousVersionCode > 0 && + previousVersionCode < CutoverCoordinator.FIRST_CUTOVER_VERSION_CODE diff --git a/wallet/src/de/schildbach/wallet/service/platform/sdk/SdkWalletBinder.kt b/wallet/src/de/schildbach/wallet/service/platform/sdk/SdkWalletBinder.kt index 42a359bef5..24734cc0cc 100644 --- a/wallet/src/de/schildbach/wallet/service/platform/sdk/SdkWalletBinder.kt +++ b/wallet/src/de/schildbach/wallet/service/platform/sdk/SdkWalletBinder.kt @@ -1020,6 +1020,17 @@ class SdkWalletBinder internal constructor( sdkService.bindAppWallet(words, birthTimeSecs).also { boundWalletIdHex = it boundWalletFingerprint = fingerprint + // MO-995: durable evidence that this install's keystore can + // actually serve the SDK bind. The UPGRADE cutover seam refuses + // to hand L1 to the SDK until this is set, so a device whose + // Keystore denies the lock-bound master alias stays on dashj + // instead of being left with no engine at all. Best-effort: + // a failed write only delays the next launch's commit. + runCatching { dashPayConfig.set(DashPayConfig.SDK_BIND_EVER_SUCCEEDED, true) } + .onFailure { t -> + if (t is CancellationException) throw t + log.warn("failed to persist the SDK bind-success marker", t) + } } } diff --git a/wallet/src/de/schildbach/wallet/ui/dashpay/utils/DashPayConfig.kt b/wallet/src/de/schildbach/wallet/ui/dashpay/utils/DashPayConfig.kt index 2845608c2c..9ff89fb91a 100644 --- a/wallet/src/de/schildbach/wallet/ui/dashpay/utils/DashPayConfig.kt +++ b/wallet/src/de/schildbach/wallet/ui/dashpay/utils/DashPayConfig.kt @@ -426,6 +426,48 @@ open class DashPayConfig @Inject constructor( */ val CUTOVER_UPGRADE_NOTICE_PENDING = booleanPreferencesKey("cutover_upgrade_notice_pending") + /** + * MO-995: set the first time the SDK wallet bind succeeds on this + * install, and never cleared except by a wallet wipe. It is the + * evidence the UPGRADE cutover seam requires before it will hand L1 + * to the SDK — see + * [de.schildbach.wallet.service.platform.sdk.CutoverCoordinator + * .commitForUpgradedWalletAsync]. + * + * WHY PERSISTED and not the binder's in-memory state: the upgrade seam + * runs from `finalizeInitialization`, BEFORE the first bind pass of the + * process (platform sync starts the binder). An in-process signal is + * therefore always false at the seam and would defer every upgrade to + * the readiness-gated auto-commit. Persisting it means a device that + * has ever bound successfully still commits promptly, while a device + * whose keystore denies the bind never commits at all — the difference + * between walletC/D (bind fine, cut over) and walletB (16 keystore + * denials, cut over anyway, left with NO L1 engine). + */ + val SDK_BIND_EVER_SUCCEEDED = booleanPreferencesKey("sdk_bind_ever_succeeded") + + /** + * MO-995: set the first time a launch is seen to have crossed the + * cutover boundary (the previous launch ran a pre-11.10 build), and + * never cleared except by a wallet wipe. + * + * WHY THIS EXISTS: `Configuration.lastVersionCode` is "the version the + * PREVIOUS LAUNCH ran", not "the version this install upgraded from" — + * `updateLastVersionCode` overwrites it on every startup. So the fact + * that an install crossed the boundary is visible for exactly ONE + * launch, and that is the very launch on which the SDK bind has not + * run yet (the seam is in `finalizeInitialization`; the binder starts + * with platform sync). Without a durable latch, the upgrade seam's + * two conditions could never both hold and it would never commit + * again — losing the one-time sync explainer with it. + * + * Latching it means the commit can land on whichever later launch + * first has a working bind, which is what a device like walletB needs: + * its keystore may deny the bind for many launches before it heals. + */ + val CUTOVER_UPGRADE_BOUNDARY_CROSSED = + booleanPreferencesKey("cutover_upgrade_boundary_crossed") + /** * DIAGNOSTIC toggle (Tools screen, debug instrumentation): un-hold the * dashj L1 engine AFTER the Phase 5d cutover has committed, so the diff --git a/wallet/test/de/schildbach/wallet/service/platform/sdk/CutoverCoordinatorTest.kt b/wallet/test/de/schildbach/wallet/service/platform/sdk/CutoverCoordinatorTest.kt index 25f87b2983..5c0672546a 100644 --- a/wallet/test/de/schildbach/wallet/service/platform/sdk/CutoverCoordinatorTest.kt +++ b/wallet/test/de/schildbach/wallet/service/platform/sdk/CutoverCoordinatorTest.kt @@ -36,6 +36,7 @@ import kotlinx.coroutines.test.UnconfinedTestDispatcher import kotlinx.coroutines.test.runTest import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Test @@ -78,12 +79,20 @@ class CutoverCoordinatorTest { private fun coordinator( stored: String? = null, flag: Boolean? = true, - evidence: CutoverEvidence = readyEvidence() + evidence: CutoverEvidence = readyEvidence(), + // MO-995 GATE 2: the UPGRADE seam refuses to commit until the SDK bind + // has succeeded at least once on this install. Defaults to true so the + // pre-existing cases keep exercising what they were written for; the + // gate itself has its own tests below. + bindEverSucceeded: Boolean? = true ): Pair String?> { var current = stored val config = mockk() coEvery { config.get(DashPayConfig.CUTOVER_STATE) } answers { current } coEvery { config.get(DashPayConfig.USE_KOTLIN_SDK_L1_SHADOW) } returns flag + coEvery { config.get(DashPayConfig.SDK_BIND_EVER_SUCCEEDED) } returns bindEverSucceeded + coEvery { config.get(DashPayConfig.CUTOVER_UPGRADE_BOUNDARY_CROSSED) } returns false + coEvery { config.set(DashPayConfig.CUTOVER_UPGRADE_BOUNDARY_CROSSED, any()) } just Runs coEvery { config.set(DashPayConfig.CUTOVER_STATE, any()) } answers { current = secondArg() Unit @@ -311,13 +320,22 @@ class CutoverCoordinatorTest { */ private fun noticeCoordinator( stored: String? = null, - flag: Boolean? = true + flag: Boolean? = true, + bindEverSucceeded: Boolean? = true, + boundaryAlreadyLatched: Boolean = false ): Triple String?, () -> Boolean> { var current = stored var noticeArmed = false + var boundaryLatched = boundaryAlreadyLatched val config = mockk() coEvery { config.get(DashPayConfig.CUTOVER_STATE) } answers { current } coEvery { config.get(DashPayConfig.USE_KOTLIN_SDK_L1_SHADOW) } returns flag + coEvery { config.get(DashPayConfig.SDK_BIND_EVER_SUCCEEDED) } returns bindEverSucceeded + coEvery { config.get(DashPayConfig.CUTOVER_UPGRADE_BOUNDARY_CROSSED) } answers { boundaryLatched } + coEvery { config.set(DashPayConfig.CUTOVER_UPGRADE_BOUNDARY_CROSSED, any()) } answers { + boundaryLatched = secondArg() + Unit + } coEvery { config.set(DashPayConfig.CUTOVER_STATE, any()) } answers { current = secondArg() Unit @@ -337,6 +355,13 @@ class CutoverCoordinatorTest { /** A previous-launch versionCode already ON the 11.10 line (11.10.1). */ private val on1110VersionCode = 11100100 + /** + * The versionCode of THIS build — what `lastVersionCode` reads on every + * launch after the first one. walletB reached the seam with exactly this + * shape (previous code 12000001 on a 12000001 build). + */ + private val sameBuildVersionCode = 12000001 + @Test fun upgradeNotice_armed_onAGenuineUpgradeFromPre1110ThatFlipsTheState() = runBlocking { val (coordinator, stored, armed) = noticeCoordinator(stored = null) @@ -359,7 +384,12 @@ class CutoverCoordinatorTest { // crossing the pre-cutover boundary — no explainer. val (coordinator, stored, armed) = noticeCoordinator(stored = null) coordinator.commitForUpgradedWalletAsync(on1110VersionCode) - assertEquals("the commit itself must be unaffected by the notice gate", CutoverState.CUT_OVER.name, stored()) + // MO-995 GATE 1 (behaviour CHANGE): the version-code test now gates the + // COMMIT too, not just the explainer. An 11.10+ previous code means this + // launch did not cross the cutover boundary, so the upgrade seam must + // leave the state alone — walletB committed here on a same-version + // relaunch and was left with no L1 engine when its bind then failed. + assertNull("a non-boundary-crossing launch must not commit", stored()) assertFalse("an 11.10.x -> 11.10.y update must not re-explain the resync", armed()) } @@ -371,7 +401,9 @@ class CutoverCoordinatorTest { // e.g. any future path reaching this seam without setWallet). val (coordinator, stored, armed) = noticeCoordinator(stored = null) coordinator.commitForUpgradedWalletAsync(0) - assertEquals(CutoverState.CUT_OVER.name, stored()) + // GATE 1 again: a fresh install did not cross the boundary either, and + // the fresh-wallet seam owns that commit. + assertNull("a fresh install must not commit through the UPGRADE seam", stored()) assertFalse("a fresh install has nothing to explain", armed()) } @@ -423,6 +455,9 @@ class CutoverCoordinatorTest { val config = mockk() coEvery { config.get(DashPayConfig.CUTOVER_STATE) } answers { current } coEvery { config.get(DashPayConfig.USE_KOTLIN_SDK_L1_SHADOW) } answers { sdkL1Enabled } + coEvery { config.get(DashPayConfig.SDK_BIND_EVER_SUCCEEDED) } returns true + coEvery { config.get(DashPayConfig.CUTOVER_UPGRADE_BOUNDARY_CROSSED) } returns true + coEvery { config.set(DashPayConfig.CUTOVER_UPGRADE_BOUNDARY_CROSSED, any()) } just Runs coEvery { config.set(DashPayConfig.CUTOVER_STATE, any()) } answers { current = secondArg() Unit @@ -477,4 +512,100 @@ class CutoverCoordinatorTest { assertEquals(CutoverState.SETTLED, status.state) assertEquals(CutoverState.SETTLED.name, stored()) } + + // ── MO-995: the UPGRADE seam's commit gates ─────────────────────── + + @Test + fun upgradeSeam_doesNotCommit_whenTheSdkBindHasNeverSucceeded() = runBlocking { + // walletB, exactly: a genuine pre-11.10 upgrade, but this install's + // Keystore keeps denying the lock-bound master alias so the SDK has + // never bound. Committing would hold dashj and leave NO L1 engine — + // no sync, no incoming transactions, "setup is incomplete". + val (coordinator, stored, armed) = noticeCoordinator(stored = null, bindEverSucceeded = false) + coordinator.commitForUpgradedWalletAsync(pre1110VersionCode) + assertNull("a never-bound SDK must not be handed L1", stored()) + assertFalse("nothing committed, so nothing to explain", armed()) + assertTrue("dashj must stay available as the only engine", coordinator.dashjEngineMayStart()) + } + + @Test + fun upgradeSeam_doesNotCommit_whenTheBindMarkerIsAbsent() = runBlocking { + // An absent key reads as "never bound" — fail safe, not fail open. + val (coordinator, stored, _) = noticeCoordinator(stored = null, bindEverSucceeded = null) + coordinator.commitForUpgradedWalletAsync(pre1110VersionCode) + assertNull("an absent bind marker must be treated as never-bound", stored()) + } + + @Test + fun upgradeSeam_commits_onAGenuineUpgradeOnceTheBindHasSucceeded() = runBlocking { + // walletC/D: same seam, same version-code path, but the bind works. + val (coordinator, stored, armed) = noticeCoordinator(stored = null, bindEverSucceeded = true) + coordinator.commitForUpgradedWalletAsync(pre1110VersionCode) + assertEquals(CutoverState.CUT_OVER.name, stored()) + assertTrue("a genuine boundary-crossing upgrade still explains the resync", armed()) + } + + @Test + fun isPreCutoverUpgrade_pinsTheBoundary() { + assertFalse("0 = fresh install, never ran before", isPreCutoverUpgrade(0)) + assertFalse("negative is nonsense — fail safe", isPreCutoverUpgrade(-1)) + assertTrue("11.9.0 crossed the boundary", isPreCutoverUpgrade(11090000)) + assertTrue( + "one below the line still crosses it", + isPreCutoverUpgrade(CutoverCoordinator.FIRST_CUTOVER_VERSION_CODE - 1) + ) + assertFalse( + "the line itself is already 11.10", + isPreCutoverUpgrade(CutoverCoordinator.FIRST_CUTOVER_VERSION_CODE) + ) + assertFalse("walletB: a relaunch of the SAME 12.x build is not an upgrade", isPreCutoverUpgrade(12000001)) + } + + @Test + fun upgradeSeam_latchesTheBoundary_thenCommitsOnALaterLaunchOnceTheBindWorks() = runBlocking { + // The two-launch sequence a healthy upgrade actually takes. + // + // LAUNCH 1: previous launch ran 11.9.0, so the boundary is crossed — + // but the bind has not run yet (this seam is in finalizeInitialization; + // the binder starts with platform sync). Latch, do not commit. + val (l1, storedL1, armedL1) = noticeCoordinator(stored = null, bindEverSucceeded = false) + l1.commitForUpgradedWalletAsync(pre1110VersionCode) + assertNull("launch 1 must not commit — no bind yet", storedL1()) + assertFalse(armedL1()) + + // LAUNCH 2: lastVersionCode now reads THIS build, so the live version + // test fails; only the latch keeps the seam alive. The bind succeeded + // during launch 1, so it commits and explains. + val (l2, storedL2, armedL2) = noticeCoordinator( + stored = null, + bindEverSucceeded = true, + boundaryAlreadyLatched = true + ) + l2.commitForUpgradedWalletAsync(sameBuildVersionCode) + assertEquals("the latch must carry the crossing past launch 1", CutoverState.CUT_OVER.name, storedL2()) + assertTrue("the one-time explainer must survive the deferral", armedL2()) + } + + @Test + fun upgradeSeam_neverCommits_whenTheBindNeverWorks_evenWithTheBoundaryLatched() = runBlocking { + // walletB: latched on its upgrade launch, but 16 keystore denials later + // the bind has still never succeeded. It must stay on dashj forever + // rather than be handed an L1 it cannot serve. + val (coordinator, stored, _) = noticeCoordinator( + stored = null, + bindEverSucceeded = false, + boundaryAlreadyLatched = true + ) + coordinator.commitForUpgradedWalletAsync(sameBuildVersionCode) + assertNull("a latched boundary must not override a broken bind", stored()) + assertTrue(coordinator.dashjEngineMayStart()) + } + + @Test + fun upgradeSeam_doesNotLatch_whenNoBoundaryWasCrossed() = runBlocking { + // A same-build relaunch that never had a crossing must not invent one. + val (coordinator, stored, _) = noticeCoordinator(stored = null, bindEverSucceeded = true) + coordinator.commitForUpgradedWalletAsync(sameBuildVersionCode) + assertNull("no crossing ever seen — the seam stays out of it", stored()) + } } From 93795401baf6cd6dfe43cd550f202b306884b4cf Mon Sep 17 00:00:00 2001 From: HashEngineering Date: Mon, 31 Aug 2026 10:07:58 -0700 Subject: [PATCH 004/181] fix: persist the bind-success marker on every successful bind, not just a new one (MO-995) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- scripts/cutover-emulator-test.sh | 373 ++++++++++++++++++ .../service/platform/sdk/SdkWalletBinder.kt | 39 +- .../platform/sdk/CutoverCoordinatorTest.kt | 39 +- .../platform/sdk/SdkBindRetryServiceTest.kt | 67 ++++ 4 files changed, 493 insertions(+), 25 deletions(-) create mode 100755 scripts/cutover-emulator-test.sh diff --git a/scripts/cutover-emulator-test.sh b/scripts/cutover-emulator-test.sh new file mode 100755 index 0000000000..685992633e --- /dev/null +++ b/scripts/cutover-emulator-test.sh @@ -0,0 +1,373 @@ +#!/bin/bash +# +# MO-995 cutover / SPV-restart test harness for the Android emulator. +# +# WHY A RELEASE BUILD: PlatformSyncService.shutdown() skips stopSdkEngines() +# when BuildConfig.DEBUG (PlatformSyncService.kt:656) — a deliberate warm-SPV +# battery trade-off. On a debug build the SDK engine never stops on teardown, +# so neither MO-995 nor its fix can be observed. Everything below needs the +# _testNet3Release variant. +# +# WHY THE AOSP EMULATOR IMAGE: steps 1-3 fake the previous-launch versionCode +# by writing shared_prefs directly, which needs `adb root`. The +# android-36/google_apis_playstore image blocks root; android-36/default +# allows it. Use Pixel_9_API_36_AOPS (Android 16, arm64-v8a — matches all four +# field devices). +# +# WHAT THIS CANNOT REPRODUCE: walletB's FALSE-LOCKED classification +# (KeyguardManager reports unlocked while Keystore denies). That is the HONOR +# PTP-N49 OEM defect; on AOSP every denial classifies as "genuinely locked". +# The false-locked branch is covered by SdkBindRetryServiceTest instead. A +# green run here does NOT clear the HONOR case. +# +# ORDER OF OPERATIONS: you install the PRE-CUTOVER build yourself and onboard +# /sync it. Then `setup` builds, signs and installs the fix build OVER it — +# that install is the upgrade under test. The scenarios run after that. +# +# Usage: ./scripts/cutover-emulator-test.sh +# ./scripts/cutover-emulator-test.sh setup # build+sign+upgrade-install +# ./scripts/cutover-emulator-test.sh s1 # upgrade, bind works +# ./scripts/cutover-emulator-test.sh s2 # upgrade, bind denied +# ./scripts/cutover-emulator-test.sh s3 # denied then healed +# ./scripts/cutover-emulator-test.sh s4 # trim -> SPV restart +# ./scripts/cutover-emulator-test.sh log # pull + tail wallet.log +set -uo pipefail + +PKG=hashengineering.darkcoin.wallet_test +SVC=$PKG/de.schildbach.wallet.service.BlockchainServiceImpl +AVD=Pixel_9_API_36_AOPS +PIN=1234 +PREV_VERSION_PRE_CUTOVER=11090000 # any value < FIRST_CUTOVER_VERSION_CODE (11100000) +BT=~/Library/Android/sdk/build-tools/35.0.1 +APK_DIR=wallet/build/outputs/apk/_testNet3/release +OUT=/tmp/mo995-emulator +mkdir -p "$OUT" + +say() { printf '\n\033[1m== %s\033[0m\n' "$*"; } +note() { printf ' %s\n' "$*"; } +adbs() { adb shell "$@"; } + +require_device() { + adb get-state >/dev/null 2>&1 || { echo "No device. Start it: emulator -avd $AVD -no-snapshot-load"; exit 1; } +} + +# Root is required for the shared_prefs poke and for `run-as`-free log reads. +require_root() { + adb root >/dev/null 2>&1 + adb wait-for-device >/dev/null 2>&1 + if [ "$(adbs id -u | tr -d '\r')" != "0" ]; then + echo "adb root failed — are you on the google_apis_playstore image? Use $AVD (android-36/default)." + exit 1 + fi +} + +# Pull the log, NEVER silently truncating our copy. The old version fell back +# to `run-as`, which cannot work on a release build (not debuggable) — so a +# failed pull wrote an EMPTY file and every assertion reported a false FAIL +# while the log on the device was fine. Now: re-assert root, retry, and keep +# the previous copy rather than clobbering it with nothing. +pull_log() { + local dst="$OUT/wallet.log" tmp="$OUT/.wallet.pull" i + for i in 1 2 3; do + rm -f "$tmp" + if adb pull "/data/data/$PKG/files/log/wallet.log" "$tmp" >/dev/null 2>&1 && [ -s "$tmp" ]; then + mv -f "$tmp" "$dst"; echo "$dst"; return 0 + fi + adb root >/dev/null 2>&1; adb wait-for-device >/dev/null 2>&1; sleep 2 + done + rm -f "$tmp" + if [ -s "$dst" ]; then + echo " WARN: could not refresh the log (adb pull failed) — asserting on the last good copy" >&2 + echo "$dst"; return 0 + fi + echo " ERROR: cannot read wallet.log from the device (need adb root; release builds are not run-as-able)" >&2 + : > "$dst"; echo "$dst"; return 1 +} + +# Assertions are SCOPED TO LINES ADDED SINCE mark_log(), not to a truncated +# file. `rm`-ing wallet.log proved unreliable (logback may hold or recreate the +# handle), and a stale line from an earlier run silently satisfied a refute — +# which is exactly how a contaminated S1 reported a false failure. Line-offset +# scoping is immune to that, and to log rotation (guarded below). +LOG_MARK=0 + +mark_log() { + local f; f=$(pull_log) + LOG_MARK=$(wc -l < "$f" 2>/dev/null | tr -d ' ') + : "${LOG_MARK:=0}" + note "log marked at line $LOG_MARK — assertions below only see NEW lines" +} + +# Lines appended since the mark. If the log shrank (rotation/truncation), +# fall back to the whole file rather than silently asserting on nothing. +# Writes the window to a FILE and echoes its path. Callers must grep the file, +# never `since_mark | grep -q`: under `set -o pipefail`, grep -q exits on the +# first match, tail dies of SIGPIPE, and the pipeline reports 141 — so a match +# looks like a failure. That produced false FAILs for patterns appearing early +# in a large window while later ones passed. +since_mark() { + local f n out="$OUT/.window"; f=$(pull_log); n=$(wc -l < "$f" | tr -d ' ') + if [ "${n:-0}" -lt "${LOG_MARK:-0}" ]; then cp -f "$f" "$out" + else tail -n +$((LOG_MARK + 1)) "$f" > "$out"; fi + echo "$out" +} + +# assert_log