fix(voting): MO-977 — real vote errors, already-cast votes, and dark-mode rows - #1566
Open
HashEngineering wants to merge 209 commits into
Open
HashEngineering wants to merge 209 commits into
HashEngineering wants to merge 209 commits into
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>
… cleanup stuck past 5 min in the background BlockchainServiceImpl.onCreate rightly refuses to start while a previous instance's cleanup is unfinished, but nothing ever gave up on a cleanup that was never going to finish. On 2026-09-22 twelve alarm-driven starts over four hours each logged "deadlock in onDestroy" and stopped themselves while the process lived on with the engine off. Plan section 37. The refused start now measures how long the active cleanup has been stuck (cleanupStartedAtMs, set when cleanupDeferred is created). Past CLEANUP_DEADLOCK_EXIT_MS (5 min) with the app in the background it ends the process, so the next start — the alarm or the user — begins from a clean one. Never while the app is visible: that start is the user's own, and exiting would close the app on them; they get the refusal as before. decideOnCleanupDeadlock is pure and pinned on both axes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…m issue 17 drafted Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… committed-range sweep, not a stuck block fetch Andrei's 2026-09-22 engine log settles it: the final batch's 65 matched blocks were all processed within six seconds, and the batch still did not commit for seven minutes. The line in between is `Rescan committed filters (934848-2539848) … (sweep #1)`: before committing the last batch, dash-spv re-tests every filter since wallet birth against the scripts derived during the scan, inline on the filter task, with no persisted progress. At P=19, M=784931 a 13,024-script set false-matches 1.66% of all 1.6M mainnet filters — ~26,000 block fetches for nothing — and a phone never finishes before something stops the engine. Our own integration-branch commit 80e07b8f (durable pending-sweep set, #979) re-seeds the set at every start, which is why the park recurs identically after every restart. Already open upstream: rust-dashcore#1002 describes this wallet shape; rust-dashcore#1016 (dash-spv owner, draft) drops the sweep. The issue draft this section promised is retired in favour of a comment there. Also: the 06:08 watchdog restart in the same log was a device-offline stall (all peers ping-timed out, DNS down 21 min, new network at 06:21:49), not an engine defect. The four app log lines and KDocs that stated the pending-block story are reworded; no test asserted on them. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
int22 is the int21 engine pin d525f431 plus two cherry-picks: rust-dashcore #1015 (key-wallet: re-apply a spend whose coin was funded after it) and the block-counter fix that stops re-applications inflating `processed`. It does NOT carry #1016: the committed-range sweep and our durable pending-sweep set are both still present, so the plan §34 final-batch park is unchanged on this build. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…he per-launch re-walk is the DashPay backfill, not a park Plan §38: emulator (debug 12000017) and Samsung (release 12000018) both restored the job flower wallet on int22 to the correct seed with no watchdog verdict, stall WARN or restart; the emulator's engine log shows no committed-range sweep this time, with the ordering hypothesis stated as such. Plan §34.1a corrected and §38.2: the 1,532,170 re-walk boundary seen on every emulator session is coreHeightCreatedAt of the wallet's earliest DashPay contact request; rs-platform-wallet's reconcile_dashpay_rescan rewinds synced_height to it at every startup behind an in-memory guard (dashpay/platform#4302, PR #4740). The mod-5,000 residue the earlier text read as a batch-boundary park is the batch grid anchored at that start. QA: A-01 gains the int22 restore evidence; A-03 records the per-launch re-walk as an SDK defect; header and summary updated. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ly at clean stop The alarm had one call site: WalletApplication.scheduleStartBlockchainService from onDestroy's cleanup coroutine, at the "detaching from wallet" step after the onCreate latch and the check-mutex acquisition. A kill that lands before that step leaves the wallet with no pending restart. Samsung SM-S901U, 2026-09-22: synced at 17:39, task swiped ~17:43 (deferred while the service was started), idle stop 17:47:01.0, deferred "remove task" kill at 17:47:01.2 — 600 ms into cleanup. dumpsys alarm then listed no alarm for the app's uid, and 70 minutes later there was still no process on an awake, charging, battery-exempt device. lowmemorykiller on a cached process gives no onDestroy at all and has the same effect. Plan section 36.6. onCreate now arms the alarm after the foreground promotion. The scheduler cancels and replaces its own PendingIntent, so the two arms are idempotent, and a start delivered while the service is already running is an ordinary onStartCommand. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ole bring-up against a stop that timed out
Two review findings on the bounded stop.
Lock leak at the timeout boundary: `withTimeoutOrNull { lock.lock() }` can
deliver ownership to this coroutine while the timeout still wins the
completion, returning null with the lock held and nothing to release it —
on this singleton every later bring-up would then wait forever. stop() now
acquires with a tryLock poll (tryLockWithin): tryLock is non-suspending and
atomic, so ownership and the result cannot disagree, and the only suspension
is the poll delay, during which nothing is held.
Fence: the generation guard covered configure/bind only. A bring-up past it
could be parked in the native sync-loop start when stop() timed out and tore
the Kotlin side down; on return it published ready and started the
collectors and the pending-shield sweep over the teardown. And
hasShieldedSupport()/boundWalletIdOrNull() call ensureStarted(), so a stop in
that bootstrap interval was absorbed by a snapshot taken later. The
generation is now sampled first and re-checked after every native step; a
superseded bring-up stops the loop it started (or found running) and
declines.
Tests: a bring-up parked in the native loop start is superseded by stop(),
stops the loop it started and declines, and a fresh bring-up is whole again;
a stop during the bootstrap interval aborts the bring-up before it binds.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…g-up that took the lock mid-poll is superseded too A second bring-up queued on the lock wins the handoff over stop()'s tryLock when the first releases, and snapshots the generation stop had already produced at entry. If it then outlasts the timeout, the fallback teardown left its snapshot equal to the current generation, so it started the loop and published ready over the teardown. The fallback now bumps the generation once more before tearing down, so every snapshot taken during the poll window is stale by the time the teardown runs. Test: stop_fallbackAlsoSupersedesASecondBringUpThatTookTheLockDuringItsPoll — two bring-ups gated in their native binds around a polling stop; both decline, no loop starts, a fresh bring-up afterwards is whole. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… sweep at 67,658 scripts; QA J-01 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ntactFloor; the backfill gate is the no-op binding Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Debug.getPss() walks /proc/self/smaps, and on the reference install's 2 GB replay process that held the main thread — inside the tick broadcast receiver — for 5 s or more, four times in one evening (plan §39.6). The sample now runs on the service scope, one in flight at a time; a slow one skips ticks rather than queueing behind itself. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… sweep" reading; what int22 changes for Joel's wallet The AAR's native-library hashes are the recipe's #1015 + #1016 build (9d1804d6), not the #1015-only 08c0c04e that section 38 named; the 09-22 debug APK and the 09-23 release APK were both built against it. Section 38.1's provisioning-order explanation for "no sweep tonight" was wrong: there is no sweep in int22. Section 39.4 separates the scan's own matching (438–817 blocks per batch) from the sweep's additions (up to 800), and 39.7 / QA J-01 now say what 12000018 would and would not change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… in tenths
Over the whole chain the figure said nothing: a rewind to 2,167,092 under a
2,544,483 tip read 95.1 → 99.9% for three hours, and the 27,000-block
re-walk every launch pays sat at 99% from start to finish (Joel,
2026-09-25, plan §39.8).
ShadowSyncProgress carries the session's floor — the lowest header and
filter cursor reported since the engine started, a running minimum so the
DashPay backfill's rewind a few seconds in lowers it — and
shadowSyncPermille measures progress over [floor, target]. A fresh
restore (floor 0) reads as before. The persisted percentageSync stays a
whole number for its == 100 consumers; L1SyncUiStatus gains
percentageTenths and the home header shows "99.1%" / "99.9%" ("100%",
never "100.0%"; dashj's whole number pre-cutover). The 100 decision is
unchanged.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…bserved on screen; the balance question Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…lished carries the recipe's §5 mid-sync-kill fund loss Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… DASH across seven change outputs Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…t gap; the kill run was confounded by the once-only widening Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…t once per heal version The Rust side keeps AddressPool::gap_limit in memory only — no changeset, no persistence — so every fresh process and every re-created SDK wallet comes up at the defaults (30 for BIP44, 100 for CoinJoin). The widening to 1000 ran once per heal version, so a process killed mid-scan resumed at the defaults and never rediscovered the outputs paid to change addresses the killed session had derived past used + 30: the kill-test fund loss of 2026-09-23 and 2026-09-25 (plan §39.10), which only the now-removed committed-range sweep (rust-dashcore#1016) used to mask. The widening now runs on every bind, before the engine starts; the heal's retroactive half (the rewind to birth and the coverage invalidation) stays once per version. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…al to the clean control Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
HashEngineering
force-pushed
the
fix/sync-process-stalls
branch
from
September 25, 2026 22:21
70b9aae to
8ee23d4
Compare
…-cast vote as failure
Two defects in BroadcastUsernameVotesWorker, found voting a contested username
on emulator-5554 (testnet, 12000013). They interact: the second masked the
first.
1. The failure paths themselves threw. Both the all-failed and the
partial-failure branches built KEY_LABELS as `arrayOfnames.map { labelMap[it] }`
— a List. `androidx.work.Data` accepts String[] only, so every attempt to
report a vote error died in Data.Builder.put with
IllegalArgumentException: Key BroadcastUsernameVotesWorker.LABELS
has invalid type class java.util.ArrayList
and the user saw that instead of the real error (Firebase logged it as the
reason on username_voting_details_btn_vote_fail; UsernameRequestsFragment
logged "error processing vote information"). The success branch was already
correct — it passes the String[] straight from inputData.
`.toTypedArray()` alone would not have fixed it: `labelMap[it]` is a
nullable lookup, and Data rejects Array<String?> for the same reason. A name
with no display label now falls back to the NORMALIZED NAME rather than
being filtered out — the observer reads KEY_LABELS and KEY_NORMALIZED_LABELS
as parallel arrays, so dropping an element would desync them, and the
normalized name is still truthful to show.
Relatedly, a failed vote carries no Vote and so no poll name; the name array
was substituting the literal string "null" for it, which is precisely the
key that had no labelMap entry. Those are now dropped, falling back to the
submitted labels when nothing is left.
2. "Vote is already present" was unwrapped as a hard failure. That error means
the masternode has ALREADY voted this poll — the desired end state is true —
yet it surfaced as "all votes failed". Failures are now classified and only
terminal ones count against the broadcast; an already-cast vote reconciles
as success and logs at info instead of error.
The vote-budget error the worker's comments anticipate alongside it
("already voted 5 times ... they can only vote 5 times") stays terminal and
is deliberately NOT collapsed into the same verdict. Since it also contains
the words "already voted", the limit marker is matched FIRST; reversing the
two branches would report a spent budget as a successful vote.
classifyVoteFailure / voteFailureText / labelsFor are pure top-level functions,
host-JVM testable without WorkManager or the SDK — the same shape as
isOwnContestedCandidate and contestedNameCandidates in RestoreIdentityWorker.
The two verbatim engine messages are pinned in VoteFailureClassificationTest so
an SDK reword fails loudly instead of silently reclassifying a duplicate vote
as fatal.
Not addressed here: an already-cast vote still writes no UsernameVote row,
because the failed Triple carries only the vote choice and not the poll name.
The success path's updateUsernameRequestWithVotes refresh reconciles the true
count from the network; recording it locally would mean widening
PlatformBroadcastService's return type.
Tests: 2009 pass, 0 failures (1996 baseline + 13 new). ktlintCheck clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
MO-977 (Andrei, 12000014): the Username Voting list renders correctly in dark mode until a contested name with multiple requests is expanded — each child row then draws as a solid white bar with no visible text. `username_request_view.xml` backs its row with `@drawable/selectable_round_corners_border_light`, whose fill was a hardcoded `@android:color/white`. The text on that row uses theme-aware styles (`@style/Overline`), so in dark mode it turns white and disappears against the white fill — white on white. The group row above it does not have the problem: it uses `@drawable/rounded_background` with `?attr/backgroundColor`. Fixed by making the fill follow the theme. `background_secondary` resolves to `palette_white` (#FFFFFF) by day, which is byte-identical to the `@android:color/white` it replaces, so light mode is unchanged; at night it resolves to `palette_black_800`, matching the group card the rows sit inside — the same relationship light mode already had, with the 1dp stroke providing the separation in both themes. The stroke and ripple are left alone: `button_ripple_light` is a 25%-alpha gray (#40B0B6BC) that composites correctly over either background. Scope: this drawable has exactly one usage, the row being fixed, so there is no collateral. Its "_light" name is now a misnomer, kept to hold the diff to the defect; a comment in the file records why the fill must stay theme-aware. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
HashEngineering
force-pushed
the
fix/username-voting-already-cast
branch
from
September 26, 2026 22:40
914515a to
69f90a5
Compare
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
thepastaclaw
suggested changes
Sep 27, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the complete diff at 68c2a65 and traced voting outcomes through the service, worker, operation, and UI. Both prior findings are fixed, but batches containing fresh successes and terminal failures now reach callers as unqualified success without exposing the failures. The theme-aware drawable change is consistent with the day/night resources; whitespace checks passed, and tests were inspected but not rerun.
🔴 1 blocking
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: general); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
normalbygpt-6-astra(effort low) — The diff changes nontrivial vote-error classification, terminal-failure propagation, and parallel output-label handling plus dark-mode rendering and tests, but does not alter consensus, funds movement, cryptography, key handling, network deserialization, or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 13% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— general (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/ui/dashpay/work/BroadcastUsernameVotesWorker.kt`:
- [BLOCKING] wallet/src/de/schildbach/wallet/ui/dashpay/work/BroadcastUsernameVotesWorker.kt:210-215: Expose terminal errors when another vote succeeds
When at least one fresh vote succeeds and another has a terminal failure, this branch returns success with every submitted name and no error information. For example, a quick-vote batch where Alice succeeds and Bob reaches the vote limit becomes Status.SUCCESS in BroadcastUsernameVotesOperation.convertState; UsernameRequestsFragment shows the success indicator and removes its observer, while Bob's error remains only in logs. This is exposed by this PR's serialization fix: the old List-valued KEY_LABELS threw before this branch could return success. Preserve the votes already recorded, but propagate the terminal error either through Result.failure with KEY_ERROR_MESSAGE or through an explicit partial-result contract handled by the operation and UI. Extend the mixed-success regression test to assert that observers receive the failure information, rather than only asserting Result.Success.
Comment on lines
210
to
215
| Result.success( | ||
| workDataOf( | ||
| KEY_NORMALIZED_LABELS to arrayOfnames, | ||
| KEY_LABELS to arrayOfnames.map { labelMap[it] }, | ||
| KEY_LABELS to labelsFor(arrayOfnames, labelMap), | ||
| KEY_VOTE_CHOICES to voteChoices, | ||
| KEY_QUICK_VOTING to isQuickVoting |
There was a problem hiding this comment.
🔴 Blocking: Expose terminal errors when another vote succeeds
When at least one fresh vote succeeds and another has a terminal failure, this branch returns success with every submitted name and no error information. For example, a quick-vote batch where Alice succeeds and Bob reaches the vote limit becomes Status.SUCCESS in BroadcastUsernameVotesOperation.convertState; UsernameRequestsFragment shows the success indicator and removes its observer, while Bob's error remains only in logs. This is exposed by this PR's serialization fix: the old List-valued KEY_LABELS threw before this branch could return success. Preserve the votes already recorded, but propagate the terminal error either through Result.failure with KEY_ERROR_MESSAGE or through an explicit partial-result contract handled by the operation and UI. Extend the mixed-success regression test to assert that observers receive the failure information, rather than only asserting Result.Success.
source: gpt-6-astra (phase2-reviewer: general)
HashEngineering
force-pushed
the
fix/sync-process-stalls
branch
2 times, most recently
from
September 28, 2026 02:47
bc798f6 to
c7fe51b
Compare
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
2 participants
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.
Three defects on the Username Voting screen (MO-977). The first two are in
BroadcastUsernameVotesWorkerand interact — the second masked the first — so they are fixed together; the third is the dark-mode rendering Andrei reported on 12000014.1. The failure path itself threw, hiding every vote error
Both the all-failed and the partial-failure branches built
KEY_LABELSasarrayOfnames.map { labelMap[it] }— aList.androidx.work.DataacceptsString[]only, so every attempt to report a vote error died before returning:The user saw that instead of the real error. Firebase logged it as the reason on
username_voting_details_btn_vote_fail;UsernameRequestsFragmentlogged"error processing vote information". The success branch was already correct — it passes theString[]straight through frominputData..toTypedArray()alone would not have fixed it:labelMap[it]is a nullable lookup, andDatarejectsArray<String?>for the same reason.Nullability decision: a name with no display label falls back to the normalized name, rather than being filtered out.
UsernameRequestsFragmentreadsKEY_LABELSandKEY_NORMALIZED_LABELSas parallel arrays (showVoteIndicator(votes, usernames, …)indexes into both), so dropping an element would desync them — and the normalized name is still truthful to show.There was a second half to this. A failed vote carries no
Voteand so no poll name, and the name array was substituting the literal string"null"for it — which is precisely the keylabelMaphad no entry for. Those are now dropped, falling back to the submitted labels when nothing is left.2. An already-cast vote was unwrapped as a hard failure
"Vote is already present" means the masternode has already voted this poll — the desired end state is already true. Failures are now classified, only terminal ones count against the broadcast, and an already-cast vote reconciles as success and logs at info instead of error.
The vote-budget error the worker's comments have long anticipated alongside it (
already voted 5 times … they can only vote 5 times) stays terminal and is deliberately not collapsed into the same verdict. Since it also contains the words "already voted", the limit marker is matched first — reversing those two branches would report a spent vote budget as a successful vote.Shape
classifyVoteFailure,voteFailureTextandlabelsForare pure top-level functions, host-JVM testable without WorkManager or the SDK — the same pattern asisOwnContestedCandidate/contestedNameCandidatesinRestoreIdentityWorker, tested in the style ofSecondaryNameCollisionTest.voteFailureTextflattens the cause chain (cycle-safe), because the field exception arrives wrapped inAttempted to unwrap a Failure: ….Both verbatim engine messages are pinned in
VoteFailureClassificationTest, along with an assertion that the markers actually occur in the messages they claim to match, so an SDK reword fails loudly rather than silently reclassifying a duplicate vote as fatal.Not addressed here
An already-cast vote still writes no
UsernameVoterow: the failedTriple<ResourceVoteChoice, Vote?, Exception?>carries the vote choice but not the poll name. The success path'supdateUsernameRequestWithVotesrefresh reconciles the true count from the network anyway; recording it locally would mean wideningPlatformBroadcastService.broadcastUsernameVotes's return type, which is wider than these two bugs.3. Expanded contested-name rows were white in dark mode
Reported on the ticket against 12000014: the voting list renders correctly in dark mode until a contested name with multiple requests is expanded — each child row then draws as a solid white bar with no visible text.
username_request_view.xmlbacks its row with@drawable/selectable_round_corners_border_light, whose fill was a hardcoded@android:color/white. The text on that row uses theme-aware styles (@style/Overline), so in dark mode it turns white and vanishes against the white fill. The group row above it is fine because it uses@drawable/rounded_backgroundwith?attr/backgroundColor.The fill now follows the theme.
background_secondaryresolves topalette_white(#FFFFFF) by day — byte-identical to the@android:color/whiteit replaces, so light mode cannot regress — and topalette_black_800at night, matching the group card the rows sit inside. That is the same relationship light mode already had, with the 1dp stroke providing the separation in both themes. The stroke and ripple are left alone:button_ripple_lightis a 25%-alpha gray (#40B0B6BC) that composites correctly over either background.The drawable has exactly one usage — the row being fixed — so there is no collateral. Its
_lightname is now a misnomer, kept to hold the diff to the defect; a comment in the file records why the fill must stay theme-aware.Testing
./gradlew :wallet:compile_testNet3DebugKotlin— BUILD SUCCESSFUL./gradlew :wallet:test_testNet3DebugUnitTest— 2009 tests, 0 failures (1996 baseline + 13 new)./gradlew ktlintCheck— clean./gradlew :wallet:assembleProdDebug— BUILD SUCCESSFUL; installed on an emulator in dark mode for the fix in §3Note: CI
buildalready fails on the base branch withUnresolved reference 'startWalletSubsystems'(L1ShadowSyncService.kt) — the base branch calls a Kotlin SDK API newer than thedashSdkVersionits commits pin. Pre-existing and unrelated to this change.🤖 Generated with Claude Code
Summary by CodeRabbit