chore: log cause of keychain load failures#898
Merged
Conversation
Probes for aliasPresent, entryPresent and walletIndex previously collapsed any exception inside runCatching to the default value, making a keystore subsystem / DataStore / Room error indistinguishable from a legitimate 'alias missing'. Render tri-state instead: 'true' / 'false' / 'error:ClassName'. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Member
Author
|
@ovitrif @piotr-iohk The next step would be launch a 2.2.1 version (public or just internally) so could get the logs without requiring the app uninstall |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related to #897
This PR:
Keychain.load/save/upsertString/deletecatch a failure, so the real exception propagates throughKeychainErrorKeychain.loadon the first decrypt failure per key per process, capturingaliasPresent,entryPresent,walletIndex, and the cause chain of class names. Each probe is tri-state —'true'/'false'/'error:ClassName'— so a failure inside the diagnostic itself (keystore subsystem error, DataStore I/O, Room error) is distinguishable from a genuinefalseCancellationExceptionthrough unwrapped from all four mutating pathsAndroidKeyStore.containsAlias()so the diagnostic can distinguish "alias missing" from "alias present but unusable"Description
The reporter on issue #897 is stuck at launch because
keychain.loadString(BIP39_MNEMONIC)throws, and every one of the repeatedERRORlines in their log says the same thing:[AppError='Failed to load BIP39_MNEMONIC from keychain.']. That string tells us the load failed but not why. The catch block inKeychain.loaddiscards the underlyingThrowableentirely, so the log cannot distinguish:keyStore.getKey(...)returning null — alias gone (terminal, hardware-side)Cipher.doFinalAEADBadTagException— alias regenerated under the same name (terminal, hardware-side)UnrecoverableKeyException/KeyStoreException— alias present but unusableEach of these has different remediation and different user messaging. For a Bitcoin wallet we should not guess between them. This PR is diagnostics only — no retries, no routing changes, no destructive changes to stored data. After this ships, the next occurrence of the bug will include the cause class in the existing
AppError=…line and a single enrichedWARNwith the adjacent keystore/datastore state, which is enough to pick the right fix in a follow-up PR.The new WARN is intentionally leak-safe: it only logs class names (not
cause.message), does not pass theThrowabletoLogger(which would re-introduce the message viaerrorLogOf), and is one-shot per key per process so a start-retry loop cannot flood the log.Preview
N/A — diagnostic logging only, no UI changes.
QA Notes
1. Happy path (no regression)
WARN … Decrypt failed for key=…lines appear in the log.(cause='…')suffix appears on anyAppError=…error line.5. Unit tests
./gradlew testDevDebugUnitTest --tests to.bitkit.data.keychain.KeychainErrorTestpasses./gradlew testDevDebugUnitTest --tests to.bitkit.repositories.WalletRepoTestpasses./gradlew detektclean./gradlew compileDevDebugKotlincleanOut of scope (intentionally deferred)
keychainCorruptedstate, seed-restore copy) — to be designed once these logs tell us what class of failure users are actually hitting.Keychain— would risk masking the signal we're trying to capture.